fix: do not set max deadline for workspaces on template update (#12446)

deansheather · Emyrk · mtojek · commit 67c79197ec12 · 2024-03-08T14:14:06.000+01:00
* 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.

* test: add unit test to excercise template schedule bug
---------

Co-authored-by: Steven Masley &lt;stevenmasley@gmail.com&gt;
diff --git a/enterprise/coderd/schedule/template.go b/enterprise/coderd/schedule/template.go
@@ -277,15 +277,24 @@ 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.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)
 	}
 
 	// 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
+	}
+
+	// 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
 	}
 
diff --git 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"
@@ -27,30 +28,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)
@@ -62,20 +68,23 @@ 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
 	}{
 		{
 			name:        "SkippedWorkspaceMaxDeadlineTooSoon",
 			now:         buildTime,
 			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),
 		},
 		{
@@ -85,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),
@@ -97,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),
@@ -109,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,
 		},
 		{
@@ -120,7 +129,56 @@ 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,
+		},
+		{
+			// 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
+			// Should be unchanged
+			newDeadline:    ptr.Ref(buildTime.Add(time.Hour * 8)),
+			newMaxDeadline: time.Time{},
+			noQuietHours:   true,
+			autostopReq: &agplschedule.TemplateAutostopRequirement{
+				DaysOfWeek: 0,
+				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,
 		},
 	}
@@ -131,6 +189,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 +280,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,
@@ -238,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.
