-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Plumb contexts into health checks #4141
Plumb contexts into health checks #4141
Conversation
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.
PTAL @thaJeztah @Jamstah @squizzi
} | ||
updater := health.NewThresholdStatusUpdater(app.Config.Health.StorageDriver.Threshold) | ||
healthRegistry.Register("storagedriver_"+app.Config.Storage.Type(), updater) | ||
go health.Poll(app, updater, storageDriverCheck, interval) |
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.
Love that we've ripped out the goroutine launch from the original updated implementation where it was hiding to this place <3
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 noticed that the original logic is simple and clear, and it doesn't require invoking the go function each time to register a periodic checker.
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 noticed that the original logic is simple and clear, and it doesn't require invoking the go function each time to register a periodic checker.
This isn't registering the check - this is starting a goroutine that is doing the polling
This needs a rebase @corhere |
Ping @corhere wanna rebase and squash this? We've got approvals to merge this in |
Signed-off-by: Cory Snider <csnider@mirantis.com>
Allow health checkers to abort if the request context is canceled. Modify the checkers to respect context cancelation and return wrapped errors so the caller of CheckStatus() would be able to discriminate true failed checks from checks which were aborted because the context became done. Signed-off-by: Cory Snider <csnider@mirantis.com>
The API for periodic health checks is repetitive, with a distinct function for polling a checker to each kind of updater. It also gives the user no control over the lifetime of the polling goroutines nor which context is passed into the checker. Replace the existing PeriodicXYZChecker functions with a single Poll function which composes an Updater with a Checker. Its context parameter is passed into the checker and also controls when the polling loop terminates. To guard against health checks failing closed (ostensibly healthy) when the polling loop is terminated, the updater is forcefully updated to an error status, overriding any configured threshold. Signed-off-by: Cory Snider <csnider@mirantis.com>
0ce4eab
to
f2cbfe2
Compare
See individual commit messages for details.