Skip to content

Commit e21a301

Browse files
DanielleMaywoodDanielleMaywood
authored
fix: make GetWorkspacesEligibleForTransition return even less false positives (coder#15594)
Relates to coder#15082 Further to coder#15429, this reduces the amount of false-positives returned by the 'is eligible for autostart' part of the query. We achieve this by calculating the 'next start at' time of the workspace, storing it in the database, and using it in our `GetWorkspacesEligibleForTransition` query. The prior implementation of the 'is eligible for autostart' query would return _all_ workspaces that at some point in the future _might_ be eligible for autostart. This now ensures we only return workspaces that _should_ be eligible for autostart. We also now pass `currentTick` instead of `t` to the `GetWorkspacesEligibleForTransition` query as otherwise we'll have one round of workspaces that are skipped by `isEligibleForTransition` due to `currentTick` being a truncated version of `t`.
1 parent 2b57dcc commit e21a301

35 files changed

+1012
-75
lines changed

‎cli/testdata/coder_list_--output_json.golden

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
},
6666
"automatic_updates": "never",
6767
"allow_renames": false,
68-
"favorite": false
68+
"favorite": false,
69+
"next_start_at": "[timestamp]"
6970
}
7071
]

‎coderd/apidoc/docs.go

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/autobuild/lifecycle_executor.go

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
142142
// NOTE: If a workspace build is created with a given TTL and then the user either
143143
// changes or unsets the TTL, the deadline for the workspace build will not
144144
// have changed. This behavior is as expected per #2229.
145-
workspaces, err := e.db.GetWorkspacesEligibleForTransition(e.ctx, t)
145+
workspaces, err := e.db.GetWorkspacesEligibleForTransition(e.ctx, currentTick)
146146
if err != nil {
147147
e.log.Error(e.ctx, "get workspaces for autostart or autostop", slog.Error(err))
148148
return stats
@@ -205,6 +205,23 @@ func (e *Executor) runOnce(t time.Time) Stats {
205205
return xerrors.Errorf("get template scheduling options: %w", err)
206206
}
207207

208+
// If next start at is not valid we need to re-compute it
209+
if !ws.NextStartAt.Valid && ws.AutostartSchedule.Valid {
210+
next, err := schedule.NextAllowedAutostart(currentTick, ws.AutostartSchedule.String, templateSchedule)
211+
if err == nil {
212+
nextStartAt := sql.NullTime{Valid: true, Time: dbtime.Time(next.UTC())}
213+
if err = tx.UpdateWorkspaceNextStartAt(e.ctx, database.UpdateWorkspaceNextStartAtParams{
214+
ID: wsID,
215+
NextStartAt: nextStartAt,
216+
}); err != nil {
217+
return xerrors.Errorf("update workspace next start at: %w", err)
218+
}
219+
220+
// Save re-fetching the workspace
221+
ws.NextStartAt = nextStartAt
222+
}
223+
}
224+
208225
tmpl, err = tx.GetTemplateByID(e.ctx, ws.TemplateID)
209226
if err != nil {
210227
return xerrors.Errorf("get template by ID: %w", err)
@@ -463,8 +480,8 @@ func isEligibleForAutostart(user database.User, ws database.Workspace, build dat
463480
return false
464481
}
465482

466-
nextTransition, allowed := schedule.NextAutostart(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
467-
if !allowed {
483+
nextTransition, err := schedule.NextAllowedAutostart(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
484+
if err != nil {
468485
return false
469486
}
470487

‎coderd/autobuild/lifecycle_executor_test.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1083,6 +1083,10 @@ func TestNotifications(t *testing.T) {
10831083
IncludeProvisionerDaemon: true,
10841084
NotificationsEnqueuer: &notifyEnq,
10851085
TemplateScheduleStore: schedule.MockTemplateScheduleStore{
1086+
SetFn: func(ctx context.Context, db database.Store, template database.Template, options schedule.TemplateScheduleOptions) (database.Template, error) {
1087+
template.TimeTilDormant = int64(options.TimeTilDormant)
1088+
return schedule.NewAGPLTemplateScheduleStore().Set(ctx, db, template, options)
1089+
},
10861090
GetFn: func(_ context.Context, _ database.Store, _ uuid.UUID) (schedule.TemplateScheduleOptions, error) {
10871091
return schedule.TemplateScheduleOptions{
10881092
UserAutostartEnabled: false,
@@ -1099,7 +1103,9 @@ func TestNotifications(t *testing.T) {
10991103
)
11001104

11011105
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
1102-
template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
1106+
template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
1107+
ctr.TimeTilDormantMillis = ptr.Ref(timeTilDormant.Milliseconds())
1108+
})
11031109
userClient, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
11041110
workspace := coderdtest.CreateWorkspace(t, userClient, template.ID)
11051111
coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID)

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,6 +1053,13 @@ func (q *querier) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg databa
10531053
return q.db.BatchUpdateWorkspaceLastUsedAt(ctx, arg)
10541054
}
10551055

1056+
func (q *querier) BatchUpdateWorkspaceNextStartAt(ctx context.Context, arg database.BatchUpdateWorkspaceNextStartAtParams) error {
1057+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceWorkspace.All()); err != nil {
1058+
return err
1059+
}
1060+
return q.db.BatchUpdateWorkspaceNextStartAt(ctx, arg)
1061+
}
1062+
10561063
func (q *querier) BulkMarkNotificationMessagesFailed(ctx context.Context, arg database.BulkMarkNotificationMessagesFailedParams) (int64, error) {
10571064
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceNotificationMessage); err != nil {
10581065
return 0, err
@@ -2840,6 +2847,13 @@ func (q *querier) GetWorkspacesAndAgentsByOwnerID(ctx context.Context, ownerID u
28402847
return q.db.GetAuthorizedWorkspacesAndAgentsByOwnerID(ctx, ownerID, prep)
28412848
}
28422849

2850+
func (q *querier) GetWorkspacesByTemplateID(ctx context.Context, templateID uuid.UUID) ([]database.WorkspaceTable, error) {
2851+
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
2852+
return nil, err
2853+
}
2854+
return q.db.GetWorkspacesByTemplateID(ctx, templateID)
2855+
}
2856+
28432857
func (q *querier) GetWorkspacesEligibleForTransition(ctx context.Context, now time.Time) ([]database.GetWorkspacesEligibleForTransitionRow, error) {
28442858
return q.db.GetWorkspacesEligibleForTransition(ctx, now)
28452859
}
@@ -4085,6 +4099,13 @@ func (q *querier) UpdateWorkspaceLastUsedAt(ctx context.Context, arg database.Up
40854099
return update(q.log, q.auth, fetch, q.db.UpdateWorkspaceLastUsedAt)(ctx, arg)
40864100
}
40874101

4102+
func (q *querier) UpdateWorkspaceNextStartAt(ctx context.Context, arg database.UpdateWorkspaceNextStartAtParams) error {
4103+
fetch := func(ctx context.Context, arg database.UpdateWorkspaceNextStartAtParams) (database.Workspace, error) {
4104+
return q.db.GetWorkspaceByID(ctx, arg.ID)
4105+
}
4106+
return update(q.log, q.auth, fetch, q.db.UpdateWorkspaceNextStartAt)(ctx, arg)
4107+
}
4108+
40884109
func (q *querier) UpdateWorkspaceProxy(ctx context.Context, arg database.UpdateWorkspaceProxyParams) (database.WorkspaceProxy, error) {
40894110
fetch := func(ctx context.Context, arg database.UpdateWorkspaceProxyParams) (database.WorkspaceProxy, error) {
40904111
return q.db.GetWorkspaceProxyByID(ctx, arg.ID)

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1908,6 +1908,19 @@ func (s *MethodTestSuite) TestWorkspace() {
19081908
ID: ws.ID,
19091909
}).Asserts(ws, policy.ActionUpdate).Returns()
19101910
}))
1911+
s.Run("UpdateWorkspaceNextStartAt", s.Subtest(func(db database.Store, check *expects) {
1912+
ws := dbgen.Workspace(s.T(), db, database.WorkspaceTable{})
1913+
check.Args(database.UpdateWorkspaceNextStartAtParams{
1914+
ID: ws.ID,
1915+
NextStartAt: sql.NullTime{Valid: true, Time: dbtime.Now()},
1916+
}).Asserts(ws, policy.ActionUpdate)
1917+
}))
1918+
s.Run("BatchUpdateWorkspaceNextStartAt", s.Subtest(func(db database.Store, check *expects) {
1919+
check.Args(database.BatchUpdateWorkspaceNextStartAtParams{
1920+
IDs: []uuid.UUID{uuid.New()},
1921+
NextStartAts: []time.Time{dbtime.Now()},
1922+
}).Asserts(rbac.ResourceWorkspace.All(), policy.ActionUpdate)
1923+
}))
19111924
s.Run("BatchUpdateWorkspaceLastUsedAt", s.Subtest(func(db database.Store, check *expects) {
19121925
ws1 := dbgen.Workspace(s.T(), db, database.WorkspaceTable{})
19131926
ws2 := dbgen.Workspace(s.T(), db, database.WorkspaceTable{})
@@ -2784,6 +2797,9 @@ func (s *MethodTestSuite) TestSystemFunctions() {
27842797
s.Run("GetTemplateAverageBuildTime", s.Subtest(func(db database.Store, check *expects) {
27852798
check.Args(database.GetTemplateAverageBuildTimeParams{}).Asserts(rbac.ResourceSystem, policy.ActionRead)
27862799
}))
2800+
s.Run("GetWorkspacesByTemplateID", s.Subtest(func(db database.Store, check *expects) {
2801+
check.Args(uuid.Nil).Asserts(rbac.ResourceSystem, policy.ActionRead)
2802+
}))
27872803
s.Run("GetWorkspacesEligibleForTransition", s.Subtest(func(db database.Store, check *expects) {
27882804
check.Args(time.Time{}).Asserts()
27892805
}))

‎coderd/database/dbgen/dbgen.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,7 @@ func Workspace(t testing.TB, db database.Store, orig database.WorkspaceTable) da
260260
AutostartSchedule: orig.AutostartSchedule,
261261
Ttl: orig.Ttl,
262262
AutomaticUpdates: takeFirst(orig.AutomaticUpdates, database.AutomaticUpdatesNever),
263+
NextStartAt: orig.NextStartAt,
263264
})
264265
require.NoError(t, err, "insert workspace")
265266
return workspace

‎coderd/database/dbmem/dbmem.go

Lines changed: 77 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,7 @@ func (q *FakeQuerier) convertToWorkspaceRowsNoLock(ctx context.Context, workspac
475475
DeletingAt: w.DeletingAt,
476476
AutomaticUpdates: w.AutomaticUpdates,
477477
Favorite: w.Favorite,
478+
NextStartAt: w.NextStartAt,
478479

479480
OwnerAvatarUrl: extended.OwnerAvatarUrl,
480481
OwnerUsername: extended.OwnerUsername,
@@ -1431,6 +1432,35 @@ func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(_ context.Context, arg data
14311432
return nil
14321433
}
14331434

1435+
func (q *FakeQuerier) BatchUpdateWorkspaceNextStartAt(_ context.Context, arg database.BatchUpdateWorkspaceNextStartAtParams) error {
1436+
err := validateDatabaseType(arg)
1437+
if err != nil {
1438+
return err
1439+
}
1440+
1441+
q.mutex.Lock()
1442+
defer q.mutex.Unlock()
1443+
1444+
for i, workspace := range q.workspaces {
1445+
for j, workspaceID := range arg.IDs {
1446+
if workspace.ID != workspaceID {
1447+
continue
1448+
}
1449+
1450+
nextStartAt := arg.NextStartAts[j]
1451+
if nextStartAt.IsZero() {
1452+
q.workspaces[i].NextStartAt = sql.NullTime{}
1453+
} else {
1454+
q.workspaces[i].NextStartAt = sql.NullTime{Valid: true, Time: nextStartAt}
1455+
}
1456+
1457+
break
1458+
}
1459+
}
1460+
1461+
return nil
1462+
}
1463+
14341464
func (*FakeQuerier) BulkMarkNotificationMessagesFailed(_ context.Context, arg database.BulkMarkNotificationMessagesFailedParams) (int64, error) {
14351465
err := validateDatabaseType(arg)
14361466
if err != nil {
@@ -6908,6 +6938,20 @@ func (q *FakeQuerier) GetWorkspacesAndAgentsByOwnerID(ctx context.Context, owner
69086938
return q.GetAuthorizedWorkspacesAndAgentsByOwnerID(ctx, ownerID, nil)
69096939
}
69106940

6941+
func (q *FakeQuerier) GetWorkspacesByTemplateID(_ context.Context, templateID uuid.UUID) ([]database.WorkspaceTable, error) {
6942+
q.mutex.RLock()
6943+
defer q.mutex.RUnlock()
6944+
6945+
workspaces := []database.WorkspaceTable{}
6946+
for _, workspace := range q.workspaces {
6947+
if workspace.TemplateID == templateID {
6948+
workspaces = append(workspaces, workspace)
6949+
}
6950+
}
6951+
6952+
return workspaces, nil
6953+
}
6954+
69116955
func (q *FakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, now time.Time) ([]database.GetWorkspacesEligibleForTransitionRow, error) {
69126956
q.mutex.RLock()
69136957
defer q.mutex.RUnlock()
@@ -6952,7 +6996,13 @@ func (q *FakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, no
69526996
if user.Status == database.UserStatusActive &&
69536997
job.JobStatus != database.ProvisionerJobStatusFailed &&
69546998
build.Transition == database.WorkspaceTransitionStop &&
6955-
workspace.AutostartSchedule.Valid {
6999+
workspace.AutostartSchedule.Valid &&
7000+
// We do not know if workspace with a zero next start is eligible
7001+
// for autostart, so we accept this false-positive. This can occur
7002+
// when a coder version is upgraded and next_start_at has yet to
7003+
// be set.
7004+
(workspace.NextStartAt.Time.IsZero() ||
7005+
!now.Before(workspace.NextStartAt.Time)) {
69567006
workspaces = append(workspaces, database.GetWorkspacesEligibleForTransitionRow{
69577007
ID: workspace.ID,
69587008
Name: workspace.Name,
@@ -6962,7 +7012,7 @@ func (q *FakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, no
69627012

69637013
if !workspace.DormantAt.Valid &&
69647014
template.TimeTilDormant > 0 &&
6965-
now.Sub(workspace.LastUsedAt) > time.Duration(template.TimeTilDormant) {
7015+
now.Sub(workspace.LastUsedAt) >= time.Duration(template.TimeTilDormant) {
69667016
workspaces = append(workspaces, database.GetWorkspacesEligibleForTransitionRow{
69677017
ID: workspace.ID,
69687018
Name: workspace.Name,
@@ -7927,6 +7977,7 @@ func (q *FakeQuerier) InsertWorkspace(_ context.Context, arg database.InsertWork
79277977
Ttl: arg.Ttl,
79287978
LastUsedAt: arg.LastUsedAt,
79297979
AutomaticUpdates: arg.AutomaticUpdates,
7980+
NextStartAt: arg.NextStartAt,
79307981
}
79317982
q.workspaces = append(q.workspaces, workspace)
79327983
return workspace, nil
@@ -9868,6 +9919,7 @@ func (q *FakeQuerier) UpdateWorkspaceAutostart(_ context.Context, arg database.U
98689919
continue
98699920
}
98709921
workspace.AutostartSchedule = arg.AutostartSchedule
9922+
workspace.NextStartAt = arg.NextStartAt
98719923
q.workspaces[index] = workspace
98729924
return nil
98739925
}
@@ -10017,6 +10069,29 @@ func (q *FakeQuerier) UpdateWorkspaceLastUsedAt(_ context.Context, arg database.
1001710069
return sql.ErrNoRows
1001810070
}
1001910071

10072+
func (q *FakeQuerier) UpdateWorkspaceNextStartAt(_ context.Context, arg database.UpdateWorkspaceNextStartAtParams) error {
10073+
err := validateDatabaseType(arg)
10074+
if err != nil {
10075+
return err
10076+
}
10077+
10078+
q.mutex.Lock()
10079+
defer q.mutex.Unlock()
10080+
10081+
for index, workspace := range q.workspaces {
10082+
if workspace.ID != arg.ID {
10083+
continue
10084+
}
10085+
10086+
workspace.NextStartAt = arg.NextStartAt
10087+
q.workspaces[index] = workspace
10088+
10089+
return nil
10090+
}
10091+
10092+
return sql.ErrNoRows
10093+
}
10094+
1002010095
func (q *FakeQuerier) UpdateWorkspaceProxy(_ context.Context, arg database.UpdateWorkspaceProxyParams) (database.WorkspaceProxy, error) {
1002110096
q.mutex.Lock()
1002210097
defer q.mutex.Unlock()

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 21 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)