- Notifications
You must be signed in to change notification settings - Fork928
feat(coderd/healthcheck): add health check for proxy#10846
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
84b0060 toe80c05dCompare11597b6 toc7e202cCompareThere 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.
Good test coverage, minor things only.
coderd/coderd.go Outdated
| AppSecurityKey workspaceapps.SecurityKey | ||
| AppSecurityKey workspaceapps.SecurityKey | ||
| // The following two functions are dependencies of HealthcheckFunc but are only implemented |
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.
maybe move the comment to the place where you assign these no-op{}
Uh oh!
There was an error while loading.Please reload this page.
| @@ -153,6 +174,9 @@ func Run(ctx context.Context, opts *ReportOptions) *Report { | |||
| if !report.Database.Healthy { | |||
| report.FailingSections = append(report.FailingSections, SectionDatabase) | |||
| } | |||
| if !report.WorkspaceProxy.Healthy { | |||
| report.FailingSections = append(report.FailingSections, SectionWorkspaceProxy) | |||
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.
side thought: frontend does not useFailingSections at all. Maybe we should remove it for simplicity.
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.
👍 Will file a follow-up PR
Edit:#10854
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 think it's beneficial to have a structure that can be navigated in a generic way.
for(letsectionofreport.failing_sections){console.log(report[section].reason);}
Given a good design, it will "always works", even when new fields and structures are added. Granted, I'm not a proponent for the current design, just for the ability to do so.
| // health.SeverityError if all proxies are unhealthy, | ||
| // health.SeverityOK if all proxies are healthy, | ||
| // health.SeverityWarning otherwise. | ||
| func calculateSeverity(total, healthy int) health.Severity { |
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.
👍
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.
TBH this could probably be moved to a separateutils file
| }, | ||
| }, nil | ||
| }), | ||
| updateProxyHealth: ptr.Ref(func(ctx context.Context) error { |
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.
nit: you can create a no-op function somewhere around, and just refer to it
coderd/coderd.go Outdated
| @@ -396,9 +402,19 @@ func New(options *Options) *API { | |||
| *options.UpdateCheckOptions, | |||
| ) | |||
| } | |||
| if options.FetchWorkspaceProxiesFunc == nil { | |||
| options.FetchWorkspaceProxiesFunc = &atomic.Pointer[func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)]{} | |||
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.
Random comment: This looks pretty gnarly (generics within generics), I kind of get why the Go team delayed generics for as long as they did. 😄
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.
To be fair, generics within generics look gnarly in any language!
Uh oh!
There was an error while loading.Please reload this page.
| @@ -153,6 +174,9 @@ func Run(ctx context.Context, opts *ReportOptions) *Report { | |||
| if !report.Database.Healthy { | |||
| report.FailingSections = append(report.FailingSections, SectionDatabase) | |||
| } | |||
| if !report.WorkspaceProxy.Healthy { | |||
| report.FailingSections = append(report.FailingSections, SectionWorkspaceProxy) | |||
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 think it's beneficial to have a structure that can be navigated in a generic way.
for(letsectionofreport.failing_sections){console.log(report[section].reason);}
Given a good design, it will "always works", even when new fields and structures are added. Granted, I'm not a proponent for the current design, just for the ability to do so.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| for _, proxy := range r.WorkspaceProxies.Regions { | ||
| if vErr := checkVersion(proxy, opts.CurrentVersion); vErr != nil { | ||
| r.Healthy = false | ||
| r.Severity = health.SeverityError |
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.
Not suggesting, just asking: Is this always an error? Could it be a warning instead if it's a patch version difference, for instance?
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.
Our current behaviour won't even let workspace proxies register if they don't match major.minor, but makes no further complaints if it's just a patch version difference.
Making it warn on a patch version difference sounds like a good idea in theory, but it will mean every time you upgrade, you'll get a short period of warning that there's a patch version diff. Although that's the current behaviour as well when you do a major version upgrade, so... 🤷
I'll see about adding it in a follow-up.
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!
| // in an actually useful and meaningful way. | ||
| type workspaceProxiesFetchUpdater struct { | ||
| fetchFunc func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) | ||
| updateFunc func(context.Context) error |
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.
Suggestion: You could either export this as-is to re-use in tests, or you could simplify this to just pass in the whole API (api *coderd.API) and call it from the methods.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#9558
Adds a health check for workspace proxies: