From e25d8aba0cff9016866c1bae2e8d06f059956e1f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Sep 2023 17:23:47 +0100 Subject: [PATCH 01/17] first stab at query --- coderd/activitybump.go | 116 ++++++++++++----------- coderd/database/dbauthz/dbauthz.go | 4 + coderd/database/dbfake/dbfake.go | 9 ++ coderd/database/dbmetrics/dbmetrics.go | 7 ++ coderd/database/dbmock/dbmock.go | 14 +++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 32 +++++++ coderd/database/queries/activitybump.sql | 20 ++++ 8 files changed, 147 insertions(+), 56 deletions(-) create mode 100644 coderd/database/queries/activitybump.sql diff --git a/coderd/activitybump.go b/coderd/activitybump.go index 0b84f0a225c98..06ee3d4aa45a1 100644 --- a/coderd/activitybump.go +++ b/coderd/activitybump.go @@ -2,8 +2,6 @@ package coderd import ( "context" - "database/sql" - "errors" "time" "github.com/google/uuid" @@ -23,67 +21,73 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto defer cancel() err := db.InTx(func(s database.Store) error { - build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) - if errors.Is(err, sql.ErrNoRows) { - return nil - } else if err != nil { - return xerrors.Errorf("get latest workspace build: %w", err) + if err := s.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{ + WorkspaceID: workspaceID, + UpdatedAt: dbtime.Now(), + }); err != nil { + return xerrors.Errorf("activity bump workspace: %w", err) } + // build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) + // if errors.Is(err, sql.ErrNoRows) { + // return nil + // } else if err != nil { + // return xerrors.Errorf("get latest workspace build: %w", err) + // } - job, err := s.GetProvisionerJobByID(ctx, build.JobID) - if err != nil { - return xerrors.Errorf("get provisioner job: %w", err) - } + // job, err := s.GetProvisionerJobByID(ctx, build.JobID) + // if err != nil { + // return xerrors.Errorf("get provisioner job: %w", err) + // } - if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { - return nil - } + // if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { + // return nil + // } - if build.Deadline.IsZero() { - // Workspace shutdown is manual - return nil - } + // if build.Deadline.IsZero() { + // // Workspace shutdown is manual + // return nil + // } - workspace, err := s.GetWorkspaceByID(ctx, workspaceID) - if err != nil { - return xerrors.Errorf("get workspace: %w", err) - } - - var ( - // We bump by the original TTL to prevent counter-intuitive behavior - // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off - // work at midnight, come back at 10am, I would want another full day - // of uptime. In the prior implementation, the workspace would enter - // a state of always expiring 1 hour in the future - bumpAmount = time.Duration(workspace.Ttl.Int64) - // DB writes are expensive so we only bump when 5% of the deadline - // has elapsed. - bumpEvery = bumpAmount / 20 - timeSinceLastBump = bumpAmount - time.Until(build.Deadline) - ) + // workspace, err := s.GetWorkspaceByID(ctx, workspaceID) + // if err != nil { + // return xerrors.Errorf("get workspace: %w", err) + // } - if timeSinceLastBump < bumpEvery { - return nil - } + // var ( + // We bump by the original TTL to prevent counter-intuitive behavior + // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off + // work at midnight, come back at 10am, I would want another full day + // of uptime. In the prior implementation, the workspace would enter + // a state of always expiring 1 hour in the future + // bumpAmount = time.Duration(workspace.Ttl.Int64) + // DB writes are expensive so we only bump when 5% of the deadline + // has elapsed. + // bumpEvery = bumpAmount / 20 + // timeSinceLastBump = bumpAmount - time.Until(build.Deadline) + // ) - if bumpAmount == 0 { - return nil - } - - newDeadline := dbtime.Now().Add(bumpAmount) - if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) { - newDeadline = build.MaxDeadline - } - - if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ - ID: build.ID, - UpdatedAt: dbtime.Now(), - ProvisionerState: build.ProvisionerState, - Deadline: newDeadline, - MaxDeadline: build.MaxDeadline, - }); err != nil { - return xerrors.Errorf("update workspace build: %w", err) - } + // if timeSinceLastBump < bumpEvery { + // return nil + // } + // + // if bumpAmount == 0 { + // return nil + // } + // + // newDeadline := dbtime.Now().Add(bumpAmount) + // if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) { + // newDeadline = build.MaxDeadline + // } + // + // if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ + // ID: build.ID, + // UpdatedAt: dbtime.Now(), + // ProvisionerState: build.ProvisionerState, + // Deadline: newDeadline, + // MaxDeadline: build.MaxDeadline, + // }); err != nil { + // return xerrors.Errorf("update workspace build: %w", err) + // } return nil }, nil) if err != nil { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 8ddd779d795e9..58d044130db97 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -657,6 +657,10 @@ func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.Acquir return q.db.AcquireProvisionerJob(ctx, arg) } +func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { + panic("not implemented") +} + func (q *querier) CleanTailnetCoordinators(ctx context.Context) error { if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceTailnetCoordinator); err != nil { return err diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index e73578a61a7df..fb2bdcc230b01 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -744,6 +744,15 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu return database.ProvisionerJob{}, sql.ErrNoRows } +func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { + err := validateDatabaseType(arg) + if err != nil { + return err + } + + panic("not implemented") +} + func (*FakeQuerier) CleanTailnetCoordinators(_ context.Context) error { return ErrUnimplemented } diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 0a02896200f60..280e2e006bd12 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -93,6 +93,13 @@ func (m metricsStore) AcquireProvisionerJob(ctx context.Context, arg database.Ac return provisionerJob, err } +func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { + start := time.Now() + r0 := m.s.ActivityBumpWorkspace(ctx, arg) + m.queryLatencies.WithLabelValues("ActivityBumpWorkspace").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) CleanTailnetCoordinators(ctx context.Context) error { start := time.Now() err := m.s.CleanTailnetCoordinators(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index be1f994d81161..2ab7879ee6be3 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -68,6 +68,20 @@ func (mr *MockStoreMockRecorder) AcquireProvisionerJob(arg0, arg1 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AcquireProvisionerJob", reflect.TypeOf((*MockStore)(nil).AcquireProvisionerJob), arg0, arg1) } +// ActivityBumpWorkspace mocks base method. +func (m *MockStore) ActivityBumpWorkspace(arg0 context.Context, arg1 database.ActivityBumpWorkspaceParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ActivityBumpWorkspace", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// ActivityBumpWorkspace indicates an expected call of ActivityBumpWorkspace. +func (mr *MockStoreMockRecorder) ActivityBumpWorkspace(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ActivityBumpWorkspace", reflect.TypeOf((*MockStore)(nil).ActivityBumpWorkspace), arg0, arg1) +} + // CleanTailnetCoordinators mocks base method. func (m *MockStore) CleanTailnetCoordinators(arg0 context.Context) error { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index cdf4d184544bb..b65308c98f5a5 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -24,6 +24,7 @@ type sqlcQuerier interface { // multiple provisioners from acquiring the same jobs. See: // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) + ActivityBumpWorkspace(ctx context.Context, arg ActivityBumpWorkspaceParams) error CleanTailnetCoordinators(ctx context.Context) error DeleteAPIKeyByID(ctx context.Context, id string) error DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4d9bc72a37157..22ce0ff2b49e9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -15,6 +15,38 @@ import ( "github.com/sqlc-dev/pqtype" ) +const activityBumpWorkspace = `-- name: ActivityBumpWorkspace :exec +UPDATE + workspace_builds +SET + updated_at = $2, + deadline = LEAST(workspace_builds.deadline + workspace.ttl, workspace_builds.max_deadline) +WHERE + workspace_builds.id IN ( + SELECT wb.id + FROM workspace_builds wb + JOIN provisioner_jobs pj + ON pj.id = wb.job_id + WHERE wb.workspace_id = $1 + AND wb.transition == 'start' + AND wb.deadline > $2 + AND wb.deadline != wb.max_deadline + AND pj.completed_at IS NOT NULL + ORDER BY wb.build_number ASC + LIMIT 1 +) +` + +type ActivityBumpWorkspaceParams struct { + WorkspaceID uuid.UUID `db:"workspace_id" json:"workspace_id"` + UpdatedAt time.Time `db:"updated_at" json:"updated_at"` +} + +func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, arg ActivityBumpWorkspaceParams) error { + _, err := q.db.ExecContext(ctx, activityBumpWorkspace, arg.WorkspaceID, arg.UpdatedAt) + return err +} + const deleteAPIKeyByID = `-- name: DeleteAPIKeyByID :exec DELETE FROM api_keys diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql new file mode 100644 index 0000000000000..f795a8466a9a2 --- /dev/null +++ b/coderd/database/queries/activitybump.sql @@ -0,0 +1,20 @@ +-- name: ActivityBumpWorkspace :exec +UPDATE + workspace_builds +SET + updated_at = $2, + deadline = LEAST(workspace_builds.deadline + workspace.ttl, workspace_builds.max_deadline) +WHERE + workspace_builds.id IN ( + SELECT wb.id + FROM workspace_builds wb + JOIN provisioner_jobs pj + ON pj.id = wb.job_id + WHERE wb.workspace_id = $1 + AND wb.transition == 'start' + AND wb.deadline > $2 + AND wb.deadline != wb.max_deadline + AND pj.completed_at IS NOT NULL + ORDER BY wb.build_number ASC + LIMIT 1 +); From 1ee5403002c0a109be24142d26a09da262d3c6ed Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 8 Sep 2023 20:02:51 +0100 Subject: [PATCH 02/17] update query --- coderd/activitybump.go | 6 +-- coderd/database/dbauthz/dbauthz.go | 7 ++- coderd/database/dbfake/dbfake.go | 35 +++++++++++++-- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 2 +- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 55 +++++++++++++----------- coderd/database/queries/activitybump.sql | 45 ++++++++++++------- 8 files changed, 101 insertions(+), 53 deletions(-) diff --git a/coderd/activitybump.go b/coderd/activitybump.go index 06ee3d4aa45a1..d966749cd2945 100644 --- a/coderd/activitybump.go +++ b/coderd/activitybump.go @@ -9,7 +9,6 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbtime" ) // activityBumpWorkspace automatically bumps the workspace's auto-off timer @@ -21,10 +20,7 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto defer cancel() err := db.InTx(func(s database.Store) error { - if err := s.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{ - WorkspaceID: workspaceID, - UpdatedAt: dbtime.Now(), - }); err != nil { + if err := s.ActivityBumpWorkspace(ctx, workspaceID); err != nil { return xerrors.Errorf("activity bump workspace: %w", err) } // build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 58d044130db97..6156329cf7ddd 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -657,8 +657,11 @@ func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.Acquir return q.db.AcquireProvisionerJob(ctx, arg) } -func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { - panic("not implemented") +func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error { + fetch := func(ctx context.Context, arg uuid.UUID) (database.Workspace, error) { + return q.db.GetWorkspaceByID(ctx, arg) + } + return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg) } func (q *querier) CleanTailnetCoordinators(ctx context.Context) error { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index fb2bdcc230b01..f20be57089edf 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -685,6 +685,13 @@ func (q *FakeQuerier) GetActiveDBCryptKeys(_ context.Context) ([]database.DBCryp return ks, nil } +func minTime(t, u time.Time) time.Time { + if t.Before(u) { + return t + } + return u +} + func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error { return xerrors.New("AcquireLock must only be called within a transaction") } @@ -744,13 +751,35 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu return database.ProvisionerJob{}, sql.ErrNoRows } -func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { - err := validateDatabaseType(arg) +func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error { + err := validateDatabaseType(workspaceID) + if err != nil { + return err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + workspace, err := q.getWorkspaceByIDNoLock(ctx, workspaceID) if err != nil { return err } + latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, workspaceID) + if err != nil { + return err + } + + for i := range q.workspaceBuilds { + if q.workspaceBuilds[i].BuildNumber != latestBuild.BuildNumber { + continue + } + newDeadline := q.workspaceBuilds[i].Deadline.Add(time.Duration(workspace.Ttl.Int64)) + q.workspaceBuilds[i].UpdatedAt = latestBuild.UpdatedAt + q.workspaceBuilds[i].Deadline = minTime(newDeadline, q.workspaceBuilds[i].MaxDeadline) + return nil + } - panic("not implemented") + return sql.ErrNoRows } func (*FakeQuerier) CleanTailnetCoordinators(_ context.Context) error { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 280e2e006bd12..768c1d4adbcca 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -93,7 +93,7 @@ func (m metricsStore) AcquireProvisionerJob(ctx context.Context, arg database.Ac return provisionerJob, err } -func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error { +func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error { start := time.Now() r0 := m.s.ActivityBumpWorkspace(ctx, arg) m.queryLatencies.WithLabelValues("ActivityBumpWorkspace").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 2ab7879ee6be3..641dd7315b936 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -69,7 +69,7 @@ func (mr *MockStoreMockRecorder) AcquireProvisionerJob(arg0, arg1 interface{}) * } // ActivityBumpWorkspace mocks base method. -func (m *MockStore) ActivityBumpWorkspace(arg0 context.Context, arg1 database.ActivityBumpWorkspaceParams) error { +func (m *MockStore) ActivityBumpWorkspace(arg0 context.Context, arg1 uuid.UUID) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ActivityBumpWorkspace", arg0, arg1) ret0, _ := ret[0].(error) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index b65308c98f5a5..b16ec2af53e89 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -24,7 +24,7 @@ type sqlcQuerier interface { // multiple provisioners from acquiring the same jobs. See: // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) - ActivityBumpWorkspace(ctx context.Context, arg ActivityBumpWorkspaceParams) error + ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error CleanTailnetCoordinators(ctx context.Context) error DeleteAPIKeyByID(ctx context.Context, id string) error DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 22ce0ff2b49e9..2a82d0b4a852c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -16,34 +16,41 @@ import ( ) const activityBumpWorkspace = `-- name: ActivityBumpWorkspace :exec -UPDATE - workspace_builds -SET - updated_at = $2, - deadline = LEAST(workspace_builds.deadline + workspace.ttl, workspace_builds.max_deadline) -WHERE - workspace_builds.id IN ( - SELECT wb.id - FROM workspace_builds wb - JOIN provisioner_jobs pj - ON pj.id = wb.job_id - WHERE wb.workspace_id = $1 - AND wb.transition == 'start' - AND wb.deadline > $2 - AND wb.deadline != wb.max_deadline - AND pj.completed_at IS NOT NULL - ORDER BY wb.build_number ASC +WITH latest AS ( + SELECT + workspace_builds.id, + workspace_builds.deadline, + workspace_builds.max_deadline, + workspaces.ttl, + (workspace_builds.deadline + (workspaces.ttl/1000 || ' microsecond')::interval ) AS new_deadline + FROM workspace_builds + JOIN provisioner_jobs + ON provisioner_jobs.id = workspace_builds.job_id + JOIN workspaces + ON workspaces.id = workspace_builds.workspace_id + WHERE workspace_builds.workspace_id = $1::uuid + AND workspace_builds.transition = 'start' + AND workspace_builds.deadline > NOW() + AND provisioner_jobs.completed_at IS NOT NULL + ORDER BY workspace_builds.build_number ASC LIMIT 1 ) +UPDATE + workspace_builds wb +SET + updated_at = NOW(), + deadline = CASE + WHEN l.max_deadline = '0001-01-01 00:00:00' + THEN l.new_deadline + ELSE LEAST(l.new_deadline, l.max_deadline) + END +FROM latest l +WHERE + wb.id = l.id ` -type ActivityBumpWorkspaceParams struct { - WorkspaceID uuid.UUID `db:"workspace_id" json:"workspace_id"` - UpdatedAt time.Time `db:"updated_at" json:"updated_at"` -} - -func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, arg ActivityBumpWorkspaceParams) error { - _, err := q.db.ExecContext(ctx, activityBumpWorkspace, arg.WorkspaceID, arg.UpdatedAt) +func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error { + _, err := q.db.ExecContext(ctx, activityBumpWorkspace, dollar_1) return err } diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index f795a8466a9a2..bf28c86912ea6 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -1,20 +1,33 @@ -- name: ActivityBumpWorkspace :exec +WITH latest AS ( + SELECT + workspace_builds.id, + workspace_builds.deadline, + workspace_builds.max_deadline, + workspaces.ttl, + (workspace_builds.deadline + (workspaces.ttl/1000 || ' microsecond')::interval ) AS new_deadline + FROM workspace_builds + JOIN provisioner_jobs + ON provisioner_jobs.id = workspace_builds.job_id + JOIN workspaces + ON workspaces.id = workspace_builds.workspace_id + WHERE workspace_builds.workspace_id = $1::uuid + AND workspace_builds.transition = 'start' + AND workspace_builds.deadline > NOW() + AND provisioner_jobs.completed_at IS NOT NULL + ORDER BY workspace_builds.build_number ASC + LIMIT 1 +) UPDATE - workspace_builds + workspace_builds wb SET - updated_at = $2, - deadline = LEAST(workspace_builds.deadline + workspace.ttl, workspace_builds.max_deadline) + updated_at = NOW(), + deadline = CASE + WHEN l.max_deadline = '0001-01-01 00:00:00' + THEN l.new_deadline + ELSE LEAST(l.new_deadline, l.max_deadline) + END +FROM latest l WHERE - workspace_builds.id IN ( - SELECT wb.id - FROM workspace_builds wb - JOIN provisioner_jobs pj - ON pj.id = wb.job_id - WHERE wb.workspace_id = $1 - AND wb.transition == 'start' - AND wb.deadline > $2 - AND wb.deadline != wb.max_deadline - AND pj.completed_at IS NOT NULL - ORDER BY wb.build_number ASC - LIMIT 1 -); + wb.id = l.id +; From a71a8090f696dde2cf80a0cb1175a0864414790d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 12 Sep 2023 13:25:17 +0100 Subject: [PATCH 03/17] add unit-style tests for activityBumpWorkspace --- coderd/activitybump_internal_test.go | 175 +++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 coderd/activitybump_internal_test.go diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go new file mode 100644 index 0000000000000..1e428e01f36aa --- /dev/null +++ b/coderd/activitybump_internal_test.go @@ -0,0 +1,175 @@ +package coderd + +import ( + "database/sql" + "testing" + "time" + + "github.com/google/uuid" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/testutil" + + "github.com/stretchr/testify/require" +) + +func Test_ActivityBumpWorkspace(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + transition database.WorkspaceTransition + jobCompletedAt sql.NullTime + buildDeadline time.Time + maxDeadline time.Time + workspaceTTL time.Duration + expectedBump time.Duration + }{ + { + name: "NotFinishedYet", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{}, + buildDeadline: dbtime.Now().Add(8 * time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, + }, + { + name: "ManualShutdown", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, + buildDeadline: time.Time{}, + expectedBump: 0, + }, + { + name: "NotTimeToBumpYet", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, + buildDeadline: dbtime.Now().Add(8 * time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, + }, + { + name: "TimeToBump", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadline: dbtime.Now().Add(8*time.Hour - 24*time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 8 * time.Hour, + }, + { + name: "MaxDeadline", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadline: dbtime.Now().Add(time.Minute), // last chance to bump! + maxDeadline: dbtime.Now().Add(time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 1 * time.Hour, + }, + { + // A workspace that is still running, has passed its deadline, but has not + // yet been auto-stopped should still bump the deadline. + name: "PastDeadlineStillBumps", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadline: dbtime.Now().Add(-time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 8 * time.Hour, + }, + { + // A stopped workspace should never bump. + name: "StoppedWorkspace", + transition: database.WorkspaceTransitionStop, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-time.Minute)}, + buildDeadline: dbtime.Now().Add(-time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var ( + ctx = testutil.Context(t, testutil.WaitShort) + log = slogtest.Make(t, nil) + db, _ = dbtestutil.NewDB(t) + org = dbgen.Organization(t, db, database.Organization{}) + user = dbgen.User(t, db, database.User{ + Status: database.UserStatusActive, + }) + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + }) + templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{ + OrganizationID: org.ID, + CreatedBy: user.ID, + }) + template = dbgen.Template(t, db, database.Template{ + OrganizationID: org.ID, + ActiveVersionID: templateVersion.ID, + CreatedBy: user.ID, + }) + ws = dbgen.Workspace(t, db, database.Workspace{ + OwnerID: user.ID, + OrganizationID: org.ID, + TemplateID: template.ID, + Ttl: sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)}, + }) + job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{ + OrganizationID: org.ID, + CompletedAt: tt.jobCompletedAt, + }) + _ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{ + JobID: job.ID, + }) + buildID = uuid.New() + ) + // dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set + err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ + ID: buildID, + CreatedAt: dbtime.Now(), + UpdatedAt: dbtime.Now(), + BuildNumber: 1, + InitiatorID: user.ID, + Reason: database.BuildReasonInitiator, + WorkspaceID: ws.ID, + JobID: job.ID, + TemplateVersionID: templateVersion.ID, + Transition: tt.transition, + Deadline: tt.buildDeadline, + MaxDeadline: tt.maxDeadline, + }) + require.NoError(t, err, "unexpected error inserting workspace build") + bld, err := db.GetWorkspaceBuildByID(ctx, buildID) + require.NoError(t, err, "unexpected error fetching inserted workspace build") + + // Validate our initial state before bump + require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump") + require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump") + require.Equal(t, tt.buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump") + require.Equal(t, tt.maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") + require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump") + + // new deadline is calculated from the time of the bump + approxBumpTime := dbtime.Now() + activityBumpWorkspace(ctx, log, db, bld.WorkspaceID) + + // Validate our state after bump + updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID) + require.NoError(t, err, "unexpected error getting latest workspace build") + if tt.expectedBump == 0 { + require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at") + require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline") + } else { + require.WithinDuration(t, dbtime.Now(), updatedBuild.UpdatedAt, 15*time.Second, "unexpected updated at time after bump") + expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC() + require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump") + } + }) + } +} From 3e583d975ed959a6d6f38e95d5ed66305762eeba Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 12 Sep 2023 21:01:26 +0100 Subject: [PATCH 04/17] improve query --- coderd/activitybump.go | 123 ++++++++++++----------- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbtestutil/db.go | 1 + coderd/database/queries.sql.go | 30 +++--- coderd/database/queries/activitybump.sql | 30 +++--- 5 files changed, 100 insertions(+), 86 deletions(-) diff --git a/coderd/activitybump.go b/coderd/activitybump.go index d966749cd2945..e376c5184ab5d 100644 --- a/coderd/activitybump.go +++ b/coderd/activitybump.go @@ -2,6 +2,8 @@ package coderd import ( "context" + "database/sql" + "errors" "time" "github.com/google/uuid" @@ -9,6 +11,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtime" ) // activityBumpWorkspace automatically bumps the workspace's auto-off timer @@ -18,72 +21,78 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto // low priority operations fail first. ctx, cancel := context.WithTimeout(ctx, time.Second*15) defer cancel() + // if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil { + // if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) { + // // Bump will fail if the context is canceled, but this is ok. + // log.Error(ctx, "bump failed", slog.Error(err), + // slog.F("workspace_id", workspaceID), + // ) + // } + // return + // } err := db.InTx(func(s database.Store) error { - if err := s.ActivityBumpWorkspace(ctx, workspaceID); err != nil { - return xerrors.Errorf("activity bump workspace: %w", err) + build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) + if errors.Is(err, sql.ErrNoRows) { + return nil + } else if err != nil { + return xerrors.Errorf("get latest workspace build: %w", err) } - // build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) - // if errors.Is(err, sql.ErrNoRows) { - // return nil - // } else if err != nil { - // return xerrors.Errorf("get latest workspace build: %w", err) - // } - // job, err := s.GetProvisionerJobByID(ctx, build.JobID) - // if err != nil { - // return xerrors.Errorf("get provisioner job: %w", err) - // } + job, err := s.GetProvisionerJobByID(ctx, build.JobID) + if err != nil { + return xerrors.Errorf("get provisioner job: %w", err) + } + + if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { + return nil + } - // if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { - // return nil - // } + if build.Deadline.IsZero() { + // Workspace shutdown is manual + return nil + } - // if build.Deadline.IsZero() { - // // Workspace shutdown is manual - // return nil - // } + workspace, err := s.GetWorkspaceByID(ctx, workspaceID) + if err != nil { + return xerrors.Errorf("get workspace: %w", err) + } - // workspace, err := s.GetWorkspaceByID(ctx, workspaceID) - // if err != nil { - // return xerrors.Errorf("get workspace: %w", err) - // } + var ( + // We bump by the original TTL to prevent counter-intuitive behavior + // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off + // work at midnight, come back at 10am, I would want another full day + // of uptime. In the prior implementation, the workspace would enter + // a state of always expiring 1 hour in the future + bumpAmount = time.Duration(workspace.Ttl.Int64) + // DB writes are expensive so we only bump when 5% of the deadline + // has elapsed. + bumpEvery = bumpAmount / 20 + timeSinceLastBump = bumpAmount - time.Until(build.Deadline) + ) - // var ( - // We bump by the original TTL to prevent counter-intuitive behavior - // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off - // work at midnight, come back at 10am, I would want another full day - // of uptime. In the prior implementation, the workspace would enter - // a state of always expiring 1 hour in the future - // bumpAmount = time.Duration(workspace.Ttl.Int64) - // DB writes are expensive so we only bump when 5% of the deadline - // has elapsed. - // bumpEvery = bumpAmount / 20 - // timeSinceLastBump = bumpAmount - time.Until(build.Deadline) - // ) + if timeSinceLastBump < bumpEvery { + return nil + } - // if timeSinceLastBump < bumpEvery { - // return nil - // } - // - // if bumpAmount == 0 { - // return nil - // } - // - // newDeadline := dbtime.Now().Add(bumpAmount) - // if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) { - // newDeadline = build.MaxDeadline - // } - // - // if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ - // ID: build.ID, - // UpdatedAt: dbtime.Now(), - // ProvisionerState: build.ProvisionerState, - // Deadline: newDeadline, - // MaxDeadline: build.MaxDeadline, - // }); err != nil { - // return xerrors.Errorf("update workspace build: %w", err) - // } + if bumpAmount == 0 { + return nil + } + + newDeadline := dbtime.Now().Add(bumpAmount) + if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) { + newDeadline = build.MaxDeadline + } + + if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ + ID: build.ID, + UpdatedAt: dbtime.Now(), + ProvisionerState: build.ProvisionerState, + Deadline: newDeadline, + MaxDeadline: build.MaxDeadline, + }); err != nil { + return xerrors.Errorf("update workspace build: %w", err) + } return nil }, nil) if err != nil { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 768c1d4adbcca..46c1cff5aacd6 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -93,7 +93,7 @@ func (m metricsStore) AcquireProvisionerJob(ctx context.Context, arg database.Ac return provisionerJob, err } -func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error { +func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) (error, error) { start := time.Now() r0 := m.s.ActivityBumpWorkspace(ctx, arg) m.queryLatencies.WithLabelValues("ActivityBumpWorkspace").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbtestutil/db.go b/coderd/database/dbtestutil/db.go index 00eae9dd11218..ee5a7658976b3 100644 --- a/coderd/database/dbtestutil/db.go +++ b/coderd/database/dbtestutil/db.go @@ -33,6 +33,7 @@ func NewDB(t testing.TB) (database.Store, pubsub.Pubsub) { ) connectionURL, closePg, err = postgres.Open() require.NoError(t, err) + t.Logf("using postgres connection url: %s", connectionURL) t.Cleanup(closePg) } sqlDB, err := sql.Open("postgres", connectionURL) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2a82d0b4a852c..fb15c1bff334a 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -18,21 +18,20 @@ import ( const activityBumpWorkspace = `-- name: ActivityBumpWorkspace :exec WITH latest AS ( SELECT - workspace_builds.id, - workspace_builds.deadline, - workspace_builds.max_deadline, - workspaces.ttl, - (workspace_builds.deadline + (workspaces.ttl/1000 || ' microsecond')::interval ) AS new_deadline + workspace_builds.id::uuid AS build_id, + workspace_builds.deadline::timestamp AS build_deadline, + workspace_builds.max_deadline::timestamp AS build_max_deadline, + workspace_builds.transition AS build_transition, + provisioner_jobs.completed_at::timestamp AS job_completed_at, + (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval, + (NOW() AT TIME ZONE 'UTC')::timestamp as now_utc FROM workspace_builds JOIN provisioner_jobs ON provisioner_jobs.id = workspace_builds.job_id JOIN workspaces ON workspaces.id = workspace_builds.workspace_id WHERE workspace_builds.workspace_id = $1::uuid - AND workspace_builds.transition = 'start' - AND workspace_builds.deadline > NOW() - AND provisioner_jobs.completed_at IS NOT NULL - ORDER BY workspace_builds.build_number ASC + ORDER BY workspace_builds.build_number DESC LIMIT 1 ) UPDATE @@ -40,13 +39,16 @@ UPDATE SET updated_at = NOW(), deadline = CASE - WHEN l.max_deadline = '0001-01-01 00:00:00' - THEN l.new_deadline - ELSE LEAST(l.new_deadline, l.max_deadline) + WHEN l.build_max_deadline = '0001-01-01 00:00:00' + THEN l.now_utc + l.ttl_interval + ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END FROM latest l -WHERE - wb.id = l.id +WHERE wb.id = l.build_id +AND l.build_transition = 'start' +AND l.build_deadline != '0001-01-01 00:00:00' +AND l.job_completed_at IS NOT NULL +AND l.build_deadline + (l.ttl_interval * 0.05) < NOW() ` func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error { diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index bf28c86912ea6..655bb53550885 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -1,21 +1,20 @@ -- name: ActivityBumpWorkspace :exec WITH latest AS ( SELECT - workspace_builds.id, - workspace_builds.deadline, - workspace_builds.max_deadline, - workspaces.ttl, - (workspace_builds.deadline + (workspaces.ttl/1000 || ' microsecond')::interval ) AS new_deadline + workspace_builds.id::uuid AS build_id, + workspace_builds.deadline::timestamp AS build_deadline, + workspace_builds.max_deadline::timestamp AS build_max_deadline, + workspace_builds.transition AS build_transition, + provisioner_jobs.completed_at::timestamp AS job_completed_at, + (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval, + (NOW() AT TIME ZONE 'UTC')::timestamp as now_utc FROM workspace_builds JOIN provisioner_jobs ON provisioner_jobs.id = workspace_builds.job_id JOIN workspaces ON workspaces.id = workspace_builds.workspace_id WHERE workspace_builds.workspace_id = $1::uuid - AND workspace_builds.transition = 'start' - AND workspace_builds.deadline > NOW() - AND provisioner_jobs.completed_at IS NOT NULL - ORDER BY workspace_builds.build_number ASC + ORDER BY workspace_builds.build_number DESC LIMIT 1 ) UPDATE @@ -23,11 +22,14 @@ UPDATE SET updated_at = NOW(), deadline = CASE - WHEN l.max_deadline = '0001-01-01 00:00:00' - THEN l.new_deadline - ELSE LEAST(l.new_deadline, l.max_deadline) + WHEN l.build_max_deadline = '0001-01-01 00:00:00' + THEN l.now_utc + l.ttl_interval + ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END FROM latest l -WHERE - wb.id = l.id +WHERE wb.id = l.build_id +AND l.build_transition = 'start' +AND l.build_deadline != '0001-01-01 00:00:00' +AND l.job_completed_at IS NOT NULL +AND l.build_deadline + (l.ttl_interval * 0.05) < NOW() ; From 1829a25ffa0822173a4abf257f5052ae589c246e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 12 Sep 2023 22:27:06 +0100 Subject: [PATCH 05/17] fix bugged query --- coderd/activitybump.go | 79 +----------------------- coderd/activitybump_internal_test.go | 2 +- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 11 +++- coderd/database/queries/activitybump.sql | 11 +++- 6 files changed, 23 insertions(+), 84 deletions(-) diff --git a/coderd/activitybump.go b/coderd/activitybump.go index e376c5184ab5d..87e9ede552d2e 100644 --- a/coderd/activitybump.go +++ b/coderd/activitybump.go @@ -2,8 +2,6 @@ package coderd import ( "context" - "database/sql" - "errors" "time" "github.com/google/uuid" @@ -11,7 +9,6 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbtime" ) // activityBumpWorkspace automatically bumps the workspace's auto-off timer @@ -21,81 +18,7 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto // low priority operations fail first. ctx, cancel := context.WithTimeout(ctx, time.Second*15) defer cancel() - // if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil { - // if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) { - // // Bump will fail if the context is canceled, but this is ok. - // log.Error(ctx, "bump failed", slog.Error(err), - // slog.F("workspace_id", workspaceID), - // ) - // } - // return - // } - - err := db.InTx(func(s database.Store) error { - build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) - if errors.Is(err, sql.ErrNoRows) { - return nil - } else if err != nil { - return xerrors.Errorf("get latest workspace build: %w", err) - } - - job, err := s.GetProvisionerJobByID(ctx, build.JobID) - if err != nil { - return xerrors.Errorf("get provisioner job: %w", err) - } - - if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { - return nil - } - - if build.Deadline.IsZero() { - // Workspace shutdown is manual - return nil - } - - workspace, err := s.GetWorkspaceByID(ctx, workspaceID) - if err != nil { - return xerrors.Errorf("get workspace: %w", err) - } - - var ( - // We bump by the original TTL to prevent counter-intuitive behavior - // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off - // work at midnight, come back at 10am, I would want another full day - // of uptime. In the prior implementation, the workspace would enter - // a state of always expiring 1 hour in the future - bumpAmount = time.Duration(workspace.Ttl.Int64) - // DB writes are expensive so we only bump when 5% of the deadline - // has elapsed. - bumpEvery = bumpAmount / 20 - timeSinceLastBump = bumpAmount - time.Until(build.Deadline) - ) - - if timeSinceLastBump < bumpEvery { - return nil - } - - if bumpAmount == 0 { - return nil - } - - newDeadline := dbtime.Now().Add(bumpAmount) - if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) { - newDeadline = build.MaxDeadline - } - - if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ - ID: build.ID, - UpdatedAt: dbtime.Now(), - ProvisionerState: build.ProvisionerState, - Deadline: newDeadline, - MaxDeadline: build.MaxDeadline, - }); err != nil { - return xerrors.Errorf("update workspace build: %w", err) - } - return nil - }, nil) - if err != nil { + if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil { if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) { // Bump will fail if the context is canceled, but this is ok. log.Error(ctx, "bump failed", slog.Error(err), diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index 1e428e01f36aa..46336e0ae8d70 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -166,7 +166,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at") require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline") } else { - require.WithinDuration(t, dbtime.Now(), updatedBuild.UpdatedAt, 15*time.Second, "unexpected updated at time after bump") + require.NotEqual(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should have bumped updated_at") expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC() require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump") } diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 46c1cff5aacd6..768c1d4adbcca 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -93,7 +93,7 @@ func (m metricsStore) AcquireProvisionerJob(ctx context.Context, arg database.Ac return provisionerJob, err } -func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) (error, error) { +func (m metricsStore) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error { start := time.Now() r0 := m.s.ActivityBumpWorkspace(ctx, arg) m.queryLatencies.WithLabelValues("ActivityBumpWorkspace").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index b16ec2af53e89..07a042e874100 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -24,6 +24,8 @@ type sqlcQuerier interface { // multiple provisioners from acquiring the same jobs. See: // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) + // Workspace shutdown is manual. + // We only bump when 5% of the deadline has elapsed. ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error CleanTailnetCoordinators(ctx context.Context) error DeleteAPIKeyByID(ctx context.Context, id string) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index fb15c1bff334a..6887634db4eb9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -40,17 +40,24 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' + -- We bump by the original TTL to prevent counter-intuitive behavior + -- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off + -- work at midnight, come back at 10am, I would want another full day + -- of uptime. In the prior implementation, the workspace would enter + -- a state of always expiring 1 hour in the future. THEN l.now_utc + l.ttl_interval ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END FROM latest l WHERE wb.id = l.build_id +AND l.job_completed_at IS NOT NULL AND l.build_transition = 'start' AND l.build_deadline != '0001-01-01 00:00:00' -AND l.job_completed_at IS NOT NULL -AND l.build_deadline + (l.ttl_interval * 0.05) < NOW() +AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() ` +// Workspace shutdown is manual. +// We only bump when 5% of the deadline has elapsed. func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error { _, err := q.db.ExecContext(ctx, activityBumpWorkspace, dollar_1) return err diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index 655bb53550885..975cf80548c5d 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -23,13 +23,20 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' + -- We bump by the original TTL to prevent counter-intuitive behavior + -- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off + -- work at midnight, come back at 10am, I would want another full day + -- of uptime. In the prior implementation, the workspace would enter + -- a state of always expiring 1 hour in the future. THEN l.now_utc + l.ttl_interval ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END FROM latest l WHERE wb.id = l.build_id +AND l.job_completed_at IS NOT NULL AND l.build_transition = 'start' +-- Workspace shutdown is manual. AND l.build_deadline != '0001-01-01 00:00:00' -AND l.job_completed_at IS NOT NULL -AND l.build_deadline + (l.ttl_interval * 0.05) < NOW() +-- We only bump when 5% of the deadline has elapsed. +AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() ; From cf5e55b62d65a9a9c278b0c28e7fb3728125d601 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 12 Sep 2023 22:27:55 +0100 Subject: [PATCH 06/17] fixup! fix bugged query --- coderd/database/dbtestutil/db.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/database/dbtestutil/db.go b/coderd/database/dbtestutil/db.go index ee5a7658976b3..00eae9dd11218 100644 --- a/coderd/database/dbtestutil/db.go +++ b/coderd/database/dbtestutil/db.go @@ -33,7 +33,6 @@ func NewDB(t testing.TB) (database.Store, pubsub.Pubsub) { ) connectionURL, closePg, err = postgres.Open() require.NoError(t, err) - t.Logf("using postgres connection url: %s", connectionURL) t.Cleanup(closePg) } sqlDB, err := sql.Open("postgres", connectionURL) From 9ac9424a3d2d61847d53f8dfc6be1843c589e49d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 10:42:21 +0100 Subject: [PATCH 07/17] add MaxDeadline to FakeQuerier.InsertWorkspaceBuild --- coderd/database/dbfake/dbfake.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index f20be57089edf..df87ce124c9a2 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -4779,6 +4779,7 @@ func (q *FakeQuerier) InsertWorkspaceBuild(_ context.Context, arg database.Inser JobID: arg.JobID, ProvisionerState: arg.ProvisionerState, Deadline: arg.Deadline, + MaxDeadline: arg.MaxDeadline, Reason: arg.Reason, } q.workspaceBuilds = append(q.workspaceBuilds, workspaceBuild) From 0e8675c636692e09f83c2985ee3138047b0bd4e9 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 10:53:47 +0100 Subject: [PATCH 08/17] fix dbfake impl --- coderd/database/dbfake/dbfake.go | 36 +++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index df87ce124c9a2..ab7363b275a2f 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -769,13 +769,43 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui return err } + now := dbtime.Now() for i := range q.workspaceBuilds { if q.workspaceBuilds[i].BuildNumber != latestBuild.BuildNumber { continue } - newDeadline := q.workspaceBuilds[i].Deadline.Add(time.Duration(workspace.Ttl.Int64)) - q.workspaceBuilds[i].UpdatedAt = latestBuild.UpdatedAt - q.workspaceBuilds[i].Deadline = minTime(newDeadline, q.workspaceBuilds[i].MaxDeadline) + // If the build is not active, do not bump. + if q.workspaceBuilds[i].Transition != database.WorkspaceTransitionStart { + return nil + } + // If the provisioner job is not completed, do not bump. + pj, err := q.getProvisionerJobByIDNoLock(ctx, q.workspaceBuilds[i].JobID) + if err != nil { + return err + } + if !pj.CompletedAt.Valid { + return nil + } + // Do not bump if the deadline is not set. + if q.workspaceBuilds[i].Deadline.IsZero() { + return nil + } + // Only bump if 5% of the deadline has passed. + ttlDur := time.Duration(workspace.Ttl.Int64) + ttlDur95 := ttlDur - (ttlDur / 20) + minBumpDeadline := q.workspaceBuilds[i].Deadline.Add(-ttlDur95) + if now.Before(minBumpDeadline) { + return nil + } + + // Bump. + newDeadline := now.Add(ttlDur) + q.workspaceBuilds[i].UpdatedAt = now + if !q.workspaceBuilds[i].MaxDeadline.IsZero() { + q.workspaceBuilds[i].Deadline = minTime(newDeadline, q.workspaceBuilds[i].MaxDeadline) + } else { + q.workspaceBuilds[i].Deadline = newDeadline + } return nil } From a4a6355406cbc7038ff78ee3215cb91a41894e8b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 11:03:30 +0100 Subject: [PATCH 09/17] add some previous workspace builds --- coderd/activitybump_internal_test.go | 29 +++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index 46336e0ae8d70..406a5ba3da845 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -129,12 +129,22 @@ func Test_ActivityBumpWorkspace(t *testing.T) { }) buildID = uuid.New() ) + + var buildNumber int32 = 1 + // Insert a number of previous workspace builds. + for i := 0; i < 5; i++ { + insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber) + buildNumber++ + insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber) + buildNumber++ + } + // dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ ID: buildID, CreatedAt: dbtime.Now(), UpdatedAt: dbtime.Now(), - BuildNumber: 1, + BuildNumber: buildNumber, InitiatorID: user.ID, Reason: database.BuildReasonInitiator, WorkspaceID: ws.ID, @@ -173,3 +183,20 @@ func Test_ActivityBumpWorkspace(t *testing.T) { }) } } + +func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, workspaceID uuid.UUID, transition database.WorkspaceTransition, buildNumber int32) { + t.Helper() + + job := dbgen.ProvisionerJob(t, db, database.ProvisionerJob{ + OrganizationID: orgID, + }) + _ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{ + JobID: job.ID, + }) + _ = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + BuildNumber: buildNumber, + WorkspaceID: workspaceID, + JobID: job.ID, + TemplateVersionID: tvID, + }) +} From 0209449e161c87630201398b68335fcc99c29b01 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 11:11:47 +0100 Subject: [PATCH 10/17] fixup! add some previous workspace builds --- coderd/activitybump_internal_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index 406a5ba3da845..d0341b82828d3 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -198,5 +198,6 @@ func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, work WorkspaceID: workspaceID, JobID: job.ID, TemplateVersionID: tvID, + Transition: transition, }) } From d05d1f1b3a83d33943cc630b497f31e442031f1c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 13:41:54 +0100 Subject: [PATCH 11/17] hopefully reduce flake likelihood --- coderd/activitybump_internal_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index d0341b82828d3..e512323e68c35 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -165,9 +165,14 @@ func Test_ActivityBumpWorkspace(t *testing.T) { require.Equal(t, tt.maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump") - // new deadline is calculated from the time of the bump - approxBumpTime := dbtime.Now() + start := dbtime.Now() activityBumpWorkspace(ctx, log, db, bld.WorkspaceID) + elapsed := time.Since(start) + if elapsed > 15*time.Second { + t.Logf("warning: activityBumpWorkspace took longer than 15 seconds: %s", elapsed) + } + // Guessing at the approximate time of the bump here, if it happened. + approxBumpTime := start.Add(elapsed / 2) // Validate our state after bump updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID) @@ -178,6 +183,8 @@ func Test_ActivityBumpWorkspace(t *testing.T) { } else { require.NotEqual(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should have bumped updated_at") expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC() + // Note: if CI is especially slow, this test may fail. There is an internal 15-second + // deadline in activityBumpWorkspace, so we allow the same window here. require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump") } }) From b75ff569ce7b0baba4b6f8c944c3c2be6084f38c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 14:39:26 +0100 Subject: [PATCH 12/17] fix comment location --- coderd/database/querier.go | 9 +++++++-- coderd/database/queries.sql.go | 16 ++++++++-------- coderd/database/queries/activitybump.sql | 14 +++++++------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 07a042e874100..383b272180c83 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -24,9 +24,14 @@ type sqlcQuerier interface { // multiple provisioners from acquiring the same jobs. See: // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) - // Workspace shutdown is manual. + // We bump by the original TTL to prevent counter-intuitive behavior + // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off + // work at midnight, come back at 10am, I would want another full day + // of uptime. In the prior implementation, the workspace would enter + // a state of always expiring 1 hour in the future. + // We only bump if workspace shutdown is manual. // We only bump when 5% of the deadline has elapsed. - ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error + ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error CleanTailnetCoordinators(ctx context.Context) error DeleteAPIKeyByID(ctx context.Context, id string) error DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 6887634db4eb9..090dae21d8b9d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -40,11 +40,6 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' - -- We bump by the original TTL to prevent counter-intuitive behavior - -- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off - -- work at midnight, come back at 10am, I would want another full day - -- of uptime. In the prior implementation, the workspace would enter - -- a state of always expiring 1 hour in the future. THEN l.now_utc + l.ttl_interval ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END @@ -56,10 +51,15 @@ AND l.build_deadline != '0001-01-01 00:00:00' AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() ` -// Workspace shutdown is manual. +// We bump by the original TTL to prevent counter-intuitive behavior +// as the TTL wraps. For example, if I set the TTL to 12 hours, sign off +// work at midnight, come back at 10am, I would want another full day +// of uptime. In the prior implementation, the workspace would enter +// a state of always expiring 1 hour in the future. +// We only bump if workspace shutdown is manual. // We only bump when 5% of the deadline has elapsed. -func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, dollar_1 uuid.UUID) error { - _, err := q.db.ExecContext(ctx, activityBumpWorkspace, dollar_1) +func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error { + _, err := q.db.ExecContext(ctx, activityBumpWorkspace, workspaceID) return err } diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index 975cf80548c5d..96d0c46c2b7d4 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -1,3 +1,8 @@ +-- We bump by the original TTL to prevent counter-intuitive behavior +-- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off +-- work at midnight, come back at 10am, I would want another full day +-- of uptime. In the prior implementation, the workspace would enter +-- a state of always expiring 1 hour in the future. -- name: ActivityBumpWorkspace :exec WITH latest AS ( SELECT @@ -13,7 +18,7 @@ WITH latest AS ( ON provisioner_jobs.id = workspace_builds.job_id JOIN workspaces ON workspaces.id = workspace_builds.workspace_id - WHERE workspace_builds.workspace_id = $1::uuid + WHERE workspace_builds.workspace_id = @workspace_id::uuid ORDER BY workspace_builds.build_number DESC LIMIT 1 ) @@ -23,11 +28,6 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' - -- We bump by the original TTL to prevent counter-intuitive behavior - -- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off - -- work at midnight, come back at 10am, I would want another full day - -- of uptime. In the prior implementation, the workspace would enter - -- a state of always expiring 1 hour in the future. THEN l.now_utc + l.ttl_interval ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) END @@ -35,7 +35,7 @@ FROM latest l WHERE wb.id = l.build_id AND l.job_completed_at IS NOT NULL AND l.build_transition = 'start' --- Workspace shutdown is manual. +-- We only bump if workspace shutdown is manual. AND l.build_deadline != '0001-01-01 00:00:00' -- We only bump when 5% of the deadline has elapsed. AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() From 9272f4b24d4897bca933b11e41d432ccd4ae5d6d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 14:44:57 +0100 Subject: [PATCH 13/17] sleep to work around windows time resolution --- coderd/activitybump_internal_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index e512323e68c35..302fb1d3b5304 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -2,6 +2,7 @@ package coderd import ( "database/sql" + "runtime" "testing" "time" @@ -165,6 +166,8 @@ func Test_ActivityBumpWorkspace(t *testing.T) { require.Equal(t, tt.maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump") + workaroundWindowsTimeResolution(t) + start := dbtime.Now() activityBumpWorkspace(ctx, log, db, bld.WorkspaceID) elapsed := time.Since(start) @@ -208,3 +211,11 @@ func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, work Transition: transition, }) } + +func workaroundWindowsTimeResolution(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Logf("workaround: sleeping for a short time to avoid time resolution issues on Windows") + <-time.After(testutil.IntervalSlow) + } +} From 49dbc55cc6bbed60493f8600ffe4fa31783b9b5e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 15:02:39 +0100 Subject: [PATCH 14/17] modify deadline measurement --- coderd/activitybump_internal_test.go | 112 ++++++++++++++------------- 1 file changed, 60 insertions(+), 52 deletions(-) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index 302fb1d3b5304..5b629bc5014bb 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/testutil" "github.com/stretchr/testify/require" @@ -22,72 +23,72 @@ func Test_ActivityBumpWorkspace(t *testing.T) { t.Parallel() for _, tt := range []struct { - name string - transition database.WorkspaceTransition - jobCompletedAt sql.NullTime - buildDeadline time.Time - maxDeadline time.Time - workspaceTTL time.Duration - expectedBump time.Duration + name string + transition database.WorkspaceTransition + jobCompletedAt sql.NullTime + buildDeadlineOffset *time.Duration + maxDeadline time.Time + workspaceTTL time.Duration + expectedBump time.Duration }{ { - name: "NotFinishedYet", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{}, - buildDeadline: dbtime.Now().Add(8 * time.Hour), - workspaceTTL: 8 * time.Hour, - expectedBump: 0, + name: "NotFinishedYet", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{}, + buildDeadlineOffset: ptr.Ref(8 * time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, }, { - name: "ManualShutdown", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, - buildDeadline: time.Time{}, - expectedBump: 0, + name: "ManualShutdown", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, + buildDeadlineOffset: nil, + expectedBump: 0, }, { - name: "NotTimeToBumpYet", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, - buildDeadline: dbtime.Now().Add(8 * time.Hour), - workspaceTTL: 8 * time.Hour, - expectedBump: 0, + name: "NotTimeToBumpYet", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, + buildDeadlineOffset: ptr.Ref(8 * time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, }, { - name: "TimeToBump", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, - buildDeadline: dbtime.Now().Add(8*time.Hour - 24*time.Minute), - workspaceTTL: 8 * time.Hour, - expectedBump: 8 * time.Hour, + name: "TimeToBump", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadlineOffset: ptr.Ref(8*time.Hour - 24*time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 8 * time.Hour, }, { - name: "MaxDeadline", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, - buildDeadline: dbtime.Now().Add(time.Minute), // last chance to bump! - maxDeadline: dbtime.Now().Add(time.Hour), - workspaceTTL: 8 * time.Hour, - expectedBump: 1 * time.Hour, + name: "MaxDeadline", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadlineOffset: ptr.Ref(time.Minute), // last chance to bump! + maxDeadline: dbtime.Now().Add(time.Hour), + workspaceTTL: 8 * time.Hour, + expectedBump: 1 * time.Hour, }, { // A workspace that is still running, has passed its deadline, but has not // yet been auto-stopped should still bump the deadline. - name: "PastDeadlineStillBumps", - transition: database.WorkspaceTransitionStart, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, - buildDeadline: dbtime.Now().Add(-time.Minute), - workspaceTTL: 8 * time.Hour, - expectedBump: 8 * time.Hour, + name: "PastDeadlineStillBumps", + transition: database.WorkspaceTransitionStart, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, + buildDeadlineOffset: ptr.Ref(-time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 8 * time.Hour, }, { // A stopped workspace should never bump. - name: "StoppedWorkspace", - transition: database.WorkspaceTransitionStop, - jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-time.Minute)}, - buildDeadline: dbtime.Now().Add(-time.Minute), - workspaceTTL: 8 * time.Hour, - expectedBump: 0, + name: "StoppedWorkspace", + transition: database.WorkspaceTransitionStop, + jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-time.Minute)}, + buildDeadlineOffset: ptr.Ref(-time.Minute), + workspaceTTL: 8 * time.Hour, + expectedBump: 0, }, } { tt := tt @@ -95,6 +96,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { t.Parallel() var ( + now = dbtime.Now() ctx = testutil.Context(t, testutil.WaitShort) log = slogtest.Make(t, nil) db, _ = dbtestutil.NewDB(t) @@ -141,6 +143,10 @@ func Test_ActivityBumpWorkspace(t *testing.T) { } // dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set + var buildDeadline time.Time + if tt.buildDeadlineOffset != nil { + buildDeadline = now.Add(*tt.buildDeadlineOffset) + } err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ ID: buildID, CreatedAt: dbtime.Now(), @@ -152,7 +158,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { JobID: job.ID, TemplateVersionID: templateVersion.ID, Transition: tt.transition, - Deadline: tt.buildDeadline, + Deadline: buildDeadline, MaxDeadline: tt.maxDeadline, }) require.NoError(t, err, "unexpected error inserting workspace build") @@ -162,19 +168,21 @@ func Test_ActivityBumpWorkspace(t *testing.T) { // Validate our initial state before bump require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump") require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump") - require.Equal(t, tt.buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump") + require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump") require.Equal(t, tt.maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump") workaroundWindowsTimeResolution(t) + // Bump duration is measured from the time of the bump, so we measure from here. start := dbtime.Now() activityBumpWorkspace(ctx, log, db, bld.WorkspaceID) elapsed := time.Since(start) if elapsed > 15*time.Second { t.Logf("warning: activityBumpWorkspace took longer than 15 seconds: %s", elapsed) } - // Guessing at the approximate time of the bump here, if it happened. + // The actual bump could have happened anywhere in the elapsed time, so we + // guess at the approximate time of the bump. approxBumpTime := start.Add(elapsed / 2) // Validate our state after bump From 50f7578bea0f303664237f2ca711fccecf1710bf Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 15:23:02 +0100 Subject: [PATCH 15/17] fix max deadline --- coderd/activitybump_internal_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go index 5b629bc5014bb..81fb8aaf36c34 100644 --- a/coderd/activitybump_internal_test.go +++ b/coderd/activitybump_internal_test.go @@ -27,7 +27,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { transition database.WorkspaceTransition jobCompletedAt sql.NullTime buildDeadlineOffset *time.Duration - maxDeadline time.Time + maxDeadlineOffset *time.Duration workspaceTTL time.Duration expectedBump time.Duration }{ @@ -67,7 +67,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { transition: database.WorkspaceTransitionStart, jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)}, buildDeadlineOffset: ptr.Ref(time.Minute), // last chance to bump! - maxDeadline: dbtime.Now().Add(time.Hour), + maxDeadlineOffset: ptr.Ref(time.Hour), workspaceTTL: 8 * time.Hour, expectedBump: 1 * time.Hour, }, @@ -147,6 +147,10 @@ func Test_ActivityBumpWorkspace(t *testing.T) { if tt.buildDeadlineOffset != nil { buildDeadline = now.Add(*tt.buildDeadlineOffset) } + var maxDeadline time.Time + if tt.maxDeadlineOffset != nil { + maxDeadline = now.Add(*tt.maxDeadlineOffset) + } err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ ID: buildID, CreatedAt: dbtime.Now(), @@ -159,7 +163,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { TemplateVersionID: templateVersion.ID, Transition: tt.transition, Deadline: buildDeadline, - MaxDeadline: tt.maxDeadline, + MaxDeadline: maxDeadline, }) require.NoError(t, err, "unexpected error inserting workspace build") bld, err := db.GetWorkspaceBuildByID(ctx, buildID) @@ -169,7 +173,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump") require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump") require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump") - require.Equal(t, tt.maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") + require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump") require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump") workaroundWindowsTimeResolution(t) From dc37bf97860f08b954ac133065acdd8d524e975d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 16:55:50 +0100 Subject: [PATCH 16/17] address PR comments --- coderd/database/querier.go | 3 +-- coderd/database/queries.sql.go | 10 ++++------ coderd/database/queries/activitybump.sql | 10 ++++------ 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 383b272180c83..63c1f7321dc15 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -27,8 +27,7 @@ type sqlcQuerier interface { // We bump by the original TTL to prevent counter-intuitive behavior // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off // work at midnight, come back at 10am, I would want another full day - // of uptime. In the prior implementation, the workspace would enter - // a state of always expiring 1 hour in the future. + // of uptime. // We only bump if workspace shutdown is manual. // We only bump when 5% of the deadline has elapsed. ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 090dae21d8b9d..dfddf452d4edf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -23,8 +23,7 @@ WITH latest AS ( workspace_builds.max_deadline::timestamp AS build_max_deadline, workspace_builds.transition AS build_transition, provisioner_jobs.completed_at::timestamp AS job_completed_at, - (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval, - (NOW() AT TIME ZONE 'UTC')::timestamp as now_utc + (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval FROM workspace_builds JOIN provisioner_jobs ON provisioner_jobs.id = workspace_builds.job_id @@ -40,8 +39,8 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' - THEN l.now_utc + l.ttl_interval - ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) + THEN NOW() + l.ttl_interval + ELSE LEAST(NOW() + l.ttl_interval, l.build_max_deadline) END FROM latest l WHERE wb.id = l.build_id @@ -54,8 +53,7 @@ AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() // We bump by the original TTL to prevent counter-intuitive behavior // as the TTL wraps. For example, if I set the TTL to 12 hours, sign off // work at midnight, come back at 10am, I would want another full day -// of uptime. In the prior implementation, the workspace would enter -// a state of always expiring 1 hour in the future. +// of uptime. // We only bump if workspace shutdown is manual. // We only bump when 5% of the deadline has elapsed. func (q *sqlQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error { diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index 96d0c46c2b7d4..cc3c78ba5edd7 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -1,8 +1,7 @@ -- We bump by the original TTL to prevent counter-intuitive behavior -- as the TTL wraps. For example, if I set the TTL to 12 hours, sign off -- work at midnight, come back at 10am, I would want another full day --- of uptime. In the prior implementation, the workspace would enter --- a state of always expiring 1 hour in the future. +-- of uptime. -- name: ActivityBumpWorkspace :exec WITH latest AS ( SELECT @@ -11,8 +10,7 @@ WITH latest AS ( workspace_builds.max_deadline::timestamp AS build_max_deadline, workspace_builds.transition AS build_transition, provisioner_jobs.completed_at::timestamp AS job_completed_at, - (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval, - (NOW() AT TIME ZONE 'UTC')::timestamp as now_utc + (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval FROM workspace_builds JOIN provisioner_jobs ON provisioner_jobs.id = workspace_builds.job_id @@ -28,8 +26,8 @@ SET updated_at = NOW(), deadline = CASE WHEN l.build_max_deadline = '0001-01-01 00:00:00' - THEN l.now_utc + l.ttl_interval - ELSE LEAST(l.now_utc + l.ttl_interval, l.build_max_deadline) + THEN NOW() + l.ttl_interval + ELSE LEAST(NOW() + l.ttl_interval, l.build_max_deadline) END FROM latest l WHERE wb.id = l.build_id From 5dcb743d2c5281194aa77f9fdba361d75a340856 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 13 Sep 2023 17:56:30 +0100 Subject: [PATCH 17/17] set +00 explicitly in query --- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/activitybump.sql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index dfddf452d4edf..eac279a1228d7 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -38,7 +38,7 @@ UPDATE SET updated_at = NOW(), deadline = CASE - WHEN l.build_max_deadline = '0001-01-01 00:00:00' + WHEN l.build_max_deadline = '0001-01-01 00:00:00+00' THEN NOW() + l.ttl_interval ELSE LEAST(NOW() + l.ttl_interval, l.build_max_deadline) END @@ -46,7 +46,7 @@ FROM latest l WHERE wb.id = l.build_id AND l.job_completed_at IS NOT NULL AND l.build_transition = 'start' -AND l.build_deadline != '0001-01-01 00:00:00' +AND l.build_deadline != '0001-01-01 00:00:00+00' AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() ` diff --git a/coderd/database/queries/activitybump.sql b/coderd/database/queries/activitybump.sql index cc3c78ba5edd7..5b6dab41dabe2 100644 --- a/coderd/database/queries/activitybump.sql +++ b/coderd/database/queries/activitybump.sql @@ -25,7 +25,7 @@ UPDATE SET updated_at = NOW(), deadline = CASE - WHEN l.build_max_deadline = '0001-01-01 00:00:00' + WHEN l.build_max_deadline = '0001-01-01 00:00:00+00' THEN NOW() + l.ttl_interval ELSE LEAST(NOW() + l.ttl_interval, l.build_max_deadline) END @@ -34,7 +34,7 @@ WHERE wb.id = l.build_id AND l.job_completed_at IS NOT NULL AND l.build_transition = 'start' -- We only bump if workspace shutdown is manual. -AND l.build_deadline != '0001-01-01 00:00:00' +AND l.build_deadline != '0001-01-01 00:00:00+00' -- We only bump when 5% of the deadline has elapsed. AND l.build_deadline - (l.ttl_interval * 0.95) < NOW() ;