From b32d79ef0be2dc6ada17abfbdc1fe80643668d86 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 5 Oct 2023 14:11:39 -0500 Subject: [PATCH] fix: fix failed workspaces continuously auto-deleting (#10069) - Fixes an issue where workspaces that are eligible for auto-deletion are retried every tick (1 minute) even if the previous deletion transition failed. The updated logic only attempts to delete workspaces that previously failed once a day (24 hours since last attempt). --- coderd/autobuild/lifecycle_executor.go | 23 ++++--- coderd/database/modelmethods.go | 16 +++++ enterprise/coderd/workspaces_test.go | 87 ++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 8 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index e2ab0d7dc2..0b26d95738 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -24,7 +24,6 @@ import ( "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/coderd/schedule/cron" "github.com/coder/coder/v2/coderd/wsbuilder" - "github.com/coder/coder/v2/codersdk" ) // Executor automatically starts or stops workspaces. @@ -310,7 +309,7 @@ func getNextTransition( // make it dormant. return "", database.BuildReasonAutolock, nil - case isEligibleForDelete(ws, templateSchedule, currentTick): + case isEligibleForDelete(ws, templateSchedule, latestBuild, latestJob, currentTick): return database.WorkspaceTransitionDelete, database.BuildReasonAutodelete, nil default: return "", "", xerrors.Errorf("last transition not valid for autostart or autostop") @@ -320,7 +319,7 @@ func getNextTransition( // isEligibleForAutostart returns true if the workspace should be autostarted. func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { // Don't attempt to autostart failed workspaces. - if codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed { + if job.JobStatus == database.ProvisionerJobStatusFailed { return false } @@ -354,7 +353,7 @@ func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild // isEligibleForAutostart returns true if the workspace should be autostopped. func isEligibleForAutostop(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, currentTick time.Time) bool { - if codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed { + if job.JobStatus == database.ProvisionerJobStatusFailed { return false } @@ -381,13 +380,21 @@ func isEligibleForDormantStop(ws database.Workspace, templateSchedule schedule.T currentTick.Sub(ws.LastUsedAt) > templateSchedule.TimeTilDormant } -func isEligibleForDelete(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool { - // Only attempt to delete dormant workspaces. - return ws.DormantAt.Valid && ws.DeletingAt.Valid && +func isEligibleForDelete(ws database.Workspace, templateSchedule schedule.TemplateScheduleOptions, lastBuild database.WorkspaceBuild, lastJob database.ProvisionerJob, currentTick time.Time) bool { + eligible := ws.DormantAt.Valid && ws.DeletingAt.Valid && // Dormant workspaces should only be deleted if a time_til_dormant_autodelete value is specified. templateSchedule.TimeTilDormantAutoDelete > 0 && // The workspace must breach the time_til_dormant_autodelete value. currentTick.After(ws.DeletingAt.Time) + + // If the last delete job failed we should wait 24 hours before trying again. + // Builds are resource-intensive so retrying every minute is not productive + // and will hold compute hostage. + if lastBuild.Transition == database.WorkspaceTransitionDelete && lastJob.JobStatus == database.ProvisionerJobStatusFailed { + return eligible && lastJob.Finished() && currentTick.Sub(lastJob.FinishedAt()) > time.Hour*24 + } + + return eligible } // isEligibleForFailedStop returns true if the workspace is eligible to be stopped @@ -396,7 +403,7 @@ func isEligibleForFailedStop(build database.WorkspaceBuild, job database.Provisi // If the template has specified a failure TLL. return templateSchedule.FailureTTL > 0 && // And the job resulted in failure. - codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed && + job.JobStatus == database.ProvisionerJobStatusFailed && build.Transition == database.WorkspaceTransitionStart && // And sufficient time has elapsed since the job has completed. job.CompletedAt.Valid && diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 98eca86de3..057d81a1a0 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -367,3 +367,19 @@ func ConvertWorkspaceRows(rows []GetWorkspacesRow) []Workspace { func (g Group) IsEveryone() bool { return g.ID == g.OrganizationID } + +func (p ProvisionerJob) Finished() bool { + return p.CanceledAt.Valid || p.CompletedAt.Valid +} + +func (p ProvisionerJob) FinishedAt() time.Time { + if p.CompletedAt.Valid { + return p.CompletedAt.Time + } + + if p.CanceledAt.Valid { + return p.CanceledAt.Time + } + + return time.Time{} +} diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index e010d5edb0..e6cd47524c 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -649,6 +649,93 @@ func TestWorkspaceAutobuild(t *testing.T) { stats = <-statsCh require.Len(t, stats.Transitions, 0) }) + + // Test that failing to auto-delete a workspace will only retry + // once a day. + t.Run("FailedDeleteRetryDaily", func(t *testing.T) { + t.Parallel() + + var ( + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + transitionTTL = time.Minute + ctx = testutil.Context(t, testutil.WaitMedium) + ) + + client, user := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: schedule.NewEnterpriseTemplateScheduleStore(agplUserQuietHoursScheduleStore()), + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{codersdk.FeatureAdvancedTemplateScheduling: 1}, + }, + }) + + // Create a template version that passes to get a functioning workspace. + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.PlanComplete, + ProvisionApply: echo.ApplyComplete, + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) + + // Create a new version that will fail when we try to delete a workspace. + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.PlanComplete, + ProvisionApply: echo.ApplyFailed, + }, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.TemplateID = template.ID + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + + // Try to delete the workspace. This simulates a "failed" autodelete. + build, err := client.CreateWorkspaceBuild(ctx, ws.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionDelete, + TemplateVersionID: version.ID, + }) + require.NoError(t, err) + + build = coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) + require.NotEmpty(t, build.Job.Error) + + // Update our workspace to be dormant so that it qualifies for auto-deletion. + err = client.UpdateWorkspaceDormancy(ctx, ws.ID, codersdk.UpdateWorkspaceDormancy{ + Dormant: true, + }) + require.NoError(t, err) + + // Enable auto-deletion for the template. + _, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + TimeTilDormantAutoDeleteMillis: transitionTTL.Milliseconds(), + }) + require.NoError(t, err) + + ws = coderdtest.MustWorkspace(t, client, ws.ID) + require.NotNil(t, ws.DeletingAt) + + // Simulate ticking an hour after the workspace is expected to be deleted. + // Under normal circumstances this should result in a transition but + // since our last build resulted in failure it should be skipped. + ticker <- build.Job.CompletedAt.Add(time.Hour) + stats := <-statCh + require.Len(t, stats.Transitions, 0) + + // Simulate ticking a day after the workspace was last attempted to + // be deleted. This should result in an attempt. + ticker <- build.Job.CompletedAt.Add(time.Hour * 25) + stats = <-statCh + require.Len(t, stats.Transitions, 1) + require.Equal(t, database.WorkspaceTransitionDelete, stats.Transitions[ws.ID]) + }) } func TestWorkspacesFiltering(t *testing.T) {