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
fix comment location
  • Loading branch information
@johnstcn
johnstcn committed Sep 13, 2023
commit b75ff569ce7b0baba4b6f8c944c3c2be6084f38c
9 changes: 7 additions & 2 deletions coderd/database/querier.go
View file
Open in desktop

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

16 changes: 8 additions & 8 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.

14 changes: 7 additions & 7 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,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
Expand All @@ -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
)
Expand All @@ -23,19 +28,14 @@ SET
updated_at = NOW(),
deadline = CASE
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.

-- 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.
-- 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()
Expand Down