From 5c21409ea9a92be5197bf25f63e01e9c98e42274 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Fri, 17 Nov 2023 21:16:52 +0000 Subject: [PATCH 1/2] fix: skip autostart for suspended/dormant users --- coderd/autobuild/lifecycle_executor.go | 17 +++++-- .../lifecycle_executor_internal_test.go | 19 +++++++- coderd/autobuild/lifecycle_executor_test.go | 44 +++++++++++++++++++ 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index 9fa1aaedb0ee4..992fa06d94dc5 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -149,6 +149,11 @@ func (e *Executor) runOnce(t time.Time) Stats { return xerrors.Errorf("get workspace by id: %w", err) } + user, err := tx.GetUserByID(e.ctx, ws.OwnerID) + if err != nil { + return xerrors.Errorf("get user by id: %w", err) + } + // Determine the workspace state based on its latest build. latestBuild, err := tx.GetLatestWorkspaceBuildByWorkspaceID(e.ctx, ws.ID) if err != nil { @@ -172,7 +177,7 @@ func (e *Executor) runOnce(t time.Time) Stats { accessControl := (*(e.accessControlStore.Load())).GetTemplateAccessControl(template) - nextTransition, reason, err := getNextTransition(ws, latestBuild, latestJob, templateSchedule, currentTick) + nextTransition, reason, err := getNextTransition(user, ws, latestBuild, latestJob, templateSchedule, currentTick) if err != nil { log.Debug(e.ctx, "skipping workspace", slog.Error(err)) // err is used to indicate that a workspace is not eligible @@ -300,6 +305,7 @@ func (e *Executor) runOnce(t time.Time) Stats { // may be "transitioning" to a new state (such as an inactive, stopped // workspace transitioning to the dormant state). func getNextTransition( + user database.User, ws database.Workspace, latestBuild database.WorkspaceBuild, latestJob database.ProvisionerJob, @@ -313,7 +319,7 @@ func getNextTransition( switch { case isEligibleForAutostop(ws, latestBuild, latestJob, currentTick): return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil - case isEligibleForAutostart(ws, latestBuild, latestJob, templateSchedule, currentTick): + case isEligibleForAutostart(user, ws, latestBuild, latestJob, templateSchedule, currentTick): return database.WorkspaceTransitionStart, database.BuildReasonAutostart, nil case isEligibleForFailedStop(latestBuild, latestJob, templateSchedule, currentTick): return database.WorkspaceTransitionStop, database.BuildReasonAutostop, nil @@ -334,7 +340,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 { +func isEligibleForAutostart(user database.User, ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { + // Don't attempt to autostart workspaces for suspended users. + if user.Status != database.UserStatusActive { + return false + } + // Don't attempt to autostart failed workspaces. if job.JobStatus == database.ProvisionerJobStatusFailed { return false diff --git a/coderd/autobuild/lifecycle_executor_internal_test.go b/coderd/autobuild/lifecycle_executor_internal_test.go index 33d3143fac19d..2b75a9782d7b6 100644 --- a/coderd/autobuild/lifecycle_executor_internal_test.go +++ b/coderd/autobuild/lifecycle_executor_internal_test.go @@ -25,6 +25,7 @@ func Test_isEligibleForAutostart(t *testing.T) { // 5s after the autostart in UTC. okTick := time.Date(2021, 1, 1, 20, 0, 5, 0, localLocation).UTC() + okUser := database.User{Status: database.UserStatusActive} okWorkspace := database.Workspace{ DormantAt: sql.NullTime{Valid: false}, AutostartSchedule: sql.NullString{ @@ -57,6 +58,7 @@ func Test_isEligibleForAutostart(t *testing.T) { testCases := []struct { Name string + User database.User Workspace database.Workspace Build database.WorkspaceBuild Job database.ProvisionerJob @@ -67,6 +69,7 @@ func Test_isEligibleForAutostart(t *testing.T) { }{ { Name: "Ok", + User: okUser, Workspace: okWorkspace, Build: okBuild, Job: okJob, @@ -74,8 +77,19 @@ func Test_isEligibleForAutostart(t *testing.T) { Tick: okTick, ExpectedResponse: true, }, + { + Name: "SuspendedUser", + User: database.User{Status: database.UserStatusSuspended}, + Workspace: okWorkspace, + Build: okBuild, + Job: okJob, + TemplateSchedule: okTemplateSchedule, + Tick: okTick, + ExpectedResponse: false, + }, { Name: "AutostartOnlyDayEnabled", + User: okUser, Workspace: okWorkspace, Build: okBuild, Job: okJob, @@ -91,6 +105,7 @@ func Test_isEligibleForAutostart(t *testing.T) { }, { Name: "AutostartOnlyDayDisabled", + User: okUser, Workspace: okWorkspace, Build: okBuild, Job: okJob, @@ -106,6 +121,7 @@ func Test_isEligibleForAutostart(t *testing.T) { }, { Name: "AutostartAllDaysDisabled", + User: okUser, Workspace: okWorkspace, Build: okBuild, Job: okJob, @@ -121,6 +137,7 @@ func Test_isEligibleForAutostart(t *testing.T) { }, { Name: "BuildTransitionNotStop", + User: okUser, Workspace: okWorkspace, Build: func(b database.WorkspaceBuild) database.WorkspaceBuild { cpy := b @@ -139,7 +156,7 @@ func Test_isEligibleForAutostart(t *testing.T) { t.Run(c.Name, func(t *testing.T) { t.Parallel() - autostart := isEligibleForAutostart(c.Workspace, c.Build, c.Job, c.TemplateSchedule, c.Tick) + autostart := isEligibleForAutostart(c.User, c.Workspace, c.Build, c.Job, c.TemplateSchedule, c.Tick) require.Equal(t, c.ExpectedResponse, autostart, "autostart not expected") }) } diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index 9ac535bdb6612..dbb506bbbbb5d 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -265,6 +265,50 @@ func TestExecutorAutostartNotEnabled(t *testing.T) { require.Len(t, stats.Transitions, 0) } +func TestExecutorAutostartUserSuspended(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + sched = mustSchedule(t, "CRON_TZ=UTC 0 * * * *") + tickCh = make(chan time.Time) + statsCh = make(chan autobuild.Stats) + client = coderdtest.New(t, &coderdtest.Options{ + AutobuildTicker: tickCh, + IncludeProvisionerDaemon: true, + AutobuildStats: statsCh, + }) + ) + + admin := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + userClient, user := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + workspace := coderdtest.CreateWorkspace(t, userClient, admin.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.AutostartSchedule = ptr.Ref(sched.String()) + }) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID) + workspace = coderdtest.MustWorkspace(t, userClient, workspace.ID) + + // Given: workspace is stopped, and the user is suspended. + workspace = coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + _, err := client.UpdateUserStatus(ctx, user.ID.String(), codersdk.UserStatusSuspended) + require.NoError(t, err, "update user status") + + // When: the autobuild executor ticks after the scheduled time + go func() { + tickCh <- sched.Next(workspace.LatestBuild.CreatedAt) + close(tickCh) + }() + + // Then: nothing should happen + stats := <-statsCh + assert.NoError(t, stats.Error) + assert.Len(t, stats.Transitions, 0) +} + func TestExecutorAutostopOK(t *testing.T) { t.Parallel() From e40a3020fea578701e323f8f4570b9baedfd1d4f Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 22 Nov 2023 00:04:39 +0000 Subject: [PATCH 2/2] jon review --- coderd/autobuild/lifecycle_executor_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index dbb506bbbbb5d..366abdca7b2e8 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -269,7 +269,7 @@ func TestExecutorAutostartUserSuspended(t *testing.T) { t.Parallel() var ( - ctx = context.Background() + ctx = testutil.Context(t, testutil.WaitShort) sched = mustSchedule(t, "CRON_TZ=UTC 0 * * * *") tickCh = make(chan time.Time) statsCh = make(chan autobuild.Stats) @@ -304,7 +304,7 @@ func TestExecutorAutostartUserSuspended(t *testing.T) { }() // Then: nothing should happen - stats := <-statsCh + stats := testutil.RequireRecvCtx(ctx, t, statsCh) assert.NoError(t, stats.Error) assert.Len(t, stats.Transitions, 0) }