Skip to content

feat(coderd): add parameter to force health check in /debug/health #10677

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

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Nov 14, 2023

Part of #8969
Modifies the /debug/health endpoint to accept a force=(true|_) parameter.
Iff set to true, will re-run the healthcheck immediately.
UI changes will come in a separate PR.

@johnstcn johnstcn self-assigned this Nov 14, 2023
@johnstcn johnstcn force-pushed the cj/healthcheck-force-recheck branch from 4029aed to 0b8e032 Compare November 15, 2023 13:07
@johnstcn johnstcn marked this pull request as ready for review November 15, 2023 13:41
@johnstcn johnstcn requested a review from mtojek November 15, 2023 13:41
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

This is one of these cases, when unit tests are longer than actual implementation.


// Get cached report if it exists and the requester did not force a refresh.
if !forced {
if report := api.healthCheckCache.Load(); report != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need an extra safety check to ensure that only 1 request is in-flight?

Copy link
Member Author

Choose a reason for hiding this comment

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

I belive the healthcheck singleflight group should take care of that.

formatHealthcheck(ctx, rw, r, report)
return
// Check if the forced query parameter is set.
forced := r.URL.Query().Get("force") == "true"
Copy link
Member

Choose a reason for hiding this comment

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

I get the invalidation concept, but if it does not take too much time, maybe we can also run the health check without caching?

Copy link
Member Author

Choose a reason for hiding this comment

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

if it does not take too much time

I think this is a pretty big 'if' :-)

@mtojek mtojek self-requested a review November 15, 2023 14:52
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

nothing blocking here 👍

@johnstcn johnstcn changed the title feat(coderd): /debug/health: add parameter to force healthcheck feat(coderd): add parameter to force health check in /debug/health Nov 15, 2023
@johnstcn johnstcn merged commit 9d31038 into main Nov 15, 2023
@johnstcn johnstcn deleted the cj/healthcheck-force-recheck branch November 15, 2023 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants