fix: exclude prebuilt workspaces from lifecycle executor #18762
Conversation
732e33e to
328f906
Compare
|
|
||
| // Set the clock to dbtime.Now() to match the workspace build's CreatedAt | ||
| clock := quartz.NewMock(t) | ||
| clock.Set(dbtime.Now()) |
There was a problem hiding this comment.
WorkspaceBuild.CreatedAt is set using dbtime.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's LatestBuild.CreatedAt https://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.
docs/admin/templates/extending-templates/prebuilt-workspaces.md
Outdated
Show resolved
Hide resolved
| 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.
Suggestion: It would be useful to have this in dbgen; I think we have this in a couple of other places.
There was a problem hiding this comment.
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 to dbgen and make it generic. I will try addressing this in a separate PR.
The templateWithAgentAndPresetsWithPrebuilds function is also a good candidate to be moved somewhere for being reused. Not sure where 🤔
| // 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.
This is a really good test to have. 👍
There was a problem hiding this comment.
I also wanted to do the same for the Autostart test, but couldn't because of the workspace build's createdAt using dbtime.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.
👀 Nice catch, didn't know this was always missing.
…s-lifecycle-executor
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