Skip to content

chore: cover deadline crossing autostart border #13115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 1, 2024
Merged

chore: cover deadline crossing autostart border #13115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7523edf
chore: cover deadline crossing autostart border
Emyrk Apr 30, 2024
bf7e698
Add more unit tests
Emyrk Apr 30, 2024
44bd435
fixup compile error
Emyrk Apr 30, 2024
a6b5a0b
Pass workspace autostart into CalculateAutoStop
Emyrk Apr 30, 2024
94872aa
function name remove Schedule
Emyrk May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions coderd/agentapi/stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"cdr.dev/slog"
agentproto "github.com/coder/coder/v2/agent/proto"
"github.com/coder/coder/v2/coderd/autobuild"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/database/pubsub"
Expand Down Expand Up @@ -84,7 +83,7 @@ func (a *StatsAPI) UpdateStats(ctx context.Context, req *agentproto.UpdateStatsR
slog.Error(err),
)
} else {
next, allowed := autobuild.NextAutostartSchedule(now, workspace.AutostartSchedule.String, templateSchedule)
next, allowed := schedule.NextAutostart(now, workspace.AutostartSchedule.String, templateSchedule)
if allowed {
nextAutostart = next
}
Expand Down
26 changes: 1 addition & 25 deletions coderd/autobuild/lifecycle_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
"github.com/coder/coder/v2/coderd/database/pubsub"
"github.com/coder/coder/v2/coderd/schedule"
"github.com/coder/coder/v2/coderd/schedule/cron"
"github.com/coder/coder/v2/coderd/wsbuilder"
)

Expand Down Expand Up @@ -368,7 +367,7 @@ func isEligibleForAutostart(user database.User, ws database.Workspace, build dat
return false
}

nextTransition, allowed := NextAutostartSchedule(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
nextTransition, allowed := schedule.NextAutostart(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
if !allowed {
return false
}
Expand All @@ -377,29 +376,6 @@ func isEligibleForAutostart(user database.User, ws database.Workspace, build dat
return !currentTick.Before(nextTransition)
}

// NextAutostartSchedule takes the workspace and template schedule and returns the next autostart schedule
// after "at". The boolean returned is if the autostart should be allowed to start based on the template
// schedule.
func NextAutostartSchedule(at time.Time, wsSchedule string, templateSchedule schedule.TemplateScheduleOptions) (time.Time, bool) {
sched, err := cron.Weekly(wsSchedule)
if err != nil {
return time.Time{}, false
}

// Round down to the nearest minute, as this is the finest granularity cron supports.
// Truncate is probably not necessary here, but doing it anyway to be sure.
nextTransition := sched.Next(at).Truncate(time.Minute)

// The nextTransition is when the auto start should kick off. If it lands on a
// forbidden day, do not allow the auto start. We use the time location of the
// schedule to determine the weekday. So if "Saturday" is disallowed, the
// definition of "Saturday" depends on the location of the schedule.
zonedTransition := nextTransition.In(sched.Location())
allowed := templateSchedule.AutostartRequirement.DaysMap()[zonedTransition.Weekday()]

return zonedTransition, allowed
}

// 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 job.JobStatus == database.ProvisionerJobStatusFailed {
Expand Down
2 changes: 2 additions & 0 deletions coderd/provisionerdserver/provisionerdserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,8 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob)
UserQuietHoursScheduleStore: *s.UserQuietHoursScheduleStore.Load(),
Now: now,
Workspace: workspace,
// Allowed to be the empty string.
WorkspaceAutostart: workspace.AutostartSchedule.String,
})
if err != nil {
return xerrors.Errorf("calculate auto stop: %w", err)
Expand Down
30 changes: 30 additions & 0 deletions coderd/schedule/autostart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package schedule

import (
"time"

"github.com/coder/coder/v2/coderd/schedule/cron"
)

// NextAutostart takes the workspace and template schedule and returns the next autostart schedule
// after "at". The boolean returned is if the autostart should be allowed to start based on the template
// schedule.
func NextAutostart(at time.Time, wsSchedule string, templateSchedule TemplateScheduleOptions) (time.Time, bool) {
sched, err := cron.Weekly(wsSchedule)
if err != nil {
return time.Time{}, false
}

// Round down to the nearest minute, as this is the finest granularity cron supports.
// Truncate is probably not necessary here, but doing it anyway to be sure.
nextTransition := sched.Next(at).Truncate(time.Minute)

// The nextTransition is when the auto start should kick off. If it lands on a
// forbidden day, do not allow the auto start. We use the time location of the
// schedule to determine the weekday. So if "Saturday" is disallowed, the
// definition of "Saturday" depends on the location of the schedule.
zonedTransition := nextTransition.In(sched.Location())
allowed := templateSchedule.AutostartRequirement.DaysMap()[zonedTransition.Weekday()]

return zonedTransition, allowed
}
38 changes: 36 additions & 2 deletions coderd/schedule/autostop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ type CalculateAutostopParams struct {
Database database.Store
TemplateScheduleStore TemplateScheduleStore
UserQuietHoursScheduleStore UserQuietHoursScheduleStore
// WorkspaceAutostart can be the empty string if no workspace autostart
// is configured.
// If configured, this is expected to be a cron weekly event parsable
// by autobuild.NextAutostart
WorkspaceAutostart string

Now time.Time
Workspace database.Workspace
Expand Down Expand Up @@ -90,6 +95,14 @@ func CalculateAutostop(ctx context.Context, params CalculateAutostopParams) (Aut
autostop AutostopTime
)

var ttl time.Duration
if workspace.Ttl.Valid {
// When the workspace is made it copies the template's TTL, and the user
// can unset it to disable it (unless the template has
// UserAutoStopEnabled set to false, see below).
ttl = time.Duration(workspace.Ttl.Int64)
}

if workspace.Ttl.Valid {
// When the workspace is made it copies the template's TTL, and the user
// can unset it to disable it (unless the template has
Expand All @@ -104,9 +117,30 @@ func CalculateAutostop(ctx context.Context, params CalculateAutostopParams) (Aut
if !templateSchedule.UserAutostopEnabled {
// The user is not permitted to set their own TTL, so use the template
// default.
autostop.Deadline = time.Time{}
ttl = 0
if templateSchedule.DefaultTTL > 0 {
autostop.Deadline = now.Add(templateSchedule.DefaultTTL)
ttl = templateSchedule.DefaultTTL
}
}

if ttl > 0 {
// Only apply non-zero TTLs.
autostop.Deadline = now.Add(ttl)
if params.WorkspaceAutostart != "" {
// If the deadline passes the next autostart, we need to extend the deadline to
// autostart + deadline. ActivityBumpWorkspace already covers this case
// when extending the deadline.
//
// Situation this is solving.
// 1. User has workspace with auto-start at 9:00am, 12 hour auto-stop.
// 2. Coder stops workspace at 9pm
// 3. User starts workspace at 9:45pm.
// - The initial deadline is calculated to be 9:45am
// - This crosses the autostart deadline, so the deadline is extended to 9pm
nextAutostart, ok := NextAutostart(params.Now, params.WorkspaceAutostart, templateSchedule)
if ok && autostop.Deadline.After(nextAutostart) {
autostop.Deadline = nextAutostart.Add(ttl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle the min(deadline, max_deadline) here too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is handled down below.

coder/coderd/schedule/autostop.go

Line 238 in 94872aa

autostop.Deadline = autostop.MaxDeadline

I'd rather not duplicate that logic since it also handles the case where MaxDeadline is not set.

}
}
}

Expand Down
142 changes: 136 additions & 6 deletions coderd/schedule/autostop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ func TestCalculateAutoStop(t *testing.T) {

now := time.Now()

chicago, err := time.LoadLocation("America/Chicago")
require.NoError(t, err, "loading chicago time location")

// pastDateNight is 9:45pm on a wednesday
pastDateNight := time.Date(2024, 2, 14, 21, 45, 0, 0, chicago)

// Wednesday the 8th of February 2023 at midnight. This date was
// specifically chosen as it doesn't fall on a applicable week for both
// fortnightly and triweekly autostop requirements.
Expand Down Expand Up @@ -70,8 +76,12 @@ func TestCalculateAutoStop(t *testing.T) {
t.Log("saturdayMidnightAfterDstOut", saturdayMidnightAfterDstOut)

cases := []struct {
name string
now time.Time
name string
now time.Time

wsAutostart string
templateAutoStart schedule.TemplateAutostartRequirement

templateAllowAutostop bool
templateDefaultTTL time.Duration
templateAutostopRequirement schedule.TemplateAutostopRequirement
Expand Down Expand Up @@ -364,6 +374,115 @@ func TestCalculateAutoStop(t *testing.T) {
// expectedDeadline is copied from expectedMaxDeadline.
expectedMaxDeadline: dstOutQuietHoursExpectedTime,
},
{
// A user expects this workspace to be online from 9am -> 9pm.
// So if a deadline is going to land in the middle of this range,
// we should bump it to the end.
// This is already done on `ActivityBumpWorkspace`, but that requires
// activity on the workspace.
name: "AutostopCrossAutostartBorder",
// Starting at 9:45pm, with the autostart at 9am.
now: pastDateNight,
templateAllowAutostop: false,
templateDefaultTTL: time.Hour * 12,
workspaceTTL: time.Hour * 12,
// At 9am every morning
wsAutostart: "CRON_TZ=America/Chicago 0 9 * * *",

// No quiet hours
templateAutoStart: schedule.TemplateAutostartRequirement{
// Just allow all days of the week
DaysOfWeek: 0b01111111,
},
templateAutostopRequirement: schedule.TemplateAutostopRequirement{},
userQuietHoursSchedule: "",

expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 21, 0, 0, 0, chicago),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you use pastDateNight.AddDate() here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddDate cannot add hours. So it would be AddDate(0,0,1).Add(time.Hour*X) But the number of hours is 23.25 hours, which is such a strange amount.

pastDateNight = 9:45pm on Wednesday
expectedDeadline = 9pm Thursday, which is 12hrs + autostart (which is a cron string, not a Go time.Time).

It was easier for me to reason because the offset being 23.25 hours is not really the assertion I am making. I want to assert it's 12hrs after autostart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair!

expectedMaxDeadline: time.Time{},
errContains: "",
},
{
// Same as AutostopCrossAutostartBorder, but just misses the autostart.
name: "AutostopCrossMissAutostartBorder",
// Starting at 8:45pm, with the autostart at 9am.
now: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day(), 20, 30, 0, 0, chicago),
templateAllowAutostop: false,
templateDefaultTTL: time.Hour * 12,
workspaceTTL: time.Hour * 12,
// At 9am every morning
wsAutostart: "CRON_TZ=America/Chicago 0 9 * * *",

// No quiet hours
templateAutoStart: schedule.TemplateAutostartRequirement{
// Just allow all days of the week
DaysOfWeek: 0b01111111,
},
templateAutostopRequirement: schedule.TemplateAutostopRequirement{},
userQuietHoursSchedule: "",

expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 8, 30, 0, 0, chicago),
expectedMaxDeadline: time.Time{},
errContains: "",
},
{
// Same as AutostopCrossAutostartBorderMaxEarlyDeadline with max deadline to limit it.
// The autostop deadline is before the autostart threshold.
name: "AutostopCrossAutostartBorderMaxEarlyDeadline",
// Starting at 9:45pm, with the autostart at 9am.
now: pastDateNight,
templateAllowAutostop: false,
templateDefaultTTL: time.Hour * 12,
workspaceTTL: time.Hour * 12,
// At 9am every morning
wsAutostart: "CRON_TZ=America/Chicago 0 9 * * *",

// No quiet hours
templateAutoStart: schedule.TemplateAutostartRequirement{
// Just allow all days of the week
DaysOfWeek: 0b01111111,
},
templateAutostopRequirement: schedule.TemplateAutostopRequirement{
// Autostop every day
DaysOfWeek: 0b01111111,
Weeks: 0,
},
// 6am quiet hours
userQuietHoursSchedule: "CRON_TZ=America/Chicago 0 6 * * *",

expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 6, 0, 0, 0, chicago),
expectedMaxDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 6, 0, 0, 0, chicago),
errContains: "",
},
{
// Same as AutostopCrossAutostartBorder with max deadline to limit it.
// The autostop deadline is after autostart threshold.
// So the deadline is > 12 hours, but stops at the max deadline.
name: "AutostopCrossAutostartBorderMaxDeadline",
// Starting at 9:45pm, with the autostart at 9am.
now: pastDateNight,
templateAllowAutostop: false,
templateDefaultTTL: time.Hour * 12,
workspaceTTL: time.Hour * 12,
// At 9am every morning
wsAutostart: "CRON_TZ=America/Chicago 0 9 * * *",

// No quiet hours
templateAutoStart: schedule.TemplateAutostartRequirement{
// Just allow all days of the week
DaysOfWeek: 0b01111111,
},
templateAutostopRequirement: schedule.TemplateAutostopRequirement{
// Autostop every day
DaysOfWeek: 0b01111111,
Weeks: 0,
},
// 11am quiet hours, yea this is werid case.
userQuietHoursSchedule: "CRON_TZ=America/Chicago 0 11 * * *",

expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 11, 0, 0, 0, chicago),
expectedMaxDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 11, 0, 0, 0, chicago),
errContains: "",
},
}

for _, c := range cases {
Expand All @@ -382,6 +501,7 @@ func TestCalculateAutoStop(t *testing.T) {
UserAutostopEnabled: c.templateAllowAutostop,
DefaultTTL: c.templateDefaultTTL,
AutostopRequirement: c.templateAutostopRequirement,
AutostartRequirement: c.templateAutoStart,
}, nil
},
}
Expand Down Expand Up @@ -433,11 +553,20 @@ func TestCalculateAutoStop(t *testing.T) {
Valid: true,
}
}

autostart := sql.NullString{}
if c.wsAutostart != "" {
autostart = sql.NullString{
String: c.wsAutostart,
Valid: true,
}
}
workspace := dbgen.Workspace(t, db, database.Workspace{
TemplateID: template.ID,
OrganizationID: org.ID,
OwnerID: user.ID,
Ttl: workspaceTTL,
TemplateID: template.ID,
OrganizationID: org.ID,
OwnerID: user.ID,
Ttl: workspaceTTL,
AutostartSchedule: autostart,
})

autostop, err := schedule.CalculateAutostop(ctx, schedule.CalculateAutostopParams{
Expand All @@ -446,6 +575,7 @@ func TestCalculateAutoStop(t *testing.T) {
UserQuietHoursScheduleStore: userQuietHoursScheduleStore,
Now: c.now,
Workspace: workspace,
WorkspaceAutostart: c.wsAutostart,
})
if c.errContains != "" {
require.Error(t, err)
Expand Down
4 changes: 2 additions & 2 deletions coderd/workspaceagents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"cdr.dev/slog"
agentproto "github.com/coder/coder/v2/agent/proto"
"github.com/coder/coder/v2/coderd/agentapi"
"github.com/coder/coder/v2/coderd/autobuild"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbauthz"
Expand All @@ -37,6 +36,7 @@ import (
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/prometheusmetrics"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/schedule"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/agentsdk"
"github.com/coder/coder/v2/codersdk/workspacesdk"
Expand Down Expand Up @@ -1186,7 +1186,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
slog.Error(err),
)
} else {
next, allowed := autobuild.NextAutostartSchedule(time.Now(), workspace.AutostartSchedule.String, templateSchedule)
next, allowed := schedule.NextAutostart(time.Now(), workspace.AutostartSchedule.String, templateSchedule)
if allowed {
nextAutostart = next
}
Expand Down
5 changes: 3 additions & 2 deletions enterprise/coderd/schedule/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,9 @@ func (s *EnterpriseTemplateScheduleStore) updateWorkspaceBuild(ctx context.Conte
TemplateScheduleStore: s,
UserQuietHoursScheduleStore: *s.UserQuietHoursScheduleStore.Load(),
// Use the job completion time as the time we calculate autostop from.
Now: job.CompletedAt.Time,
Workspace: workspace,
Now: job.CompletedAt.Time,
Workspace: workspace,
WorkspaceAutostart: workspace.AutostartSchedule.String,
})
if err != nil {
return xerrors.Errorf("calculate new autostop for workspace %q: %w", workspace.ID, err)
Expand Down
Loading