-
Notifications
You must be signed in to change notification settings - Fork 928
fix: make GetWorkspacesEligibleForTransition return less false-positives #15429
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
Changes from 1 commit
fb857e0
de6e6af
e81a985
190617c
18e9550
5aff6e3
b953902
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -580,7 +580,11 @@ WHERE | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- isEligibleForAutostop | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- A workspace may be eligible for autostop if the following are true: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The provisioner job has not failed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace is not dormant. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace build was a start transition. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace's owner is suspended OR the workspace build deadline has passed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provisioner_jobs.job_status != 'failed'::provisioner_job_status AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspaces.dormant_at IS NULL AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -592,22 +596,33 @@ WHERE | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) OR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- isEligibleForAutostart | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- A workspace may be eligible for autostart if the following are true: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace's owner is active. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The provisioner job did not failed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace build was a stop transition. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace has an autostart schedule. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
users.status = 'active'::user_status AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provisioner_jobs.job_status != 'failed'::provisioner_job_status AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspace_builds.transition = 'stop'::workspace_transition AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspaces.autostart_schedule IS NOT NULL | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) OR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- isEligibleForDormantStop | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- A workspace may be eligible for dormant stop if the following are true: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace is not dormant. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who/when marks a workspace as dormant? Is there a chance the workspace can be marked dormant and we never trigger either autostop or dormant stop? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Autobuild executor does it here: coder/coderd/autobuild/lifecycle_executor.go Lines 497 to 506 in 7b33ab0
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, that plus this seems relevant: coder/coderd/autobuild/lifecycle_executor.go Lines 421 to 428 in 7b33ab0
As does this: coder/coderd/autobuild/lifecycle_executor.go Lines 254 to 264 in 7b33ab0
So one thing that can happen at least in the code-version is that a workspace has remained in a failed stop state for a long time. Then it becomes eligible for dormant stop but since it wasn't a start, we don't try to stop it but mark it as dormant anyway. Then we'll have a workspace marked dormant that we never attempted to stop. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The template has set a time til dormant. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielleMaywood marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace has been unused for longer than the time til dormancy. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspaces.dormant_at IS NULL AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
templates.time_til_dormant > 0 AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(@now :: timestamptz) - workspaces.last_used_at > (INTERVAL '1 millisecond' * (templates.time_til_dormant / 1000000)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize this is probably already in use elsewhere but not going to lie, it's weird seeing nanos being converted into millis. I know ms is the smallest unit in pg but feels like this isn't super intuitive for humans to understand what's going on here. Obviously storing ns in the db is the main problem (and we're not fixing that here). But I'd prefer to see the conversion into seconds instead of ms as that feels like a more intuitive unit. Even the db column for Not a blocker, but we should probably do something about this at some point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every day I regret my previous decision 😓 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) OR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- isEligibleForDelete | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- A workspace may be eligible for deletion if the following are true: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace is dormant. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace is scheduled to be deleted. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * If there was a prior attempt to delete the workspace that failed: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * This attempt was at least 24 hours ago> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielleMaywood marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspaces.dormant_at IS NOT NULL AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspaces.deleting_at IS NOT NULL AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -632,11 +647,16 @@ WHERE | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
END | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) OR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- isEligibleForFailedStop | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- A workspace may be eligible for failed stop if the following are true: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The template has a failure ttl set. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The workspace build was a start transition. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we failed to autostop previously, then this limitation will prevent this case from triggering and I think we'll never retry it because of the other cases. Is that as intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be the same logic that's done later on? We're not changing any logic here, just front-loading it. coder/coderd/autobuild/lifecycle_executor.go Lines 525 to 536 in 7b33ab0
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't look at the original logic, but then I suppose I'm proposing that the original logic doesn't take this into account? Here, unless the failed build is "start", we'll never try to stop it. The failed build could just as well be a cancelled (or failed) "stop" which has left resources behind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good catch. We might want to adjust this behaviour in a separate PR. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The provisioner job failed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The provisioner job had completed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
-- * The provisioner job has been completed for longer than the failure ttl. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
templates.failure_ttl > 0 AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provisioner_jobs.job_status = 'failed'::provisioner_job_status AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workspace_builds.transition = 'start'::workspace_transition AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provisioner_jobs.job_status = 'failed'::provisioner_job_status AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provisioner_jobs.completed_at IS NOT NULL AND | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(@now :: timestamptz) - provisioner_jobs.completed_at > (INTERVAL '1 millisecond' * (templates.failure_ttl / 1000000)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.