-
Notifications
You must be signed in to change notification settings - Fork 937
chore: optimize GetPrebuiltWorkspaces query #18717
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.
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
Changes from all commits
2c48c95
68a4304
c95b6d0
6b4d6dc
53c3ba5
b237251
5dc69d9
5158046
3e69a1b
2e760c7
a0f4f36
30c86cc
1d6b4c0
6a1c635
e43b5ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,64 @@ WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a pre | |
-- AND NOT t.deleted -- We don't exclude deleted templates because there's no constraint in the DB preventing a soft deletion on a template while workspaces are running. | ||
AND (t.id = sqlc.narg('template_id')::uuid OR sqlc.narg('template_id') IS NULL); | ||
|
||
-- name: GetRunningPrebuiltWorkspacesOptimized :many | ||
WITH latest_prebuilds AS ( | ||
johnstcn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
-- All workspaces that match the following criteria: | ||
-- 1. Owned by prebuilds user | ||
-- 2. Not deleted | ||
-- 3. Latest build is a 'start' transition | ||
-- 4. Latest build was successful | ||
SELECT | ||
workspaces.id, | ||
workspaces.name, | ||
workspaces.template_id, | ||
workspace_latest_builds.template_version_id, | ||
workspace_latest_builds.job_id, | ||
workspaces.created_at | ||
FROM workspace_latest_builds | ||
JOIN workspaces ON workspaces.id = workspace_latest_builds.workspace_id | ||
WHERE workspace_latest_builds.transition = 'start'::workspace_transition | ||
AND workspace_latest_builds.job_status = 'succeeded'::provisioner_job_status | ||
AND workspaces.owner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::UUID | ||
AND NOT workspaces.deleted | ||
), | ||
workspace_latest_presets AS ( | ||
-- For each of the above workspaces, the preset_id of the most recent | ||
-- successful start transition. | ||
SELECT DISTINCT ON (latest_prebuilds.id) | ||
latest_prebuilds.id AS workspace_id, | ||
workspace_builds.template_version_preset_id AS current_preset_id | ||
FROM latest_prebuilds | ||
JOIN workspace_builds ON workspace_builds.workspace_id = latest_prebuilds.id | ||
WHERE workspace_builds.transition = 'start'::workspace_transition | ||
AND workspace_builds.template_version_preset_id IS NOT NULL | ||
ORDER BY latest_prebuilds.id, workspace_builds.build_number DESC | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't quite grasp the purpose of this CTE, it seems to pretty much do the same as Might make more sense to just join There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I omitted this CTE it caused different results for one specific prebuild instance, but I'm now not seeing the same discrepancy when I run the queries side-by-side. I'll try the modifications you suggest and see if it still yields the same results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more I look at that CTE, the more I question it too. On the dogfood database, all of the workspace builds owned by the prebuild user have a non-null template_version_preset_id, whereas almost all subsequent ones have a null preset ID. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIU, whenever a template version is added, new presets are inserted into the database. I don't think we ever delete presets from the database. Except maybe when a template is deleted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After spending some more time looking at this, it appears that the current correct approach is to query the most recent non-null preset ID for a successful start transition. Simply selecting from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prebuilt workspaces should ideally never change their preset. This means that the first build should contain a preset id and all subsequent builds should have a null preset ID. We could optimise this by dropping the This would probably be much more performant. The issue with this optimisation is the potentially negligent edge case where a human being restarts a prebuilt workspace with a new preset. In the optimized case such a restarted prebuilt workspace would be claimed with the wrong preset. If we're willing to tolerate this edge case, we can optimize by dropping the CTE. Otherwise, the CTE remains necessary in order to find the last set preset id for a prebuilt workspace. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll leave the CTE for now. There is an argument to be made that once a human touches a prebuild that it's no longer valid to be claimed and as such only prebuilds with But what do we say to the God of Arguments? Not today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why do we nullify the
How would this actually happen? Is it really possible for a prebuilt workspace to be restarted with a different preset? If we consider the association to the preset immutable once the prebuilt workspace is created, this edge case shouldn’t occur, right? Or is there a specific workflow that allows this to happen? Either way, this is not a blocker for the PR, we can definitely move forward and revisit this discussion later if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We don't explicitly nullify them. The fact that they are null on subsequent build is rather because there's nothing to tell a subsequent build what the
Any workspace, prebuilt or not can be restarted with new parameters as long as those parameters are mutable. The same goes for presets. It is only possible via API right now. Neither the UI nor the CLI support changing a preset. This means it is theoretically possible for a user with sufficient privileges to send an API call to restart with a new preset, but it is definitely not supported. |
||
ready_agents AS ( | ||
-- For each of the above workspaces, check if all agents are ready. | ||
SELECT | ||
latest_prebuilds.job_id, | ||
BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready | ||
FROM latest_prebuilds | ||
JOIN workspace_resources ON workspace_resources.job_id = latest_prebuilds.job_id | ||
JOIN workspace_agents ON workspace_agents.resource_id = workspace_resources.id | ||
WHERE workspace_agents.deleted = false | ||
AND workspace_agents.parent_id IS NULL | ||
GROUP BY latest_prebuilds.job_id | ||
) | ||
SELECT | ||
latest_prebuilds.id, | ||
latest_prebuilds.name, | ||
latest_prebuilds.template_id, | ||
latest_prebuilds.template_version_id, | ||
workspace_latest_presets.current_preset_id, | ||
COALESCE(ready_agents.ready, false)::boolean AS ready, | ||
latest_prebuilds.created_at | ||
FROM latest_prebuilds | ||
LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id | ||
LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id | ||
ORDER BY latest_prebuilds.id; | ||
|
||
-- name: GetRunningPrebuiltWorkspaces :many | ||
SELECT | ||
p.id, | ||
|
@@ -60,7 +118,8 @@ SELECT | |
FROM workspace_prebuilds p | ||
INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id | ||
WHERE (b.transition = 'start'::workspace_transition | ||
AND b.job_status = 'succeeded'::provisioner_job_status); | ||
AND b.job_status = 'succeeded'::provisioner_job_status) | ||
ORDER BY p.id; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. review: adding stable ordering for diffing |
||
|
||
-- name: CountInProgressPrebuilds :many | ||
-- CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🌟