From 8a39344950dd09979279150cdd3f034881555dc8 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 20 Jun 2023 02:17:01 +0000 Subject: [PATCH 1/9] feat: add auto-locking/deleting to autobuild --- coderd/apidoc/docs.go | 2 +- coderd/apidoc/swagger.json | 2 +- coderd/autobuild/lifecycle_executor.go | 118 +++++++- coderd/autobuild/lifecycle_executor_test.go | 60 +++- coderd/database/dbfake/dbfake.go | 23 +- coderd/database/dump.sql | 5 +- .../000132_workspace_build_reason.down.sql | 1 + .../000132_workspace_build_reason.up.sql | 5 + coderd/database/models.go | 17 +- coderd/database/queries.sql.go | 16 + coderd/database/queries/workspaces.sql | 16 + coderd/templates.go | 9 + coderd/templates_test.go | 4 +- codersdk/organizations.go | 4 +- .../coderd/coderdenttest/coderdenttest.go | 4 + enterprise/coderd/workspaces_test.go | 274 +++++++++++++++++- 16 files changed, 527 insertions(+), 33 deletions(-) create mode 100644 coderd/database/migrations/000132_workspace_build_reason.down.sql create mode 100644 coderd/database/migrations/000132_workspace_build_reason.up.sql diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index aef7e1b05f89d..ce5855dc078b1 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6772,7 +6772,7 @@ const docTemplate = `{ "type": "integer" }, "locked_ttl_ms": { - "description": "LockedTTL allows optionally specifying the max lifetime before Coder\npermanently deletes locked workspaces created from this template.", + "description": "LockedTTLMillis allows optionally specifying the max lifetime before Coder\npermanently deletes locked workspaces created from this template.", "type": "integer" }, "max_ttl_ms": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index c3570a10f9c36..24f7e13968b46 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6033,7 +6033,7 @@ "type": "integer" }, "locked_ttl_ms": { - "description": "LockedTTL allows optionally specifying the max lifetime before Coder\npermanently deletes locked workspaces created from this template.", + "description": "LockedTTLMillis allows optionally specifying the max lifetime before Coder\npermanently deletes locked workspaces created from this template.", "type": "integer" }, "max_ttl_ms": { diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index 8095666554ab6..c382f987a44cf 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -160,23 +160,65 @@ func (e *Executor) runOnce(t time.Time) Stats { return nil } - builder := wsbuilder.New(ws, nextTransition). - SetLastWorkspaceBuildInTx(&latestBuild). - SetLastWorkspaceBuildJobInTx(&latestJob). - Reason(reason) - - if _, _, err := builder.Build(e.ctx, tx, nil); err != nil { - log.Error(e.ctx, "workspace build error", - slog.F("transition", nextTransition), - slog.Error(err), + if nextTransition != "" { + builder := wsbuilder.New(ws, nextTransition). + SetLastWorkspaceBuildInTx(&latestBuild). + SetLastWorkspaceBuildJobInTx(&latestJob). + Reason(reason) + + if _, _, err := builder.Build(e.ctx, tx, nil); err != nil { + log.Error(e.ctx, "unable to transition workspace", + slog.F("transition", nextTransition), + slog.Error(err), + ) + return nil + } + } + + // Lock the workspace if it has breached the template's + // threshold for inactivity. + if reason == database.BuildReasonAutolock { + err = tx.UpdateWorkspaceLockedAt(e.ctx, database.UpdateWorkspaceLockedAtParams{ + ID: ws.ID, + LockedAt: sql.NullTime{ + Time: database.Now(), + Valid: true, + }, + }) + if err != nil { + log.Error(e.ctx, "unable to lock workspace", + slog.F("transition", nextTransition), + slog.Error(err), + ) + return nil + } + + log.Info(e.ctx, "locked workspace", + slog.F("last_used_at", ws.LastUsedAt), + slog.F("inactivity_ttl", templateSchedule.InactivityTTL), + slog.F("since_last_used_at", time.Since(ws.LastUsedAt)), + ) + } + + if reason == database.BuildReasonAutodelete { + log.Info(e.ctx, "deleted workspace", + slog.F("locked_at", ws.LockedAt.Time), + slog.F("locked_ttl", templateSchedule.LockedTTL), ) + } + + if nextTransition == "" { return nil } + statsMu.Lock() stats.Transitions[ws.ID] = nextTransition statsMu.Unlock() - log.Info(e.ctx, "scheduling workspace transition", slog.F("transition", nextTransition)) + log.Info(e.ctx, "scheduling workspace transition", + slog.F("transition", nextTransition), + slog.F("reason", reason), + ) return nil @@ -199,6 +241,12 @@ func (e *Executor) runOnce(t time.Time) Stats { return stats } +// getNextTransition returns the next eligible transition for the workspace +// as well as the reason for why it is transitioning. It is possible +// for this function to return a nil error as well as an empty transition. +// In such cases it means no provisioning should occur but the workspace +// may be "transitioning" to a new state (such as a inactive, stopped +// workspace transitioning to the locked state). func getNextTransition( ws database.Workspace, latestBuild database.WorkspaceBuild, @@ -211,12 +259,23 @@ func getNextTransition( error, ) { switch { - case isEligibleForAutostop(latestBuild, latestJob, currentTick): + case isEligibleForAutostop(ws, latestBuild, latestJob, currentTick): return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil case isEligibleForAutostart(ws, latestBuild, latestJob, templateSchedule, currentTick): return database.WorkspaceTransitionStart, database.BuildReasonAutostart, nil case isEligibleForFailedStop(latestBuild, latestJob, templateSchedule): return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil + case isEligibleForLockedStop(ws, templateSchedule): + // Only stop started workspaces. + if latestBuild.Transition == database.WorkspaceTransitionStart { + return database.WorkspaceTransitionStop, database.BuildReasonAutolock, nil + } + // We shouldn't transition the workspace but we should still + // lock it. + return "", database.BuildReasonAutolock, nil + + case isEligibleForDelete(ws, templateSchedule): + return database.WorkspaceTransitionDelete, database.BuildReasonAutodelete, nil default: return "", "", xerrors.Errorf("last transition not valid for autostart or autostop") } @@ -225,7 +284,12 @@ func getNextTransition( // isEligibleForAutostart returns true if the workspace should be autostarted. func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { // Don't attempt to autostart failed workspaces. - if !job.CompletedAt.Valid || job.Error.String != "" { + if db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed { + return false + } + + // If the workspace is locked we should not autostart it. + if ws.LockedAt.Valid { return false } @@ -253,9 +317,13 @@ func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild } // isEligibleForAutostart returns true if the workspace should be autostopped. -func isEligibleForAutostop(build database.WorkspaceBuild, job database.ProvisionerJob, currentTick time.Time) bool { - // Don't attempt to autostop failed workspaces. - if !job.CompletedAt.Valid || job.Error.String != "" { +func isEligibleForAutostop(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, currentTick time.Time) bool { + if db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed { + return false + } + + // If the workspace is locked we should not autostop it. + if ws.LockedAt.Valid { return false } @@ -266,6 +334,26 @@ func isEligibleForAutostop(build database.WorkspaceBuild, job database.Provision !currentTick.Before(build.Deadline) } +// isEligibleForLockedStop returns true if the workspace should be locked +// for breaching the inactivity threshold of the template. +func isEligibleForLockedStop(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions) bool { + // Only attempt to lock workspaces not already locked. + return !ws.LockedAt.Valid && + // The template must specify an inactivity TTL. + templateSchedule.InactivityTTL > 0 && + // The workspace must breach the inactivity TTL. + database.Now().Sub(ws.LastUsedAt) > templateSchedule.InactivityTTL +} + +func isEligibleForDelete(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions) bool { + // Only attempt to delete locked workspaces. + return ws.LockedAt.Valid && + // Locked workspaces should only be deleted if a locked_ttl is specified. + templateSchedule.LockedTTL > 0 && + // The workspace must breach the locked_ttl. + database.Now().Sub(ws.LockedAt.Time) > templateSchedule.LockedTTL +} + // isEligibleForFailedStop returns true if the workspace is eligible to be stopped // due to a failed build. func isEligibleForFailedStop(build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions) bool { diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index 3da342066ad12..56fd5aeb0b1d1 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -651,8 +651,9 @@ func TestExecutorAutostartTemplateDisabled(t *testing.T) { assert.Len(t, stats.Transitions, 0) } -// TesetExecutorFailedWorkspace tests that failed workspaces that breach -// their template failed_ttl threshold trigger a stop job. +// TestExecutorFailedWorkspace test AGPL functionality which mainly +// ensures that autostop actions as a result of a failed workspace +// build do not trigger. // For enterprise functionality see enterprise/coderd/workspaces_test.go func TestExecutorFailedWorkspace(t *testing.T) { t.Parallel() @@ -705,6 +706,61 @@ func TestExecutorFailedWorkspace(t *testing.T) { }) } +// TestExecutorInactiveWorkspace test AGPL functionality which mainly +// ensures that autostop actions as a result of an inactive workspace +// do not trigger. +// For enterprise functionality see enterprise/coderd/workspaces_test.go +func TestExecutorInactiveWorkspace(t *testing.T) { + t.Parallel() + + // Test that an AGPL TemplateScheduleStore properly disables + // functionality. + t.Run("OK", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + logger = slogtest.Make(t, &slogtest.Options{ + // We ignore errors here since we expect to fail + // builds. + IgnoreErrors: true, + }) + inactiveTTL = time.Millisecond + + client = coderdtest.New(t, &coderdtest.Options{ + Logger: &logger, + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: schedule.NewAGPLTemplateScheduleStore(), + }) + ) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > inactiveTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() + stats := <-statCh + // Expect no transitions since we're using AGPL. + require.Len(t, stats.Transitions, 0) + }) +} + func mustProvisionWorkspace(t *testing.T, client *codersdk.Client, mut ...func(*codersdk.CreateWorkspaceRequest)) codersdk.Workspace { t.Helper() user := coderdtest.CreateFirstUser(t, client) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 50195b5d77306..80ba98335527a 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -3482,12 +3482,17 @@ func (q *fakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, no return nil, err } - if build.Transition == database.WorkspaceTransitionStart && !build.Deadline.IsZero() && build.Deadline.Before(now) { + if build.Transition == database.WorkspaceTransitionStart && + !build.Deadline.IsZero() && + build.Deadline.Before(now) && + !workspace.LockedAt.Valid { workspaces = append(workspaces, workspace) continue } - if build.Transition == database.WorkspaceTransitionStop && workspace.AutostartSchedule.Valid { + if build.Transition == database.WorkspaceTransitionStop && + workspace.AutostartSchedule.Valid && + !workspace.LockedAt.Valid { workspaces = append(workspaces, workspace) continue } @@ -3500,6 +3505,19 @@ func (q *fakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, no workspaces = append(workspaces, workspace) continue } + + template, err := q.GetTemplateByID(ctx, workspace.TemplateID) + if err != nil { + return nil, xerrors.Errorf("get template by ID: %w", err) + } + if !workspace.LockedAt.Valid && template.InactivityTTL > 0 { + workspaces = append(workspaces, workspace) + continue + } + if workspace.LockedAt.Valid && template.LockedTTL > 0 { + workspaces = append(workspaces, workspace) + continue + } } return workspaces, nil @@ -4688,6 +4706,7 @@ func (q *fakeQuerier) UpdateTemplateScheduleByID(_ context.Context, arg database tpl.MaxTTL = arg.MaxTTL tpl.FailureTTL = arg.FailureTTL tpl.InactivityTTL = arg.InactivityTTL + tpl.LockedTTL = arg.LockedTTL q.templates[idx] = tpl return tpl.DeepCopy(), nil } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 9ded04e035ddf..5ce9682e93e5a 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -25,7 +25,10 @@ CREATE TYPE audit_action AS ENUM ( CREATE TYPE build_reason AS ENUM ( 'initiator', 'autostart', - 'autostop' + 'autostop', + 'autolock', + 'failedstop', + 'autodelete' ); CREATE TYPE log_level AS ENUM ( diff --git a/coderd/database/migrations/000132_workspace_build_reason.down.sql b/coderd/database/migrations/000132_workspace_build_reason.down.sql new file mode 100644 index 0000000000000..383c118f65bef --- /dev/null +++ b/coderd/database/migrations/000132_workspace_build_reason.down.sql @@ -0,0 +1 @@ +-- It's not possible to delete enum values. diff --git a/coderd/database/migrations/000132_workspace_build_reason.up.sql b/coderd/database/migrations/000132_workspace_build_reason.up.sql new file mode 100644 index 0000000000000..ae9d30fae9861 --- /dev/null +++ b/coderd/database/migrations/000132_workspace_build_reason.up.sql @@ -0,0 +1,5 @@ +BEGIN; +ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'autolock'; +ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'failedstop'; +ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'autodelete'; +COMMIT; diff --git a/coderd/database/models.go b/coderd/database/models.go index fb25ec7b9dc8a..a8c199c1572c4 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -214,9 +214,12 @@ func AllAuditActionValues() []AuditAction { type BuildReason string const ( - BuildReasonInitiator BuildReason = "initiator" - BuildReasonAutostart BuildReason = "autostart" - BuildReasonAutostop BuildReason = "autostop" + BuildReasonInitiator BuildReason = "initiator" + BuildReasonAutostart BuildReason = "autostart" + BuildReasonAutostop BuildReason = "autostop" + BuildReasonAutolock BuildReason = "autolock" + BuildReasonFailedstop BuildReason = "failedstop" + BuildReasonAutodelete BuildReason = "autodelete" ) func (e *BuildReason) Scan(src interface{}) error { @@ -258,7 +261,10 @@ func (e BuildReason) Valid() bool { switch e { case BuildReasonInitiator, BuildReasonAutostart, - BuildReasonAutostop: + BuildReasonAutostop, + BuildReasonAutolock, + BuildReasonFailedstop, + BuildReasonAutodelete: return true } return false @@ -269,6 +275,9 @@ func AllBuildReasonValues() []BuildReason { BuildReasonInitiator, BuildReasonAutostart, BuildReasonAutostop, + BuildReasonAutolock, + BuildReasonFailedstop, + BuildReasonAutodelete, } } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a4b1ed784f174..88e8acc2fd204 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8595,6 +8595,8 @@ LEFT JOIN workspace_builds ON workspace_builds.workspace_id = workspaces.id INNER JOIN provisioner_jobs ON workspace_builds.job_id = provisioner_jobs.id +INNER JOIN + templates ON workspaces.template_id = templates.id WHERE workspace_builds.build_number = ( SELECT @@ -8632,6 +8634,20 @@ WHERE provisioner_jobs.error IS NOT NULL AND provisioner_jobs.error != '' AND workspace_builds.transition = 'start'::workspace_transition + ) OR + + -- If the workspace's template has an inactivity_ttl set + -- it may be eligible for locking. + ( + templates.inactivity_ttl > 0 AND + workspaces.locked_at IS NULL + ) OR + + -- If the workspace's template has a locked_ttl set + -- and the workspace is already locked + ( + templates.locked_ttl > 0 AND + workspaces.locked_at IS NOT NULL ) ) AND workspaces.deleted = 'false' ` diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index f90b66055a2f4..f3eef3a283d8d 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -414,6 +414,8 @@ LEFT JOIN workspace_builds ON workspace_builds.workspace_id = workspaces.id INNER JOIN provisioner_jobs ON workspace_builds.job_id = provisioner_jobs.id +INNER JOIN + templates ON workspaces.template_id = templates.id WHERE workspace_builds.build_number = ( SELECT @@ -451,6 +453,20 @@ WHERE provisioner_jobs.error IS NOT NULL AND provisioner_jobs.error != '' AND workspace_builds.transition = 'start'::workspace_transition + ) OR + + -- If the workspace's template has an inactivity_ttl set + -- it may be eligible for locking. + ( + templates.inactivity_ttl > 0 AND + workspaces.locked_at IS NULL + ) OR + + -- If the workspace's template has a locked_ttl set + -- and the workspace is already locked + ( + templates.locked_ttl > 0 AND + workspaces.locked_at IS NOT NULL ) ) AND workspaces.deleted = 'false'; diff --git a/coderd/templates.go b/coderd/templates.go index c8a06f3b88a8f..807ad22a9b568 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -227,6 +227,7 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque maxTTL time.Duration failureTTL time.Duration inactivityTTL time.Duration + lockedTTL time.Duration ) if createTemplate.DefaultTTLMillis != nil { defaultTTL = time.Duration(*createTemplate.DefaultTTLMillis) * time.Millisecond @@ -240,6 +241,9 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque if createTemplate.InactivityTTLMillis != nil { inactivityTTL = time.Duration(*createTemplate.InactivityTTLMillis) * time.Millisecond } + if createTemplate.LockedTTLMillis != nil { + lockedTTL = time.Duration(*createTemplate.LockedTTLMillis) * time.Millisecond + } var validErrs []codersdk.ValidationError if defaultTTL < 0 { @@ -257,6 +261,10 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque if inactivityTTL < 0 { validErrs = append(validErrs, codersdk.ValidationError{Field: "inactivity_ttl_ms", Detail: "Must be a positive integer."}) } + if lockedTTL < 0 { + validErrs = append(validErrs, codersdk.ValidationError{Field: "locked_ttl_ms", Detail: "Must be a positive integer."}) + } + if len(validErrs) > 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid create template request.", @@ -312,6 +320,7 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque MaxTTL: maxTTL, FailureTTL: failureTTL, InactivityTTL: inactivityTTL, + LockedTTL: lockedTTL, }) if err != nil { return xerrors.Errorf("set template schedule options: %s", err) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index e476519f55d2e..4bd65387dbfd1 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -662,7 +662,7 @@ func TestPatchTemplateMeta(t *testing.T) { template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { ctr.FailureTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) ctr.InactivityTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) - ctr.LockedTTL = ptr.Ref(0 * time.Hour.Milliseconds()) + ctr.LockedTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) }) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) @@ -697,7 +697,7 @@ func TestPatchTemplateMeta(t *testing.T) { template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { ctr.FailureTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) ctr.InactivityTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) - ctr.LockedTTL = ptr.Ref(0 * time.Hour.Milliseconds()) + ctr.LockedTTLMillis = ptr.Ref(0 * time.Hour.Milliseconds()) }) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) diff --git a/codersdk/organizations.go b/codersdk/organizations.go index bcb360132b8b9..206dd12c0665f 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -108,9 +108,9 @@ type CreateTemplateRequest struct { // InactivityTTLMillis allows optionally specifying the max lifetime before Coder // locks inactive workspaces created from this template. InactivityTTLMillis *int64 `json:"inactivity_ttl_ms,omitempty"` - // LockedTTL allows optionally specifying the max lifetime before Coder + // LockedTTLMillis allows optionally specifying the max lifetime before Coder // permanently deletes locked workspaces created from this template. - LockedTTL *int64 `json:"locked_ttl_ms,omitempty"` + LockedTTLMillis *int64 `json:"locked_ttl_ms,omitempty"` // DisableEveryoneGroupAccess allows optionally disabling the default // behavior of granting the 'everyone' group access to use the template. diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 06758d013ded8..116ff7584e8d7 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -112,6 +112,10 @@ type LicenseOptions struct { Features license.Features } +func AddFullLicense(t *testing.T, client *codersdk.Client) codersdk.License { + return AddLicense(t, client, LicenseOptions{AllFeatures: true}) +} + // AddLicense generates a new license with the options provided and inserts it. func AddLicense(t *testing.T, client *codersdk.Client, options LicenseOptions) codersdk.License { l, err := client.AddLicense(context.Background(), codersdk.AddLicenseRequest{ diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 770f3ace3fe3b..8301fd85f7c33 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -184,7 +184,9 @@ func TestWorkspaceAutobuild(t *testing.T) { require.Len(t, stats.Transitions, 0) }) - t.Run("FailureTTLUnset", func(t *testing.T) { + // This just provides a baseline that no actions are being taken + // against a workspace when none of the TTL fields are set. + t.Run("TemplateTTLsUnset", func(t *testing.T) { t.Parallel() var ( @@ -215,19 +217,285 @@ func TestWorkspaceAutobuild(t *testing.T) { version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, - ProvisionApply: echo.ProvisionFailed, + ProvisionApply: echo.ProvisionComplete, }) // Create a template without setting a failure_ttl. template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) coderdtest.AwaitTemplateVersionJob(t, client, version.ID) ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) - require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) ticker <- time.Now() stats := <-statCh // Expect no transitions since the field is unset on the template. require.Len(t, stats.Transitions, 0) }) + + t.Run("InactiveTTLOK", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + inactiveTTL = time.Millisecond + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddFullLicense(t, client) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > inactiveTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() + stats := <-statCh + // Expect workspace to transition to stopped state for breaching + // failure TTL. + require.Len(t, stats.Transitions, 1) + require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionStop) + ws = coderdtest.MustWorkspace(t, client, ws.ID) + // The workspace should be locked. + require.NotNil(t, ws.LockedAt) + }) + + t.Run("InactiveTTLTooEarly", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + inactiveTTL = time.Minute + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAdvancedTemplateScheduling: 1, + }, + }) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + ticker <- time.Now() + stats := <-statCh + // Expect no transitions since not enough time has elapsed. + require.Len(t, stats.Transitions, 0) + }) + + t.Run("UnlockedWorkspacesNotDeleted", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + lockedTTL = time.Millisecond + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAdvancedTemplateScheduling: 1, + }, + }) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.LockedTTLMillis = ptr.Ref[int64](lockedTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > lockedTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + + ticker <- time.Now() + stats := <-statCh + // Expect no transitions since workspace is unlocked. + require.Len(t, stats.Transitions, 0) + }) + + t.Run("InactiveStoppedWorkspaceNoTransition", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + inactiveTTL = time.Millisecond + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAdvancedTemplateScheduling: 1, + }, + }) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + + // Stop the workspace so we can assert autobuild does nothing + // if we breach our inactivity threshold. + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Wait to breach our threshold. + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > inactiveTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + + ticker <- time.Now() + stats := <-statCh + // Expect no transitions since workspace is stopped. + require.Len(t, stats.Transitions, 0) + ws = coderdtest.MustWorkspace(t, client, ws.ID) + // The workspace should still be locked even though we didn't + // transition the workspace. + require.NotNil(t, ws.LockedAt) + }) + + // Test the flow of a workspace transitioning from + // inactive -> locked -> deleted. + t.Run("WorkspaceInactiveDeleteTransition", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + transitionTTL = time.Millisecond + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddFullLicense(t, client) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.InactivityTTLMillis = ptr.Ref[int64](transitionTTL.Milliseconds()) + ctr.LockedTTLMillis = ptr.Ref[int64](transitionTTL.Milliseconds()) + }) + + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > transitionTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() + stats := <-statCh + // Expect workspace to transition to stopped state for breaching + // inactive TTL. + require.Len(t, stats.Transitions, 1) + require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionStop) + ws = coderdtest.MustWorkspace(t, client, ws.ID) + // The workspace should be locked. + require.NotNil(t, ws.LockedAt) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + // Wait for the workspace to breach the locked threshold. + require.Eventually(t, + func() bool { + return database.Now().Sub(*ws.LockedAt) > transitionTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() + stats = <-statCh + require.Len(t, stats.Transitions, 1) + // The workspace should be scheduled for deletion. + require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionDelete) + + ws = coderdtest.MustWorkspace(t, client, ws.ID) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + + _, err := client.Workspace(testutil.Context(t, testutil.WaitShort), ws.ID) + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusGone, cerr.StatusCode()) + }) } func TestWorkspacesFiltering(t *testing.T) { From 0ce8aa3748290bd822c51468e5826772788d798b Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Wed, 28 Jun 2023 22:33:40 +0000 Subject: [PATCH 2/9] add some tests/fix some minor bugs --- coderd/autobuild/lifecycle_executor.go | 2 +- coderd/database/dbfake/dbfake.go | 1 + coderd/database/queries.sql.go | 3 +- coderd/database/queries/workspaces.sql | 3 +- coderd/workspaces_test.go | 4 +- enterprise/coderd/workspaces_test.go | 93 ++++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 4 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index c382f987a44cf..eecc602de9f88 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -245,7 +245,7 @@ func (e *Executor) runOnce(t time.Time) Stats { // as well as the reason for why it is transitioning. It is possible // for this function to return a nil error as well as an empty transition. // In such cases it means no provisioning should occur but the workspace -// may be "transitioning" to a new state (such as a inactive, stopped +// may be "transitioning" to a new state (such as an inactive, stopped // workspace transitioning to the locked state). func getNextTransition( ws database.Workspace, diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 80ba98335527a..4eea11c975f73 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -5229,6 +5229,7 @@ func (q *fakeQuerier) UpdateWorkspaceLockedAt(_ context.Context, arg database.Up continue } workspace.LockedAt = arg.LockedAt + workspace.LastUsedAt = database.Now() q.workspaces[index] = workspace return nil } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 88e8acc2fd204..2cda2e1beb7e7 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8847,7 +8847,8 @@ const updateWorkspaceLockedAt = `-- name: UpdateWorkspaceLockedAt :exec UPDATE workspaces SET - locked_at = $2 + locked_at = $2, + last_used_at = now() at time zone 'utc' WHERE id = $1 ` diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index f3eef3a283d8d..523592e4257c5 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -474,6 +474,7 @@ WHERE UPDATE workspaces SET - locked_at = $2 + locked_at = $2, + last_used_at = now() at time zone 'utc' WHERE id = $1; diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 0df6a05808c99..fdaed96137435 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -2399,11 +2399,12 @@ func TestWorkspaceLock(t *testing.T) { }) require.NoError(t, err) - workspace, err = client.Workspace(ctx, workspace.ID) + workspace = coderdtest.MustWorkspace(t, client, workspace.ID) require.NoError(t, err, "fetch provisioned workspace") require.NotNil(t, workspace.LockedAt) require.WithinRange(t, *workspace.LockedAt, time.Now().Add(-time.Second*10), time.Now()) + lastUsedAt := workspace.LastUsedAt err = client.UpdateWorkspaceLock(ctx, workspace.ID, codersdk.UpdateWorkspaceLock{ Lock: false, }) @@ -2412,6 +2413,7 @@ func TestWorkspaceLock(t *testing.T) { workspace, err = client.Workspace(ctx, workspace.ID) require.NoError(t, err, "fetch provisioned workspace") require.Nil(t, workspace.LockedAt) + require.True(t, workspace.LastUsedAt.After(lastUsedAt)) }) t.Run("CannotStart", func(t *testing.T) { diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 8301fd85f7c33..be46295531e5a 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/coderd/autobuild" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/schedule" "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" "github.com/coder/coder/enterprise/coderd" @@ -235,6 +236,7 @@ func TestWorkspaceAutobuild(t *testing.T) { t.Parallel() var ( + ctx = testutil.Context(t, testutil.WaitMedium) ticker = make(chan time.Time) statCh = make(chan autobuild.Stats) inactiveTTL = time.Millisecond @@ -277,6 +279,15 @@ func TestWorkspaceAutobuild(t *testing.T) { ws = coderdtest.MustWorkspace(t, client, ws.ID) // The workspace should be locked. require.NotNil(t, ws.LockedAt) + lastUsedAt := ws.LastUsedAt + + err := client.UpdateWorkspaceLock(ctx, ws.ID, codersdk.UpdateWorkspaceLock{Lock: false}) + require.NoError(t, err) + + // Assert that we updated our last_used_at so that we don't immediately + // retrigger another lock action. + ws = coderdtest.MustWorkspace(t, client, ws.ID) + require.True(t, ws.LastUsedAt.After(lastUsedAt)) }) t.Run("InactiveTTLTooEarly", func(t *testing.T) { @@ -496,6 +507,88 @@ func TestWorkspaceAutobuild(t *testing.T) { require.True(t, ok) require.Equal(t, http.StatusGone, cerr.StatusCode()) }) + + t.Run("LockedNoAutostart", func(t *testing.T) { + t.Parallel() + + var ( + ctx = testutil.Context(t, testutil.WaitMedium) + tickCh = make(chan time.Time) + statsCh = make(chan autobuild.Stats) + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: tickCh, + IncludeProvisionerDaemon: true, + AutobuildStats: statsCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + inactiveTTL = time.Millisecond + ) + + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddFullLicense(t, client) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + sched, err := schedule.Weekly("CRON_TZ=UTC 0 * * * *") + require.NoError(t, err) + + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.AutostartSchedule = ptr.Ref(sched.String()) + }) + coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Assert that autostart works when the workspace isn't locked.. + tickCh <- sched.Next(ws.LatestBuild.CreatedAt) + // Then: the workspace should eventually be started + stats := <-statsCh + require.NoError(t, stats.Error) + require.Len(t, stats.Transitions, 1) + require.Contains(t, stats.Transitions, ws.ID) + require.Equal(t, database.WorkspaceTransitionStart, stats.Transitions[ws.ID]) + ws = coderdtest.MustWorkspace(t, client, ws.ID) + coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + + // Now that we've validated that the workspace is eligible for autostart + // lets cause it to become locked. + _, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + InactivityTTLMillis: inactiveTTL.Milliseconds(), + }) + require.NoError(t, err) + // Wait for the workspace to breach the inactivity threshold. + require.Eventually(t, + func() bool { + return database.Now().Sub(ws.LastUsedAt) > inactiveTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + + tickCh <- time.Now() + // Then: the workspace should eventually be started + stats = <-statsCh + require.NoError(t, stats.Error) + require.Len(t, stats.Transitions, 1) + require.Contains(t, stats.Transitions, ws.ID) + require.Equal(t, database.WorkspaceTransitionStop, stats.Transitions[ws.ID]) + ws = coderdtest.MustWorkspace(t, client, ws.ID) + coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + // The workspace should be locked now. + require.NotNil(t, ws.LockedAt) + + // Assert that autostart is no longer triggered since workspace is locked. + tickCh <- sched.Next(ws.LatestBuild.CreatedAt) + // Then: the workspace should eventually be started + stats = <-statsCh + require.Len(t, stats.Transitions, 0) + }) } func TestWorkspacesFiltering(t *testing.T) { From 62357a70eb96176b67bccc3c4cc908da6383bc07 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 29 Jun 2023 04:01:31 +0000 Subject: [PATCH 3/9] fix an issue in updating templates --- coderd/templates.go | 2 +- .../coderd/coderdenttest/coderdenttest.go | 1 + enterprise/coderd/templates_test.go | 49 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/coderd/templates.go b/coderd/templates.go index 807ad22a9b568..b2cfb4bf3c229 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -534,7 +534,7 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { req.MaxTTLMillis == time.Duration(template.MaxTTL).Milliseconds() && req.FailureTTLMillis == time.Duration(template.FailureTTL).Milliseconds() && req.InactivityTTLMillis == time.Duration(template.InactivityTTL).Milliseconds() && - req.FailureTTLMillis == time.Duration(template.LockedTTL).Milliseconds() { + req.LockedTTLMillis == time.Duration(template.LockedTTL).Milliseconds() { return nil } diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 116ff7584e8d7..7c34e8ef2066d 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -112,6 +112,7 @@ type LicenseOptions struct { Features license.Features } +// AddFullLicense generates a license with all features enabled. func AddFullLicense(t *testing.T, client *codersdk.Client) codersdk.License { return AddLicense(t, client, LicenseOptions{AllFeatures: true}) } diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index ca63cbc0801d1..fb0730191135f 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -206,6 +206,55 @@ func TestTemplates(t *testing.T) { require.NoError(t, err) require.EqualValues(t, exp, *ws.TTLMillis) }) + + t.Run("CleanupTTLs", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + client := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + }) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddFullLicense(t, client) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + require.EqualValues(t, 0, template.InactivityTTLMillis) + require.EqualValues(t, 0, template.FailureTTLMillis) + require.EqualValues(t, 0, template.LockedTTLMillis) + + var ( + failureTTL int64 = 1 + inactivityTTL int64 = 2 + lockedTTL int64 = 3 + ) + + updated, err := client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + Name: template.Name, + DisplayName: template.DisplayName, + Description: template.Description, + Icon: template.Icon, + AllowUserCancelWorkspaceJobs: template.AllowUserCancelWorkspaceJobs, + InactivityTTLMillis: inactivityTTL, + FailureTTLMillis: failureTTL, + LockedTTLMillis: lockedTTL, + }) + require.NoError(t, err) + require.Equal(t, failureTTL, updated.FailureTTLMillis) + require.Equal(t, inactivityTTL, updated.InactivityTTLMillis) + require.Equal(t, lockedTTL, updated.LockedTTLMillis) + + // Validate fetching the template returns the same values as updating + // the template. + template, err = client.Template(ctx, template.ID) + require.NoError(t, err) + require.Equal(t, failureTTL, updated.FailureTTLMillis) + require.Equal(t, inactivityTTL, updated.InactivityTTLMillis) + require.Equal(t, lockedTTL, updated.LockedTTLMillis) + }) } func TestTemplateACL(t *testing.T) { From 416d39eefec5537afb60fa6a74719689897ff9bb Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 29 Jun 2023 04:32:33 +0000 Subject: [PATCH 4/9] add a test for locking --- enterprise/coderd/workspaces_test.go | 72 ++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index be46295531e5a..c05f736d01036 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -508,6 +508,78 @@ func TestWorkspaceAutobuild(t *testing.T) { require.Equal(t, http.StatusGone, cerr.StatusCode()) }) + t.Run("LockedTTTooEarly", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + lockedTTL = time.Minute + + client = coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, + }, + }) + ) + user := coderdtest.CreateFirstUser(t, client) + _ = coderdenttest.AddFullLicense(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.ProvisionComplete, + ProvisionApply: echo.ProvisionComplete, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.LockedTTLMillis = ptr.Ref[int64](lockedTTL.Milliseconds()) + }) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + + ctx := testutil.Context(t, testutil.WaitMedium) + err := client.UpdateWorkspaceLock(ctx, ws.ID, codersdk.UpdateWorkspaceLock{ + Lock: true, + }) + require.NoError(t, err) + + ws = coderdtest.MustWorkspace(t, client, ws.ID) + require.NotNil(t, ws.LockedAt) + + ticker <- time.Now() + stats := <-statCh + // Expect no transitions since not enough time has elapsed. + require.Len(t, stats.Transitions, 0) + + // Update the lock time to trigger a deletion. + lockedTTL = time.Millisecond + _, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + Name: template.Name, + DisplayName: template.DisplayName, + Description: template.Description, + Icon: template.Icon, + AllowUserCancelWorkspaceJobs: template.AllowUserCancelWorkspaceJobs, + LockedTTLMillis: lockedTTL.Milliseconds(), + }) + require.NoError(t, err) + + // Wait for the workspace to breach the locked threshold. + require.Eventually(t, + func() bool { + return database.Now().Sub(*ws.LockedAt) > lockedTTL + }, + testutil.IntervalMedium, testutil.IntervalFast) + + ticker <- time.Now() + stats = <-statCh + // Expect no transitions since not enough time has elapsed. + require.Len(t, stats.Transitions, 1) + require.Equal(t, database.WorkspaceTransitionDelete, stats.Transitions[ws.ID]) + }) + t.Run("LockedNoAutostart", func(t *testing.T) { t.Parallel() From 4b29a8010c2297ef9d51129b46527b006ff2aff6 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 29 Jun 2023 04:54:00 +0000 Subject: [PATCH 5/9] fix some grammar --- coderd/database/queries/workspaces.sql | 10 ++-- enterprise/coderd/workspaces_test.go | 70 +++++++++++++++----------- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 523592e4257c5..5bcfa02bf17c9 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -415,7 +415,7 @@ LEFT JOIN INNER JOIN provisioner_jobs ON workspace_builds.job_id = provisioner_jobs.id INNER JOIN - templates ON workspaces.template_id = templates.id + templates ON workspaces.template_id = templates.id WHERE workspace_builds.build_number = ( SELECT @@ -458,15 +458,15 @@ WHERE -- If the workspace's template has an inactivity_ttl set -- it may be eligible for locking. ( - templates.inactivity_ttl > 0 AND - workspaces.locked_at IS NULL + templates.inactivity_ttl > 0 AND + workspaces.locked_at IS NULL ) OR -- If the workspace's template has a locked_ttl set -- and the workspace is already locked ( - templates.locked_ttl > 0 AND - workspaces.locked_at IS NOT NULL + templates.locked_ttl > 0 AND + workspaces.locked_at IS NOT NULL ) ) AND workspaces.deleted = 'false'; diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index c05f736d01036..9d310058ad599 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -210,11 +210,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, @@ -222,13 +218,17 @@ func TestWorkspaceAutobuild(t *testing.T) { }) // Create a template without setting a failure_ttl. template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + require.Zero(t, template.InactivityTTLMillis) + require.Zero(t, template.FailureTTLMillis) + require.Zero(t, template.LockedTTLMillis) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) ticker <- time.Now() stats := <-statCh - // Expect no transitions since the field is unset on the template. + // Expect no transitions since the fields are unset on the template. require.Len(t, stats.Transitions, 0) }) @@ -258,13 +258,18 @@ func TestWorkspaceAutobuild(t *testing.T) { ProvisionPlan: echo.ProvisionComplete, ProvisionApply: echo.ProvisionComplete, }) + // Create a template with a short inactivity ttl so we don't have to wait + // too long. template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) }) coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + + // Wait to trigger the inactivity threshold. require.Eventually(t, func() bool { return database.Now().Sub(ws.LastUsedAt) > inactiveTTL @@ -272,12 +277,14 @@ func TestWorkspaceAutobuild(t *testing.T) { testutil.IntervalMedium, testutil.IntervalFast) ticker <- time.Now() stats := <-statCh + // Expect workspace to transition to stopped state for breaching // failure TTL. require.Len(t, stats.Transitions, 1) require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionStop) - ws = coderdtest.MustWorkspace(t, client, ws.ID) + // The workspace should be locked. + ws = coderdtest.MustWorkspace(t, client, ws.ID) require.NotNil(t, ws.LockedAt) lastUsedAt := ws.LastUsedAt @@ -308,11 +315,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, @@ -331,6 +334,9 @@ func TestWorkspaceAutobuild(t *testing.T) { require.Len(t, stats.Transitions, 0) }) + // This is kind of a dumb test but it exists to offer some marginal + // confidence that bug in the auto-deletion logic doesn't delete running + // workspaces. t.Run("UnlockedWorkspacesNotDeleted", func(t *testing.T) { t.Parallel() @@ -349,11 +355,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, @@ -365,6 +367,7 @@ func TestWorkspaceAutobuild(t *testing.T) { coderdtest.AwaitTemplateVersionJob(t, client, version.ID) ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + require.Nil(t, ws.LockedAt) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) require.Eventually(t, func() bool { @@ -378,6 +381,9 @@ func TestWorkspaceAutobuild(t *testing.T) { require.Len(t, stats.Transitions, 0) }) + // Assert that a stopped workspace that breaches the inactivity threshold + // does not trigger a build transition but is still placed in the + // lock state. t.Run("InactiveStoppedWorkspaceNoTransition", func(t *testing.T) { t.Parallel() @@ -396,11 +402,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, @@ -410,6 +412,7 @@ func TestWorkspaceAutobuild(t *testing.T) { ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) }) coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) @@ -466,41 +469,50 @@ func TestWorkspaceAutobuild(t *testing.T) { ctr.InactivityTTLMillis = ptr.Ref[int64](transitionTTL.Milliseconds()) ctr.LockedTTLMillis = ptr.Ref[int64](transitionTTL.Milliseconds()) }) - coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + require.Eventually(t, func() bool { return database.Now().Sub(ws.LastUsedAt) > transitionTTL }, testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() stats := <-statCh // Expect workspace to transition to stopped state for breaching // inactive TTL. require.Len(t, stats.Transitions, 1) require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionStop) + ws = coderdtest.MustWorkspace(t, client, ws.ID) // The workspace should be locked. require.NotNil(t, ws.LockedAt) + + // Wait for the autobuilder to stop the workspace. _ = coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) - // Wait for the workspace to breach the locked threshold. + + // Wait to trigger our locked threshold. require.Eventually(t, func() bool { return database.Now().Sub(*ws.LockedAt) > transitionTTL }, testutil.IntervalMedium, testutil.IntervalFast) + ticker <- time.Now() stats = <-statCh require.Len(t, stats.Transitions, 1) // The workspace should be scheduled for deletion. require.Equal(t, stats.Transitions[ws.ID], database.WorkspaceTransitionDelete) + // Wait for the workspace to be deleted. ws = coderdtest.MustWorkspace(t, client, ws.ID) _ = coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + // Assert that the workspace is actually deleted. _, err := client.Workspace(testutil.Context(t, testutil.WaitShort), ws.ID) require.Error(t, err) cerr, ok := codersdk.AsError(err) @@ -575,11 +587,11 @@ func TestWorkspaceAutobuild(t *testing.T) { ticker <- time.Now() stats = <-statCh - // Expect no transitions since not enough time has elapsed. require.Len(t, stats.Transitions, 1) require.Equal(t, database.WorkspaceTransitionDelete, stats.Transitions[ws.ID]) }) + // Assert that a locked workspace does not autostart. t.Run("LockedNoAutostart", func(t *testing.T) { t.Parallel() @@ -621,12 +633,12 @@ func TestWorkspaceAutobuild(t *testing.T) { // Assert that autostart works when the workspace isn't locked.. tickCh <- sched.Next(ws.LatestBuild.CreatedAt) - // Then: the workspace should eventually be started stats := <-statsCh require.NoError(t, stats.Error) require.Len(t, stats.Transitions, 1) require.Contains(t, stats.Transitions, ws.ID) require.Equal(t, database.WorkspaceTransitionStart, stats.Transitions[ws.ID]) + ws = coderdtest.MustWorkspace(t, client, ws.ID) coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) @@ -643,21 +655,21 @@ func TestWorkspaceAutobuild(t *testing.T) { }, testutil.IntervalMedium, testutil.IntervalFast) + // We should see the workspace get stopped now. tickCh <- time.Now() - // Then: the workspace should eventually be started stats = <-statsCh require.NoError(t, stats.Error) require.Len(t, stats.Transitions, 1) require.Contains(t, stats.Transitions, ws.ID) require.Equal(t, database.WorkspaceTransitionStop, stats.Transitions[ws.ID]) + + // The workspace should be locked now. ws = coderdtest.MustWorkspace(t, client, ws.ID) coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) - // The workspace should be locked now. require.NotNil(t, ws.LockedAt) // Assert that autostart is no longer triggered since workspace is locked. tickCh <- sched.Next(ws.LatestBuild.CreatedAt) - // Then: the workspace should eventually be started stats = <-statsCh require.Len(t, stats.Transitions, 0) }) From 52214bccec6de9809dd5c734ab9f63f4748bb8b8 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 29 Jun 2023 04:59:02 +0000 Subject: [PATCH 6/9] indentation --- coderd/database/queries.sql.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2cda2e1beb7e7..10ac39794462d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8596,7 +8596,7 @@ LEFT JOIN INNER JOIN provisioner_jobs ON workspace_builds.job_id = provisioner_jobs.id INNER JOIN - templates ON workspaces.template_id = templates.id + templates ON workspaces.template_id = templates.id WHERE workspace_builds.build_number = ( SELECT @@ -8639,15 +8639,15 @@ WHERE -- If the workspace's template has an inactivity_ttl set -- it may be eligible for locking. ( - templates.inactivity_ttl > 0 AND - workspaces.locked_at IS NULL + templates.inactivity_ttl > 0 AND + workspaces.locked_at IS NULL ) OR -- If the workspace's template has a locked_ttl set -- and the workspace is already locked ( - templates.locked_ttl > 0 AND - workspaces.locked_at IS NOT NULL + templates.locked_ttl > 0 AND + workspaces.locked_at IS NOT NULL ) ) AND workspaces.deleted = 'false' ` From 8439ef78691d57609962d030d70e8d857381c693 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Sun, 2 Jul 2023 23:01:06 +0000 Subject: [PATCH 7/9] migrations --- ...ild_reason.down.sql => 000134_workspace_build_reason.down.sql} | 0 ...e_build_reason.up.sql => 000134_workspace_build_reason.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000132_workspace_build_reason.down.sql => 000134_workspace_build_reason.down.sql} (100%) rename coderd/database/migrations/{000132_workspace_build_reason.up.sql => 000134_workspace_build_reason.up.sql} (100%) diff --git a/coderd/database/migrations/000132_workspace_build_reason.down.sql b/coderd/database/migrations/000134_workspace_build_reason.down.sql similarity index 100% rename from coderd/database/migrations/000132_workspace_build_reason.down.sql rename to coderd/database/migrations/000134_workspace_build_reason.down.sql diff --git a/coderd/database/migrations/000132_workspace_build_reason.up.sql b/coderd/database/migrations/000134_workspace_build_reason.up.sql similarity index 100% rename from coderd/database/migrations/000132_workspace_build_reason.up.sql rename to coderd/database/migrations/000134_workspace_build_reason.up.sql From e8b76054d18379883ac69f44480602911267ed9c Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Sun, 2 Jul 2023 23:01:46 +0000 Subject: [PATCH 8/9] pr comments --- coderd/autobuild/lifecycle_executor.go | 19 ++-- coderd/autobuild/lifecycle_executor_test.go | 15 +-- enterprise/coderd/workspaces_test.go | 118 +++++--------------- 3 files changed, 41 insertions(+), 111 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index eecc602de9f88..d3dc80814b2b8 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -263,9 +263,9 @@ func getNextTransition( return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil case isEligibleForAutostart(ws, latestBuild, latestJob, templateSchedule, currentTick): return database.WorkspaceTransitionStart, database.BuildReasonAutostart, nil - case isEligibleForFailedStop(latestBuild, latestJob, templateSchedule): + case isEligibleForFailedStop(latestBuild, latestJob, templateSchedule, currentTick): return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil - case isEligibleForLockedStop(ws, templateSchedule): + case isEligibleForLockedStop(ws, templateSchedule, currentTick): // Only stop started workspaces. if latestBuild.Transition == database.WorkspaceTransitionStart { return database.WorkspaceTransitionStop, database.BuildReasonAutolock, nil @@ -274,7 +274,7 @@ func getNextTransition( // lock it. return "", database.BuildReasonAutolock, nil - case isEligibleForDelete(ws, templateSchedule): + case isEligibleForDelete(ws, templateSchedule, currentTick): return database.WorkspaceTransitionDelete, database.BuildReasonAutodelete, nil default: return "", "", xerrors.Errorf("last transition not valid for autostart or autostop") @@ -336,32 +336,33 @@ func isEligibleForAutostop(ws database.Workspace, build database.WorkspaceBuild, // isEligibleForLockedStop returns true if the workspace should be locked // for breaching the inactivity threshold of the template. -func isEligibleForLockedStop(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions) bool { +func isEligibleForLockedStop(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { // Only attempt to lock workspaces not already locked. return !ws.LockedAt.Valid && // The template must specify an inactivity TTL. templateSchedule.InactivityTTL > 0 && // The workspace must breach the inactivity TTL. - database.Now().Sub(ws.LastUsedAt) > templateSchedule.InactivityTTL + currentTick.Sub(ws.LastUsedAt) > templateSchedule.InactivityTTL } -func isEligibleForDelete(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions) bool { +func isEligibleForDelete(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { // Only attempt to delete locked workspaces. return ws.LockedAt.Valid && // Locked workspaces should only be deleted if a locked_ttl is specified. templateSchedule.LockedTTL > 0 && // The workspace must breach the locked_ttl. - database.Now().Sub(ws.LockedAt.Time) > templateSchedule.LockedTTL + currentTick.Sub(ws.LockedAt.Time) > templateSchedule.LockedTTL } // isEligibleForFailedStop returns true if the workspace is eligible to be stopped // due to a failed build. -func isEligibleForFailedStop(build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions) bool { +func isEligibleForFailedStop(build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { // If the template has specified a failure TLL. return templateSchedule.FailureTTL > 0 && // And the job resulted in failure. db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed && build.Transition == database.WorkspaceTransitionStart && // And sufficient time has elapsed since the job has completed. - job.CompletedAt.Valid && database.Now().Sub(job.CompletedAt.Time) > templateSchedule.FailureTTL + job.CompletedAt.Valid && + currentTick.Sub(job.CompletedAt.Time) > templateSchedule.FailureTTL } diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index 56fd5aeb0b1d1..0a9baf29571dc 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -21,7 +21,6 @@ import ( "github.com/coder/coder/codersdk" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" - "github.com/coder/coder/testutil" ) func TestExecutorAutostartOK(t *testing.T) { @@ -694,12 +693,7 @@ func TestExecutorFailedWorkspace(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) - require.Eventually(t, - func() bool { - return database.Now().Sub(*build.Job.CompletedAt) > failureTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - ticker <- time.Now() + ticker <- (*build.Job.CompletedAt).Add(failureTTL * 2) stats := <-statCh // Expect no transitions since we're using AGPL. require.Len(t, stats.Transitions, 0) @@ -749,12 +743,7 @@ func TestExecutorInactiveWorkspace(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > inactiveTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - ticker <- time.Now() + ticker <- ws.LastUsedAt.Add(inactiveTTL * 2) stats := <-statCh // Expect no transitions since we're using AGPL. require.Len(t, stats.Transitions, 0) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 9d310058ad599..24e1552a3e4b1 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -94,7 +94,7 @@ func TestWorkspaceAutobuild(t *testing.T) { // builds. IgnoreErrors: true, }) - failureTTL = time.Millisecond + failureTTL = time.Minute client = coderdenttest.New(t, &coderdenttest.Options{ Options: &coderdtest.Options{ @@ -107,11 +107,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, @@ -125,12 +121,7 @@ func TestWorkspaceAutobuild(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) - require.Eventually(t, - func() bool { - return database.Now().Sub(*build.Job.CompletedAt) > failureTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - ticker <- time.Now() + ticker <- build.Job.CompletedAt.Add(failureTTL * 2) stats := <-statCh // Expect workspace to transition to stopped state for breaching // failure TTL. @@ -162,11 +153,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) ) user := coderdtest.CreateFirstUser(t, client) - _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureAdvancedTemplateScheduling: 1, - }, - }) + _ = coderdenttest.AddFullLicense(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionPlan: echo.ProvisionComplete, @@ -179,7 +166,8 @@ func TestWorkspaceAutobuild(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) - ticker <- time.Now() + // Make it impossible to trigger the failure TTL. + ticker <- (*build.Job.CompletedAt).Add(-failureTTL * 2) stats := <-statCh // Expect no transitions since not enough time has elapsed. require.Len(t, stats.Transitions, 0) @@ -239,7 +227,7 @@ func TestWorkspaceAutobuild(t *testing.T) { ctx = testutil.Context(t, testutil.WaitMedium) ticker = make(chan time.Time) statCh = make(chan autobuild.Stats) - inactiveTTL = time.Millisecond + inactiveTTL = time.Minute client = coderdenttest.New(t, &coderdenttest.Options{ Options: &coderdtest.Options{ @@ -258,8 +246,6 @@ func TestWorkspaceAutobuild(t *testing.T) { ProvisionPlan: echo.ProvisionComplete, ProvisionApply: echo.ProvisionComplete, }) - // Create a template with a short inactivity ttl so we don't have to wait - // too long. template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { ctr.InactivityTTLMillis = ptr.Ref[int64](inactiveTTL.Milliseconds()) }) @@ -268,14 +254,8 @@ func TestWorkspaceAutobuild(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) - - // Wait to trigger the inactivity threshold. - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > inactiveTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - ticker <- time.Now() + // Simulate being inactive. + ticker <- ws.LastUsedAt.Add(inactiveTTL * 2) stats := <-statCh // Expect workspace to transition to stopped state for breaching @@ -328,14 +308,15 @@ func TestWorkspaceAutobuild(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) - ticker <- time.Now() + // Make it impossible to trigger the inactive ttl. + ticker <- ws.LastUsedAt.Add(-inactiveTTL) stats := <-statCh // Expect no transitions since not enough time has elapsed. require.Len(t, stats.Transitions, 0) }) // This is kind of a dumb test but it exists to offer some marginal - // confidence that bug in the auto-deletion logic doesn't delete running + // confidence that a bug in the auto-deletion logic doesn't delete running // workspaces. t.Run("UnlockedWorkspacesNotDeleted", func(t *testing.T) { t.Parallel() @@ -343,7 +324,7 @@ func TestWorkspaceAutobuild(t *testing.T) { var ( ticker = make(chan time.Time) statCh = make(chan autobuild.Stats) - lockedTTL = time.Millisecond + lockedTTL = time.Minute client = coderdenttest.New(t, &coderdenttest.Options{ Options: &coderdtest.Options{ @@ -369,13 +350,7 @@ func TestWorkspaceAutobuild(t *testing.T) { build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Nil(t, ws.LockedAt) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > lockedTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - - ticker <- time.Now() + ticker <- ws.LastUsedAt.Add(lockedTTL * 2) stats := <-statCh // Expect no transitions since workspace is unlocked. require.Len(t, stats.Transitions, 0) @@ -390,7 +365,7 @@ func TestWorkspaceAutobuild(t *testing.T) { var ( ticker = make(chan time.Time) statCh = make(chan autobuild.Stats) - inactiveTTL = time.Millisecond + inactiveTTL = time.Minute client = coderdenttest.New(t, &coderdenttest.Options{ Options: &coderdtest.Options{ @@ -421,14 +396,8 @@ func TestWorkspaceAutobuild(t *testing.T) { // if we breach our inactivity threshold. ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - // Wait to breach our threshold. - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > inactiveTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - - ticker <- time.Now() + // Simulate not having accessed the workspace in a while. + ticker <- ws.LastUsedAt.Add(2 * inactiveTTL) stats := <-statCh // Expect no transitions since workspace is stopped. require.Len(t, stats.Transitions, 0) @@ -446,7 +415,7 @@ func TestWorkspaceAutobuild(t *testing.T) { var ( ticker = make(chan time.Time) statCh = make(chan autobuild.Stats) - transitionTTL = time.Millisecond + transitionTTL = time.Minute client = coderdenttest.New(t, &coderdenttest.Options{ Options: &coderdtest.Options{ @@ -475,13 +444,8 @@ func TestWorkspaceAutobuild(t *testing.T) { build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > transitionTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - - ticker <- time.Now() + // Simulate not having accessed the workspace in a while. + ticker <- ws.LastUsedAt.Add(2 * transitionTTL) stats := <-statCh // Expect workspace to transition to stopped state for breaching // inactive TTL. @@ -495,14 +459,8 @@ func TestWorkspaceAutobuild(t *testing.T) { // Wait for the autobuilder to stop the workspace. _ = coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) - // Wait to trigger our locked threshold. - require.Eventually(t, - func() bool { - return database.Now().Sub(*ws.LockedAt) > transitionTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - - ticker <- time.Now() + // Simulate the workspace being locked beyond the threshold. + ticker <- ws.LockedAt.Add(2 * transitionTTL) stats = <-statCh require.Len(t, stats.Transitions, 1) // The workspace should be scheduled for deletion. @@ -561,31 +519,19 @@ func TestWorkspaceAutobuild(t *testing.T) { ws = coderdtest.MustWorkspace(t, client, ws.ID) require.NotNil(t, ws.LockedAt) - ticker <- time.Now() + // Ensure we haven't breached our threshold. + ticker <- ws.LockedAt.Add(-lockedTTL * 2) stats := <-statCh // Expect no transitions since not enough time has elapsed. require.Len(t, stats.Transitions, 0) - // Update the lock time to trigger a deletion. - lockedTTL = time.Millisecond _, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ - Name: template.Name, - DisplayName: template.DisplayName, - Description: template.Description, - Icon: template.Icon, - AllowUserCancelWorkspaceJobs: template.AllowUserCancelWorkspaceJobs, - LockedTTLMillis: lockedTTL.Milliseconds(), + LockedTTLMillis: lockedTTL.Milliseconds(), }) require.NoError(t, err) - // Wait for the workspace to breach the locked threshold. - require.Eventually(t, - func() bool { - return database.Now().Sub(*ws.LockedAt) > lockedTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) - - ticker <- time.Now() + // Simlute the workspace breaching the threshold. + ticker <- ws.LockedAt.Add(lockedTTL * 2) stats = <-statCh require.Len(t, stats.Transitions, 1) require.Equal(t, database.WorkspaceTransitionDelete, stats.Transitions[ws.ID]) @@ -607,7 +553,7 @@ func TestWorkspaceAutobuild(t *testing.T) { TemplateScheduleStore: &coderd.EnterpriseTemplateScheduleStore{}, }, }) - inactiveTTL = time.Millisecond + inactiveTTL = time.Minute ) user := coderdtest.CreateFirstUser(t, client) @@ -648,15 +594,9 @@ func TestWorkspaceAutobuild(t *testing.T) { InactivityTTLMillis: inactiveTTL.Milliseconds(), }) require.NoError(t, err) - // Wait for the workspace to breach the inactivity threshold. - require.Eventually(t, - func() bool { - return database.Now().Sub(ws.LastUsedAt) > inactiveTTL - }, - testutil.IntervalMedium, testutil.IntervalFast) // We should see the workspace get stopped now. - tickCh <- time.Now() + tickCh <- ws.LastUsedAt.Add(inactiveTTL * 2) stats = <-statsCh require.NoError(t, stats.Error) require.Len(t, stats.Transitions, 1) From 016221601f47e3ec10709d7ae834e993240eeb00 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Mon, 3 Jul 2023 00:38:29 +0000 Subject: [PATCH 9/9] lint --- coderd/autobuild/lifecycle_executor_test.go | 2 +- enterprise/coderd/workspaces_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index 0a9baf29571dc..548fffb937e29 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -693,7 +693,7 @@ func TestExecutorFailedWorkspace(t *testing.T) { ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) - ticker <- (*build.Job.CompletedAt).Add(failureTTL * 2) + ticker <- build.Job.CompletedAt.Add(failureTTL * 2) stats := <-statCh // Expect no transitions since we're using AGPL. require.Len(t, stats.Transitions, 0) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 24e1552a3e4b1..740d4ef77cae9 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -167,7 +167,7 @@ func TestWorkspaceAutobuild(t *testing.T) { build := coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) // Make it impossible to trigger the failure TTL. - ticker <- (*build.Job.CompletedAt).Add(-failureTTL * 2) + ticker <- build.Job.CompletedAt.Add(-failureTTL * 2) stats := <-statCh // Expect no transitions since not enough time has elapsed. require.Len(t, stats.Transitions, 0)