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

Stop health check futures when upgrading the Supervisor #6717

Merged

Conversation

@christophermaier
Copy link
Contributor

commented Jul 10, 2019

When we shut down the Supervisor normally, we stop the long-running
control gateway future, and then spawn a bunch of service shutdown
futures. Those aren't "infinite" futures, so they'll eventually be
driven to completion. Then, we just wait for all the futures on the
reactor to finish and we shut down.

When we shut down for a Supervisor upgrade, however, we don't shut
down the services. Since their health checks now run in an infinite
future, these futures will effectively prevent the Supervisor from
shutting down.

This commit introduces a "detach" method for services, which is now
called in the Supervisor upgrade scenario. It currently just stops
health checks, but can be augmented to do more as we transition more
functionality over to asynchronous futures.

Similarly, the existing "reattach" logic is now extracted into a
"reattach" method, which will now restart health checking.

Fixes #6712

Signed-off-by: Christopher Maier cmaier@chef.io

@baumanj
Copy link
Member

left a comment

I don't see anything that look like a logic error, but since this is still in draft, I'll wait on formal approval

components/sup/src/manager/service.rs Outdated Show resolved Hide resolved
components/sup/src/manager/service.rs Outdated Show resolved Hide resolved
components/sup/src/manager/service.rs Show resolved Hide resolved
@jeremymv2

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@christophermaier I compiled this branch and successfully observed the anticipated detach and reattach working as intended after a SIGHUP to hab-launch!

Would it be worth while for us to introduce a test to help ensure the behavior remains consistent?

@christophermaier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@jeremymv2 Glad to hear it's working for you 😄 And yeah, I'm looking at creating a test for this right now.

@christophermaier christophermaier force-pushed the cm/stop-health-checks-on-supervisor-upgrade branch 2 times, most recently from 7be3122 to 392d33a Jul 10, 2019

@christophermaier christophermaier marked this pull request as ready for review Jul 10, 2019

@christophermaier christophermaier requested a review from raskchanky as a code owner Jul 10, 2019

Stop health check futures when upgrading the Supervisor
When we shut down the Supervisor normally, we stop the long-running
control gateway future, and then spawn a bunch of service shutdown
futures. Those aren't "infinite" futures, so they'll eventually be
driven to completion. Then, we just wait for all the futures on the
reactor to finish and we shut down.

When we shut down for a Supervisor upgrade, however, we _don't_ shut
down the services. Since their health checks now run in an infinite
future, these futures will effectively prevent the Supervisor from
shutting down.

This commit introduces a "detatch" method for services, which is now
called in the Supervisor upgrade scenario. It currently just stops
health checks, but can be augmented to do more as we transition more
functionality over to asynchronous futures.

Similarly, the existing "reattach" logic is now extracted into a
"reattach" method, which will now restart health checking.

Add minor tweaks to the existing Expect test that should have caught
this regression.

Fixes #6712

Signed-off-by: Christopher Maier <cmaier@chef.io>

@christophermaier christophermaier force-pushed the cm/stop-health-checks-on-supervisor-upgrade branch from 392d33a to 5667247 Jul 11, 2019

@christophermaier christophermaier merged commit 5bf0d9d into master Jul 11, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2626 passed (35 minutes, 4 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #3223 passed (2 minutes, 54 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@christophermaier christophermaier deleted the cm/stop-health-checks-on-supervisor-upgrade branch Jul 11, 2019

chef-ci added a commit that referenced this pull request Jul 11, 2019

Update CHANGELOG.md with details from pull request #6717
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.