-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add readiness probe #84
Conversation
Currently, manager pods turn Ready as soon as their binary starts. However, it takes ~40 more seconds for the webhook to actually start serving requests. In order to avoid that, we introduce a readiness probe that marks the pod as Ready only once the server starts. Please note that there is no way to do it properly with the current controller-runtime. We don't have a hook point that would get executed once the webhook has started. Therefore we have a Sleep there. That should be dropped once kubernetes-sigs/cluster-api#1855 is implemented. Signed-off-by: Petr Horacek <phoracek@redhat.com>
command: | ||
- cat | ||
- /tmp/ready | ||
initialDelaySeconds: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need any golang code for this hack? Running cat
of an ever-existing file every 5 seconds feels silly.
Would it not be enough to have
initialDelaySeconds: 20
with a fat comment carrying the same explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to create the file once the server starts.
I don't think saving 3 cat
runs is worth an extra line and comment.
We can replace cat
with HTTP GET once controller-runtime implements support for it. I'm not aware of any option that would allow us to stop querying once it succeeds once. https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was speaking about something like
readinessProbe:
exec:
command:
- true
initialDelaySeconds: 20
periodSeconds: 99999
and dropping the golang changes altogether. But I'm splitting hairs here. The important thing is that you have found the races that @AlonaKaplan has spoken about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would not help. True can succeed as soon as the pod starts. That would be the same as happens with default readiness.
The point here is, that we write the file only once we go through all the initialization and we start the webhook server. In other words, we are marked Ready only once we are able to respond to HTTPS requests for /mutate-virtualmachines.
The extra 20 seconds wait is on top of all the initialization (collecting existing MACs, getting certificates). Those 20 seconds are waiting until the Start
hopefully really starts serving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have races in the test code #83. Unfortunately, the tests are more resilient than the kubemacpool code, so it is all hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha.
In the current code, setRange and deleteLeaderManager functions are not waiting for the old pods to disappear and new ones to become available. Due to that, tests using this functions don't have any webhook available or worse, they are being served by the old one. This issue was not exposed due to Eventually we have around all the Create calls. This commit makes sure the old pods are removed but since the readiness probe proposed in k8snetworkplumbingwg#84 is not merged yet, a sleep is used here. Signed-off-by: Petr Horacek <phoracek@redhat.com> Signed-off-by: Alona Kaplan <alkaplan@redhat.com>
In the current code, setRange and deleteLeaderManager functions are not waiting for the old pods to disappear and new ones to become available. Due to that, tests using this functions don't have any webhook available or worse, they are being served by the old one. This issue was not exposed due to Eventually we have around all the Create calls. This commit makes sure the old pods are removed but since the readiness probe proposed in k8snetworkplumbingwg#84 is not merged yet, a sleep is used here. Signed-off-by: Petr Horacek <phoracek@redhat.com> Signed-off-by: Alona Kaplan <alkaplan@redhat.com>
Currently, manager pods turn Ready as soon as their binary starts.
However, it takes ~40 more seconds for the webhook to actually start
serving requests. In order to avoid that, we introduce a readiness
probe that marks the pod as Ready only once the server starts.
Please note that there is no way to do it properly with the current
controller-runtime. We don't have a hook point that would get executed
once the webhook has started. Therefore we have a Sleep there. That
should be dropped once
kubernetes-sigs/cluster-api#1855
is implemented.