diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9af6e50764dfd..a12db9aa6919f 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2654,14 +2654,6 @@ func (q *querier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]database. return q.db.GetRunningPrebuiltWorkspaces(ctx) } -func (q *querier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { - // This query returns only prebuilt workspaces, but we decided to require permissions for all workspaces. - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.All()); err != nil { - return nil, err - } - return q.db.GetRunningPrebuiltWorkspacesOptimized(ctx) -} - func (q *querier) GetRuntimeConfig(ctx context.Context, key string) (string, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return "", err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index c153974394650..2b0801024eb8d 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -178,8 +178,7 @@ func TestDBAuthzRecursive(t *testing.T) { if method.Name == "InTx" || method.Name == "Ping" || method.Name == "Wrappers" || - method.Name == "PGLocks" || - method.Name == "GetRunningPrebuiltWorkspacesOptimized" { + method.Name == "PGLocks" { continue } // easy to know which method failed. diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index d4dacb78a4d50..3fc4b06b7f69d 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -41,8 +41,6 @@ var skipMethods = map[string]string{ "Wrappers": "Not relevant", "AcquireLock": "Not relevant", "TryAcquireLock": "Not relevant", - // This method will be removed once we know this works correctly. - "GetRunningPrebuiltWorkspacesOptimized": "Not relevant", } // TestMethodTestSuite runs MethodTestSuite. diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 7a7c3cb2d41c6..d4e1db1612790 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1356,13 +1356,6 @@ func (m queryMetricsStore) GetRunningPrebuiltWorkspaces(ctx context.Context) ([] return r0, r1 } -func (m queryMetricsStore) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { - start := time.Now() - r0, r1 := m.s.GetRunningPrebuiltWorkspacesOptimized(ctx) - m.queryLatencies.WithLabelValues("GetRunningPrebuiltWorkspacesOptimized").Observe(time.Since(start).Seconds()) - return r0, r1 -} - func (m queryMetricsStore) GetRuntimeConfig(ctx context.Context, key string) (string, error) { start := time.Now() r0, r1 := m.s.GetRuntimeConfig(ctx, key) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index fba3deb45e4be..f3ed6c2bc78ca 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2852,21 +2852,6 @@ func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspaces(ctx any) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspaces", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspaces), ctx) } -// GetRunningPrebuiltWorkspacesOptimized mocks base method. -func (m *MockStore) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRunningPrebuiltWorkspacesOptimized", ctx) - ret0, _ := ret[0].([]database.GetRunningPrebuiltWorkspacesOptimizedRow) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetRunningPrebuiltWorkspacesOptimized indicates an expected call of GetRunningPrebuiltWorkspacesOptimized. -func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspacesOptimized(ctx any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspacesOptimized", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspacesOptimized), ctx) -} - // GetRuntimeConfig mocks base method. func (m *MockStore) GetRuntimeConfig(ctx context.Context, key string) (string, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 24893a9197815..6471d79defa6c 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -301,7 +301,6 @@ type sqlcQuerier interface { GetReplicaByID(ctx context.Context, id uuid.UUID) (Replica, error) GetReplicasUpdatedAfter(ctx context.Context, updatedAt time.Time) ([]Replica, error) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) - GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]GetRunningPrebuiltWorkspacesOptimizedRow, error) GetRuntimeConfig(ctx context.Context, key string) (string, error) GetTailnetAgents(ctx context.Context, id uuid.UUID) ([]TailnetAgent, error) GetTailnetClientsForAgent(ctx context.Context, agentID uuid.UUID) ([]TailnetClient, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0ef4553149465..47d46a4e74a8b 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7361,63 +7361,6 @@ func (q *sqlQuerier) GetPresetsBackoff(ctx context.Context, lookback time.Time) } const getRunningPrebuiltWorkspaces = `-- name: GetRunningPrebuiltWorkspaces :many -SELECT - p.id, - p.name, - p.template_id, - b.template_version_id, - p.current_preset_id AS current_preset_id, - p.ready, - p.created_at -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) -ORDER BY p.id -` - -type GetRunningPrebuiltWorkspacesRow struct { - ID uuid.UUID `db:"id" json:"id"` - Name string `db:"name" json:"name"` - TemplateID uuid.UUID `db:"template_id" json:"template_id"` - TemplateVersionID uuid.UUID `db:"template_version_id" json:"template_version_id"` - CurrentPresetID uuid.NullUUID `db:"current_preset_id" json:"current_preset_id"` - Ready bool `db:"ready" json:"ready"` - CreatedAt time.Time `db:"created_at" json:"created_at"` -} - -func (q *sqlQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) { - rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspaces) - if err != nil { - return nil, err - } - defer rows.Close() - var items []GetRunningPrebuiltWorkspacesRow - for rows.Next() { - var i GetRunningPrebuiltWorkspacesRow - if err := rows.Scan( - &i.ID, - &i.Name, - &i.TemplateID, - &i.TemplateVersionID, - &i.CurrentPresetID, - &i.Ready, - &i.CreatedAt, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - -const getRunningPrebuiltWorkspacesOptimized = `-- name: GetRunningPrebuiltWorkspacesOptimized :many WITH latest_prebuilds AS ( -- All workspaces that match the following criteria: -- 1. Owned by prebuilds user @@ -7476,7 +7419,7 @@ LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = la ORDER BY latest_prebuilds.id ` -type GetRunningPrebuiltWorkspacesOptimizedRow struct { +type GetRunningPrebuiltWorkspacesRow struct { ID uuid.UUID `db:"id" json:"id"` Name string `db:"name" json:"name"` TemplateID uuid.UUID `db:"template_id" json:"template_id"` @@ -7486,15 +7429,15 @@ type GetRunningPrebuiltWorkspacesOptimizedRow struct { CreatedAt time.Time `db:"created_at" json:"created_at"` } -func (q *sqlQuerier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]GetRunningPrebuiltWorkspacesOptimizedRow, error) { - rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspacesOptimized) +func (q *sqlQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) { + rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspaces) if err != nil { return nil, err } defer rows.Close() - var items []GetRunningPrebuiltWorkspacesOptimizedRow + var items []GetRunningPrebuiltWorkspacesRow for rows.Next() { - var i GetRunningPrebuiltWorkspacesOptimizedRow + var i GetRunningPrebuiltWorkspacesRow if err := rows.Scan( &i.ID, &i.Name, diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 7e1dbc71f4a26..37bff9487928e 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -48,7 +48,7 @@ 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 +-- name: GetRunningPrebuiltWorkspaces :many WITH latest_prebuilds AS ( -- All workspaces that match the following criteria: -- 1. Owned by prebuilds user @@ -106,21 +106,6 @@ 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, - p.name, - p.template_id, - b.template_version_id, - p.current_preset_id AS current_preset_id, - p.ready, - p.created_at -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) -ORDER BY p.id; - -- name: CountInProgressPrebuilds :many -- CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition. -- Prebuild considered in-progress if it's in the "starting", "stopping", or "deleting" state. diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index cce39ea251323..049568c7e7f0c 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -12,7 +12,6 @@ import ( "sync/atomic" "time" - "github.com/google/go-cmp/cmp" "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" @@ -405,15 +404,6 @@ func (c *StoreReconciler) SnapshotState(ctx context.Context, store database.Stor return xerrors.Errorf("failed to get running prebuilds: %w", err) } - // Compare with optimized query to ensure behavioral correctness - optimized, err := db.GetRunningPrebuiltWorkspacesOptimized(ctx) - if err != nil { - // Log the error but continue with original results - c.logger.Error(ctx, "optimized GetRunningPrebuiltWorkspacesOptimized failed", slog.Error(err)) - } else { - CompareGetRunningPrebuiltWorkspacesResults(ctx, c.logger, allRunningPrebuilds, optimized) - } - allPrebuildsInProgress, err := db.CountInProgressPrebuilds(ctx) if err != nil { return xerrors.Errorf("failed to get prebuilds in progress: %w", err) @@ -933,30 +923,3 @@ func SetPrebuildsReconciliationPaused(ctx context.Context, db database.Store, pa } return db.UpsertPrebuildsSettings(ctx, string(settingsJSON)) } - -// CompareGetRunningPrebuiltWorkspacesResults compares the original and optimized -// query results and logs any differences found. This function can be easily -// removed once we're confident the optimized query works correctly. -// TODO(Cian): Remove this function once the optimized query is stable and correct. -func CompareGetRunningPrebuiltWorkspacesResults( - ctx context.Context, - logger slog.Logger, - original []database.GetRunningPrebuiltWorkspacesRow, - optimized []database.GetRunningPrebuiltWorkspacesOptimizedRow, -) { - if len(original) == 0 && len(optimized) == 0 { - return - } - // Convert optimized results to the same type as original for comparison - optimizedConverted := make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) - for i, row := range optimized { - optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row) - } - - // Compare the results and log an error if they differ. - // NOTE: explicitly not sorting here as both query results are ordered by ID. - if diff := cmp.Diff(original, optimizedConverted); diff != "" { - logger.Error(ctx, "results differ for GetRunningPrebuiltWorkspacesOptimized", - slog.F("diff", diff)) - } -} diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 858b01abc00b9..5ba36912ce5c8 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -5,7 +5,6 @@ import ( "database/sql" "fmt" "sort" - "strings" "sync" "testing" "time" @@ -27,7 +26,6 @@ import ( "tailscale.com/types/ptr" "cdr.dev/slog" - "cdr.dev/slog/sloggers/slogjson" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/quartz" @@ -2333,164 +2331,3 @@ func TestReconciliationRespectsPauseSetting(t *testing.T) { require.NoError(t, err) require.Len(t, workspaces, 2, "should have recreated 2 prebuilds after resuming") } - -func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { - t.Parallel() - - ctx := context.Background() - - // Helper to create test data - createWorkspaceRow := func(id string, name string, ready bool) database.GetRunningPrebuiltWorkspacesRow { - uid := uuid.MustParse(id) - return database.GetRunningPrebuiltWorkspacesRow{ - ID: uid, - Name: name, - TemplateID: uuid.New(), - TemplateVersionID: uuid.New(), - CurrentPresetID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, - Ready: ready, - CreatedAt: time.Now(), - } - } - - createOptimizedRow := func(row database.GetRunningPrebuiltWorkspacesRow) database.GetRunningPrebuiltWorkspacesOptimizedRow { - return database.GetRunningPrebuiltWorkspacesOptimizedRow(row) - } - - t.Run("identical results - no logging", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - - original := []database.GetRunningPrebuiltWorkspacesRow{ - createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), - createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false), - } - - optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ - createOptimizedRow(original[0]), - createOptimizedRow(original[1]), - } - - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - - // Should not log any errors when results are identical - require.Empty(t, strings.TrimSpace(sb.String())) - }) - - t.Run("count mismatch - logs error", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - - original := []database.GetRunningPrebuiltWorkspacesRow{ - createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), - } - - optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ - createOptimizedRow(original[0]), - createOptimizedRow(createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false)), - } - - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - - // Should log exactly one error. - if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { - require.Len(t, lines, 1) - assert.Contains(t, lines[0], "ERROR") - assert.Contains(t, lines[0], "workspace2") - assert.Contains(t, lines[0], "CurrentPresetID") - } - }) - - t.Run("count mismatch - other direction", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - - original := []database.GetRunningPrebuiltWorkspacesRow{} - - optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ - createOptimizedRow(createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false)), - } - - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - - if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { - require.Len(t, lines, 1) - assert.Contains(t, lines[0], "ERROR") - assert.Contains(t, lines[0], "workspace2") - assert.Contains(t, lines[0], "CurrentPresetID") - } - }) - - t.Run("field differences - logs errors", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - - workspace1 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true) - workspace2 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false) - - original := []database.GetRunningPrebuiltWorkspacesRow{workspace1, workspace2} - - // Create optimized with different values - optimized1 := createOptimizedRow(workspace1) - optimized1.Name = "different-name" // Different name - optimized1.Ready = false // Different ready status - - optimized2 := createOptimizedRow(workspace2) - optimized2.CurrentPresetID = uuid.NullUUID{Valid: false} // Different preset ID (NULL) - - optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{optimized1, optimized2} - - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - - // Should log exactly one error with a cmp.Diff output - if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { - require.Len(t, lines, 1) - assert.Contains(t, lines[0], "ERROR") - assert.Contains(t, lines[0], "different-name") - assert.Contains(t, lines[0], "workspace1") - assert.Contains(t, lines[0], "Ready") - assert.Contains(t, lines[0], "CurrentPresetID") - } - }) - - t.Run("empty results - no logging", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - - original := []database.GetRunningPrebuiltWorkspacesRow{} - optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{} - - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - - // Should not log any errors when both results are empty - require.Empty(t, strings.TrimSpace(sb.String())) - }) - - t.Run("nil original", func(t *testing.T) { - t.Parallel() - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, nil, []database.GetRunningPrebuiltWorkspacesOptimizedRow{}) - // Should not log any errors when original is nil - require.Empty(t, strings.TrimSpace(sb.String())) - }) - - t.Run("nil optimized ", func(t *testing.T) { - t.Parallel() - var sb strings.Builder - logger := slog.Make(slogjson.Sink(&sb)) - prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, []database.GetRunningPrebuiltWorkspacesRow{}, nil) - // Should not log any errors when optimized is nil - require.Empty(t, strings.TrimSpace(sb.String())) - }) -}