-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Fix Rancher Healthcheck when upgrading a service #2962
Conversation
WDYT @containous/rancher ? |
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.
Hi @jmirc ,
Thanks for your contribution. One question though: what is the idea to not filter upgrading containers? To not filter "upgraded" is fine imho. But I'm wondering about "upgrading"?
Hi @SantoDE , I am not filtering a service with the upgrading state because it doesn't mean the service can't receive load. An upgrading state is just meaning that the service is upgrading. The solution to avoid any downtimes of your service is to check the "Start before stopping" checkbox. With this option, a new container is started and once it is running the previous version is stopped. It doesn't mean the service can't receive any calls. Actually, the service is filtering when the state is upgrading, that's mean a call to this service will return a 404 which is not what we want. Let me know if it is not clear enough. |
Hi @jmirc, okay, then it's what I expected :) For Services, I'm fine to pass them traffic as long as there healthstate is upgrading. However, I'm unsure about not filtering containers which state is upgrading. Imho, it would be better to not pass them traffic until they are healthy. WDYT? |
Hi SantoDE, I agree with you. We should not pass traffic to the container with an upgrading state Jerome |
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.
LGTM
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.
LGTM
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.
LGTM 🐮
What does this PR do?
Here is the small change I did to fix the issue. I tested locally and it works.
Motivation
Fixes #2279
More
Additional Notes
It is my first go changes so I am sure this could be better.