From 0893e9f0a8100d33300d6d1cb5e6bbc715b17e2d Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Wed, 3 Jan 2024 10:38:19 +0400 Subject: [PATCH] fix: change coder start to be a no-op if workspace is started --- cli/start.go | 37 +++++++++---- cli/start_test.go | 65 ++++++++++++++++++++++ coderd/database/dbfake/dbfake.go | 93 +++++++++++++++++++++++++++----- 3 files changed, 172 insertions(+), 23 deletions(-) diff --git a/cli/start.go b/cli/start.go index d5c51ddc3ad38..a2daebde3c0cc 100644 --- a/cli/start.go +++ b/cli/start.go @@ -30,18 +30,33 @@ func (r *RootCmd) start() *clibase.Cmd { if err != nil { return err } - - build, err := startWorkspace(inv, client, workspace, parameterFlags, WorkspaceStart) - // It's possible for a workspace build to fail due to the template requiring starting - // workspaces with the active version. - if cerr, ok := codersdk.AsError(err); ok && cerr.StatusCode() == http.StatusForbidden { - _, _ = fmt.Fprintln(inv.Stdout, "Failed to restart with the template version from your last build. Policy may require you to restart with the current active template version.") - build, err = startWorkspace(inv, client, workspace, parameterFlags, WorkspaceUpdate) - if err != nil { - return xerrors.Errorf("start workspace with active template version: %w", err) + var build codersdk.WorkspaceBuild + switch workspace.LatestBuild.Status { + case codersdk.WorkspaceStatusRunning: + _, _ = fmt.Fprintf( + inv.Stdout, "\nThe %s workspace is already running!\n", + cliui.Keyword(workspace.Name), + ) + return nil + case codersdk.WorkspaceStatusStarting: + _, _ = fmt.Fprintf( + inv.Stdout, "\nThe %s workspace is already starting.\n", + cliui.Keyword(workspace.Name), + ) + build = workspace.LatestBuild + default: + build, err = startWorkspace(inv, client, workspace, parameterFlags, WorkspaceStart) + // It's possible for a workspace build to fail due to the template requiring starting + // workspaces with the active version. + if cerr, ok := codersdk.AsError(err); ok && cerr.StatusCode() == http.StatusForbidden { + _, _ = fmt.Fprintln(inv.Stdout, "Failed to restart with the template version from your last build. Policy may require you to restart with the current active template version.") + build, err = startWorkspace(inv, client, workspace, parameterFlags, WorkspaceUpdate) + if err != nil { + return xerrors.Errorf("start workspace with active template version: %w", err) + } + } else if err != nil { + return err } - } else if err != nil { - return err } err = cliui.WorkspaceBuild(inv.Context(), inv.Stdout, client, build.ID) diff --git a/cli/start_test.go b/cli/start_test.go index f7db8b867342f..5f85b41750f04 100644 --- a/cli/start_test.go +++ b/cli/start_test.go @@ -11,6 +11,7 @@ import ( "github.com/coder/coder/v2/cli/clitest" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbfake" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisioner/echo" "github.com/coder/coder/v2/provisionersdk/proto" @@ -109,6 +110,9 @@ func TestStart(t *testing.T) { template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, member, owner.OrganizationID, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + // Stop the workspace + workspaceBuild := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStop) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspaceBuild.ID) inv, root := clitest.New(t, "start", workspace.Name, "--build-options") clitest.SetupConfig(t, member, root) @@ -160,6 +164,9 @@ func TestStart(t *testing.T) { template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, member, owner.OrganizationID, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + // Stop the workspace + workspaceBuild := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStop) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspaceBuild.ID) inv, root := clitest.New(t, "start", workspace.Name, "--build-option", fmt.Sprintf("%s=%s", ephemeralParameterName, ephemeralParameterValue)) @@ -374,3 +381,61 @@ func TestStartAutoUpdate(t *testing.T) { }) } } + +func TestStart_AlreadyRunning(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + client, db := coderdtest.NewWithDatabase(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + r := dbfake.WorkspaceBuild(t, db, database.Workspace{ + OwnerID: member.ID, + OrganizationID: owner.OrganizationID, + }).Do() + + inv, root := clitest.New(t, "start", r.Workspace.Name) + clitest.SetupConfig(t, memberClient, root) + doneChan := make(chan struct{}) + pty := ptytest.New(t).Attach(inv) + go func() { + defer close(doneChan) + err := inv.Run() + assert.NoError(t, err) + }() + + pty.ExpectMatch("workspace is already running") + _ = testutil.RequireRecvCtx(ctx, t, doneChan) +} + +func TestStart_Starting(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + client, db := coderdtest.NewWithDatabase(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + r := dbfake.WorkspaceBuild(t, db, database.Workspace{ + OwnerID: member.ID, + OrganizationID: owner.OrganizationID, + }). + Starting(). + Do() + + inv, root := clitest.New(t, "start", r.Workspace.Name) + clitest.SetupConfig(t, memberClient, root) + doneChan := make(chan struct{}) + pty := ptytest.New(t).Attach(inv) + go func() { + defer close(doneChan) + err := inv.Run() + assert.NoError(t, err) + }() + + pty.ExpectMatch("workspace is already starting") + + _ = dbfake.JobComplete(t, db, r.Build.JobID).Do() + pty.ExpectMatch("workspace has been started") + + _ = testutil.RequireRecvCtx(ctx, t, doneChan) +} diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 4cac09d1dc44f..bc50c352ebc54 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "testing" + "time" "github.com/google/uuid" "github.com/sqlc-dev/pqtype" @@ -47,6 +48,11 @@ type WorkspaceBuildBuilder struct { resources []*sdkproto.Resource params []database.WorkspaceBuildParameter agentToken string + dispo workspaceBuildDisposition +} + +type workspaceBuildDisposition struct { + starting bool } // WorkspaceBuild generates a workspace build for the provided workspace. @@ -100,6 +106,12 @@ func (b WorkspaceBuildBuilder) WithAgent(mutations ...func([]*sdkproto.Agent) [] return b } +func (b WorkspaceBuildBuilder) Starting() WorkspaceBuildBuilder { + //nolint: revive // returns modified struct + b.dispo.starting = true + return b +} + // Do generates all the resources associated with a workspace build. // Template and TemplateVersion will be optionally populated if no // TemplateID is set on the provided workspace. @@ -166,20 +178,43 @@ func (b WorkspaceBuildBuilder) Do() WorkspaceResponse { }) require.NoError(b.t, err, "insert job") - err = b.db.UpdateProvisionerJobWithCompleteByID(ownerCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ - ID: job.ID, - UpdatedAt: dbtime.Now(), - Error: sql.NullString{}, - ErrorCode: sql.NullString{}, - CompletedAt: sql.NullTime{ - Time: dbtime.Now(), - Valid: true, - }, - }) - require.NoError(b.t, err, "complete job") + if b.dispo.starting { + // might need to do this multiple times if we got a template version + // import job as well + for { + j, err := b.db.AcquireProvisionerJob(ownerCtx, database.AcquireProvisionerJobParams{ + StartedAt: sql.NullTime{ + Time: dbtime.Now(), + Valid: true, + }, + WorkerID: uuid.NullUUID{ + UUID: uuid.New(), + Valid: true, + }, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + Tags: nil, + }) + require.NoError(b.t, err, "acquire starting job") + if j.ID == job.ID { + break + } + } + } else { + err = b.db.UpdateProvisionerJobWithCompleteByID(ownerCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ + ID: job.ID, + UpdatedAt: dbtime.Now(), + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + CompletedAt: sql.NullTime{ + Time: dbtime.Now(), + Valid: true, + }, + }) + require.NoError(b.t, err, "complete job") + ProvisionerJobResources(b.t, b.db, job.ID, b.seed.Transition, b.resources...).Do() + } resp.Build = dbgen.WorkspaceBuild(b.t, b.db, b.seed) - ProvisionerJobResources(b.t, b.db, job.ID, b.seed.Transition, b.resources...).Do() for i := range b.params { b.params[i].WorkspaceBuildID = resp.Build.ID @@ -340,6 +375,40 @@ func (t TemplateVersionBuilder) Do() TemplateVersionResponse { return resp } +type JobCompleteBuilder struct { + t testing.TB + db database.Store + jobID uuid.UUID +} + +type JobCompleteResponse struct { + CompletedAt time.Time +} + +func JobComplete(t testing.TB, db database.Store, jobID uuid.UUID) JobCompleteBuilder { + return JobCompleteBuilder{ + t: t, + db: db, + jobID: jobID, + } +} + +func (b JobCompleteBuilder) Do() JobCompleteResponse { + r := JobCompleteResponse{CompletedAt: dbtime.Now()} + err := b.db.UpdateProvisionerJobWithCompleteByID(ownerCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ + ID: b.jobID, + UpdatedAt: r.CompletedAt, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + CompletedAt: sql.NullTime{ + Time: r.CompletedAt, + Valid: true, + }, + }) + require.NoError(b.t, err, "complete job") + return r +} + func must[V any](v V, err error) V { if err != nil { panic(err)