mirror of https://github.com/coder/coder.git
fix: do not set max deadline for workspaces on template update (#12446)
* 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 <stevenmasley@gmail.com>
This commit is contained in:
Dean Sheather
GitHub
parent
17caf58b5e
commit
586586e9dd
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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,12 +68,15 @@ 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",
|
||||
|
|
@ -75,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),
|
||||
},
|
||||
{
|
||||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue