fix: complete job and mark workspace as deleted when no provisioners are available (#18465)

Alternate fix for https://github.com/coder/coder/issues/18080

Modifies wsbuilder to complete the provisioner job and mark the
workspace as deleted if it is clear that no provisioner will be able to
pick up the delete build.

This has a significant advantage of not deviating too much from the
current semantics of `POST /api/v2/workspacebuilds`.
https://github.com/coder/coder/pull/18460 ends up returning a 204 on
orphan delete due to no build being created.

Downside is that we have to duplicate some responsibilities of
provisionerdserver in wsbuilder.

There is a slight gotcha to this approach though: if you stop a
provisioner and then immediately try to orphan-delete, the job will
still be created because of the provisioner heartbeat interval. However
you can cancel it and try again.
This commit is contained in:
Cian Johnston
2025-06-23 14:07:42 +01:00
committed by GitHub
parent c3bc1e75ec
commit 2f55e29466
7 changed files with 501 additions and 85 deletions
+2 -1
View File
@@ -4497,7 +4497,8 @@ func (q *FakeQuerier) GetProvisionerDaemons(_ context.Context) ([]database.Provi
defer q.mutex.RUnlock()
if len(q.provisionerDaemons) == 0 {
return nil, sql.ErrNoRows
// Returning err=nil here for consistency with real querier
return []database.ProvisionerDaemon{}, nil
}
// copy the data so that the caller can't manipulate any data inside dbmem
// after returning
+41 -4
View File
@@ -3,6 +3,7 @@ package coderd
import (
"context"
"database/sql"
"encoding/json"
"errors"
"fmt"
"math"
@@ -433,20 +434,56 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
return
}
var queuePos database.GetProvisionerJobsByIDsWithQueuePositionRow
if provisionerJob != nil {
queuePos.ProvisionerJob = *provisionerJob
queuePos.QueuePosition = 0
if err := provisionerjobs.PostJob(api.Pubsub, *provisionerJob); err != nil {
// Client probably doesn't care about this error, so just log it.
api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err))
}
// We may need to complete the audit if wsbuilder determined that
// no provisioner could handle an orphan-delete job and completed it.
if createBuild.Orphan && createBuild.Transition == codersdk.WorkspaceTransitionDelete && provisionerJob.CompletedAt.Valid {
api.Logger.Warn(ctx, "orphan delete handled by wsbuilder due to no eligible provisioners",
slog.F("workspace_id", workspace.ID),
slog.F("workspace_build_id", workspaceBuild.ID),
slog.F("provisioner_job_id", provisionerJob.ID),
)
buildResourceInfo := audit.AdditionalFields{
WorkspaceName: workspace.Name,
BuildNumber: strconv.Itoa(int(workspaceBuild.BuildNumber)),
BuildReason: workspaceBuild.Reason,
WorkspaceID: workspace.ID,
WorkspaceOwner: workspace.OwnerName,
}
briBytes, err := json.Marshal(buildResourceInfo)
if err != nil {
api.Logger.Error(ctx, "failed to marshal build resource info for audit", slog.Error(err))
}
auditor := api.Auditor.Load()
bag := audit.BaggageFromContext(ctx)
audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.WorkspaceBuild]{
Audit: *auditor,
Log: api.Logger,
UserID: provisionerJob.InitiatorID,
OrganizationID: workspace.OrganizationID,
RequestID: provisionerJob.ID,
IP: bag.IP,
Action: database.AuditActionDelete,
Old: previousWorkspaceBuild,
New: *workspaceBuild,
Status: http.StatusOK,
AdditionalFields: briBytes,
})
}
}
apiBuild, err := api.convertWorkspaceBuild(
*workspaceBuild,
workspace,
database.GetProvisionerJobsByIDsWithQueuePositionRow{
ProvisionerJob: *provisionerJob,
QueuePosition: 0,
},
queuePos,
[]database.WorkspaceResource{},
[]database.WorkspaceResourceMetadatum{},
[]database.WorkspaceAgent{},
+162 -28
View File
@@ -1,6 +1,7 @@
package coderd_test
import (
"bytes"
"context"
"database/sql"
"errors"
@@ -25,6 +26,7 @@ import (
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/database/dbtime"
@@ -371,42 +373,174 @@ func TestWorkspaceBuildsProvisionerState(t *testing.T) {
t.Run("Orphan", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
first := coderdtest.CreateFirstUser(t, client)
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
t.Run("WithoutDelete", func(t *testing.T) {
t.Parallel()
client, store := coderdtest.NewWithDatabase(t, nil)
first := coderdtest.CreateFirstUser(t, client)
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin())
version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil)
template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID)
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
OwnerID: templateAdminUser.ID,
OrganizationID: first.OrganizationID,
}).Do()
workspace := coderdtest.CreateWorkspace(t, client, template.ID)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
// Providing both state and orphan fails.
_, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
Transition: codersdk.WorkspaceTransitionDelete,
ProvisionerState: []byte(" "),
Orphan: true,
// Trying to orphan without delete transition fails.
_, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: r.TemplateVersion.ID,
Transition: codersdk.WorkspaceTransitionStart,
Orphan: true,
})
require.Error(t, err, "Orphan is only permitted when deleting a workspace.")
cerr := coderdtest.SDKError(t, err)
require.Equal(t, http.StatusBadRequest, cerr.StatusCode())
})
require.Error(t, err)
cerr := coderdtest.SDKError(t, err)
require.Equal(t, http.StatusBadRequest, cerr.StatusCode())
// Regular orphan operation succeeds.
build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
Transition: codersdk.WorkspaceTransitionDelete,
Orphan: true,
t.Run("WithState", func(t *testing.T) {
t.Parallel()
client, store := coderdtest.NewWithDatabase(t, nil)
first := coderdtest.CreateFirstUser(t, client)
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin())
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
OwnerID: templateAdminUser.ID,
OrganizationID: first.OrganizationID,
}).Do()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
// Providing both state and orphan fails.
_, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: r.TemplateVersion.ID,
Transition: codersdk.WorkspaceTransitionDelete,
ProvisionerState: []byte(" "),
Orphan: true,
})
require.Error(t, err)
cerr := coderdtest.SDKError(t, err)
require.Equal(t, http.StatusBadRequest, cerr.StatusCode())
})
require.NoError(t, err)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID)
_, err = client.Workspace(ctx, workspace.ID)
require.Error(t, err)
require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode())
t.Run("NoPermission", func(t *testing.T) {
t.Parallel()
client, store := coderdtest.NewWithDatabase(t, nil)
first := coderdtest.CreateFirstUser(t, client)
member, memberUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
OwnerID: memberUser.ID,
OrganizationID: first.OrganizationID,
}).Do()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
// Trying to orphan without being a template admin fails.
_, err := member.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: r.TemplateVersion.ID,
Transition: codersdk.WorkspaceTransitionDelete,
Orphan: true,
})
require.Error(t, err)
cerr := coderdtest.SDKError(t, err)
require.Equal(t, http.StatusForbidden, cerr.StatusCode())
})
t.Run("OK", func(t *testing.T) {
// Include a provisioner so that we can test that provisionerdserver
// performs deletion.
auditor := audit.NewMock()
client, store := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor})
first := coderdtest.CreateFirstUser(t, client)
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin())
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
// This is a valid zip file. Without this the job will fail to complete.
// TODO: add this to dbfake by default.
zipBytes := make([]byte, 22)
zipBytes[0] = 80
zipBytes[1] = 75
zipBytes[2] = 0o5
zipBytes[3] = 0o6
uploadRes, err := client.Upload(ctx, codersdk.ContentTypeZip, bytes.NewReader(zipBytes))
require.NoError(t, err)
tv := dbfake.TemplateVersion(t, store).
FileID(uploadRes.ID).
Seed(database.TemplateVersion{
OrganizationID: first.OrganizationID,
CreatedBy: templateAdminUser.ID,
}).
Do()
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
OwnerID: templateAdminUser.ID,
OrganizationID: first.OrganizationID,
TemplateID: tv.Template.ID,
}).Do()
auditor.ResetLogs()
// Regular orphan operation succeeds.
build, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: r.TemplateVersion.ID,
Transition: codersdk.WorkspaceTransitionDelete,
Orphan: true,
})
require.NoError(t, err)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID)
// Validate that the deletion was audited.
require.True(t, auditor.Contains(t, database.AuditLog{
ResourceID: build.ID,
Action: database.AuditActionDelete,
}))
})
t.Run("NoProvisioners", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client, store := coderdtest.NewWithDatabase(t, &coderdtest.Options{Auditor: auditor})
first := coderdtest.CreateFirstUser(t, client)
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin())
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
OwnerID: templateAdminUser.ID,
OrganizationID: first.OrganizationID,
}).Do()
// nolint:gocritic // For testing
daemons, err := store.GetProvisionerDaemons(dbauthz.AsSystemReadProvisionerDaemons(ctx))
require.NoError(t, err)
require.Empty(t, daemons, "Provisioner daemons should be empty for this test")
// Orphan deletion still succeeds despite no provisioners being available.
build, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: r.TemplateVersion.ID,
Transition: codersdk.WorkspaceTransitionDelete,
Orphan: true,
})
require.NoError(t, err)
require.Equal(t, codersdk.WorkspaceTransitionDelete, build.Transition)
require.Equal(t, codersdk.ProvisionerJobSucceeded, build.Job.Status)
require.Empty(t, build.Job.Error)
ws, err := client.Workspace(ctx, r.Workspace.ID)
require.Empty(t, ws)
require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode())
// Validate that the deletion was audited.
require.True(t, auditor.Contains(t, database.AuditLog{
ResourceID: build.ID,
Action: database.AuditActionDelete,
}))
})
})
}
+44
View File
@@ -464,6 +464,50 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object
return BuildError{http.StatusInternalServerError, "get workspace build", err}
}
// If the requestor is trying to orphan-delete a workspace and there are no
// provisioners available, we should complete the build and mark the
// workspace as deleted ourselves.
// There are cases where tagged provisioner daemons have been decommissioned
// without deleting the relevant workspaces, and without any provisioners
// available these workspaces cannot be deleted.
// Orphan-deleting a workspace sends an empty state to Terraform, which means
// it won't actually delete anything. So we actually don't need to execute a
// provisioner job at all for an orphan delete, but deleting without a workspace
// build or provisioner job would result in no audit log entry, which is a deal-breaker.
hasActiveEligibleProvisioner := false
for _, pd := range provisionerDaemons {
age := now.Sub(pd.ProvisionerDaemon.LastSeenAt.Time)
if age <= provisionerdserver.StaleInterval {
hasActiveEligibleProvisioner = true
break
}
}
if b.state.orphan && !hasActiveEligibleProvisioner {
// nolint: gocritic // At this moment, we are pretending to be provisionerd.
if err := store.UpdateProvisionerJobWithCompleteWithStartedAtByID(dbauthz.AsProvisionerd(b.ctx), database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams{
CompletedAt: sql.NullTime{Valid: true, Time: now},
Error: sql.NullString{Valid: false},
ErrorCode: sql.NullString{Valid: false},
ID: provisionerJob.ID,
StartedAt: sql.NullTime{Valid: true, Time: now},
UpdatedAt: now,
}); err != nil {
return BuildError{http.StatusInternalServerError, "mark orphan-delete provisioner job as completed", err}
}
// Re-fetch the completed provisioner job.
if pj, err := store.GetProvisionerJobByID(b.ctx, provisionerJob.ID); err == nil {
provisionerJob = pj
}
if err := store.UpdateWorkspaceDeletedByID(b.ctx, database.UpdateWorkspaceDeletedByIDParams{
ID: b.workspace.ID,
Deleted: true,
}); err != nil {
return BuildError{http.StatusInternalServerError, "mark workspace as deleted", err}
}
}
return nil
}, nil)
if err != nil {
+188
View File
@@ -839,6 +839,147 @@ func TestWorkspaceBuildWithPreset(t *testing.T) {
req.NoError(err)
}
func TestWorkspaceBuildDeleteOrphan(t *testing.T) {
t.Parallel()
t.Run("WithActiveProvisioners", func(t *testing.T) {
t.Parallel()
req := require.New(t)
asrt := assert.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
var buildID uuid.UUID
mDB := expectDB(t,
// Inputs
withTemplate,
withInactiveVersion(nil),
withLastBuildFound,
withTemplateVersionVariables(inactiveVersionID, nil),
withRichParameters(nil),
withWorkspaceTags(inactiveVersionID, nil),
withProvisionerDaemons([]database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow{{
JobID: inactiveJobID,
ProvisionerDaemon: database.ProvisionerDaemon{
LastSeenAt: sql.NullTime{Valid: true, Time: dbtime.Now()},
},
}}),
// Outputs
expectProvisionerJob(func(job database.InsertProvisionerJobParams) {
asrt.Equal(userID, job.InitiatorID)
asrt.Equal(inactiveFileID, job.FileID)
input := provisionerdserver.WorkspaceProvisionJob{}
err := json.Unmarshal(job.Input, &input)
req.NoError(err)
// store build ID for later
buildID = input.WorkspaceBuildID
}),
withInTx,
expectBuild(func(bld database.InsertWorkspaceBuildParams) {
asrt.Equal(inactiveVersionID, bld.TemplateVersionID)
asrt.Equal(workspaceID, bld.WorkspaceID)
asrt.Equal(int32(2), bld.BuildNumber)
asrt.Empty(string(bld.ProvisionerState))
asrt.Equal(userID, bld.InitiatorID)
asrt.Equal(database.WorkspaceTransitionDelete, bld.Transition)
asrt.Equal(database.BuildReasonInitiator, bld.Reason)
asrt.Equal(buildID, bld.ID)
}),
withBuild,
expectBuildParameters(func(params database.InsertWorkspaceBuildParametersParams) {
asrt.Equal(buildID, params.WorkspaceBuildID)
asrt.Empty(params.Name)
asrt.Empty(params.Value)
}),
)
ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID}
uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan()
// nolint: dogsled
_, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{})
req.NoError(err)
})
t.Run("NoActiveProvisioners", func(t *testing.T) {
t.Parallel()
req := require.New(t)
asrt := assert.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
var buildID uuid.UUID
var jobID uuid.UUID
mDB := expectDB(t,
// Inputs
withTemplate,
withInactiveVersion(nil),
withLastBuildFound,
withTemplateVersionVariables(inactiveVersionID, nil),
withRichParameters(nil),
withWorkspaceTags(inactiveVersionID, nil),
withProvisionerDaemons([]database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow{}),
// Outputs
expectProvisionerJob(func(job database.InsertProvisionerJobParams) {
asrt.Equal(userID, job.InitiatorID)
asrt.Equal(inactiveFileID, job.FileID)
input := provisionerdserver.WorkspaceProvisionJob{}
err := json.Unmarshal(job.Input, &input)
req.NoError(err)
// store build ID for later
buildID = input.WorkspaceBuildID
// store job ID for later
jobID = job.ID
}),
withInTx,
expectBuild(func(bld database.InsertWorkspaceBuildParams) {
asrt.Equal(inactiveVersionID, bld.TemplateVersionID)
asrt.Equal(workspaceID, bld.WorkspaceID)
asrt.Equal(int32(2), bld.BuildNumber)
asrt.Empty(string(bld.ProvisionerState))
asrt.Equal(userID, bld.InitiatorID)
asrt.Equal(database.WorkspaceTransitionDelete, bld.Transition)
asrt.Equal(database.BuildReasonInitiator, bld.Reason)
asrt.Equal(buildID, bld.ID)
}),
withBuild,
expectBuildParameters(func(params database.InsertWorkspaceBuildParametersParams) {
asrt.Equal(buildID, params.WorkspaceBuildID)
asrt.Empty(params.Name)
asrt.Empty(params.Value)
}),
// Because no provisioners were available and the request was to delete --orphan
expectUpdateProvisionerJobWithCompleteWithStartedAtByID(func(params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams) {
asrt.Equal(jobID, params.ID)
asrt.False(params.Error.Valid)
asrt.True(params.CompletedAt.Valid)
asrt.True(params.StartedAt.Valid)
}),
expectUpdateWorkspaceDeletedByID(func(params database.UpdateWorkspaceDeletedByIDParams) {
asrt.Equal(workspaceID, params.ID)
asrt.True(params.Deleted)
}),
expectGetProvisionerJobByID(func(job database.ProvisionerJob) {
asrt.Equal(jobID, job.ID)
}),
)
ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID}
uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan()
// nolint: dogsled
_, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{})
req.NoError(err)
})
}
func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) {
t.Parallel()
@@ -1107,6 +1248,53 @@ func expectProvisionerJob(
}
}
// expectUpdateProvisionerJobWithCompleteWithStartedAtByID asserts a call to
// expectUpdateProvisionerJobWithCompleteWithStartedAtByID and runs the provided
// assertions against it.
func expectUpdateProvisionerJobWithCompleteWithStartedAtByID(assertions func(params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams)) func(mTx *dbmock.MockStore) {
return func(mTx *dbmock.MockStore) {
mTx.EXPECT().UpdateProvisionerJobWithCompleteWithStartedAtByID(gomock.Any(), gomock.Any()).
Times(1).
DoAndReturn(
func(ctx context.Context, params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams) error {
assertions(params)
return nil
},
)
}
}
// expectUpdateWorkspaceDeletedByID asserts a call to UpdateWorkspaceDeletedByID
// and runs the provided assertions against it.
func expectUpdateWorkspaceDeletedByID(assertions func(params database.UpdateWorkspaceDeletedByIDParams)) func(mTx *dbmock.MockStore) {
return func(mTx *dbmock.MockStore) {
mTx.EXPECT().UpdateWorkspaceDeletedByID(gomock.Any(), gomock.Any()).
Times(1).
DoAndReturn(
func(ctx context.Context, params database.UpdateWorkspaceDeletedByIDParams) error {
assertions(params)
return nil
},
)
}
}
// expectGetProvisionerJobByID asserts a call to GetProvisionerJobByID
// and runs the provided assertions against it.
func expectGetProvisionerJobByID(assertions func(job database.ProvisionerJob)) func(mTx *dbmock.MockStore) {
return func(mTx *dbmock.MockStore) {
mTx.EXPECT().GetProvisionerJobByID(gomock.Any(), gomock.Any()).
Times(1).
DoAndReturn(
func(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) {
job := database.ProvisionerJob{ID: id}
assertions(job)
return job, nil
},
)
}
}
func withBuild(mTx *dbmock.MockStore) {
mTx.EXPECT().GetWorkspaceBuildByID(gomock.Any(), gomock.Any()).Times(1).
DoAndReturn(func(ctx context.Context, id uuid.UUID) (database.WorkspaceBuild, error) {