From a145d6d559fdb64db526c16185658488f4cf5556 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 19 Apr 2022 10:02:52 -0500 Subject: [PATCH 01/28] feat: add lifecycle.Executor to autostart workspaces. --- .../autostart/lifecycle/lifecycle_executor.go | 212 ++++++++++++++++++ .../lifecycle/lifecycle_executor_test.go | 138 ++++++++++++ coderd/coderdtest/coderdtest.go | 26 ++- coderd/database/databasefake/databasefake.go | 8 + coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 43 ++++ coderd/database/queries/workspaces.sql | 8 + 7 files changed, 434 insertions(+), 2 deletions(-) create mode 100644 coderd/autostart/lifecycle/lifecycle_executor.go create mode 100644 coderd/autostart/lifecycle/lifecycle_executor_test.go diff --git a/coderd/autostart/lifecycle/lifecycle_executor.go b/coderd/autostart/lifecycle/lifecycle_executor.go new file mode 100644 index 0000000000000..0d8a28aae1fa9 --- /dev/null +++ b/coderd/autostart/lifecycle/lifecycle_executor.go @@ -0,0 +1,212 @@ +package lifecycle + +import ( + "context" + "encoding/json" + "time" + + "cdr.dev/slog" + + "github.com/coder/coder/coderd/autostart/schedule" + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/codersdk" + + "github.com/google/uuid" + "github.com/moby/moby/pkg/namesgenerator" + "golang.org/x/xerrors" +) + +//var ExecutorUUID = uuid.MustParse("00000000-0000-0000-0000-000000000000") + +// Executor executes automated workspace lifecycle operations. +type Executor struct { + ctx context.Context + db database.Store + log slog.Logger + tick <-chan time.Time +} + +func NewExecutor(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor { + le := &Executor{ + ctx: ctx, + db: db, + tick: tick, + log: log, + } + return le +} + +func (e *Executor) Run() error { + for { + select { + case t := <-e.tick: + if err := e.runOnce(t); err != nil { + e.log.Error(e.ctx, "error running once", slog.Error(err)) + } + case <-e.ctx.Done(): + return nil + default: + } + } +} + +func (e *Executor) runOnce(t time.Time) error { + currentTick := t.Round(time.Minute) + return e.db.InTx(func(db database.Store) error { + allWorkspaces, err := db.GetWorkspaces(e.ctx) + if err != nil { + return xerrors.Errorf("get all workspaces: %w", err) + } + + for _, ws := range allWorkspaces { + // We only care about workspaces with autostart enabled. + if ws.AutostartSchedule.String == "" { + continue + } + sched, err := schedule.Weekly(ws.AutostartSchedule.String) + if err != nil { + e.log.Warn(e.ctx, "workspace has invalid autostart schedule", + slog.F("workspace_id", ws.ID), + slog.F("autostart_schedule", ws.AutostartSchedule.String), + ) + continue + } + + // Determine the workspace state based on its latest build. We expect it to be stopped. + // TODO(cian): is this **guaranteed** to be the latest build??? + latestBuild, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) + if err != nil { + return xerrors.Errorf("get latest build for workspace %q: %w", ws.ID, err) + } + if latestBuild.Transition != database.WorkspaceTransitionStop { + e.log.Debug(e.ctx, "autostart: skipping workspace: wrong transition", + slog.F("transition", latestBuild.Transition), + slog.F("workspace_id", ws.ID), + ) + continue + } + + // Round time to the nearest minute, as this is the finest granularity cron supports. + earliestAutostart := sched.Next(latestBuild.CreatedAt).Round(time.Minute) + if earliestAutostart.After(currentTick) { + e.log.Debug(e.ctx, "autostart: skipping workspace: too early", + slog.F("workspace_id", ws.ID), + slog.F("earliest_autostart", earliestAutostart), + slog.F("current_tick", currentTick), + ) + continue + } + + e.log.Info(e.ctx, "autostart: scheduling workspace start", + slog.F("workspace_id", ws.ID), + ) + + if err := doBuild(e.ctx, db, ws, currentTick); err != nil { + e.log.Error(e.ctx, "autostart workspace", slog.F("workspace_id", ws.ID), slog.Error(err)) + } + } + return nil + }) +} + +// XXX: cian: this function shouldn't really exist. Refactor. +func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, now time.Time) error { + template, err := store.GetTemplateByID(ctx, workspace.TemplateID) + if err != nil { + return xerrors.Errorf("get template: %w", err) + } + + priorHistory, err := store.GetWorkspaceBuildByWorkspaceIDWithoutAfter(ctx, workspace.ID) + priorJob, err := store.GetProvisionerJobByID(ctx, priorHistory.JobID) + if err == nil && !priorJob.CompletedAt.Valid { + return xerrors.Errorf("workspace build already active") + } + + priorHistoryID := uuid.NullUUID{ + UUID: priorHistory.ID, + Valid: true, + } + + var newWorkspaceBuild database.WorkspaceBuild + // This must happen in a transaction to ensure history can be inserted, and + // the prior history can update it's "after" column to point at the new. + workspaceBuildID := uuid.New() + input, err := json.Marshal(struct { + WorkspaceBuildID string `json:"workspace_build_id"` + }{ + WorkspaceBuildID: workspaceBuildID.String(), + }) + if err != nil { + return xerrors.Errorf("marshal provision job: %w", err) + } + provisionerJobID := uuid.New() + newProvisionerJob, err := store.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ + ID: provisionerJobID, + CreatedAt: database.Now(), + UpdatedAt: database.Now(), + InitiatorID: workspace.OwnerID, + OrganizationID: template.OrganizationID, + Provisioner: template.Provisioner, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StorageMethod: priorJob.StorageMethod, + StorageSource: priorJob.StorageSource, + Input: input, + }) + if err != nil { + return xerrors.Errorf("insert provisioner job: %w", err) + } + newWorkspaceBuild, err = store.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ + ID: workspaceBuildID, + CreatedAt: database.Now(), + UpdatedAt: database.Now(), + WorkspaceID: workspace.ID, + TemplateVersionID: priorHistory.TemplateVersionID, + BeforeID: priorHistoryID, + Name: namesgenerator.GetRandomName(1), + ProvisionerState: priorHistory.ProvisionerState, + InitiatorID: workspace.OwnerID, + Transition: database.WorkspaceTransitionStart, + JobID: newProvisionerJob.ID, + }) + if err != nil { + return xerrors.Errorf("insert workspace build: %w", err) + } + + if priorHistoryID.Valid { + // Update the prior history entries "after" column. + err = store.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ + ID: priorHistory.ID, + ProvisionerState: priorHistory.ProvisionerState, + UpdatedAt: database.Now(), + AfterID: uuid.NullUUID{ + UUID: newWorkspaceBuild.ID, + Valid: true, + }, + }) + if err != nil { + return xerrors.Errorf("update prior workspace build: %w", err) + } + } + return nil +} + +func provisionerJobStatus(j database.ProvisionerJob, now time.Time) codersdk.ProvisionerJobStatus { + switch { + case j.CanceledAt.Valid: + if j.CompletedAt.Valid { + return codersdk.ProvisionerJobCanceled + } + return codersdk.ProvisionerJobCanceling + case !j.StartedAt.Valid: + return codersdk.ProvisionerJobPending + case j.CompletedAt.Valid: + if j.Error.String == "" { + return codersdk.ProvisionerJobSucceeded + } + return codersdk.ProvisionerJobFailed + case now.Sub(j.UpdatedAt) > 30*time.Second: + return codersdk.ProvisionerJobFailed + default: + return codersdk.ProvisionerJobRunning + } +} diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go new file mode 100644 index 0000000000000..1c9d24c245928 --- /dev/null +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -0,0 +1,138 @@ +package lifecycle_test + +import ( + "context" + "testing" + "time" + + "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogtest" + + "github.com/coder/coder/coderd/autostart/lifecycle" + "github.com/coder/coder/coderd/autostart/schedule" + "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/databasefake" + "github.com/coder/coder/codersdk" + + "github.com/stretchr/testify/require" +) + +func Test_Executor_Run(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + cancelCtx, cancel = context.WithCancel(context.Background()) + log = slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug) + err error + tickCh = make(chan time.Time) + db = databasefake.New() + le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleExecutor: le, + Store: db, + }) + // Given: we have a user with a workspace + _ = coderdtest.NewProvisionerDaemon(t, client) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + ) + // Given: workspace is stopped + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: database.WorkspaceTransitionStop, + }) + require.NoError(t, err, "stop workspace") + // Given: we wait for the stop to complete + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) + + // Given: we update the workspace with its new state + workspace = coderdtest.MustWorkspace(t, client, workspace.ID) + // Given: we ensure the workspace is now in a stopped state + require.Equal(t, database.WorkspaceTransitionStop, workspace.LatestBuild.Transition) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + cancel() + }() + require.NoError(t, le.Run()) + + // Then: the workspace should be started + require.Eventually(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && + ws.LatestBuild.Transition == database.WorkspaceTransitionStart + }, 10*time.Second, 1000*time.Millisecond) + }) + + t.Run("AlreadyRunning", func(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + cancelCtx, cancel = context.WithCancel(context.Background()) + log = slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug) + err error + tickCh = make(chan time.Time) + db = databasefake.New() + le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleExecutor: le, + Store: db, + }) + // Given: we have a user with a workspace + _ = coderdtest.NewProvisionerDaemon(t, client) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + ) + + // Given: we ensure the workspace is now in a stopped state + require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + cancel() + }() + require.NoError(t, le.Run()) + + // Then: the workspace should not be started. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID != workspace.LatestBuild.ID + }, 10*time.Second, 1000*time.Millisecond) + }) +} diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index e205e272db3ce..933fc8d27719d 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "github.com/coder/coder/coderd/autostart/lifecycle" + "cloud.google.com/go/compute/metadata" "github.com/fullsailor/pkcs7" "github.com/golang-jwt/jwt" @@ -57,6 +59,9 @@ type Options struct { GoogleTokenValidator *idtoken.Validator SSHKeygenAlgorithm gitsshkey.Algorithm APIRateLimit int + Store database.Store + Pubsub database.Pubsub + LifecycleExecutor *lifecycle.Executor } // New constructs an in-memory coderd instance and returns @@ -74,8 +79,14 @@ func New(t *testing.T, options *Options) *codersdk.Client { } // This can be hotswapped for a live database instance. - db := databasefake.New() - pubsub := database.NewPubsubInMemory() + db := options.Store + pubsub := options.Pubsub + if db == nil { + db = databasefake.New() + } + if pubsub == nil { + pubsub = database.NewPubsubInMemory() + } if os.Getenv("DB") != "" { connectionURL, close, err := postgres.Open() require.NoError(t, err) @@ -96,6 +107,10 @@ func New(t *testing.T, options *Options) *codersdk.Client { }) } + if options.LifecycleExecutor == nil { + options.LifecycleExecutor = &lifecycle.Executor{} + } + srv := httptest.NewUnstartedServer(nil) ctx, cancelFunc := context.WithCancel(context.Background()) srv.Config.BaseContext = func(_ net.Listener) context.Context { @@ -492,3 +507,10 @@ type roundTripper func(req *http.Request) (*http.Response, error) func (r roundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return r(req) } + +func MustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) codersdk.Workspace { + ctx := context.Background() + ws, err := client.Workspace(ctx, workspaceID) + require.NoError(t, err, "no workspace found with id %s", workspaceID) + return ws +} diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index f1ec50bbbbe2f..c54af059940d3 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -325,6 +325,14 @@ func (q *fakeQuerier) GetWorkspaceByOwnerIDAndName(_ context.Context, arg databa return database.Workspace{}, sql.ErrNoRows } +func (q *fakeQuerier) GetWorkspaces(_ context.Context) ([]database.Workspace, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + workspaces := make([]database.Workspace, len(q.workspaces)) + copy(workspaces, q.workspaces) + return workspaces, nil +} + func (q *fakeQuerier) GetWorkspaceOwnerCountsByTemplateIDs(_ context.Context, templateIDs []uuid.UUID) ([]database.GetWorkspaceOwnerCountsByTemplateIDsRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4182d15284a18..479bc113bc48a 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -69,6 +69,7 @@ type querier interface { GetWorkspaceOwnerCountsByTemplateIDs(ctx context.Context, ids []uuid.UUID) ([]GetWorkspaceOwnerCountsByTemplateIDsRow, error) GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID) (WorkspaceResource, error) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) + GetWorkspaces(ctx context.Context) ([]Workspace, error) GetWorkspacesByOrganizationID(ctx context.Context, arg GetWorkspacesByOrganizationIDParams) ([]Workspace, error) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) GetWorkspacesByOwnerID(ctx context.Context, arg GetWorkspacesByOwnerIDParams) ([]Workspace, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 362bae614c15e..a9f4400e5faf0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3202,6 +3202,49 @@ func (q *sqlQuerier) GetWorkspaceOwnerCountsByTemplateIDs(ctx context.Context, i return items, nil } +const getWorkspaces = `-- name: GetWorkspaces :many +SELECT + id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule +FROM + workspaces +WHERE + deleted = false +` + +func (q *sqlQuerier) GetWorkspaces(ctx context.Context) ([]Workspace, error) { + rows, err := q.db.QueryContext(ctx, getWorkspaces) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Workspace + for rows.Next() { + var i Workspace + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.OwnerID, + &i.OrganizationID, + &i.TemplateID, + &i.Deleted, + &i.Name, + &i.AutostartSchedule, + &i.AutostopSchedule, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getWorkspacesByOrganizationID = `-- name: GetWorkspacesByOrganizationID :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule FROM workspaces WHERE organization_id = $1 AND deleted = $2 ` diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index d271f7ddf7847..ae2b8d095ece1 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -14,6 +14,14 @@ SELECT * FROM workspaces WHERE organization_id = $1 AND deleted = $2; -- name: GetWorkspacesByOrganizationIDs :many SELECT * FROM workspaces WHERE organization_id = ANY(@ids :: uuid [ ]) AND deleted = @deleted; +-- name: GetWorkspaces :many +SELECT + * +FROM + workspaces +WHERE + deleted = false; + -- name: GetWorkspacesByTemplateID :many SELECT * From 8f401ca420813a6fb11a334bbe86903d424b65f6 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 26 Apr 2022 16:17:36 +0000 Subject: [PATCH 02/28] refactor: do not expose Store in coderdtest.Options --- .../lifecycle/lifecycle_executor_test.go | 6 ++--- coderd/coderdtest/coderdtest.go | 26 ++++++++----------- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index 1c9d24c245928..d8dd4df54dc8e 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -33,8 +33,7 @@ func Test_Executor_Run(t *testing.T) { db = databasefake.New() le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) client = coderdtest.New(t, &coderdtest.Options{ - LifecycleExecutor: le, - Store: db, + Ticker: tickCh, }) // Given: we have a user with a workspace _ = coderdtest.NewProvisionerDaemon(t, client) @@ -96,8 +95,7 @@ func Test_Executor_Run(t *testing.T) { db = databasefake.New() le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) client = coderdtest.New(t, &coderdtest.Options{ - LifecycleExecutor: le, - Store: db, + Ticker: tickCh, }) // Given: we have a user with a workspace _ = coderdtest.NewProvisionerDaemon(t, client) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 933fc8d27719d..25044ddf0ed42 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -59,9 +59,7 @@ type Options struct { GoogleTokenValidator *idtoken.Validator SSHKeygenAlgorithm gitsshkey.Algorithm APIRateLimit int - Store database.Store - Pubsub database.Pubsub - LifecycleExecutor *lifecycle.Executor + Ticker <-chan time.Time } // New constructs an in-memory coderd instance and returns @@ -79,14 +77,8 @@ func New(t *testing.T, options *Options) *codersdk.Client { } // This can be hotswapped for a live database instance. - db := options.Store - pubsub := options.Pubsub - if db == nil { - db = databasefake.New() - } - if pubsub == nil { - pubsub = database.NewPubsubInMemory() - } + db := databasefake.New() + pubsub := database.NewPubsubInMemory() if os.Getenv("DB") != "" { connectionURL, close, err := postgres.Open() require.NoError(t, err) @@ -107,12 +99,16 @@ func New(t *testing.T, options *Options) *codersdk.Client { }) } - if options.LifecycleExecutor == nil { - options.LifecycleExecutor = &lifecycle.Executor{} - } + ctx, cancelFunc := context.WithCancel(context.Background()) + lifecycleExecutor := lifecycle.NewExecutor( + ctx, + db, + slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug), + options.Ticker, + ) + go lifecycleExecutor.Run() srv := httptest.NewUnstartedServer(nil) - ctx, cancelFunc := context.WithCancel(context.Background()) srv.Config.BaseContext = func(_ net.Listener) context.Context { return ctx } From 6d8f5fef57703351cb8130148f0bf6b39d96650f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 26 Apr 2022 16:43:11 +0000 Subject: [PATCH 03/28] fixup! refactor: do not expose Store in coderdtest.Options --- coderd/coderdtest/coderdtest.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 25044ddf0ed42..7f1e18bce8dd4 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -75,6 +75,11 @@ func New(t *testing.T, options *Options) *codersdk.Client { options.GoogleTokenValidator, err = idtoken.NewValidator(ctx, option.WithoutAuthentication()) require.NoError(t, err) } + if options.Ticker == nil { + ticker := time.NewTicker(time.Second) + options.Ticker = ticker.C + t.Cleanup(ticker.Stop) + } // This can be hotswapped for a live database instance. db := databasefake.New() From cfd0d1e521eb00c4ef0985fb6fb1bfdb74a26b78 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 26 Apr 2022 20:37:09 +0000 Subject: [PATCH 04/28] stop accessing db directly, only query workspaces with autostart enabled --- .../autostart/lifecycle/lifecycle_executor.go | 9 +-- .../lifecycle/lifecycle_executor_test.go | 77 ++++++++++++------- coderd/coderdtest/coderdtest.go | 12 +-- coderd/database/databasefake/databasefake.go | 12 +++ coderd/database/models.go | 2 - coderd/database/querier.go | 3 +- coderd/database/queries.sql.go | 47 ++++++++++- coderd/database/queries/workspaces.sql | 11 +++ 8 files changed, 125 insertions(+), 48 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor.go b/coderd/autostart/lifecycle/lifecycle_executor.go index 0d8a28aae1fa9..ac2e2df3e736e 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor.go +++ b/coderd/autostart/lifecycle/lifecycle_executor.go @@ -53,16 +53,12 @@ func (e *Executor) Run() error { func (e *Executor) runOnce(t time.Time) error { currentTick := t.Round(time.Minute) return e.db.InTx(func(db database.Store) error { - allWorkspaces, err := db.GetWorkspaces(e.ctx) + autostartWorkspaces, err := db.GetWorkspacesAutostart(e.ctx) if err != nil { return xerrors.Errorf("get all workspaces: %w", err) } - for _, ws := range allWorkspaces { - // We only care about workspaces with autostart enabled. - if ws.AutostartSchedule.String == "" { - continue - } + for _, ws := range autostartWorkspaces { sched, err := schedule.Weekly(ws.AutostartSchedule.String) if err != nil { e.log.Warn(e.ctx, "workspace has invalid autostart schedule", @@ -73,7 +69,6 @@ func (e *Executor) runOnce(t time.Time) error { } // Determine the workspace state based on its latest build. We expect it to be stopped. - // TODO(cian): is this **guaranteed** to be the latest build??? latestBuild, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) if err != nil { return xerrors.Errorf("get latest build for workspace %q: %w", ws.ID, err) diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index d8dd4df54dc8e..643c884cef388 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -5,14 +5,9 @@ import ( "testing" "time" - "cdr.dev/slog" - "cdr.dev/slog/sloggers/slogtest" - - "github.com/coder/coder/coderd/autostart/lifecycle" "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/codersdk" "github.com/stretchr/testify/require" @@ -25,15 +20,11 @@ func Test_Executor_Run(t *testing.T) { t.Parallel() var ( - ctx = context.Background() - cancelCtx, cancel = context.WithCancel(context.Background()) - log = slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug) - err error - tickCh = make(chan time.Time) - db = databasefake.New() - le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) - client = coderdtest.New(t, &coderdtest.Options{ - Ticker: tickCh, + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, }) // Given: we have a user with a workspace _ = coderdtest.NewProvisionerDaemon(t, client) @@ -71,31 +62,25 @@ func Test_Executor_Run(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) - cancel() }() - require.NoError(t, le.Run()) // Then: the workspace should be started require.Eventually(t, func() bool { ws := coderdtest.MustWorkspace(t, client, workspace.ID) return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && ws.LatestBuild.Transition == database.WorkspaceTransitionStart - }, 10*time.Second, 1000*time.Millisecond) + }, 5*time.Second, 250*time.Millisecond) }) t.Run("AlreadyRunning", func(t *testing.T) { t.Parallel() var ( - ctx = context.Background() - cancelCtx, cancel = context.WithCancel(context.Background()) - log = slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug) - err error - tickCh = make(chan time.Time) - db = databasefake.New() - le = lifecycle.NewExecutor(cancelCtx, db, log, tickCh) - client = coderdtest.New(t, &coderdtest.Options{ - Ticker: tickCh, + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, }) // Given: we have a user with a workspace _ = coderdtest.NewProvisionerDaemon(t, client) @@ -123,14 +108,48 @@ func Test_Executor_Run(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) - cancel() }() - require.NoError(t, le.Run()) // Then: the workspace should not be started. require.Never(t, func() bool { ws := coderdtest.MustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID != workspace.LatestBuild.ID - }, 10*time.Second, 1000*time.Millisecond) + }, 5*time.Second, 250*time.Millisecond) + }) + + t.Run("NotEnabled", func(t *testing.T) { + t.Parallel() + + var ( + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + _ = coderdtest.NewProvisionerDaemon(t, client) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + ) + + // Given: we ensure the workspace is now in a stopped state + require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) + + // Given: the workspace has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should not be started. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID != workspace.LatestBuild.ID + }, 5*time.Second, 250*time.Millisecond) }) } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 7f1e18bce8dd4..20e261a93c765 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -59,7 +59,7 @@ type Options struct { GoogleTokenValidator *idtoken.Validator SSHKeygenAlgorithm gitsshkey.Algorithm APIRateLimit int - Ticker <-chan time.Time + LifecycleTicker <-chan time.Time } // New constructs an in-memory coderd instance and returns @@ -75,10 +75,10 @@ func New(t *testing.T, options *Options) *codersdk.Client { options.GoogleTokenValidator, err = idtoken.NewValidator(ctx, option.WithoutAuthentication()) require.NoError(t, err) } - if options.Ticker == nil { - ticker := time.NewTicker(time.Second) - options.Ticker = ticker.C - t.Cleanup(ticker.Stop) + if options.LifecycleTicker == nil { + ticker := make(chan time.Time) + options.LifecycleTicker = ticker + t.Cleanup(func() { close(ticker) }) } // This can be hotswapped for a live database instance. @@ -109,7 +109,7 @@ func New(t *testing.T, options *Options) *codersdk.Client { ctx, db, slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug), - options.Ticker, + options.LifecycleTicker, ) go lifecycleExecutor.Run() diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index c54af059940d3..8eccef9f0d49a 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -333,6 +333,18 @@ func (q *fakeQuerier) GetWorkspaces(_ context.Context) ([]database.Workspace, er return workspaces, nil } +func (q *fakeQuerier) GetWorkspacesAutostart(_ context.Context) ([]database.Workspace, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + workspaces := make([]database.Workspace, 0) + for _, ws := range q.workspaces { + if ws.AutostartSchedule.String != "" { + workspaces = append(workspaces, ws) + } + } + return workspaces, nil +} + func (q *fakeQuerier) GetWorkspaceOwnerCountsByTemplateIDs(_ context.Context, templateIDs []uuid.UUID) ([]database.GetWorkspaceOwnerCountsByTemplateIDsRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/models.go b/coderd/database/models.go index ad7277e32cbbe..e1c85c3a04a10 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1,6 +1,4 @@ // Code generated by sqlc. DO NOT EDIT. -// versions: -// sqlc v1.13.0 package database diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 479bc113bc48a..e5cd8a7997a3d 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1,6 +1,4 @@ // Code generated by sqlc. DO NOT EDIT. -// versions: -// sqlc v1.13.0 package database @@ -70,6 +68,7 @@ type querier interface { GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID) (WorkspaceResource, error) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) GetWorkspaces(ctx context.Context) ([]Workspace, error) + GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) GetWorkspacesByOrganizationID(ctx context.Context, arg GetWorkspacesByOrganizationIDParams) ([]Workspace, error) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) GetWorkspacesByOwnerID(ctx context.Context, arg GetWorkspacesByOwnerIDParams) ([]Workspace, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a9f4400e5faf0..d6da0da8123e0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1,6 +1,4 @@ // Code generated by sqlc. DO NOT EDIT. -// versions: -// sqlc v1.13.0 package database @@ -3245,6 +3243,51 @@ func (q *sqlQuerier) GetWorkspaces(ctx context.Context) ([]Workspace, error) { return items, nil } +const getWorkspacesAutostart = `-- name: GetWorkspacesAutostart :many +SELECT + id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule +FROM + workspaces +WHERE + deleted = false +AND + autostart_schedule <> '' +` + +func (q *sqlQuerier) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) { + rows, err := q.db.QueryContext(ctx, getWorkspacesAutostart) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Workspace + for rows.Next() { + var i Workspace + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.OwnerID, + &i.OrganizationID, + &i.TemplateID, + &i.Deleted, + &i.Name, + &i.AutostartSchedule, + &i.AutostopSchedule, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getWorkspacesByOrganizationID = `-- name: GetWorkspacesByOrganizationID :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule FROM workspaces WHERE organization_id = $1 AND deleted = $2 ` diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index ae2b8d095ece1..eb70d72e655f5 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -22,6 +22,17 @@ FROM WHERE deleted = false; +-- name: GetWorkspacesAutostart :many +SELECT + * +FROM + workspaces +WHERE + deleted = false +AND + autostart_schedule <> '' +; + -- name: GetWorkspacesByTemplateID :many SELECT * From ce638102dd65791e7e14693c7e853b965e8f41ce Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 26 Apr 2022 21:44:21 +0000 Subject: [PATCH 05/28] refactor unit tests, add tests for autostop --- .../lifecycle/lifecycle_executor_test.go | 374 +++++++++++------- 1 file changed, 241 insertions(+), 133 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index 643c884cef388..b543179cc2ae4 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -10,146 +10,254 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" + "github.com/google/uuid" "github.com/stretchr/testify/require" ) -func Test_Executor_Run(t *testing.T) { +func Test_Executor_Autostart_OK(t *testing.T) { t.Parallel() - t.Run("OK", func(t *testing.T) { - t.Parallel() - - var ( - ctx = context.Background() - err error - tickCh = make(chan time.Time) - client = coderdtest.New(t, &coderdtest.Options{ - LifecycleTicker: tickCh, - }) - // Given: we have a user with a workspace - _ = coderdtest.NewProvisionerDaemon(t, client) - user = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - ) - // Given: workspace is stopped - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: template.ActiveVersionID, - Transition: database.WorkspaceTransitionStop, + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, }) - require.NoError(t, err, "stop workspace") - // Given: we wait for the stop to complete - _ = coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) - - // Given: we update the workspace with its new state - workspace = coderdtest.MustWorkspace(t, client, workspace.ID) - // Given: we ensure the workspace is now in a stopped state - require.Equal(t, database.WorkspaceTransitionStop, workspace.LatestBuild.Transition) - - // Given: the workspace initially has autostart disabled - require.Empty(t, workspace.AutostartSchedule) - - // When: we enable workspace autostart - sched, err := schedule.Weekly("* * * * *") - require.NoError(t, err) - require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ - Schedule: sched.String(), - })) - - // When: the lifecycle executor ticks - go func() { - tickCh <- time.Now().UTC().Add(time.Minute) - }() - - // Then: the workspace should be started - require.Eventually(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && - ws.LatestBuild.Transition == database.WorkspaceTransitionStart - }, 5*time.Second, 250*time.Millisecond) - }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + // Given: workspace is stopped + MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - t.Run("AlreadyRunning", func(t *testing.T) { - t.Parallel() - - var ( - ctx = context.Background() - err error - tickCh = make(chan time.Time) - client = coderdtest.New(t, &coderdtest.Options{ - LifecycleTicker: tickCh, - }) - // Given: we have a user with a workspace - _ = coderdtest.NewProvisionerDaemon(t, client) - user = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - ) - - // Given: we ensure the workspace is now in a stopped state - require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) - - // Given: the workspace initially has autostart disabled - require.Empty(t, workspace.AutostartSchedule) - - // When: we enable workspace autostart - sched, err := schedule.Weekly("* * * * *") - require.NoError(t, err) - require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ - Schedule: sched.String(), - })) - - // When: the lifecycle executor ticks - go func() { - tickCh <- time.Now().UTC().Add(time.Minute) - }() - - // Then: the workspace should not be started. - require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID - }, 5*time.Second, 250*time.Millisecond) - }) + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should be started + require.Eventually(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && + ws.LatestBuild.Transition == database.WorkspaceTransitionStart + }, 5*time.Second, 250*time.Millisecond) +} + +func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + + // Given: we ensure the workspace is running + require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should not be started. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart + }, 5*time.Second, 250*time.Millisecond) +} + +func Test_Executor_Autostart_NotEnabled(t *testing.T) { + t.Parallel() + + var ( + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + + // Given: workspace is stopped + MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Given: the workspace has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should not be started. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart + }, 5*time.Second, 250*time.Millisecond) +} + +func Test_Executor_Autostop_OK(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + // Given: workspace is running + require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) - t.Run("NotEnabled", func(t *testing.T) { - t.Parallel() - - var ( - tickCh = make(chan time.Time) - client = coderdtest.New(t, &coderdtest.Options{ - LifecycleTicker: tickCh, - }) - // Given: we have a user with a workspace - _ = coderdtest.NewProvisionerDaemon(t, client) - user = coderdtest.CreateFirstUser(t, client) - version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - ) - - // Given: we ensure the workspace is now in a stopped state - require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) - - // Given: the workspace has autostart disabled - require.Empty(t, workspace.AutostartSchedule) - - // When: the lifecycle executor ticks - go func() { - tickCh <- time.Now().UTC().Add(time.Minute) - }() - - // Then: the workspace should not be started. - require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID - }, 5*time.Second, 250*time.Millisecond) + // Given: the workspace initially has autostop disabled + require.Empty(t, workspace.AutostopSchedule) + + // When: we enable workspace autostop + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostop(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostopRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should be started + require.Eventually(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart + }, 5*time.Second, 250*time.Millisecond) +} +func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + + // Given: workspace is stopped + MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostopSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostop(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostopRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should not be stopped. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop + }, 5*time.Second, 250*time.Millisecond) +} + +func Test_Executor_Autostop_NotEnabled(t *testing.T) { + t.Parallel() + + var ( + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + + // Given: workspace is running + require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) + + // Given: the workspace has autostop disabled + require.Empty(t, workspace.AutostopSchedule) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + }() + + // Then: the workspace should not be stopped. + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop + }, 5*time.Second, 250*time.Millisecond) +} + +func MustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { + t.Helper() + coderdtest.NewProvisionerDaemon(t, client) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) + return coderdtest.MustWorkspace(t, client, ws.ID) +} + +func MustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) { + t.Helper() + ctx := context.Background() + workspace, err := client.Workspace(ctx, workspaceID) + require.NoError(t, err, "unexpected error fetching workspace") + require.Equal(t, workspace.LatestBuild.Transition, from, "expected workspace state: %s got: %s", from, workspace.LatestBuild.Transition) + + template, err := client.Template(ctx, workspace.TemplateID) + require.NoError(t, err, "fetch workspace template") + + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: to, }) + require.NoError(t, err, "unexpected error transitioning workspace to %s", to) + + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) + + updated := coderdtest.MustWorkspace(t, client, workspace.ID) + require.Equal(t, to, updated.LatestBuild.Transition, "expected workspace to be in state %s but got %s", to, updated.LatestBuild.Transition) } From 579f3628325e3ad8270418a36353113d47c9c421 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Sat, 30 Apr 2022 15:10:21 +0000 Subject: [PATCH 06/28] make the new tests pass with some refactoring --- .../autostart/lifecycle/lifecycle_executor.go | 110 +++++++++--------- .../lifecycle/lifecycle_executor_test.go | 8 +- coderd/database/databasefake/databasefake.go | 26 +++++ coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 94 +++++++++++++++ coderd/database/queries/workspaces.sql | 25 ++++ 6 files changed, 211 insertions(+), 54 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor.go b/coderd/autostart/lifecycle/lifecycle_executor.go index ac2e2df3e736e..f9462735f63b0 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor.go +++ b/coderd/autostart/lifecycle/lifecycle_executor.go @@ -9,15 +9,12 @@ import ( "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/codersdk" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "golang.org/x/xerrors" ) -//var ExecutorUUID = uuid.MustParse("00000000-0000-0000-0000-000000000000") - // Executor executes automated workspace lifecycle operations. type Executor struct { ctx context.Context @@ -26,6 +23,7 @@ type Executor struct { tick <-chan time.Time } +// NewExecutor returns a new instance of Executor. func NewExecutor(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor { le := &Executor{ ctx: ctx, @@ -36,7 +34,10 @@ func NewExecutor(ctx context.Context, db database.Store, log slog.Logger, tick < return le } -func (e *Executor) Run() error { +// Run will cause executor to run workspace lifecycle operations on every +// tick from its channel. It will stop when its context is Done, or when +// its channel is closed. +func (e *Executor) Run() { for { select { case t := <-e.tick: @@ -44,7 +45,7 @@ func (e *Executor) Run() error { e.log.Error(e.ctx, "error running once", slog.Error(err)) } case <-e.ctx.Done(): - return nil + return default: } } @@ -53,65 +54,89 @@ func (e *Executor) Run() error { func (e *Executor) runOnce(t time.Time) error { currentTick := t.Round(time.Minute) return e.db.InTx(func(db database.Store) error { - autostartWorkspaces, err := db.GetWorkspacesAutostart(e.ctx) + eligibleWorkspaces, err := db.GetWorkspacesAutostartAutostop(e.ctx) if err != nil { - return xerrors.Errorf("get all workspaces: %w", err) + return xerrors.Errorf("get eligible workspaces for autostart or autostop: %w", err) } - for _, ws := range autostartWorkspaces { - sched, err := schedule.Weekly(ws.AutostartSchedule.String) - if err != nil { - e.log.Warn(e.ctx, "workspace has invalid autostart schedule", - slog.F("workspace_id", ws.ID), - slog.F("autostart_schedule", ws.AutostartSchedule.String), - ) - continue - } - - // Determine the workspace state based on its latest build. We expect it to be stopped. + for _, ws := range eligibleWorkspaces { + // Determine the workspace state based on its latest build. latestBuild, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) if err != nil { return xerrors.Errorf("get latest build for workspace %q: %w", ws.ID, err) } - if latestBuild.Transition != database.WorkspaceTransitionStop { - e.log.Debug(e.ctx, "autostart: skipping workspace: wrong transition", - slog.F("transition", latestBuild.Transition), + + var validTransition database.WorkspaceTransition + var sched *schedule.Schedule + switch latestBuild.Transition { + case database.WorkspaceTransitionStart: + validTransition = database.WorkspaceTransitionStop + sched, err = schedule.Weekly(ws.AutostopSchedule.String) + if err != nil { + e.log.Warn(e.ctx, "workspace has invalid autostop schedule, skipping", + slog.F("workspace_id", ws.ID), + slog.F("autostart_schedule", ws.AutostopSchedule.String), + ) + continue + } + case database.WorkspaceTransitionStop: + validTransition = database.WorkspaceTransitionStart + sched, err = schedule.Weekly(ws.AutostartSchedule.String) + if err != nil { + e.log.Warn(e.ctx, "workspace has invalid autostart schedule, skipping", + slog.F("workspace_id", ws.ID), + slog.F("autostart_schedule", ws.AutostartSchedule.String), + ) + continue + } + default: + e.log.Debug(e.ctx, "last transition not valid for autostart or autostop", slog.F("workspace_id", ws.ID), + slog.F("latest_build_transition", latestBuild.Transition), ) - continue } // Round time to the nearest minute, as this is the finest granularity cron supports. - earliestAutostart := sched.Next(latestBuild.CreatedAt).Round(time.Minute) - if earliestAutostart.After(currentTick) { - e.log.Debug(e.ctx, "autostart: skipping workspace: too early", + nextTransitionAt := sched.Next(latestBuild.CreatedAt).Round(time.Minute) + if nextTransitionAt.After(currentTick) { + e.log.Debug(e.ctx, "skipping workspace: too early", slog.F("workspace_id", ws.ID), - slog.F("earliest_autostart", earliestAutostart), + slog.F("next_transition_at", nextTransitionAt), + slog.F("transition", validTransition), slog.F("current_tick", currentTick), ) continue } - e.log.Info(e.ctx, "autostart: scheduling workspace start", + e.log.Info(e.ctx, "scheduling workspace transition", slog.F("workspace_id", ws.ID), + slog.F("transition", validTransition), ) - if err := doBuild(e.ctx, db, ws, currentTick); err != nil { - e.log.Error(e.ctx, "autostart workspace", slog.F("workspace_id", ws.ID), slog.Error(err)) + if err := doBuild(e.ctx, db, ws, validTransition); err != nil { + e.log.Error(e.ctx, "transition workspace", + slog.F("workspace_id", ws.ID), + slog.F("transition", validTransition), + slog.Error(err), + ) } } return nil }) } -// XXX: cian: this function shouldn't really exist. Refactor. -func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, now time.Time) error { +// TODO(cian): this function duplicates most of api.postWorkspaceBuilds. Refactor. +func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition) error { template, err := store.GetTemplateByID(ctx, workspace.TemplateID) if err != nil { return xerrors.Errorf("get template: %w", err) } priorHistory, err := store.GetWorkspaceBuildByWorkspaceIDWithoutAfter(ctx, workspace.ID) + if err != nil { + return xerrors.Errorf("get prior history: %w", err) + } + priorJob, err := store.GetProvisionerJobByID(ctx, priorHistory.JobID) if err == nil && !priorJob.CompletedAt.Valid { return xerrors.Errorf("workspace build already active") @@ -160,7 +185,7 @@ func doBuild(ctx context.Context, store database.Store, workspace database.Works Name: namesgenerator.GetRandomName(1), ProvisionerState: priorHistory.ProvisionerState, InitiatorID: workspace.OwnerID, - Transition: database.WorkspaceTransitionStart, + Transition: trans, JobID: newProvisionerJob.ID, }) if err != nil { @@ -184,24 +209,3 @@ func doBuild(ctx context.Context, store database.Store, workspace database.Works } return nil } - -func provisionerJobStatus(j database.ProvisionerJob, now time.Time) codersdk.ProvisionerJobStatus { - switch { - case j.CanceledAt.Valid: - if j.CompletedAt.Valid { - return codersdk.ProvisionerJobCanceled - } - return codersdk.ProvisionerJobCanceling - case !j.StartedAt.Valid: - return codersdk.ProvisionerJobPending - case j.CompletedAt.Valid: - if j.Error.String == "" { - return codersdk.ProvisionerJobSucceeded - } - return codersdk.ProvisionerJobFailed - case now.Sub(j.UpdatedAt) > 30*time.Second: - return codersdk.ProvisionerJobFailed - default: - return codersdk.ProvisionerJobRunning - } -} diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index b543179cc2ae4..439220d1ddef9 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -43,6 +43,7 @@ func Test_Executor_Autostart_OK(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should be started @@ -83,6 +84,7 @@ func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should not be started. @@ -113,6 +115,7 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should not be started. @@ -151,12 +154,13 @@ func Test_Executor_Autostop_OK(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should be started require.Eventually(t, func() bool { ws := coderdtest.MustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart + return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop }, 5*time.Second, 250*time.Millisecond) } func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { @@ -189,6 +193,7 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should not be stopped. @@ -219,6 +224,7 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { // When: the lifecycle executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) }() // Then: the workspace should not be stopped. diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 8eccef9f0d49a..493f0e80f2edd 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -345,6 +345,32 @@ func (q *fakeQuerier) GetWorkspacesAutostart(_ context.Context) ([]database.Work return workspaces, nil } +func (q *fakeQuerier) GetWorkspacesAutostop(_ context.Context) ([]database.Workspace, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + workspaces := make([]database.Workspace, 0) + for _, ws := range q.workspaces { + if ws.AutostopSchedule.String != "" { + workspaces = append(workspaces, ws) + } + } + return workspaces, nil +} + +func (q *fakeQuerier) GetWorkspacesAutostartAutostop(_ context.Context) ([]database.Workspace, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + workspaces := make([]database.Workspace, 0) + for _, ws := range q.workspaces { + if ws.AutostartSchedule.String != "" { + workspaces = append(workspaces, ws) + } else if ws.AutostopSchedule.String != "" { + workspaces = append(workspaces, ws) + } + } + return workspaces, nil +} + func (q *fakeQuerier) GetWorkspaceOwnerCountsByTemplateIDs(_ context.Context, templateIDs []uuid.UUID) ([]database.GetWorkspaceOwnerCountsByTemplateIDsRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index e5cd8a7997a3d..ffc77f1b9c0d7 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -69,6 +69,8 @@ type querier interface { GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) GetWorkspaces(ctx context.Context) ([]Workspace, error) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) + GetWorkspacesAutostartAutostop(ctx context.Context) ([]Workspace, error) + GetWorkspacesAutostop(ctx context.Context) ([]Workspace, error) GetWorkspacesByOrganizationID(ctx context.Context, arg GetWorkspacesByOrganizationIDParams) ([]Workspace, error) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) GetWorkspacesByOwnerID(ctx context.Context, arg GetWorkspacesByOwnerIDParams) ([]Workspace, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d6da0da8123e0..4c244eac5dc7d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3288,6 +3288,100 @@ func (q *sqlQuerier) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, e return items, nil } +const getWorkspacesAutostartAutostop = `-- name: GetWorkspacesAutostartAutostop :many +SELECT + id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule +FROM + workspaces +WHERE + deleted = false +AND +( + autostart_schedule <> '' + OR + autostop_schedule <> '' +) +` + +func (q *sqlQuerier) GetWorkspacesAutostartAutostop(ctx context.Context) ([]Workspace, error) { + rows, err := q.db.QueryContext(ctx, getWorkspacesAutostartAutostop) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Workspace + for rows.Next() { + var i Workspace + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.OwnerID, + &i.OrganizationID, + &i.TemplateID, + &i.Deleted, + &i.Name, + &i.AutostartSchedule, + &i.AutostopSchedule, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const getWorkspacesAutostop = `-- name: GetWorkspacesAutostop :many +SELECT + id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule +FROM + workspaces +WHERE + deleted = false +AND + autostop_schedule <> '' +` + +func (q *sqlQuerier) GetWorkspacesAutostop(ctx context.Context) ([]Workspace, error) { + rows, err := q.db.QueryContext(ctx, getWorkspacesAutostop) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Workspace + for rows.Next() { + var i Workspace + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.OwnerID, + &i.OrganizationID, + &i.TemplateID, + &i.Deleted, + &i.Name, + &i.AutostartSchedule, + &i.AutostopSchedule, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getWorkspacesByOrganizationID = `-- name: GetWorkspacesByOrganizationID :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule FROM workspaces WHERE organization_id = $1 AND deleted = $2 ` diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index eb70d72e655f5..f41780a214029 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -33,6 +33,31 @@ AND autostart_schedule <> '' ; +-- name: GetWorkspacesAutostop :many +SELECT + * +FROM + workspaces +WHERE + deleted = false +AND + autostop_schedule <> '' +; + +-- name: GetWorkspacesAutostartAutostop :many +SELECT + * +FROM + workspaces +WHERE + deleted = false +AND +( + autostart_schedule <> '' + OR + autostop_schedule <> '' +); + -- name: GetWorkspacesByTemplateID :many SELECT * From 6e88f67cb98ee7e2e3d4d3f7bffab85f81a196aa Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Sat, 30 Apr 2022 15:10:31 +0000 Subject: [PATCH 07/28] gitignore *.swp --- .gitignore | 1 + site/.eslintignore | 1 + site/.prettierignore | 2 ++ 3 files changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 6380b3699a8b5..1b310e84cbde2 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,4 @@ site/out/ .terraform/ .vscode/*.log +**/*.swp diff --git a/site/.eslintignore b/site/.eslintignore index ef5e018bd53a5..8c9ebc52810d4 100644 --- a/site/.eslintignore +++ b/site/.eslintignore @@ -10,3 +10,4 @@ coverage storybook-static test-results **/*.typegen.ts +**/*.swp diff --git a/site/.prettierignore b/site/.prettierignore index 5de23f9b61ffe..9599675c1dbef 100644 --- a/site/.prettierignore +++ b/site/.prettierignore @@ -17,3 +17,5 @@ coverage/ out/ storybook-static/ test-results/ + +**/*.swp From 2b1a3837194c638d3cc2446628ea4988f6ff1b1a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Sat, 30 Apr 2022 15:12:48 +0000 Subject: [PATCH 08/28] remove unused methods --- coderd/database/querier.go | 3 - coderd/database/queries.sql.go | 133 --------------------------------- 2 files changed, 136 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index ffc77f1b9c0d7..a4d3bf6290a91 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -67,10 +67,7 @@ type querier interface { GetWorkspaceOwnerCountsByTemplateIDs(ctx context.Context, ids []uuid.UUID) ([]GetWorkspaceOwnerCountsByTemplateIDsRow, error) GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID) (WorkspaceResource, error) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) - GetWorkspaces(ctx context.Context) ([]Workspace, error) - GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) GetWorkspacesAutostartAutostop(ctx context.Context) ([]Workspace, error) - GetWorkspacesAutostop(ctx context.Context) ([]Workspace, error) GetWorkspacesByOrganizationID(ctx context.Context, arg GetWorkspacesByOrganizationIDParams) ([]Workspace, error) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) GetWorkspacesByOwnerID(ctx context.Context, arg GetWorkspacesByOwnerIDParams) ([]Workspace, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4c244eac5dc7d..514429e403a63 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3200,94 +3200,6 @@ func (q *sqlQuerier) GetWorkspaceOwnerCountsByTemplateIDs(ctx context.Context, i return items, nil } -const getWorkspaces = `-- name: GetWorkspaces :many -SELECT - id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule -FROM - workspaces -WHERE - deleted = false -` - -func (q *sqlQuerier) GetWorkspaces(ctx context.Context) ([]Workspace, error) { - rows, err := q.db.QueryContext(ctx, getWorkspaces) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Workspace - for rows.Next() { - var i Workspace - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OwnerID, - &i.OrganizationID, - &i.TemplateID, - &i.Deleted, - &i.Name, - &i.AutostartSchedule, - &i.AutostopSchedule, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - -const getWorkspacesAutostart = `-- name: GetWorkspacesAutostart :many -SELECT - id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule -FROM - workspaces -WHERE - deleted = false -AND - autostart_schedule <> '' -` - -func (q *sqlQuerier) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) { - rows, err := q.db.QueryContext(ctx, getWorkspacesAutostart) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Workspace - for rows.Next() { - var i Workspace - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OwnerID, - &i.OrganizationID, - &i.TemplateID, - &i.Deleted, - &i.Name, - &i.AutostartSchedule, - &i.AutostopSchedule, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const getWorkspacesAutostartAutostop = `-- name: GetWorkspacesAutostartAutostop :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule @@ -3337,51 +3249,6 @@ func (q *sqlQuerier) GetWorkspacesAutostartAutostop(ctx context.Context) ([]Work return items, nil } -const getWorkspacesAutostop = `-- name: GetWorkspacesAutostop :many -SELECT - id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule -FROM - workspaces -WHERE - deleted = false -AND - autostop_schedule <> '' -` - -func (q *sqlQuerier) GetWorkspacesAutostop(ctx context.Context) ([]Workspace, error) { - rows, err := q.db.QueryContext(ctx, getWorkspacesAutostop) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Workspace - for rows.Next() { - var i Workspace - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OwnerID, - &i.OrganizationID, - &i.TemplateID, - &i.Deleted, - &i.Name, - &i.AutostartSchedule, - &i.AutostopSchedule, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const getWorkspacesByOrganizationID = `-- name: GetWorkspacesByOrganizationID :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, autostop_schedule FROM workspaces WHERE organization_id = $1 AND deleted = $2 ` From f31588e757599accad420eb73ffbc19a6f259bac Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Sat, 30 Apr 2022 15:16:27 +0000 Subject: [PATCH 09/28] fixup! remove unused methods --- coderd/database/databasefake/databasefake.go | 32 -------------------- 1 file changed, 32 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 493f0e80f2edd..466b586057205 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -325,38 +325,6 @@ func (q *fakeQuerier) GetWorkspaceByOwnerIDAndName(_ context.Context, arg databa return database.Workspace{}, sql.ErrNoRows } -func (q *fakeQuerier) GetWorkspaces(_ context.Context) ([]database.Workspace, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - workspaces := make([]database.Workspace, len(q.workspaces)) - copy(workspaces, q.workspaces) - return workspaces, nil -} - -func (q *fakeQuerier) GetWorkspacesAutostart(_ context.Context) ([]database.Workspace, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - workspaces := make([]database.Workspace, 0) - for _, ws := range q.workspaces { - if ws.AutostartSchedule.String != "" { - workspaces = append(workspaces, ws) - } - } - return workspaces, nil -} - -func (q *fakeQuerier) GetWorkspacesAutostop(_ context.Context) ([]database.Workspace, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - workspaces := make([]database.Workspace, 0) - for _, ws := range q.workspaces { - if ws.AutostopSchedule.String != "" { - workspaces = append(workspaces, ws) - } - } - return workspaces, nil -} - func (q *fakeQuerier) GetWorkspacesAutostartAutostop(_ context.Context) ([]database.Workspace, error) { q.mutex.RLock() defer q.mutex.RUnlock() From 80e0581c1e387cca6caf8057f24458368e0bb1b5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 9 May 2022 16:26:51 +0000 Subject: [PATCH 10/28] fix: range over channel, add continue to default switch case --- coderd/autostart/lifecycle/lifecycle_executor.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor.go b/coderd/autostart/lifecycle/lifecycle_executor.go index f9462735f63b0..c5b90cad39d2b 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor.go +++ b/coderd/autostart/lifecycle/lifecycle_executor.go @@ -38,15 +38,9 @@ func NewExecutor(ctx context.Context, db database.Store, log slog.Logger, tick < // tick from its channel. It will stop when its context is Done, or when // its channel is closed. func (e *Executor) Run() { - for { - select { - case t := <-e.tick: - if err := e.runOnce(t); err != nil { - e.log.Error(e.ctx, "error running once", slog.Error(err)) - } - case <-e.ctx.Done(): - return - default: + for t := range e.tick { + if err := e.runOnce(t); err != nil { + e.log.Error(e.ctx, "error running once", slog.Error(err)) } } } @@ -94,6 +88,7 @@ func (e *Executor) runOnce(t time.Time) error { slog.F("workspace_id", ws.ID), slog.F("latest_build_transition", latestBuild.Transition), ) + continue } // Round time to the nearest minute, as this is the finest granularity cron supports. From d1764784aa17d37433488546dedf34773e94f431 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 9 May 2022 16:27:01 +0000 Subject: [PATCH 11/28] add test for deleted workspace --- .../lifecycle/lifecycle_executor_test.go | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index 439220d1ddef9..f2a90c74cf9d6 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -234,6 +234,46 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { }, 5*time.Second, 250*time.Millisecond) } +func Test_Executor_Workspace_Deleted(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = MustProvisionWorkspace(t, client) + ) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostopSchedule) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostop(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostopRequest{ + Schedule: sched.String(), + })) + + // Given: workspace is deleted + MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) + }() + + // Then: nothing should happen + require.Never(t, func() bool { + ws := coderdtest.MustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.Transition != database.WorkspaceTransitionDelete + }, 5*time.Second, 250*time.Millisecond) +} + func MustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { t.Helper() coderdtest.NewProvisionerDaemon(t, client) From bd97c1aa176244822cbb1c45eace2d02d27fac36 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 08:16:30 +0000 Subject: [PATCH 12/28] workspaces.sql: remove unused methods --- coderd/database/models.go | 2 ++ coderd/database/querier.go | 2 ++ coderd/database/queries.sql.go | 2 ++ coderd/database/queries/workspaces.sql | 30 -------------------------- 4 files changed, 6 insertions(+), 30 deletions(-) diff --git a/coderd/database/models.go b/coderd/database/models.go index e1c85c3a04a10..ad7277e32cbbe 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1,4 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.13.0 package database diff --git a/coderd/database/querier.go b/coderd/database/querier.go index a4d3bf6290a91..088ed4f0c32c2 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1,4 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.13.0 package database diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 514429e403a63..e7527c2218a77 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1,4 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.13.0 package database diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index f41780a214029..baf42addfc2cd 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -14,36 +14,6 @@ SELECT * FROM workspaces WHERE organization_id = $1 AND deleted = $2; -- name: GetWorkspacesByOrganizationIDs :many SELECT * FROM workspaces WHERE organization_id = ANY(@ids :: uuid [ ]) AND deleted = @deleted; --- name: GetWorkspaces :many -SELECT - * -FROM - workspaces -WHERE - deleted = false; - --- name: GetWorkspacesAutostart :many -SELECT - * -FROM - workspaces -WHERE - deleted = false -AND - autostart_schedule <> '' -; - --- name: GetWorkspacesAutostop :many -SELECT - * -FROM - workspaces -WHERE - deleted = false -AND - autostop_schedule <> '' -; - -- name: GetWorkspacesAutostartAutostop :many SELECT * From 0931d258d8d2ce0cd88d66fade94814105c7a8a1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 19:07:45 +0000 Subject: [PATCH 13/28] unexport test helper methods --- .../lifecycle/lifecycle_executor_test.go | 51 +++++++++++-------- coderd/coderdtest/coderdtest.go | 7 --- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/autostart/lifecycle/lifecycle_executor_test.go index f2a90c74cf9d6..fc8f7a8aaa9ce 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/autostart/lifecycle/lifecycle_executor_test.go @@ -25,10 +25,10 @@ func Test_Executor_Autostart_OK(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is stopped - MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Given: the workspace initially has autostart disabled require.Empty(t, workspace.AutostartSchedule) @@ -48,7 +48,7 @@ func Test_Executor_Autostart_OK(t *testing.T) { // Then: the workspace should be started require.Eventually(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && ws.LatestBuild.Transition == database.WorkspaceTransitionStart }, 5*time.Second, 250*time.Millisecond) @@ -65,7 +65,7 @@ func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: we ensure the workspace is running @@ -89,7 +89,7 @@ func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { // Then: the workspace should not be started. require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart }, 5*time.Second, 250*time.Millisecond) } @@ -103,11 +103,11 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is stopped - MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Given: the workspace has autostart disabled require.Empty(t, workspace.AutostartSchedule) @@ -120,7 +120,7 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { // Then: the workspace should not be started. require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart }, 5*time.Second, 250*time.Millisecond) } @@ -136,7 +136,7 @@ func Test_Executor_Autostop_OK(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is running require.Equal(t, database.WorkspaceTransitionStart, workspace.LatestBuild.Transition) @@ -159,7 +159,7 @@ func Test_Executor_Autostop_OK(t *testing.T) { // Then: the workspace should be started require.Eventually(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop }, 5*time.Second, 250*time.Millisecond) } @@ -174,11 +174,11 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is stopped - MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Given: the workspace initially has autostart disabled require.Empty(t, workspace.AutostopSchedule) @@ -198,7 +198,7 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { // Then: the workspace should not be stopped. require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop }, 5*time.Second, 250*time.Millisecond) } @@ -212,7 +212,7 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is running @@ -229,7 +229,7 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { // Then: the workspace should not be stopped. require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop }, 5*time.Second, 250*time.Millisecond) } @@ -245,7 +245,7 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { LifecycleTicker: tickCh, }) // Given: we have a user with a workspace - workspace = MustProvisionWorkspace(t, client) + workspace = mustProvisionWorkspace(t, client) ) // Given: the workspace initially has autostart disabled @@ -259,7 +259,7 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { })) // Given: workspace is deleted - MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) + mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) // When: the lifecycle executor ticks go func() { @@ -269,12 +269,12 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { // Then: nothing should happen require.Never(t, func() bool { - ws := coderdtest.MustWorkspace(t, client, workspace.ID) + ws := mustWorkspace(t, client, workspace.ID) return ws.LatestBuild.Transition != database.WorkspaceTransitionDelete }, 5*time.Second, 250*time.Millisecond) } -func MustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { +func mustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { t.Helper() coderdtest.NewProvisionerDaemon(t, client) user := coderdtest.CreateFirstUser(t, client) @@ -283,10 +283,10 @@ func MustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Work coderdtest.AwaitTemplateVersionJob(t, client, version.ID) ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) coderdtest.AwaitWorkspaceBuildJob(t, client, ws.LatestBuild.ID) - return coderdtest.MustWorkspace(t, client, ws.ID) + return mustWorkspace(t, client, ws.ID) } -func MustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) { +func mustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) { t.Helper() ctx := context.Background() workspace, err := client.Workspace(ctx, workspaceID) @@ -304,6 +304,13 @@ func MustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID _ = coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) - updated := coderdtest.MustWorkspace(t, client, workspace.ID) + updated := mustWorkspace(t, client, workspace.ID) require.Equal(t, to, updated.LatestBuild.Transition, "expected workspace to be in state %s but got %s", to, updated.LatestBuild.Transition) } + +func mustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) codersdk.Workspace { + ctx := context.Background() + ws, err := client.Workspace(ctx, workspaceID) + require.NoError(t, err, "no workspace found with id %s", workspaceID) + return ws +} diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 20e261a93c765..5fc0769dfb39b 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -508,10 +508,3 @@ type roundTripper func(req *http.Request) (*http.Response, error) func (r roundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return r(req) } - -func MustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) codersdk.Workspace { - ctx := context.Background() - ws, err := client.Workspace(ctx, workspaceID) - require.NoError(t, err, "no workspace found with id %s", workspaceID) - return ws -} From faebe2e27e0917b6481a941212d8d3dec82ff2fe Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 19:27:16 +0000 Subject: [PATCH 14/28] chore: rename package autostart/lifecycle to lifecycle/executor --- cli/autostart.go | 2 +- cli/autostop.go | 2 +- coderd/coderdtest/coderdtest.go | 5 ++--- .../executor}/lifecycle_executor.go | 8 ++++---- .../executor}/lifecycle_executor_test.go | 4 ++-- coderd/{autostart => lifecycle}/schedule/schedule.go | 0 coderd/{autostart => lifecycle}/schedule/schedule_test.go | 2 +- coderd/workspaces.go | 2 +- coderd/workspaces_test.go | 2 +- 9 files changed, 13 insertions(+), 14 deletions(-) rename coderd/{autostart/lifecycle => lifecycle/executor}/lifecycle_executor.go (96%) rename coderd/{autostart/lifecycle => lifecycle/executor}/lifecycle_executor_test.go (99%) rename coderd/{autostart => lifecycle}/schedule/schedule.go (100%) rename coderd/{autostart => lifecycle}/schedule/schedule_test.go (98%) diff --git a/cli/autostart.go b/cli/autostart.go index b3929cac9aff3..3f8d0463de10c 100644 --- a/cli/autostart.go +++ b/cli/autostart.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" - "github.com/coder/coder/coderd/autostart/schedule" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" ) diff --git a/cli/autostop.go b/cli/autostop.go index a76dbb3f95c66..9d4673a9f53f4 100644 --- a/cli/autostop.go +++ b/cli/autostop.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" - "github.com/coder/coder/coderd/autostart/schedule" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" ) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 5fc0769dfb39b..f86afd4a90be8 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -24,8 +24,6 @@ import ( "testing" "time" - "github.com/coder/coder/coderd/autostart/lifecycle" - "cloud.google.com/go/compute/metadata" "github.com/fullsailor/pkcs7" "github.com/golang-jwt/jwt" @@ -43,6 +41,7 @@ import ( "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/coderd/gitsshkey" + "github.com/coder/coder/coderd/lifecycle/executor" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" @@ -105,7 +104,7 @@ func New(t *testing.T, options *Options) *codersdk.Client { } ctx, cancelFunc := context.WithCancel(context.Background()) - lifecycleExecutor := lifecycle.NewExecutor( + lifecycleExecutor := executor.New( ctx, db, slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug), diff --git a/coderd/autostart/lifecycle/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go similarity index 96% rename from coderd/autostart/lifecycle/lifecycle_executor.go rename to coderd/lifecycle/executor/lifecycle_executor.go index c5b90cad39d2b..ce7fb808f584c 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -1,4 +1,4 @@ -package lifecycle +package executor import ( "context" @@ -7,8 +7,8 @@ import ( "cdr.dev/slog" - "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" @@ -23,8 +23,8 @@ type Executor struct { tick <-chan time.Time } -// NewExecutor returns a new instance of Executor. -func NewExecutor(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor { +// New returns a new lifecycle executor. +func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor { le := &Executor{ ctx: ctx, db: db, diff --git a/coderd/autostart/lifecycle/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go similarity index 99% rename from coderd/autostart/lifecycle/lifecycle_executor_test.go rename to coderd/lifecycle/executor/lifecycle_executor_test.go index fc8f7a8aaa9ce..ccf2d4a93f441 100644 --- a/coderd/autostart/lifecycle/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -1,13 +1,13 @@ -package lifecycle_test +package executor_test import ( "context" "testing" "time" - "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" "github.com/google/uuid" diff --git a/coderd/autostart/schedule/schedule.go b/coderd/lifecycle/schedule/schedule.go similarity index 100% rename from coderd/autostart/schedule/schedule.go rename to coderd/lifecycle/schedule/schedule.go diff --git a/coderd/autostart/schedule/schedule_test.go b/coderd/lifecycle/schedule/schedule_test.go similarity index 98% rename from coderd/autostart/schedule/schedule_test.go rename to coderd/lifecycle/schedule/schedule_test.go index d29f5505270fa..095d138becbb7 100644 --- a/coderd/autostart/schedule/schedule_test.go +++ b/coderd/lifecycle/schedule/schedule_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/coder/coder/coderd/autostart/schedule" + "github.com/coder/coder/coderd/lifecycle/schedule" ) func Test_Weekly(t *testing.T) { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 0ac6ae516507c..fc89208de1d03 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -13,10 +13,10 @@ import ( "github.com/moby/moby/pkg/namesgenerator" "golang.org/x/xerrors" - "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" ) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 862414a62bf19..fdc0408811799 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -10,9 +10,9 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" - "github.com/coder/coder/coderd/autostart/schedule" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" From abc08543415dc5354f5320245a4bb116012062f5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 19:35:11 +0000 Subject: [PATCH 15/28] add test to ensure workspaces are not autostarted before time --- .../lifecycle/executor/lifecycle_executor.go | 2 +- .../executor/lifecycle_executor_test.go | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go index ce7fb808f584c..564972300b974 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -109,7 +109,7 @@ func (e *Executor) runOnce(t time.Time) error { ) if err := doBuild(e.ctx, db, ws, validTransition); err != nil { - e.log.Error(e.ctx, "transition workspace", + e.log.Error(e.ctx, "unable to transition workspace", slog.F("workspace_id", ws.ID), slog.F("transition", validTransition), slog.Error(err), diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index ccf2d4a93f441..6066c5f5c573f 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -2,6 +2,7 @@ package executor_test import ( "context" + "fmt" "testing" "time" @@ -274,6 +275,45 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { }, 5*time.Second, 250*time.Millisecond) } +func Test_Executor_Workspace_TooEarly(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = mustProvisionWorkspace(t, client) + ) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostopSchedule) + + // When: we enable workspace autostart with some time in the future + futureTime := time.Now().Add(time.Hour) + futureTimeCron := fmt.Sprintf("%d %d * * *", futureTime.Minute(), futureTime.Hour()) + sched, err := schedule.Weekly(futureTimeCron) + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostop(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostopRequest{ + Schedule: sched.String(), + })) + + // When: the lifecycle executor ticks + go func() { + tickCh <- time.Now().UTC() + close(tickCh) + }() + + // Then: nothing should happen + require.Never(t, func() bool { + ws := mustWorkspace(t, client, workspace.ID) + return ws.LatestBuild.Transition != database.WorkspaceTransitionStart + }, 5*time.Second, 250*time.Millisecond) +} + func mustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { t.Helper() coderdtest.NewProvisionerDaemon(t, client) From e53946af1092bf984a187979b536509620f49582 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 20:45:21 +0000 Subject: [PATCH 16/28] wire up executor to coderd --- cli/server.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cli/server.go b/cli/server.go index 3b2392f51dd9b..2e176d55b2f1e 100644 --- a/cli/server.go +++ b/cli/server.go @@ -43,6 +43,7 @@ import ( "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/devtunnel" "github.com/coder/coder/coderd/gitsshkey" + "github.com/coder/coder/coderd/lifecycle/executor" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" @@ -343,6 +344,11 @@ func server() *cobra.Command { return xerrors.Errorf("notify systemd: %w", err) } + lifecyclePoller := time.NewTicker(30 * time.Second) + defer lifecyclePoller.Stop() + lifecycleExecutor := executor.New(cmd.Context(), options.Database, logger, lifecyclePoller.C) + go lifecycleExecutor.Run() + // Because the graceful shutdown includes cleaning up workspaces in dev mode, we're // going to make it harder to accidentally skip the graceful shutdown by hitting ctrl+c // two or more times. So the stopChan is unlimited in size and we don't call From 364a27c66063b72f93327219f0efef3529b9cac6 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 10 May 2022 20:46:36 +0000 Subject: [PATCH 17/28] fix: executor: skip workspaces whose last build was not successful --- .../lifecycle/executor/lifecycle_executor.go | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go index 564972300b974..a32caf0175f48 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -57,7 +57,28 @@ func (e *Executor) runOnce(t time.Time) error { // Determine the workspace state based on its latest build. latestBuild, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) if err != nil { - return xerrors.Errorf("get latest build for workspace %q: %w", ws.ID, err) + e.log.Warn(e.ctx, "get latest workspace build", + slog.F("workspace_id", ws.ID), + slog.Error(err), + ) + continue + } + + lastBuild, err := db.GetProvisionerJobByID(e.ctx, latestBuild.JobID) + if err != nil { + e.log.Warn(e.ctx, "get last provisioner job for workspace %q: %w", + slog.F("workspace_id", ws.ID), + slog.Error(err), + ) + continue + } + + if !lastBuild.CompletedAt.Valid || lastBuild.Error.String != "" { + e.log.Warn(e.ctx, "last workspace build did not complete successfully, skipping", + slog.F("workspace_id", ws.ID), + slog.F("error", lastBuild.Error.String), + ) + continue } var validTransition database.WorkspaceTransition From e96414fc9faf0bb6bff6170ffa01af9bbb06ecda Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 12:13:42 +0100 Subject: [PATCH 18/28] address PR comments --- cli/server.go | 2 +- .../lifecycle/executor/lifecycle_executor.go | 48 ++++++++----------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/cli/server.go b/cli/server.go index 2e176d55b2f1e..fa57c6742224e 100644 --- a/cli/server.go +++ b/cli/server.go @@ -344,7 +344,7 @@ func server() *cobra.Command { return xerrors.Errorf("notify systemd: %w", err) } - lifecyclePoller := time.NewTicker(30 * time.Second) + lifecyclePoller := time.NewTicker(time.Minute) defer lifecyclePoller.Stop() lifecycleExecutor := executor.New(cmd.Context(), options.Database, logger, lifecyclePoller.C) go lifecycleExecutor.Run() diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go index a32caf0175f48..b63a83a113122 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -46,7 +46,7 @@ func (e *Executor) Run() { } func (e *Executor) runOnce(t time.Time) error { - currentTick := t.Round(time.Minute) + currentTick := t.Truncate(time.Minute) return e.db.InTx(func(db database.Store) error { eligibleWorkspaces, err := db.GetWorkspacesAutostartAutostop(e.ctx) if err != nil { @@ -55,7 +55,7 @@ func (e *Executor) runOnce(t time.Time) error { for _, ws := range eligibleWorkspaces { // Determine the workspace state based on its latest build. - latestBuild, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) + priorHistory, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID) if err != nil { e.log.Warn(e.ctx, "get latest workspace build", slog.F("workspace_id", ws.ID), @@ -64,7 +64,7 @@ func (e *Executor) runOnce(t time.Time) error { continue } - lastBuild, err := db.GetProvisionerJobByID(e.ctx, latestBuild.JobID) + priorJob, err := db.GetProvisionerJobByID(e.ctx, priorHistory.JobID) if err != nil { e.log.Warn(e.ctx, "get last provisioner job for workspace %q: %w", slog.F("workspace_id", ws.ID), @@ -73,17 +73,17 @@ func (e *Executor) runOnce(t time.Time) error { continue } - if !lastBuild.CompletedAt.Valid || lastBuild.Error.String != "" { + if !priorJob.CompletedAt.Valid || priorJob.Error.String != "" { e.log.Warn(e.ctx, "last workspace build did not complete successfully, skipping", slog.F("workspace_id", ws.ID), - slog.F("error", lastBuild.Error.String), + slog.F("error", priorJob.Error.String), ) continue } var validTransition database.WorkspaceTransition var sched *schedule.Schedule - switch latestBuild.Transition { + switch priorHistory.Transition { case database.WorkspaceTransitionStart: validTransition = database.WorkspaceTransitionStop sched, err = schedule.Weekly(ws.AutostopSchedule.String) @@ -107,14 +107,15 @@ func (e *Executor) runOnce(t time.Time) error { default: e.log.Debug(e.ctx, "last transition not valid for autostart or autostop", slog.F("workspace_id", ws.ID), - slog.F("latest_build_transition", latestBuild.Transition), + slog.F("latest_build_transition", priorHistory.Transition), ) continue } - // Round time to the nearest minute, as this is the finest granularity cron supports. - nextTransitionAt := sched.Next(latestBuild.CreatedAt).Round(time.Minute) - if nextTransitionAt.After(currentTick) { + // Round time down to the nearest minute, as this is the finest granularity cron supports. + // Truncate is probably not necessary here, but doing it anyway to be sure. + nextTransitionAt := sched.Next(priorHistory.CreatedAt).Truncate(time.Minute) + if currentTick.Before(nextTransitionAt) { e.log.Debug(e.ctx, "skipping workspace: too early", slog.F("workspace_id", ws.ID), slog.F("next_transition_at", nextTransitionAt), @@ -129,7 +130,7 @@ func (e *Executor) runOnce(t time.Time) error { slog.F("transition", validTransition), ) - if err := doBuild(e.ctx, db, ws, validTransition); err != nil { + if err := doBuild(e.ctx, db, ws, validTransition, priorHistory, priorJob); err != nil { e.log.Error(e.ctx, "unable to transition workspace", slog.F("workspace_id", ws.ID), slog.F("transition", validTransition), @@ -142,20 +143,10 @@ func (e *Executor) runOnce(t time.Time) error { } // TODO(cian): this function duplicates most of api.postWorkspaceBuilds. Refactor. -func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition) error { +func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition, priorHistory database.WorkspaceBuild, priorJob database.ProvisionerJob) error { template, err := store.GetTemplateByID(ctx, workspace.TemplateID) if err != nil { - return xerrors.Errorf("get template: %w", err) - } - - priorHistory, err := store.GetWorkspaceBuildByWorkspaceIDWithoutAfter(ctx, workspace.ID) - if err != nil { - return xerrors.Errorf("get prior history: %w", err) - } - - priorJob, err := store.GetProvisionerJobByID(ctx, priorHistory.JobID) - if err == nil && !priorJob.CompletedAt.Valid { - return xerrors.Errorf("workspace build already active") + return xerrors.Errorf("get workspace template: %w", err) } priorHistoryID := uuid.NullUUID{ @@ -176,10 +167,11 @@ func doBuild(ctx context.Context, store database.Store, workspace database.Works return xerrors.Errorf("marshal provision job: %w", err) } provisionerJobID := uuid.New() + now := database.Now() newProvisionerJob, err := store.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ ID: provisionerJobID, - CreatedAt: database.Now(), - UpdatedAt: database.Now(), + CreatedAt: now, + UpdatedAt: now, InitiatorID: workspace.OwnerID, OrganizationID: template.OrganizationID, Provisioner: template.Provisioner, @@ -193,8 +185,8 @@ func doBuild(ctx context.Context, store database.Store, workspace database.Works } newWorkspaceBuild, err = store.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ ID: workspaceBuildID, - CreatedAt: database.Now(), - UpdatedAt: database.Now(), + CreatedAt: now, + UpdatedAt: now, WorkspaceID: workspace.ID, TemplateVersionID: priorHistory.TemplateVersionID, BeforeID: priorHistoryID, @@ -213,7 +205,7 @@ func doBuild(ctx context.Context, store database.Store, workspace database.Works err = store.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ ID: priorHistory.ID, ProvisionerState: priorHistory.ProvisionerState, - UpdatedAt: database.Now(), + UpdatedAt: now, AfterID: uuid.NullUUID{ UUID: newWorkspaceBuild.ID, Valid: true, From b5bf50e7fc16adedde0b754276d78497a2ed90eb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 12:14:36 +0100 Subject: [PATCH 19/28] add goleak TestMain --- coderd/lifecycle/executor/lifecycle_executor_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index 6066c5f5c573f..0f47cf5a551df 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -10,6 +10,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" + "go.uber.org/goleak" "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -353,4 +354,8 @@ func mustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) ws, err := client.Workspace(ctx, workspaceID) require.NoError(t, err, "no workspace found with id %s", workspaceID) return ws + +} +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) } From d37cc2b220b12408683e3b0644e15e2e6511be8d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 12:39:38 +0100 Subject: [PATCH 20/28] fmt --- coderd/lifecycle/executor/lifecycle_executor_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index 0f47cf5a551df..730390eb1bae6 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -6,11 +6,12 @@ import ( "testing" "time" + "go.uber.org/goleak" + "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" - "go.uber.org/goleak" "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -354,8 +355,8 @@ func mustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) ws, err := client.Workspace(ctx, workspaceID) require.NoError(t, err, "no workspace found with id %s", workspaceID) return ws - } + func TestMain(m *testing.M) { goleak.VerifyTestMain(m) } From d11f5d75a780e0fddadf66250327577d7ffa43f7 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 14:34:56 +0100 Subject: [PATCH 21/28] mustTransitionWorkspace should return the updated workspace --- .../lifecycle/executor/lifecycle_executor_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index 730390eb1bae6..a24fd4e7cd86b 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -31,7 +31,7 @@ func Test_Executor_Autostart_OK(t *testing.T) { workspace = mustProvisionWorkspace(t, client) ) // Given: workspace is stopped - mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Given: the workspace initially has autostart disabled require.Empty(t, workspace.AutostartSchedule) @@ -110,7 +110,7 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { ) // Given: workspace is stopped - mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) // Given: the workspace has autostart disabled require.Empty(t, workspace.AutostartSchedule) @@ -181,9 +181,9 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { ) // Given: workspace is stopped - mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) - // Given: the workspace initially has autostart disabled + // Given: the workspace initially has autostop disabled require.Empty(t, workspace.AutostopSchedule) // When: we enable workspace autostart @@ -262,7 +262,7 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { })) // Given: workspace is deleted - mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) + workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) // When: the lifecycle executor ticks go func() { @@ -328,7 +328,7 @@ func mustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Work return mustWorkspace(t, client, ws.ID) } -func mustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) { +func mustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) codersdk.Workspace { t.Helper() ctx := context.Background() workspace, err := client.Workspace(ctx, workspaceID) @@ -348,6 +348,7 @@ func mustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID updated := mustWorkspace(t, client, workspace.ID) require.Equal(t, to, updated.LatestBuild.Transition, "expected workspace to be in state %s but got %s", to, updated.LatestBuild.Transition) + return updated } func mustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) codersdk.Workspace { From f6388b4552bffea673ca5b95092567fcc5adb259 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 14:36:01 +0100 Subject: [PATCH 22/28] remove usage of require.Eventually/Never which is flaky on Windows --- .../executor/lifecycle_executor_test.go | 68 ++++++++++--------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index a24fd4e7cd86b..993fd35e2bf56 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -50,11 +50,11 @@ func Test_Executor_Autostart_OK(t *testing.T) { }() // Then: the workspace should be started - require.Eventually(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobSucceeded && - ws.LatestBuild.Transition == database.WorkspaceTransitionStart - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.NotEqual(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected a workspace build to occur") + require.Equal(t, codersdk.ProvisionerJobSucceeded, ws.LatestBuild.Job.Status, "expected provisioner job to have succeeded") + require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected latest transition to be start") } func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { @@ -91,10 +91,10 @@ func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { }() // Then: the workspace should not be started. - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running") } func Test_Executor_Autostart_NotEnabled(t *testing.T) { @@ -122,10 +122,10 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { }() // Then: the workspace should not be started. - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStart - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.NotEqual(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace not to be running") } func Test_Executor_Autostop_OK(t *testing.T) { @@ -161,11 +161,13 @@ func Test_Executor_Autostop_OK(t *testing.T) { }() // Then: the workspace should be started - require.Eventually(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID != workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.NotEqual(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected a workspace build to occur") + require.Equal(t, codersdk.ProvisionerJobSucceeded, ws.LatestBuild.Job.Status, "expected provisioner job to have succeeded") + require.Equal(t, database.WorkspaceTransitionStop, ws.LatestBuild.Transition, "expected workspace not to be running") } + func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { t.Parallel() @@ -200,10 +202,10 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { }() // Then: the workspace should not be stopped. - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.Equal(t, database.WorkspaceTransitionStop, ws.LatestBuild.Transition, "expected workspace not to be running") } func Test_Executor_Autostop_NotEnabled(t *testing.T) { @@ -231,10 +233,10 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { }() // Then: the workspace should not be stopped. - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.ID == workspace.LatestBuild.ID && ws.LatestBuild.Transition == database.WorkspaceTransitionStop - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running") } func Test_Executor_Workspace_Deleted(t *testing.T) { @@ -271,10 +273,10 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { }() // Then: nothing should happen - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.Transition != database.WorkspaceTransitionDelete - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.Equal(t, database.WorkspaceTransitionDelete, ws.LatestBuild.Transition, "expected workspace to be deleted") } func Test_Executor_Workspace_TooEarly(t *testing.T) { @@ -310,10 +312,10 @@ func Test_Executor_Workspace_TooEarly(t *testing.T) { }() // Then: nothing should happen - require.Never(t, func() bool { - ws := mustWorkspace(t, client, workspace.ID) - return ws.LatestBuild.Transition != database.WorkspaceTransitionStart - }, 5*time.Second, 250*time.Millisecond) + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur") + require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running") } func mustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace { From fd0f8a34665bd04fab1ce1a791a4af415e115050 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 16:22:40 +0100 Subject: [PATCH 23/28] make lifecycle executor spawn a new goroutine automatically --- cli/server.go | 2 +- coderd/coderdtest/coderdtest.go | 2 +- coderd/lifecycle/executor/lifecycle_executor.go | 10 ++++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cli/server.go b/cli/server.go index fa57c6742224e..66e78bb9cd674 100644 --- a/cli/server.go +++ b/cli/server.go @@ -347,7 +347,7 @@ func server() *cobra.Command { lifecyclePoller := time.NewTicker(time.Minute) defer lifecyclePoller.Stop() lifecycleExecutor := executor.New(cmd.Context(), options.Database, logger, lifecyclePoller.C) - go lifecycleExecutor.Run() + lifecycleExecutor.Run() // Because the graceful shutdown includes cleaning up workspaces in dev mode, we're // going to make it harder to accidentally skip the graceful shutdown by hitting ctrl+c diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index f86afd4a90be8..89683c2be2699 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -110,7 +110,7 @@ func New(t *testing.T, options *Options) *codersdk.Client { slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug), options.LifecycleTicker, ) - go lifecycleExecutor.Run() + lifecycleExecutor.Run() srv := httptest.NewUnstartedServer(nil) srv.Config.BaseContext = func(_ net.Listener) context.Context { diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go index b63a83a113122..22867195c69fa 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -38,11 +38,13 @@ func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan ti // tick from its channel. It will stop when its context is Done, or when // its channel is closed. func (e *Executor) Run() { - for t := range e.tick { - if err := e.runOnce(t); err != nil { - e.log.Error(e.ctx, "error running once", slog.Error(err)) + go func() { + for t := range e.tick { + if err := e.runOnce(t); err != nil { + e.log.Error(e.ctx, "error running once", slog.Error(err)) + } } - } + }() } func (e *Executor) runOnce(t time.Time) error { From 7b6f2e16b27af5fe0176b55cebf325ef063ab41d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 16:22:52 +0100 Subject: [PATCH 24/28] rename unit tests --- .../executor/lifecycle_executor_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/lifecycle/executor/lifecycle_executor_test.go index 993fd35e2bf56..25719662e8ac5 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/lifecycle/executor/lifecycle_executor_test.go @@ -17,7 +17,7 @@ import ( "github.com/stretchr/testify/require" ) -func Test_Executor_Autostart_OK(t *testing.T) { +func TestExecutorAutostartOK(t *testing.T) { t.Parallel() var ( @@ -57,7 +57,7 @@ func Test_Executor_Autostart_OK(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected latest transition to be start") } -func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { +func TestExecutorAutostartAlreadyRunning(t *testing.T) { t.Parallel() var ( @@ -97,7 +97,7 @@ func Test_Executor_Autostart_AlreadyRunning(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running") } -func Test_Executor_Autostart_NotEnabled(t *testing.T) { +func TestExecutorAutostartNotEnabled(t *testing.T) { t.Parallel() var ( @@ -128,7 +128,7 @@ func Test_Executor_Autostart_NotEnabled(t *testing.T) { require.NotEqual(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace not to be running") } -func Test_Executor_Autostop_OK(t *testing.T) { +func TestExecutorAutostopOK(t *testing.T) { t.Parallel() var ( @@ -168,7 +168,7 @@ func Test_Executor_Autostop_OK(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStop, ws.LatestBuild.Transition, "expected workspace not to be running") } -func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { +func TestExecutorAutostopAlreadyStopped(t *testing.T) { t.Parallel() var ( @@ -208,7 +208,7 @@ func Test_Executor_Autostop_AlreadyStopped(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStop, ws.LatestBuild.Transition, "expected workspace not to be running") } -func Test_Executor_Autostop_NotEnabled(t *testing.T) { +func TestExecutorAutostopNotEnabled(t *testing.T) { t.Parallel() var ( @@ -239,7 +239,7 @@ func Test_Executor_Autostop_NotEnabled(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running") } -func Test_Executor_Workspace_Deleted(t *testing.T) { +func TestExecutorWorkspaceDeleted(t *testing.T) { t.Parallel() var ( @@ -279,7 +279,7 @@ func Test_Executor_Workspace_Deleted(t *testing.T) { require.Equal(t, database.WorkspaceTransitionDelete, ws.LatestBuild.Transition, "expected workspace to be deleted") } -func Test_Executor_Workspace_TooEarly(t *testing.T) { +func TestExecutorWorkspaceTooEarly(t *testing.T) { t.Parallel() var ( From a7143bdbe7b74db42772ed96a4a0678b985b2a73 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 16:24:43 +0100 Subject: [PATCH 25/28] s/doBuild/build --- coderd/lifecycle/executor/lifecycle_executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/lifecycle/executor/lifecycle_executor.go index 22867195c69fa..d339356c0dbaf 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/lifecycle/executor/lifecycle_executor.go @@ -132,7 +132,7 @@ func (e *Executor) runOnce(t time.Time) error { slog.F("transition", validTransition), ) - if err := doBuild(e.ctx, db, ws, validTransition, priorHistory, priorJob); err != nil { + if err := build(e.ctx, db, ws, validTransition, priorHistory, priorJob); err != nil { e.log.Error(e.ctx, "unable to transition workspace", slog.F("workspace_id", ws.ID), slog.F("transition", validTransition), @@ -145,7 +145,7 @@ func (e *Executor) runOnce(t time.Time) error { } // TODO(cian): this function duplicates most of api.postWorkspaceBuilds. Refactor. -func doBuild(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition, priorHistory database.WorkspaceBuild, priorJob database.ProvisionerJob) error { +func build(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition, priorHistory database.WorkspaceBuild, priorJob database.ProvisionerJob) error { template, err := store.GetTemplateByID(ctx, workspace.TemplateID) if err != nil { return xerrors.Errorf("get workspace template: %w", err) From 7d9b6965e91cdaf5720af2792d47e32a2ea59a05 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 16:29:33 +0100 Subject: [PATCH 26/28] rename parent package lifecycle to autobuild --- cli/autostart.go | 2 +- cli/autostop.go | 2 +- cli/server.go | 2 +- .../executor/lifecycle_executor.go | 8 ++++---- .../executor/lifecycle_executor_test.go | 18 +++++++++--------- .../schedule/schedule.go | 0 .../schedule/schedule_test.go | 2 +- coderd/coderdtest/coderdtest.go | 4 ++-- coderd/workspaces.go | 2 +- coderd/workspaces_test.go | 2 +- 10 files changed, 21 insertions(+), 21 deletions(-) rename coderd/{lifecycle => autobuild}/executor/lifecycle_executor.go (96%) rename coderd/{lifecycle => autobuild}/executor/lifecycle_executor_test.go (97%) rename coderd/{lifecycle => autobuild}/schedule/schedule.go (100%) rename coderd/{lifecycle => autobuild}/schedule/schedule_test.go (98%) diff --git a/cli/autostart.go b/cli/autostart.go index 3f8d0463de10c..b2c3fe37a0596 100644 --- a/cli/autostart.go +++ b/cli/autostart.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" - "github.com/coder/coder/coderd/lifecycle/schedule" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/codersdk" ) diff --git a/cli/autostop.go b/cli/autostop.go index 9d4673a9f53f4..e344338707a50 100644 --- a/cli/autostop.go +++ b/cli/autostop.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" - "github.com/coder/coder/coderd/lifecycle/schedule" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/codersdk" ) diff --git a/cli/server.go b/cli/server.go index 66e78bb9cd674..00c3db73e0623 100644 --- a/cli/server.go +++ b/cli/server.go @@ -39,11 +39,11 @@ import ( "github.com/coder/coder/cli/cliui" "github.com/coder/coder/cli/config" "github.com/coder/coder/coderd" + "github.com/coder/coder/coderd/autobuild/executor" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/devtunnel" "github.com/coder/coder/coderd/gitsshkey" - "github.com/coder/coder/coderd/lifecycle/executor" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" diff --git a/coderd/lifecycle/executor/lifecycle_executor.go b/coderd/autobuild/executor/lifecycle_executor.go similarity index 96% rename from coderd/lifecycle/executor/lifecycle_executor.go rename to coderd/autobuild/executor/lifecycle_executor.go index d339356c0dbaf..5e9d06d74a637 100644 --- a/coderd/lifecycle/executor/lifecycle_executor.go +++ b/coderd/autobuild/executor/lifecycle_executor.go @@ -7,15 +7,15 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "golang.org/x/xerrors" ) -// Executor executes automated workspace lifecycle operations. +// Executor automatically starts or stops workspaces. type Executor struct { ctx context.Context db database.Store @@ -23,7 +23,7 @@ type Executor struct { tick <-chan time.Time } -// New returns a new lifecycle executor. +// New returns a new autobuild executor. func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor { le := &Executor{ ctx: ctx, @@ -34,7 +34,7 @@ func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan ti return le } -// Run will cause executor to run workspace lifecycle operations on every +// Run will cause executor to start or stop workspaces on every // tick from its channel. It will stop when its context is Done, or when // its channel is closed. func (e *Executor) Run() { diff --git a/coderd/lifecycle/executor/lifecycle_executor_test.go b/coderd/autobuild/executor/lifecycle_executor_test.go similarity index 97% rename from coderd/lifecycle/executor/lifecycle_executor_test.go rename to coderd/autobuild/executor/lifecycle_executor_test.go index 25719662e8ac5..fddb9285df935 100644 --- a/coderd/lifecycle/executor/lifecycle_executor_test.go +++ b/coderd/autobuild/executor/lifecycle_executor_test.go @@ -8,9 +8,9 @@ import ( "go.uber.org/goleak" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" "github.com/google/uuid" @@ -43,7 +43,7 @@ func TestExecutorAutostartOK(t *testing.T) { Schedule: sched.String(), })) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -84,7 +84,7 @@ func TestExecutorAutostartAlreadyRunning(t *testing.T) { Schedule: sched.String(), })) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -115,7 +115,7 @@ func TestExecutorAutostartNotEnabled(t *testing.T) { // Given: the workspace has autostart disabled require.Empty(t, workspace.AutostartSchedule) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -154,7 +154,7 @@ func TestExecutorAutostopOK(t *testing.T) { Schedule: sched.String(), })) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -195,7 +195,7 @@ func TestExecutorAutostopAlreadyStopped(t *testing.T) { Schedule: sched.String(), })) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -226,7 +226,7 @@ func TestExecutorAutostopNotEnabled(t *testing.T) { // Given: the workspace has autostop disabled require.Empty(t, workspace.AutostopSchedule) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -266,7 +266,7 @@ func TestExecutorWorkspaceDeleted(t *testing.T) { // Given: workspace is deleted workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC().Add(time.Minute) close(tickCh) @@ -305,7 +305,7 @@ func TestExecutorWorkspaceTooEarly(t *testing.T) { Schedule: sched.String(), })) - // When: the lifecycle executor ticks + // When: the autobuild executor ticks go func() { tickCh <- time.Now().UTC() close(tickCh) diff --git a/coderd/lifecycle/schedule/schedule.go b/coderd/autobuild/schedule/schedule.go similarity index 100% rename from coderd/lifecycle/schedule/schedule.go rename to coderd/autobuild/schedule/schedule.go diff --git a/coderd/lifecycle/schedule/schedule_test.go b/coderd/autobuild/schedule/schedule_test.go similarity index 98% rename from coderd/lifecycle/schedule/schedule_test.go rename to coderd/autobuild/schedule/schedule_test.go index 095d138becbb7..1109ced96c93d 100644 --- a/coderd/lifecycle/schedule/schedule_test.go +++ b/coderd/autobuild/schedule/schedule_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/coder/coder/coderd/lifecycle/schedule" + "github.com/coder/coder/coderd/autobuild/schedule" ) func Test_Weekly(t *testing.T) { diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 89683c2be2699..535b5a49ca166 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -36,12 +36,12 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/coderd" + "github.com/coder/coder/coderd/autobuild/executor" "github.com/coder/coder/coderd/awsidentity" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/coderd/gitsshkey" - "github.com/coder/coder/coderd/lifecycle/executor" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" @@ -107,7 +107,7 @@ func New(t *testing.T, options *Options) *codersdk.Client { lifecycleExecutor := executor.New( ctx, db, - slogtest.Make(t, nil).Named("lifecycle.executor").Leveled(slog.LevelDebug), + slogtest.Make(t, nil).Named("autobuild.executor").Leveled(slog.LevelDebug), options.LifecycleTicker, ) lifecycleExecutor.Run() diff --git a/coderd/workspaces.go b/coderd/workspaces.go index fc89208de1d03..76384f7e1050c 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -13,10 +13,10 @@ import ( "github.com/moby/moby/pkg/namesgenerator" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" - "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" ) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index fdc0408811799..f7f31e0017637 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -10,9 +10,9 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" + "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/lifecycle/schedule" "github.com/coder/coder/codersdk" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" From 5cba737d8986d00ad45fbf7bb085a135639e760f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 22:44:26 +0100 Subject: [PATCH 27/28] add unit test for behaviour with an updated template --- .../executor/lifecycle_executor_test.go | 52 +++++++++++++++++++ coderd/coderdtest/coderdtest.go | 17 ++++++ 2 files changed, 69 insertions(+) diff --git a/coderd/autobuild/executor/lifecycle_executor_test.go b/coderd/autobuild/executor/lifecycle_executor_test.go index fddb9285df935..330662f3fc905 100644 --- a/coderd/autobuild/executor/lifecycle_executor_test.go +++ b/coderd/autobuild/executor/lifecycle_executor_test.go @@ -57,6 +57,58 @@ func TestExecutorAutostartOK(t *testing.T) { require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected latest transition to be start") } +func TestExecutorAutostartTemplateUpdated(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + err error + tickCh = make(chan time.Time) + client = coderdtest.New(t, &coderdtest.Options{ + LifecycleTicker: tickCh, + }) + // Given: we have a user with a workspace + workspace = mustProvisionWorkspace(t, client) + ) + // Given: workspace is stopped + workspace = mustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Given: the workspace initially has autostart disabled + require.Empty(t, workspace.AutostartSchedule) + + // Given: the workspace template has been updated + orgs, err := client.OrganizationsByUser(ctx, workspace.OwnerID) + require.NoError(t, err) + require.Len(t, orgs, 1) + + newVersion := coderdtest.UpdateTemplateVersion(t, client, orgs[0].ID, nil, workspace.TemplateID) + coderdtest.AwaitTemplateVersionJob(t, client, newVersion.ID) + require.NoError(t, client.UpdateActiveTemplateVersion(ctx, workspace.TemplateID, codersdk.UpdateActiveTemplateVersion{ + ID: newVersion.ID, + })) + + // When: we enable workspace autostart + sched, err := schedule.Weekly("* * * * *") + require.NoError(t, err) + require.NoError(t, client.UpdateWorkspaceAutostart(ctx, workspace.ID, codersdk.UpdateWorkspaceAutostartRequest{ + Schedule: sched.String(), + })) + + // When: the autobuild executor ticks + go func() { + tickCh <- time.Now().UTC().Add(time.Minute) + close(tickCh) + }() + + // Then: the workspace should be started using the previous template version, and not the updated version. + <-time.After(5 * time.Second) + ws := mustWorkspace(t, client, workspace.ID) + require.NotEqual(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected a workspace build to occur") + require.Equal(t, codersdk.ProvisionerJobSucceeded, ws.LatestBuild.Job.Status, "expected provisioner job to have succeeded") + require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected latest transition to be start") + require.Equal(t, workspace.LatestBuild.TemplateVersionID, ws.LatestBuild.TemplateVersionID, "expected workspace build to be using the old template version") +} + func TestExecutorAutostartAlreadyRunning(t *testing.T) { t.Parallel() diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 535b5a49ca166..2441dbff95946 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -261,6 +261,23 @@ func CreateTemplate(t *testing.T, client *codersdk.Client, organization uuid.UUI return template } +// UpdateTemplateVersion creates a new template version with the "echo" provisioner +// and associates it with the given templateID. +func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, res *echo.Responses, templateID uuid.UUID) codersdk.TemplateVersion { + data, err := echo.Tar(res) + require.NoError(t, err) + file, err := client.Upload(context.Background(), codersdk.ContentTypeTar, data) + require.NoError(t, err) + templateVersion, err := client.CreateTemplateVersion(context.Background(), organizationID, codersdk.CreateTemplateVersionRequest{ + TemplateID: templateID, + StorageSource: file.Hash, + StorageMethod: database.ProvisionerStorageMethodFile, + Provisioner: database.ProvisionerTypeEcho, + }) + require.NoError(t, err) + return templateVersion +} + // AwaitTemplateImportJob awaits for an import job to reach completed status. func AwaitTemplateVersionJob(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion { var templateVersion codersdk.TemplateVersion From 7627372aef8635a89fb4184fff0d8300eac81f21 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 11 May 2022 22:48:37 +0100 Subject: [PATCH 28/28] add ticket to reference TODO --- coderd/autobuild/executor/lifecycle_executor.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/autobuild/executor/lifecycle_executor.go b/coderd/autobuild/executor/lifecycle_executor.go index 5e9d06d74a637..3fd1eb7fc28af 100644 --- a/coderd/autobuild/executor/lifecycle_executor.go +++ b/coderd/autobuild/executor/lifecycle_executor.go @@ -145,6 +145,7 @@ func (e *Executor) runOnce(t time.Time) error { } // TODO(cian): this function duplicates most of api.postWorkspaceBuilds. Refactor. +// See: https://github.com/coder/coder/issues/1401 func build(ctx context.Context, store database.Store, workspace database.Workspace, trans database.WorkspaceTransition, priorHistory database.WorkspaceBuild, priorJob database.ProvisionerJob) error { template, err := store.GetTemplateByID(ctx, workspace.TemplateID) if err != nil {