Skip to content

refactor(coderd): collapse activityBumpWorkspace into a single query #9652

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Sep 14, 2023
Merged

refactor(coderd): collapse activityBumpWorkspace into a single query #9652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e25d8ab
first stab at query
johnstcn Sep 7, 2023
1ee5403
update query
johnstcn Sep 8, 2023
a71a809
add unit-style tests for activityBumpWorkspace
johnstcn Sep 12, 2023
3e583d9
improve query
johnstcn Sep 12, 2023
1829a25
fix bugged query
johnstcn Sep 12, 2023
cf5e55b
fixup! fix bugged query
johnstcn Sep 12, 2023
5bf3f51
Merge remote-tracking branch 'origin/main' into cj/activitybump_query
johnstcn Sep 13, 2023
9ac9424
add MaxDeadline to FakeQuerier.InsertWorkspaceBuild
johnstcn Sep 13, 2023
0e8675c
fix dbfake impl
johnstcn Sep 13, 2023
a4a6355
add some previous workspace builds
johnstcn Sep 13, 2023
0209449
fixup! add some previous workspace builds
johnstcn Sep 13, 2023
d05d1f1
hopefully reduce flake likelihood
johnstcn Sep 13, 2023
b75ff56
fix comment location
johnstcn Sep 13, 2023
9272f4b
sleep to work around windows time resolution
johnstcn Sep 13, 2023
49dbc55
modify deadline measurement
johnstcn Sep 13, 2023
50f7578
fix max deadline
johnstcn Sep 13, 2023
dc37bf9
address PR comments
johnstcn Sep 13, 2023
5dcb743
set +00 explicitly in query
johnstcn Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improve query
  • Loading branch information
@johnstcn
johnstcn committed Sep 12, 2023
commit 3e583d975ed959a6d6f38e95d5ed66305762eeba
123 changes: 66 additions & 57 deletions coderd/activitybump.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ package coderd

import (
"context"
"database/sql"
"errors"
"time"

"github.com/google/uuid"
"golang.org/x/xerrors"

"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
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion coderd/database/dbmetrics/dbmetrics.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions coderd/database/dbtestutil/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
30 changes: 16 additions & 14 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 16 additions & 14 deletions coderd/database/queries/activitybump.sql
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

self-review: I can inline this CTE if required, but I think the readability is important here. I'm preserving the comments from the original pure-Go implementation for reference.

Original file line number Diff line number Diff line change
@@ -1,33 +1,35 @@
-- 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,
Copy link
Member Author

Choose a reason for hiding this comment

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

self-review: it probably makes sense to migrate this column to e.g. ttl_mins, but that's out of scope of this PR.

(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
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)
WHEN l.build_max_deadline = '0001-01-01 00:00:00'
Copy link
Member

Choose a reason for hiding this comment

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

I think, maybe, this actually should be in +00 format (UTC), but it depends on what our default value is and DB being in non-UTC config. I imagine this is something we may be doing (potentially) incorrectly elsewhere too so maybe doesn't need fixing in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, max deadline is +00. I'll update that to fix, but good catch.

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()
;