From 9cfcc0be754a3a594382c4dcf1ef1f7895daa31e Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 6 Mar 2024 20:17:06 +0000 Subject: [PATCH 1/7] fix: do not set max deadline for workspaces on template update When templates are updated and schedule data is changed, we update all running workspaces to have up-to-date scheduling information that sticks to the new policy. When updating the max_deadline for existing running workspaces, if the max_deadline was before now()+2h we would set the max_deadline to now()+2h. Builds that don't/shouldn't have a max_deadline have it set to 0, which is always before now()+2h, and thus would always have the max_deadline updated. --- enterprise/coderd/schedule/template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go index 6c217c052f0ff..be913c835d5a2 100644 --- a/enterprise/coderd/schedule/template.go +++ b/enterprise/coderd/schedule/template.go @@ -278,7 +278,7 @@ func (s *EnterpriseTemplateScheduleStore) updateWorkspaceBuild(ctx context.Conte // If max deadline is before now()+2h, then set it to that. now := s.now() - if autostop.MaxDeadline.Before(now.Add(2 * time.Hour)) { + if !autostop.MaxDeadline.IsZero() && autostop.MaxDeadline.Before(now.Add(2*time.Hour)) { autostop.MaxDeadline = now.Add(time.Hour * 2) } From 7e393dc804f101b154e22ba44369f61b6ea20e92 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 6 Mar 2024 20:24:12 +0000 Subject: [PATCH 2/7] fixup! fix: do not set max deadline for workspaces on template update --- enterprise/coderd/schedule/template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go index be913c835d5a2..da3a2ceb19cff 100644 --- a/enterprise/coderd/schedule/template.go +++ b/enterprise/coderd/schedule/template.go @@ -285,7 +285,7 @@ func (s *EnterpriseTemplateScheduleStore) updateWorkspaceBuild(ctx context.Conte // If the current deadline on the build is after the new max_deadline, then // set it to the max_deadline. autostop.Deadline = build.Deadline - if autostop.Deadline.After(autostop.MaxDeadline) { + if !autostop.MaxDeadline.IsZero() && autostop.Deadline.After(autostop.MaxDeadline) { autostop.Deadline = autostop.MaxDeadline } From f249a64a030a901a158ea482287e4a72d9b56069 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 6 Mar 2024 20:43:35 +0000 Subject: [PATCH 3/7] fixup! fix: do not set max deadline for workspaces on template update --- enterprise/coderd/schedule/template.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go index da3a2ceb19cff..446f5fc13f1e6 100644 --- a/enterprise/coderd/schedule/template.go +++ b/enterprise/coderd/schedule/template.go @@ -289,6 +289,12 @@ func (s *EnterpriseTemplateScheduleStore) updateWorkspaceBuild(ctx context.Conte autostop.Deadline = autostop.MaxDeadline } + // If there's a max_deadline but the deadline is 0, then set the deadline to + // the max_deadline. + if !autostop.MaxDeadline.IsZero() && autostop.Deadline.IsZero() { + autostop.Deadline = autostop.MaxDeadline + } + // Update the workspace build deadline. err = db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{ ID: build.ID, From 3c0dc000727c9fab671e371c7c96261b978b600c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Mar 2024 15:07:57 -0600 Subject: [PATCH 4/7] test: add unit test to excercise template schedule bug added a user without quiet hours and a template without a autostop requirment to trigger the bug. --- enterprise/coderd/schedule/template_test.go | 65 +++++++++++++++------ 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/enterprise/coderd/schedule/template_test.go b/enterprise/coderd/schedule/template_test.go index ac158a795b3a3..25624b791bdfa 100644 --- a/enterprise/coderd/schedule/template_test.go +++ b/enterprise/coderd/schedule/template_test.go @@ -27,30 +27,35 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { db, _ := dbtestutil.NewDB(t) var ( - org = dbgen.Organization(t, db, database.Organization{}) - user = dbgen.User(t, db, database.User{}) + org = dbgen.Organization(t, db, database.Organization{}) + quietUser = dbgen.User(t, db, database.User{ + Username: "quiet", + }) + noQuietUser = dbgen.User(t, db, database.User{ + Username: "no-quiet", + }) file = dbgen.File(t, db, database.File{ - CreatedBy: user.ID, + CreatedBy: quietUser.ID, }) templateJob = dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ OrganizationID: org.ID, FileID: file.ID, - InitiatorID: user.ID, + InitiatorID: quietUser.ID, Tags: database.StringMap{ "foo": "bar", }, }) templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{ OrganizationID: org.ID, - CreatedBy: user.ID, + CreatedBy: quietUser.ID, JobID: templateJob.ID, }) ) const userQuietHoursSchedule = "CRON_TZ=UTC 0 0 * * *" // midnight UTC ctx := testutil.Context(t, testutil.WaitLong) - user, err := db.UpdateUserQuietHoursSchedule(ctx, database.UpdateUserQuietHoursScheduleParams{ - ID: user.ID, + quietUser, err := db.UpdateUserQuietHoursSchedule(ctx, database.UpdateUserQuietHoursScheduleParams{ + ID: quietUser.ID, QuietHoursSchedule: userQuietHoursSchedule, }) require.NoError(t, err) @@ -68,6 +73,8 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { maxDeadline time.Time newDeadline time.Time // 0 for no change newMaxDeadline time.Time + noQuietHours bool + autostopReq *agplschedule.TemplateAutostopRequirement }{ { name: "SkippedWorkspaceMaxDeadlineTooSoon", @@ -123,6 +130,20 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { newDeadline: nextQuietHours, newMaxDeadline: nextQuietHours, }, + { + name: "MaxDealineShouldBeUnset", + now: buildTime, + deadline: buildTime.Add(time.Hour * 8), + maxDeadline: time.Time{}, // No max set + // Should be unchanged + newDeadline: buildTime.Add(time.Hour * 8), + newMaxDeadline: time.Time{}, + noQuietHours: true, + autostopReq: &agplschedule.TemplateAutostopRequirement{ + DaysOfWeek: 0, + Weeks: 0, + }, + }, } for _, c := range cases { @@ -131,6 +152,11 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { t.Run(c.name, func(t *testing.T) { t.Parallel() + user := quietUser + if c.noQuietHours { + user = noQuietUser + } + t.Log("buildTime", buildTime) t.Log("nextQuietHours", nextQuietHours) t.Log("now", c.now) @@ -217,17 +243,22 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { templateScheduleStore.TimeNowFn = func() time.Time { return c.now } + + autostopReq := agplschedule.TemplateAutostopRequirement{ + // Every day + DaysOfWeek: 0b01111111, + Weeks: 0, + } + if c.autostopReq != nil { + autostopReq = *c.autostopReq + } _, err = templateScheduleStore.Set(ctx, db, template, agplschedule.TemplateScheduleOptions{ - UserAutostartEnabled: false, - UserAutostopEnabled: false, - DefaultTTL: 0, - MaxTTL: 0, - UseMaxTTL: false, - AutostopRequirement: agplschedule.TemplateAutostopRequirement{ - // Every day - DaysOfWeek: 0b01111111, - Weeks: 0, - }, + UserAutostartEnabled: false, + UserAutostopEnabled: false, + DefaultTTL: 0, + MaxTTL: 0, + UseMaxTTL: false, + AutostopRequirement: autostopReq, FailureTTL: 0, TimeTilDormant: 0, TimeTilDormantAutoDelete: 0, From e2058d19ca21301864a27c5055e9ecb6327b5183 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Mar 2024 15:18:19 -0600 Subject: [PATCH 5/7] add test comment --- enterprise/coderd/schedule/template_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/schedule/template_test.go b/enterprise/coderd/schedule/template_test.go index 25624b791bdfa..4c887754e0a38 100644 --- a/enterprise/coderd/schedule/template_test.go +++ b/enterprise/coderd/schedule/template_test.go @@ -131,7 +131,13 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { newMaxDeadline: nextQuietHours, }, { - name: "MaxDealineShouldBeUnset", + // There was a bug if a user has no quiet hours set, and autostop + // req is not turned on, then the max deadline is set to `time.Time{}`. + // This zero value was "in the past", so the workspace deadline would + // be set to "now" + 2 hours. + // This is a mistake because the max deadline being zero means + // there is no max deadline. + name: "MaxDeadlineShouldBeUnset", now: buildTime, deadline: buildTime.Add(time.Hour * 8), maxDeadline: time.Time{}, // No max set From 96256378ca680dd53dd4815359f0afbea2da7cd2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Mar 2024 15:26:19 -0600 Subject: [PATCH 6/7] add unit test to ensure deadline is never nil if maxdeadline is set --- enterprise/coderd/schedule/template_test.go | 59 ++++++++++++++++----- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/enterprise/coderd/schedule/template_test.go b/enterprise/coderd/schedule/template_test.go index 4c887754e0a38..4df05c026496f 100644 --- a/enterprise/coderd/schedule/template_test.go +++ b/enterprise/coderd/schedule/template_test.go @@ -16,6 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" agplschedule "github.com/coder/coder/v2/coderd/schedule" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/cryptorand" "github.com/coder/coder/v2/enterprise/coderd/schedule" "github.com/coder/coder/v2/testutil" @@ -67,11 +68,12 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { // Workspace old max_deadline too soon cases := []struct { - name string - now time.Time - deadline time.Time - maxDeadline time.Time - newDeadline time.Time // 0 for no change + name string + now time.Time + deadline time.Time + maxDeadline time.Time + // Set to nil for no change. + newDeadline *time.Time newMaxDeadline time.Time noQuietHours bool autostopReq *agplschedule.TemplateAutostopRequirement @@ -82,7 +84,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: buildTime, maxDeadline: buildTime.Add(1 * time.Hour), // Unchanged since the max deadline is too soon. - newDeadline: time.Time{}, + newDeadline: nil, newMaxDeadline: buildTime.Add(1 * time.Hour), }, { @@ -92,7 +94,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: buildTime, // Far into the future... maxDeadline: nextQuietHours.Add(24 * time.Hour), - newDeadline: time.Time{}, + newDeadline: nil, // We will use now() + 2 hours if the newly calculated max deadline // from the workspace build time is before now. newMaxDeadline: nextQuietHours.Add(8 * time.Hour), @@ -104,7 +106,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: buildTime, // Far into the future... maxDeadline: nextQuietHours.Add(24 * time.Hour), - newDeadline: time.Time{}, + newDeadline: nil, // We will use now() + 2 hours if the newly calculated max deadline // from the workspace build time is within the next 2 hours. newMaxDeadline: nextQuietHours.Add(1 * time.Hour), @@ -116,7 +118,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: buildTime, // Far into the future... maxDeadline: nextQuietHours.Add(24 * time.Hour), - newDeadline: time.Time{}, + newDeadline: nil, newMaxDeadline: nextQuietHours, }, { @@ -127,7 +129,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: nextQuietHours.Add(24 * time.Hour), maxDeadline: nextQuietHours.Add(24 * time.Hour), // The deadline should match since it is after the new max deadline. - newDeadline: nextQuietHours, + newDeadline: ptr.Ref(nextQuietHours), newMaxDeadline: nextQuietHours, }, { @@ -142,7 +144,7 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { deadline: buildTime.Add(time.Hour * 8), maxDeadline: time.Time{}, // No max set // Should be unchanged - newDeadline: buildTime.Add(time.Hour * 8), + newDeadline: ptr.Ref(buildTime.Add(time.Hour * 8)), newMaxDeadline: time.Time{}, noQuietHours: true, autostopReq: &agplschedule.TemplateAutostopRequirement{ @@ -150,6 +152,35 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { Weeks: 0, }, }, + { + // A bug existed where MaxDeadline could be set, but deadline was + // `time.Time{}`. This is a logical inconsistency because the "max" + // deadline was ignored. + name: "NoDeadline", + now: buildTime, + deadline: time.Time{}, + maxDeadline: time.Time{}, // No max set + // Should be unchanged + newDeadline: ptr.Ref(time.Time{}), + newMaxDeadline: time.Time{}, + noQuietHours: true, + autostopReq: &agplschedule.TemplateAutostopRequirement{ + DaysOfWeek: 0, + Weeks: 0, + }, + }, + + { + // Similar to 'NoDeadline' test. This has a MaxDeadline set, so + // the deadline of the workspace should now be set. + name: "WorkspaceDeadlineNowSet", + now: nextQuietHours.Add(-6 * time.Hour), + // Start with unset times + deadline: time.Time{}, + maxDeadline: time.Time{}, + newDeadline: ptr.Ref(nextQuietHours), + newMaxDeadline: nextQuietHours, + }, } for _, c := range cases { @@ -275,10 +306,10 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { newBuild, err := db.GetWorkspaceBuildByID(ctx, wsBuild.ID) require.NoError(t, err) - if c.newDeadline.IsZero() { - c.newDeadline = wsBuild.Deadline + if c.newDeadline == nil { + c.newDeadline = &wsBuild.Deadline } - require.WithinDuration(t, c.newDeadline, newBuild.Deadline, time.Second) + require.WithinDuration(t, *c.newDeadline, newBuild.Deadline, time.Second) require.WithinDuration(t, c.newMaxDeadline, newBuild.MaxDeadline, time.Second) // Check that the new build has the same state as before. From 295b7557260b3a7942c75c241212ddb484599b95 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Mar 2024 15:28:12 -0600 Subject: [PATCH 7/7] add comment to explain grace period --- enterprise/coderd/schedule/template.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go index 446f5fc13f1e6..04744a5ea4b86 100644 --- a/enterprise/coderd/schedule/template.go +++ b/enterprise/coderd/schedule/template.go @@ -277,6 +277,9 @@ func (s *EnterpriseTemplateScheduleStore) updateWorkspaceBuild(ctx context.Conte } // If max deadline is before now()+2h, then set it to that. + // This is intended to give ample warning to this workspace about an upcoming auto-stop. + // If we were to omit this "grace" period, then this workspace could be set to be stopped "now". + // The "2 hours" was an arbitrary decision for this window. now := s.now() if !autostop.MaxDeadline.IsZero() && autostop.MaxDeadline.Before(now.Add(2*time.Hour)) { autostop.MaxDeadline = now.Add(time.Hour * 2)