- Notifications
You must be signed in to change notification settings - Fork947
fix: exclude prebuilt workspaces from lifecycle executor#18762
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
732e33e to328f906Compare| // Set the clock to dbtime.Now() to match the workspace build's CreatedAt | ||
| clock := quartz.NewMock(t) | ||
| clock.Set(dbtime.Now()) |
ssncferreiraJul 7, 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.
WorkspaceBuild.CreatedAt is set usingdbtime.Now()https://github.com/coder/coder/blob/main/coderd/wsbuilder/wsbuilder.go#L424, which is not mockable. Since the next autostart schedule is computed based on the workspace'sLatestBuild.CreatedAthttps://github.com/coder/coder/blob/main/coderd/autobuild/lifecycle_executor.go#L523 it's not possible to control the autostart behavior precisely in tests.
This limitation is not addressed in this PR, but to improve test reliability and determinism, we should consider injecting a clock into the workspace builder (wsbuilder) in the future, so tests can simulate specific timestamps consistently.
Uh oh!
There was an error while loading.Please reload this page.
| func setupTestDBPrebuiltWorkspace( | ||
| ctx context.Context, | ||
| t *testing.T, | ||
| clock quartz.Clock, | ||
| db database.Store, | ||
| ps pubsub.Pubsub, | ||
| orgID uuid.UUID, | ||
| templateID uuid.UUID, | ||
| templateVersionID uuid.UUID, | ||
| presetID uuid.UUID, | ||
| opts ...func(*SetupPrebuiltOptions), | ||
| ) database.WorkspaceTable { | ||
| t.Helper() | ||
| // Optional parameters | ||
| options := &SetupPrebuiltOptions{} | ||
| for _, opt := range opts { | ||
| opt(options) | ||
| } | ||
| buildTransition := database.WorkspaceTransitionStart | ||
| if options.IsStopped { | ||
| buildTransition = database.WorkspaceTransitionStop | ||
| } | ||
| workspace := dbgen.Workspace(t, db, database.WorkspaceTable{ | ||
| TemplateID: templateID, | ||
| OrganizationID: orgID, | ||
| OwnerID: database.PrebuildsSystemUserID, | ||
| Deleted: false, | ||
| CreatedAt: time.Now().Add(-time.Hour * 2), | ||
| AutostartSchedule: options.AutostartSchedule, | ||
| }) | ||
| setupTestDBWorkspaceBuild(ctx, t, clock, db, ps, orgID, workspace.ID, templateVersionID, presetID, buildTransition) | ||
| return workspace | ||
| } |
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: It would be useful to have this indbgen; I think we have this in a couple of other places.
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 definitely should 😞 I have been copy-pasting this across tests, and just tweaking the parameters that I need for the specific test, but we should move it todbgen and make it generic. I will try addressing this in a separate PR.
ThetemplateWithAgentAndPresetsWithPrebuilds function is also a good candidate to be moved somewhere for being reused. Not sure where 🤔
Uh oh!
There was an error while loading.Please reload this page.
| // If the prebuild is claimed after the scheduled deadline, | ||
| // the workspace should not stop immediately, but instead respect the next | ||
| // valid scheduled deadline (the next day). | ||
| { | ||
| name: "ClaimedAfterDeadline_SchedulesForNextDay", | ||
| isClaimedBeforeDeadline: false, | ||
| }, |
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 is areally good test to have. 👍
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 also wanted to do the same for the Autostart test, but couldn't because of the workspace build'screatedAt usingdbtime.Now() (see comment#18762 (comment))
| @@ -275,11 +275,12 @@ export interface BuildInfoResponse { | |||
| } | |||
| // From codersdk/workspacebuilds.go | |||
| export type BuildReason = "autostart" | "autostop" | "initiator"; | |||
| export type BuildReason = "autostart" | "autostop" | "dormancy" | "initiator"; | |||
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.
👀 Nice catch, didn't know this was always missing.
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.
The lifecycle executor change looks good to me 👍 I've tried my best to understand the prebuild tests but I'm not sure I'd be able to spot if there were any issues there 😄
…s-lifecycle-executor
211393a intomainUh oh!
There was an error while loading.Please reload this page.
/cherry-pick release/2.24 |
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR updates the lifecycle executor to explicitly exclude prebuilt workspaces from being considered for lifecycle operations such as
autostart,autostop,dormancy,default TTLandfailure TTL.Prebuilt workspaces (i.e., those owned by the prebuild system user) are handled separately by the prebuild reconciliation loop. Including them in the lifecycle executor could lead to unintended behavior such as incorrect scheduling or state transitions.
Changes
GetWorkspacesEligibleForTransitionto exclude workspaces withowner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'(prebuilds).Fixes:#18740
Related to:#18658