Skip to content

Commit 198d50d

Browse files
johnstcnjohnstcn
authored
chore: replace original GetPrebuiltWorkspaces with optimized version (#18832)
Fixes coder/internal#715 Follow-up from #18717 Now that we've determined the updated query is safe, remove the duplication.
1 parent af01562 commit 198d50d

File tree

10 files changed

+7
-313
lines changed

10 files changed

+7
-313
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2654,14 +2654,6 @@ func (q *querier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]database.
26542654
return q.db.GetRunningPrebuiltWorkspaces(ctx)
26552655
}
26562656

2657-
func (q *querier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) {
2658-
// This query returns only prebuilt workspaces, but we decided to require permissions for all workspaces.
2659-
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.All()); err != nil {
2660-
return nil, err
2661-
}
2662-
return q.db.GetRunningPrebuiltWorkspacesOptimized(ctx)
2663-
}
2664-
26652657
func (q *querier) GetRuntimeConfig(ctx context.Context, key string) (string, error) {
26662658
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
26672659
return "", err

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,7 @@ func TestDBAuthzRecursive(t *testing.T) {
178178
if method.Name == "InTx" ||
179179
method.Name == "Ping" ||
180180
method.Name == "Wrappers" ||
181-
method.Name == "PGLocks" ||
182-
method.Name == "GetRunningPrebuiltWorkspacesOptimized" {
181+
method.Name == "PGLocks" {
183182
continue
184183
}
185184
// easy to know which method failed.

‎coderd/database/dbauthz/setup_test.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,6 @@ var skipMethods = map[string]string{
4141
"Wrappers": "Not relevant",
4242
"AcquireLock": "Not relevant",
4343
"TryAcquireLock": "Not relevant",
44-
// This method will be removed once we know this works correctly.
45-
"GetRunningPrebuiltWorkspacesOptimized": "Not relevant",
4644
}
4745

4846
// TestMethodTestSuite runs MethodTestSuite.

‎coderd/database/dbmetrics/querymetrics.go

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

‎coderd/database/dbmock/dbmock.go

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

‎coderd/database/querier.go

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

‎coderd/database/queries.sql.go

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

‎coderd/database/queries/prebuilds.sql

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a pre
4848
-- 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.
4949
AND (t.id = sqlc.narg('template_id')::uuid OR sqlc.narg('template_id') IS NULL);
5050

51-
-- name: GetRunningPrebuiltWorkspacesOptimized :many
51+
-- name: GetRunningPrebuiltWorkspaces :many
5252
WITH latest_prebuilds AS (
5353
-- All workspaces that match the following criteria:
5454
-- 1. Owned by prebuilds user
@@ -106,21 +106,6 @@ LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id
106106
LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id
107107
ORDER BY latest_prebuilds.id;
108108

109-
-- name: GetRunningPrebuiltWorkspaces :many
110-
SELECT
111-
p.id,
112-
p.name,
113-
p.template_id,
114-
b.template_version_id,
115-
p.current_preset_id AS current_preset_id,
116-
p.ready,
117-
p.created_at
118-
FROM workspace_prebuilds p
119-
INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id
120-
WHERE (b.transition = 'start'::workspace_transition
121-
AND b.job_status = 'succeeded'::provisioner_job_status)
122-
ORDER BY p.id;
123-
124109
-- name: CountInProgressPrebuilds :many
125110
-- CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition.
126111
-- Prebuild considered in-progress if it's in the "starting", "stopping", or "deleting" state.

‎enterprise/coderd/prebuilds/reconcile.go

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"sync/atomic"
1313
"time"
1414

15-
"github.com/google/go-cmp/cmp"
1615
"github.com/hashicorp/go-multierror"
1716
"github.com/prometheus/client_golang/prometheus"
1817

@@ -405,15 +404,6 @@ func (c *StoreReconciler) SnapshotState(ctx context.Context, store database.Stor
405404
return xerrors.Errorf("failed to get running prebuilds: %w", err)
406405
}
407406

408-
// Compare with optimized query to ensure behavioral correctness
409-
optimized, err := db.GetRunningPrebuiltWorkspacesOptimized(ctx)
410-
if err != nil {
411-
// Log the error but continue with original results
412-
c.logger.Error(ctx, "optimized GetRunningPrebuiltWorkspacesOptimized failed", slog.Error(err))
413-
} else {
414-
CompareGetRunningPrebuiltWorkspacesResults(ctx, c.logger, allRunningPrebuilds, optimized)
415-
}
416-
417407
allPrebuildsInProgress, err := db.CountInProgressPrebuilds(ctx)
418408
if err != nil {
419409
return xerrors.Errorf("failed to get prebuilds in progress: %w", err)
@@ -933,30 +923,3 @@ func SetPrebuildsReconciliationPaused(ctx context.Context, db database.Store, pa
933923
}
934924
return db.UpsertPrebuildsSettings(ctx, string(settingsJSON))
935925
}
936-
937-
// CompareGetRunningPrebuiltWorkspacesResults compares the original and optimized
938-
// query results and logs any differences found. This function can be easily
939-
// removed once we're confident the optimized query works correctly.
940-
// TODO(Cian): Remove this function once the optimized query is stable and correct.
941-
func CompareGetRunningPrebuiltWorkspacesResults(
942-
ctx context.Context,
943-
logger slog.Logger,
944-
original []database.GetRunningPrebuiltWorkspacesRow,
945-
optimized []database.GetRunningPrebuiltWorkspacesOptimizedRow,
946-
) {
947-
if len(original) == 0 && len(optimized) == 0 {
948-
return
949-
}
950-
// Convert optimized results to the same type as original for comparison
951-
optimizedConverted := make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized))
952-
for i, row := range optimized {
953-
optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row)
954-
}
955-
956-
// Compare the results and log an error if they differ.
957-
// NOTE: explicitly not sorting here as both query results are ordered by ID.
958-
if diff := cmp.Diff(original, optimizedConverted); diff != "" {
959-
logger.Error(ctx, "results differ for GetRunningPrebuiltWorkspacesOptimized",
960-
slog.F("diff", diff))
961-
}
962-
}

0 commit comments

Comments
 (0)