Skip to content
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

Merged
merged 2 commits into from
Mar 15, 2018
Merged

Conversation

jmirc
Copy link
Contributor

@jmirc jmirc commented Mar 2, 2018

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

  • Added/updated tests

Additional Notes

It is my first go changes so I am sure this could be better.

@jmirc jmirc requested a review from a team as a code owner March 2, 2018 14:56
@traefiker traefiker added this to the 1.5 milestone Mar 2, 2018
@ldez ldez changed the title Fix Rancher Healthcheck doesn't work as expected Fix Rancher Healthcheck when upgrading a service Mar 2, 2018
@nmengin
Copy link
Contributor

nmengin commented Mar 5, 2018

WDYT @containous/rancher ?

Copy link
Collaborator

@SantoDE SantoDE left a 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"?

@jmirc
Copy link
Contributor Author

jmirc commented Mar 7, 2018

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.

@SantoDE
Copy link
Collaborator

SantoDE commented Mar 9, 2018

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?

@jmirc
Copy link
Contributor Author

jmirc commented Mar 12, 2018

Hi SantoDE,

I agree with you. We should not pass traffic to the container with an upgrading state

Jerome

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants