- Notifications
You must be signed in to change notification settings - Fork1k
chore: improve build deadline code#19203
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
deansheather commentedAug 6, 2025
- Adds/improves a lot of comments to make the autostop calculation code clearer
- Changes the behavior of the enterprise template schedule store to match the behavior of the workspace TTL endpoint when the new TTL is zero
- Fixes a bug in the workspace TTL endpoint where it could unset the build deadline, even though a max_deadline was specified
- Adds a new constraint to the workspace_builds table that enforces the deadline is non-zero and below the max_deadline if it is set
- Adds CHECK constraint enum generation to scripts/dbgen, used for testing the above constraint
- Adds Dean and Danielle as CODEOWNERS for the autostop calculation code
- Adds/improves a lot of comments to make the autostop calculation code clearer- Changes the behavior of the enterprise template schedule store to match the behavior of the workspace TTL endpoint when the new TTL is zero- Fixes a bug in the workspace TTL endpoint where it could unset the build deadline, even though a max_deadline was specified- Adds a new constraint to the workspace_builds table that enforces the deadline is non-zero and below the max_deadline if it is set- Adds CHECK constraint enum generation to scripts/dbgen, used for testing the above constraint- Adds Dean and Danielle as CODEOWNERS for the autostop calculation code
| @@ -0,0 +1,239 @@ | |||
| package main | |||
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.
Moved all the constraint stuff to it's own file because it's quite large.
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.
Also refactored it into a reusable function rather than pasting the same code three times. I'll admit it's not the prettiest but it's probably good enough.
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 good to me
Uh oh!
There was an error while loading.Please reload this page.
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.
Overall LGTM, just a couple of nits, suggestions, and questions.
Thanks for adding the extra comments, they’re super helpful! 🙌
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // Deadline is intended as a cost saving measure, not as a hard policy. It is | ||
| // derived from either the workspace's TTL or the template's TTL, depending on | ||
| // the template's policy, to ensure workspaces are stopped when they are idle. | ||
| // | ||
| // MaxDeadline is intended as a compliance policy. It is derived from the | ||
| // template's autostop requirement to cap workspace uptime and effectively force | ||
| // people to update often. | ||
| // | ||
| // Note that only the build's CURRENT deadline property influences automation in | ||
| // the autobuild package. As stated above, the MaxDeadline property is only used | ||
| // to cap the value of a build's deadline. |
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.
🤩 really nice comment, thanks!
coderd/workspaces.go Outdated
| // If the build has a max_deadline set, we still need to | ||
| // abide by it. It will either be zero (our target), or a | ||
| // value. | ||
| Deadline:build.MaxDeadline, |
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 sure I follow the reasoning behind this change 🤔
According to the above comment:
// If autostop has been disabled, we want to remove the deadline from the// existing workspace build (if there is one).if !dbTTL.Valid {So, shouldn't deadline betime.Time{},?
ssncferreiraAug 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Actually, if I understood correctly,Deadline is derived from TTL andMaxDeadline is derived from Autostop. In that case, when autostop is disabled, shouldn’t justMaxDeadline be unset? 🤔
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.
This code is correct, but I'll update the comment to make it clearer.
// Use the max_deadline as the new build deadline. It will// either be zero (our target), or a non-zero value that we// need to abide by anyway due to template policy.//// Previously, we would always set the deadline to zero,// which was incorrect behavior. When max_deadline is// non-zero, deadline must be set to a non-zero value that// is less than max_deadline.//// Disabling TTL autostop (at a workspace or template level)// does not trump the template's autostop requirement.//// Refer to the comments on schedule.CalculateAutostop for// more information.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 new build withttl: 0, autostop_requirement: enabled will end up with the deadlines ofdeadline: required_autostop_time, max_deadline: required_autostop_time (e.g. equal deadline and max_deadline).
So this matches that behavior.
Uh oh!
There was an error while loading.Please reload this page.
dc59885 intomainUh oh!
There was an error while loading.Please reload this page.
@deansheather, should we cherry-pick this to previous release? |
No, I don't think so |