feat(coderd/healthcheck): add health check for proxy #10846
Conversation
84b0060 to
e80c05d
Compare
11597b6 to
c7e202c
Compare
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.
maybe move the comment to the place where you assign these no-op {}
| @@ -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.
side thought: frontend does not use FailingSections at all. Maybe we should remove it for simplicity.
There was a problem hiding this comment.
👍 Will file a follow-up PR
Edit: #10854
There was a problem hiding this comment.
I think it's beneficial to have a structure that can be navigated in a generic way.
for (let section of report.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.
TBH this could probably be moved to a separate utils file
| }, | ||
| }, nil | ||
| }), | ||
| updateProxyHealth: ptr.Ref(func(ctx context.Context) error { |
There was a problem hiding this comment.
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.
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.
To be fair, generics within generics look gnarly in any language!
| @@ -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.
I think it's beneficial to have a structure that can be navigated in a generic way.
for (let section of report.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.
| 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.
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.
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.
| // 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.
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.
Fixes #9558
Adds a health check for workspace proxies: