- Notifications
You must be signed in to change notification settings - Fork927
feat: Add provisioner force-cancel flag#4947
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
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.
Looks great! I'm approving with the caveat that I think changing the config structure a bit would be appropriate.
cli/deployment/config.go Outdated
| @@ -359,6 +359,14 @@ func newConfig() *codersdk.DeploymentConfig { | |||
| Flag: "user-workspace-quota", | |||
| Enterprise: true, | |||
| }, | |||
| Provisionerd: &codersdk.ProvisionerdConfig{ | |||
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.
We have another config calledProvisionerDaemons. I think it would be sensible to unify these underProvisioner.Daemons andProvisioner.ForceCancelInterval?
Should be a non-breaking change as well since the flag/env translate to the same.
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 assumed that this would be a breaking change if there is a customer who uses theProvisionerDaemons. I'm for unification, but a bit concerned about disturbing somebody's environment.
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 should be fine. Neither flag or env should break (wasPROVISIONER_DAEMONS and will be the same in the new structure, please correct me if I’m wrong@f0ssel).
The only breakage I can think if is when used as part ofserver.yaml. But it’s a recent addition and not widely used yet AFAIK.
There may be even more breaking changes to this in the future as we split out provisionerd servers from the coderd server.
| @@ -65,7 +65,7 @@ func New(clientDialer Dialer, opts *Options) *Server { | |||
| opts.UpdateInterval = 5 * time.Second | |||
| } | |||
| if opts.ForceCancelInterval == 0 { | |||
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.
A thought/musing: Do we want to support disabling this by setting it to0? Or do we just ask users to raise it to areally high number ™️?
(No change necessary here, the latter is fine.)
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.
Good question, Mathias. I wouldn't recommend setting it to infinite as it can be problematic for some cases (stuck forever). I guess that it implicates the latter option.
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.
That’s a very good point. So yeah, let’s not allow our users to create a deadlock with all provisioners waiting forever 😄
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.
The types look good! 👍
Uh oh!
There was an error while loading.Please reload this page.
Fixes:#4586
This PR adds a new flag to configure the force-cancel interval for provisionerd. It also increases the default value.