-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
GCE health check does not pick up changes to pod readinessProbe #582
Comments
Comment by @nicksardo: There's actually a TODO for deciding what to do in this case https://github.com/kubernetes/ingress/blob/6ed6475e87588494dd29ba5d33bac167ae8acfdd/controllers/gce/healthchecks/healthchecks.go#L71 The controller doesn't want to overwrite handmade changes. We could probably update the settings if they have the default value. |
But updating from default value would lead to confusing behavior where only
the first change is propagated, and it's not even possible to go back to
default, by changing the ingress, right?
I think it's reasonable to assume that if the ingress controller knows how
to manage a setting, it will overwrite manual changes.
|
I agree that it's reasonable and would be an easy solution. However, I worry about unsuspecting users who upgrade their master and find their load balancer throwing 502s. Would creating an event on health check update be sufficient? Thoughts from other users? |
Is there any way that the "ingress controller"? can track whether the value was set automatically? |
The controller does not have persistent storage. One option would be to convert the health check settings to JSON and store the state in the HealthCheck's optional description field for tracking diffs, but this seems very ugly. |
cc @ibawt |
What do you mean by "creating an event on health check update"?
|
If the controller sees a health check with settings different from default/readinessProbe, it updates the check then could create a K8s event for each parent ingress. If readiness probe exists, event could say, "Health Check for port 3XXXX updated to readinessProbe settings". If probe doesn't exist, "Health check for port 3XXXX updated to default settings (see ingress docs for defining settings via readinessProbe)". |
Just my 2c: As a kubernetes user, I have no idea how to use events — they
seem too transient to be useful (i.e. it's impossible to see when they
actually happened, and I've never seen an event that's helped me to run my
cluster better).
It might just be a UI problem — if there was a way of just tailing the
event logs, then I might comprehend them better, so maybe it's ok to add
one and assume that they'll be fixed upstream
Sent via Superhuman <https://sprh.mn/[email protected]>
…On Tue, Apr 11, 2017 at 10:10 AM, Nick ***@***.***>wrote:
If the controller sees a health check with settings different from
default/readinessProbe, it updates the check then could create a K8s event
for each parent ingress. If readiness probe exists, event could say,
"Health Check for port 3XXXX updated to readinessProbe settings". If probe
doesn't exist, "Health check for port 3XXXX updated to default settings
(see ingress docs for defining settings via readinessProbe)".
There's already an event being created at HC creation, but it doesn't seem
very useful to me: https://github.com/kubernetes/ingress/blob/
987540f/controllers/gce/controller/utils.
go#L603
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#582 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFwQP-D0IkTqISjn40vKGBNilNOffRjks5ru7QNgaJpZM4M5mnb>
.
|
Is there any workaround to make it pick up the changes, even manually? |
To answer my own question and for everybody landing here with the same problem, rotate the name of the service in your rule or backend and the controller will pick up the path correctly. I assume this could be done with 0 downtime creating a new rule and deleting the old one. |
We actually encountered an issue yesterday in regards to readinessProbes and health checks on GCE. A while ago we had some difficulties with setting up the health-check correctly through Kubernetes, adding that it doesn't pick up changes, we decided to manually update the health-check on the GCE side, everything worked. But, reading this issue, it should never update health-checks. I cannot re-trigger an update now, so it doesn't seem that this functionality has been added. But for sure the master upgrade did trigger this, is that expected behavior? |
Another gotcha, when using the example provided, https://github.com/kubernetes/ingress/blob/6ed6475e87588494dd29ba5d33bac167ae8acfdd/controllers/gce/examples/health_checks/health_check_app.yaml, if you remove the All our reset health-checks didn't have a |
Regarding your last comment, Regarding your loss of manual health check settings, the latest version of the controller creates a newer-type of health check ( What settings are you trying to override with the readiness probe? Just the path? |
@nicksardo Thanks for the information! I was not aware that the Makes sense if it created a new type of health-check, thanks for the explanation. I do think more people that manually edited their health checks will run into this issue, but, manually editing health checks is also not the best thing to do ;-) The most important changes we make with |
@bviolier |
This issue was moved to kubernetes/ingress-gce#39 |
Importing this issue from kubernetes/kubernetes#43773 as requested.
Kubernetes version (use
kubectl version
): 1.4.9Environment:
uname -a
):What happened:
I created an ingress with type GCE
This set up the health-check on the google cloud backend endpoint to call "/" as documented.
Unfortunately my service doesn't return 200 on
"/"
and so I added a readinessCheck to the pod as suggested by the documentation.What you expected to happen:
I expected the health check to be automatically updated.
Instead I had to delete the ingress and re-create it in order for the health-check to update.
How to reproduce it (as minimally and precisely as possible):
Anything else we need to know:
The text was updated successfully, but these errors were encountered: