From 6ef96703840472ed73d7453b8ace3a206b9f522a Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Wed, 21 Jan 2026 10:56:31 +0000 Subject: [PATCH] fix: limit concurrent database connections in prebuild reconciliation (#20908) ## Description This PR addresses database connection pool exhaustion during prebuilds reconciliation by introducing two changes: * `CanSkipReconciliation`: Filters out presets that don't need reconciliation before spawning goroutines. This ensures we only create goroutines for presets that will (_most likely_) perform database operations, avoiding unnecessary connection pool usage. * Dynamic `eg.SetLimit`: Limits concurrent goroutines based on the configured database connection pool size (`CODER_PG_CONN_MAX_OPEN / 2`). This replaces the previous hardcoded limit of 5, ensuring the reconciliation loop scales appropriately with the configured pool size while leaving capacity for other database operations. ## Changes * Add `CanSkipReconciliation()` method to `PresetSnapshot` that returns true for inactive presets with no running workspaces, no pending jobs, or expired prebuilds. * Add `maxDBConnections` parameter to `NewStoreReconciler` and compute `reconciliationConcurrency` as half the pool size (minimum 1). * Add `ReconciliationConcurrency()` getter method to `StoreReconciler`. * Add `eg.SetLimit(c.reconciliationConcurrency)` to bound concurrent reconciliation goroutines. * Add `PresetsTotal` and `PresetsReconciled` to `ReconcileStats` for observability. * Add `TestCanSkipReconciliation` unit tests. * Add `TestReconciliationConcurrency` unit tests. * Add benchmark tests for reconciliation performance. ## Benchmarks * `BenchmarkReconcileAll_NoOps`: Tests presets with no reconciliation actions. All presets are filtered by `CanSkipReconciliation`, resulting in no goroutines spawned and no database connections used. * `BenchmarkReconcileAll_ConnectionContention`: Tests presets where all require reconciliation actions. All presets spawn goroutines, but concurrency is limited by `eg.SetLimit(reconciliationConcurrency)`. * `BenchmarkReconcileAll_Mix`: Simulates a realistic scenario with a large subset of inactive presets (filtered by `CanSkipReconciliation`) and a smaller subset requiring reconciliation (limited by `eg.SetLimit`). Closes: https://github.com/coder/coder/issues/20606 --- coderd/prebuilds/api.go | 4 +- coderd/prebuilds/preset_snapshot.go | 43 ++ coderd/prebuilds/preset_snapshot_test.go | 256 ++++++++ enterprise/cli/create_test.go | 2 + enterprise/coderd/coderd.go | 15 +- enterprise/coderd/prebuilds/claim_test.go | 10 +- .../coderd/prebuilds/metricscollector_test.go | 50 +- enterprise/coderd/prebuilds/reconcile.go | 83 ++- .../prebuilds/reconcile_internal_test.go | 35 + enterprise/coderd/prebuilds/reconcile_test.go | 597 +++++++++++++++++- enterprise/coderd/workspaces_test.go | 6 + 11 files changed, 1060 insertions(+), 41 deletions(-) create mode 100644 enterprise/coderd/prebuilds/reconcile_internal_test.go diff --git a/coderd/prebuilds/api.go b/coderd/prebuilds/api.go index 0deab99416..cf29e29535 100644 --- a/coderd/prebuilds/api.go +++ b/coderd/prebuilds/api.go @@ -39,7 +39,9 @@ type ReconciliationOrchestrator interface { // ReconcileStats contains statistics about a reconciliation cycle. type ReconcileStats struct { - Elapsed time.Duration + Elapsed time.Duration + PresetsTotal int + PresetsReconciled int } type Reconciler interface { diff --git a/coderd/prebuilds/preset_snapshot.go b/coderd/prebuilds/preset_snapshot.go index 544d1e3ca4..0a9e57ddd2 100644 --- a/coderd/prebuilds/preset_snapshot.go +++ b/coderd/prebuilds/preset_snapshot.go @@ -82,6 +82,49 @@ func NewPresetSnapshot( } } +// CanSkipReconciliation returns true if this preset can safely be skipped during +// the reconciliation loop. +// +// This is a performance optimization to avoid spawning goroutines for presets +// that have no work to do. It only returns true for presets from inactive +// template versions that have no running workspaces, no pending jobs, and no +// in-progress builds. +func (p PresetSnapshot) CanSkipReconciliation() bool { + // Active presets are never skipped. Presets from active template versions always + // go through the reconciliation loop to ensure desired_instances is maintained correctly. + if p.isActive() { + return false + } + + // Inactive presets with running prebuilds means there are prebuilds to delete. + if len(p.Running) > 0 { + return false + } + + // Inactive presets with expired prebuilds means there are expired prebuilds to delete. + if len(p.Expired) > 0 { + return false + } + + // Inactive presets with pending jobs means there are pending jobs to cancel. + if p.PendingCount > 0 { + return false + } + + // Backoff is only populated for active presets, but check defensively. + if p.Backoff != nil { + return false + } + + // Fields not checked (only relevant for active presets): + // - PrebuildSchedules: Only affects desired instance calculation. + // - InProgress: Only populated for active template versions. + // - IsHardLimited: Only populated for active template versions. + + // Inactive preset with nothing to clean up: safe to skip. + return true +} + // ReconciliationState represents the processed state of a preset's prebuilds, // calculated from a PresetSnapshot. While PresetSnapshot contains raw data, // ReconciliationState contains derived metrics that are directly used to diff --git a/coderd/prebuilds/preset_snapshot_test.go b/coderd/prebuilds/preset_snapshot_test.go index ebc8921430..4e0c9add23 100644 --- a/coderd/prebuilds/preset_snapshot_test.go +++ b/coderd/prebuilds/preset_snapshot_test.go @@ -1527,6 +1527,262 @@ func TestCalculateDesiredInstances(t *testing.T) { } } +// TestCanSkipReconciliation ensures that CanSkipReconciliation only returns true +// when CalculateActions would return no actions. +func TestCanSkipReconciliation(t *testing.T) { + t.Parallel() + + clock := quartz.NewMock(t) + logger := testutil.Logger(t) + backoffInterval := 5 * time.Minute + + tests := []struct { + name string + preset database.GetTemplatePresetsWithPrebuildsRow + running []database.GetRunningPrebuiltWorkspacesRow + expired []database.GetRunningPrebuiltWorkspacesRow + inProgress []database.CountInProgressPrebuildsRow + pendingCount int + backoff *database.GetPresetsBackoffRow + isHardLimited bool + expectedCanSkip bool + expectedActionNoOp bool + }{ + { + name: "inactive_with_nothing_to_cleanup", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: false, + DesiredInstances: sql.NullInt32{Int32: 5, Valid: true}, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: true, // Inactive with nothing to clean up + expectedActionNoOp: true, // No actions needed + }, + { + name: "inactive_with_running_workspaces", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: false, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{ + {ID: uuid.New()}, + }, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: false, // Has running prebuilds to delete + expectedActionNoOp: false, // Returns ActionTypeDelete + }, + { + name: "inactive_with_pending_jobs", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: false, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 3, + backoff: nil, + isHardLimited: false, + expectedCanSkip: false, // Has pending jobs to cancel + expectedActionNoOp: false, // Returns ActionTypeCancelPending + }, + { + name: "inactive_with_backoff", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: false, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: &database.GetPresetsBackoffRow{ + NumFailed: 3, + LastBuildAt: clock.Now().Add(-1 * time.Minute), + }, + isHardLimited: false, + expectedCanSkip: false, // Has backoff + expectedActionNoOp: false, // Returns ActionTypeBackoff + }, + { + name: "inactive_deleted_template_with_nothing_to_cleanup", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: true, + Deprecated: false, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: true, // Deleted template with nothing to clean up + expectedActionNoOp: true, // No actions needed + }, + { + name: "inactive_deprecated_template_with_nothing_to_cleanup", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: true, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: true, // Deprecated template with nothing to clean up + expectedActionNoOp: true, // No actions needed + }, + { + name: "inactive_hard_limited", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: false, + Deleted: false, + Deprecated: false, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: true, + expectedCanSkip: true, // Hard limited but nothing to clean up + expectedActionNoOp: true, // No actions needed + }, + { + name: "active_with_desired_instances", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: true, + Deleted: false, + Deprecated: false, + DesiredInstances: sql.NullInt32{Int32: 2, Valid: true}, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{ + {ID: uuid.New()}, + {ID: uuid.New()}, + }, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: false, // Active presets are never skipped + expectedActionNoOp: true, // Already at desired count + }, + { + name: "active_with_no_workspaces", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: true, + Deleted: false, + Deprecated: false, + DesiredInstances: sql.NullInt32{Int32: 5, Valid: true}, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: false, + expectedCanSkip: false, // Active presets are never skipped + expectedActionNoOp: false, // Returns ActionTypeCreate + }, + { + name: "active_with_backoff", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: true, + Deleted: false, + Deprecated: false, + DesiredInstances: sql.NullInt32{Int32: 5, Valid: true}, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: &database.GetPresetsBackoffRow{ + NumFailed: 3, + LastBuildAt: clock.Now().Add(-1 * time.Minute), + }, + isHardLimited: false, + expectedCanSkip: false, // Active presets are never skipped + expectedActionNoOp: false, // Returns ActionTypeBackoff + }, + { + name: "active_hard_limited", + preset: database.GetTemplatePresetsWithPrebuildsRow{ + UsingActiveVersion: true, + Deleted: false, + Deprecated: false, + DesiredInstances: sql.NullInt32{Int32: 5, Valid: true}, + }, + running: []database.GetRunningPrebuiltWorkspacesRow{}, + expired: []database.GetRunningPrebuiltWorkspacesRow{}, + inProgress: []database.CountInProgressPrebuildsRow{}, + pendingCount: 0, + backoff: nil, + isHardLimited: true, + expectedCanSkip: false, // Active presets are never skipped + expectedActionNoOp: false, // Returns ActionTypeCreate (skipped in executeReconciliationAction) + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ps := prebuilds.NewPresetSnapshot( + tt.preset, + []database.TemplateVersionPresetPrebuildSchedule{}, + tt.running, + tt.expired, + tt.inProgress, + tt.pendingCount, + tt.backoff, + tt.isHardLimited, + clock, + logger, + ) + + canSkip := ps.CanSkipReconciliation() + require.Equal(t, tt.expectedCanSkip, canSkip) + + actions, err := ps.CalculateActions(backoffInterval) + require.NoError(t, err) + + actionNoOp := true + for _, action := range actions { + if !action.IsNoop() { + actionNoOp = false + break + } + } + require.Equal(t, tt.expectedActionNoOp, actionNoOp, + "CalculateActions() isNoOp mismatch") + + // IMPORTANT: If CanSkipReconciliation is true, CalculateActions must return no actions + if canSkip { + require.True(t, actionNoOp) + } + }) + } +} + func mustParseTime(t *testing.T, layout, value string) time.Time { t.Helper() parsedTime, err := time.Parse(layout, value) diff --git a/enterprise/cli/create_test.go b/enterprise/cli/create_test.go index a536f08ef7..be841dc8ae 100644 --- a/enterprise/cli/create_test.go +++ b/enterprise/cli/create_test.go @@ -369,6 +369,7 @@ func TestEnterpriseCreateWithPreset(t *testing.T) { notifications.NewNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -481,6 +482,7 @@ func TestEnterpriseCreateWithPreset(t *testing.T) { notifications.NewNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 14e67f70d5..205435f5a5 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -1311,7 +1311,18 @@ func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.Reconciliatio return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer } - reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.AGPL.FileCache, api.DeploymentValues.Prebuilds, - api.Logger.Named("prebuilds"), quartz.NewReal(), api.PrometheusRegistry, api.NotificationsEnqueuer, api.AGPL.BuildUsageChecker, api.TracerProvider) + reconciler := prebuilds.NewStoreReconciler( + api.Database, + api.Pubsub, + api.AGPL.FileCache, + api.DeploymentValues.Prebuilds, + api.Logger.Named("prebuilds"), + quartz.NewReal(), + api.PrometheusRegistry, + api.NotificationsEnqueuer, + api.AGPL.BuildUsageChecker, + api.TracerProvider, + int(api.DeploymentValues.PostgresConnMaxOpen.Value()), + ) return reconciler, prebuilds.NewEnterpriseClaimer(api.Database) } diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index c84358e2d2..5657072f12 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -166,7 +166,15 @@ func TestClaimPrebuild(t *testing.T) { defer provisionerCloser.Close() cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(spy, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + spy, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(spy) api.AGPL.PrebuildsClaimer.Store(&claimer) diff --git a/enterprise/coderd/prebuilds/metricscollector_test.go b/enterprise/coderd/prebuilds/metricscollector_test.go index 1891caaba9..2ea9667076 100644 --- a/enterprise/coderd/prebuilds/metricscollector_test.go +++ b/enterprise/coderd/prebuilds/metricscollector_test.go @@ -196,7 +196,15 @@ func TestMetricsCollector(t *testing.T) { clock := quartz.NewMock(t) db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ctx := testutil.Context(t, testutil.WaitLong) createdUsers := []uuid.UUID{database.PrebuildsSystemUserID} @@ -328,7 +336,15 @@ func TestMetricsCollector_DuplicateTemplateNames(t *testing.T) { clock := quartz.NewMock(t) db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ctx := testutil.Context(t, testutil.WaitLong) collector := prebuilds.NewMetricsCollector(db, logger, reconciler) @@ -476,7 +492,15 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() - reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + quartz.NewMock(t), + registry, + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ctx := testutil.Context(t, testutil.WaitLong) // Ensure no pause setting is set (default state) @@ -505,7 +529,15 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() - reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + quartz.NewMock(t), + registry, + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ctx := testutil.Context(t, testutil.WaitLong) // Set reconciliation to paused @@ -534,7 +566,15 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() - reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, + quartz.NewMock(t), + registry, + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ctx := testutil.Context(t, testutil.WaitLong) // Set reconciliation back to not paused diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index ee24cbbf57..f09e1998b5 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -57,6 +57,8 @@ type StoreReconciler struct { done chan struct{} provisionNotifyCh chan database.ProvisionerJob + reconciliationConcurrency int + // Prebuild state metrics metrics *MetricsCollector // Operational metrics @@ -93,20 +95,28 @@ func NewStoreReconciler(store database.Store, notifEnq notifications.Enqueuer, buildUsageChecker *atomic.Pointer[wsbuilder.UsageChecker], tracerProvider trace.TracerProvider, + maxDBConnections int, ) *StoreReconciler { + reconciliationConcurrency := calculateReconciliationConcurrency(maxDBConnections) + + logger.Debug(context.Background(), "reconciler initialized", + slog.F("reconciliation_concurrency", reconciliationConcurrency), + slog.F("max_db_connections", maxDBConnections)) + reconciler := &StoreReconciler{ - store: store, - pubsub: ps, - fileCache: fileCache, - logger: logger, - cfg: cfg, - clock: clock, - registerer: registerer, - notifEnq: notifEnq, - buildUsageChecker: buildUsageChecker, - tracer: tracerProvider.Tracer(tracing.TracerName), - done: make(chan struct{}, 1), - provisionNotifyCh: make(chan database.ProvisionerJob, 10), + store: store, + pubsub: ps, + fileCache: fileCache, + logger: logger, + cfg: cfg, + clock: clock, + registerer: registerer, + notifEnq: notifEnq, + buildUsageChecker: buildUsageChecker, + tracer: tracerProvider.Tracer(tracing.TracerName), + done: make(chan struct{}, 1), + provisionNotifyCh: make(chan database.ProvisionerJob, 10), + reconciliationConcurrency: reconciliationConcurrency, } if registerer != nil { @@ -129,6 +139,29 @@ func NewStoreReconciler(store database.Store, return reconciler } +// calculateReconciliationConcurrency determines the number of concurrent +// goroutines for preset reconciliation. Each preset may perform multiple +// database operations (creates/deletes), so we limit concurrency to avoid +// exhausting the connection pool while maintaining reasonable parallelism. +// +// Uses half the pool size, with a minimum of 1 and a maximum of 5. +// TODO(ssncferreira): If this becomes a bottleneck, consider adding a configuration option. +func calculateReconciliationConcurrency(maxDBConnections int) int { + if maxDBConnections <= 0 { + return 1 + } + + concurrency := maxDBConnections / 2 + if concurrency < 1 { + return 1 + } + if concurrency > 5 { + return 5 + } + + return concurrency +} + func (c *StoreReconciler) Run(ctx context.Context) { reconciliationInterval := c.cfg.ReconciliationInterval.Value() if reconciliationInterval <= 0 { // avoids a panic @@ -138,7 +171,8 @@ func (c *StoreReconciler) Run(ctx context.Context) { c.logger.Info(ctx, "starting reconciler", slog.F("interval", reconciliationInterval), slog.F("backoff_interval", c.cfg.ReconciliationBackoffInterval.String()), - slog.F("backoff_lookback", c.cfg.ReconciliationBackoffLookback.String())) + slog.F("backoff_lookback", c.cfg.ReconciliationBackoffLookback.String()), + slog.F("preset_concurrency", c.reconciliationConcurrency)) var wg sync.WaitGroup ticker := c.clock.NewTicker(reconciliationInterval) @@ -203,7 +237,11 @@ func (c *StoreReconciler) Run(ctx context.Context) { if c.reconciliationDuration != nil { c.reconciliationDuration.Observe(stats.Elapsed.Seconds()) } - c.logger.Debug(ctx, "reconciliation stats", slog.F("elapsed", stats.Elapsed)) + c.logger.Info(ctx, "reconciliation stats", + slog.F("elapsed", stats.Elapsed), + slog.F("presets_total", stats.PresetsTotal), + slog.F("presets_reconciled", stats.PresetsReconciled), + ) case <-ctx.Done(): // nolint:gocritic // it's okay to use slog.F() for an error in this case // because we want to differentiate two different types of errors: ctx.Err() and context.Cause() @@ -351,6 +389,11 @@ func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.Rec } var eg errgroup.Group + // Limit concurrency to avoid exhausting the coderd database connection pool. + eg.SetLimit(c.reconciliationConcurrency) + + presetsReconciled := 0 + // Reconcile presets in parallel. Each preset in its own goroutine. for _, preset := range snapshot.Presets { ps, err := snapshot.FilterByPreset(preset.ID) @@ -359,6 +402,15 @@ func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.Rec continue } + // Performance optimization: Skip presets that won't need any database operations. + // This avoids holding a slot in the errgroup limiter, reserving capacity for + // presets that actually need database connections. + if ps.CanSkipReconciliation() { + continue + } + + presetsReconciled++ + eg.Go(func() error { // Pass outer context. err = c.ReconcilePreset(ctx, *ps) @@ -375,6 +427,9 @@ func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.Rec }) } + stats.PresetsTotal = len(snapshot.Presets) + stats.PresetsReconciled = presetsReconciled + // Release lock only when all preset reconciliation goroutines are finished. return eg.Wait() }) diff --git a/enterprise/coderd/prebuilds/reconcile_internal_test.go b/enterprise/coderd/prebuilds/reconcile_internal_test.go new file mode 100644 index 0000000000..2dc3694f04 --- /dev/null +++ b/enterprise/coderd/prebuilds/reconcile_internal_test.go @@ -0,0 +1,35 @@ +package prebuilds + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCalculateReconciliationConcurrency(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + maxDBConnections int + expectedConcurrency int + }{ + {"base pool size", 10, 5}, + {"default pool size", 30, 5}, + {"large pool size", 100, 5}, + {"small pool", 4, 2}, + {"minimum pool", 2, 1}, + {"single connection", 1, 1}, + {"zero connections floors to 1", 0, 1}, + {"negative floors to 1", -5, 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result := calculateReconciliationConcurrency(tt.maxDBConnections) + require.Equal(t, tt.expectedConcurrency, result) + }) + } +} diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index c3963b7be2..e1200e6385 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -3,6 +3,7 @@ package prebuilds_test import ( "context" "database/sql" + "fmt" "sort" "sync" "sync/atomic" @@ -52,7 +53,15 @@ func TestNoReconciliationActionsIfNoPresets(t *testing.T) { } logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // given a template version with no presets org := dbgen.Organization(t, db, database.Organization{}) @@ -95,7 +104,15 @@ func TestNoReconciliationActionsIfNoPrebuilds(t *testing.T) { } logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // given there are presets, but no prebuilds org := dbgen.Organization(t, db, database.Organization{}) @@ -425,7 +442,15 @@ func (tc testCase) run(t *testing.T) { pubSub = &brokenPublisher{Pubsub: pubSub} } cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Run the reconciliation multiple times to ensure idempotency // 8 was arbitrary, but large enough to reasonably trust the result @@ -494,7 +519,15 @@ func TestMultiplePresetsPerTemplateVersion(t *testing.T) { ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -617,7 +650,15 @@ func TestPrebuildScheduling(t *testing.T) { ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -718,7 +759,15 @@ func TestInvalidPreset(t *testing.T) { ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -780,7 +829,15 @@ func TestDeletionOfPrebuiltWorkspaceWithInvalidPreset(t *testing.T) { ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + quartz.NewMock(t), + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -874,7 +931,15 @@ func TestSkippingHardLimitedPresets(t *testing.T) { fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Set up test environment with a template, version, and preset. ownerID := uuid.New() @@ -1017,7 +1082,15 @@ func TestHardLimitedPresetShouldNotBlockDeletion(t *testing.T) { fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Set up test environment with a template, version, and preset. ownerID := uuid.New() @@ -1211,7 +1284,15 @@ func TestRunLoop(t *testing.T) { ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -1339,7 +1420,15 @@ func TestFailedBuildBackoff(t *testing.T) { ).Leveled(slog.LevelDebug) db, ps := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Given: an active template version with presets and prebuilds configured. const desiredInstances = 2 @@ -1461,7 +1550,9 @@ func TestReconciliationLock(t *testing.T) { quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), - newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + newNoopUsageCheckerPtr(), noop.NewTracerProvider(), + 10, + ) reconciler.WithReconciliationLock(ctx, logger, func(_ context.Context, _ database.Store) error { lockObtained := mutex.TryLock() // As long as the postgres lock is held, this mutex should always be unlocked when we get here. @@ -1491,7 +1582,15 @@ func TestTrackResourceReplacement(t *testing.T) { fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, codersdk.PrebuildsConfig{}, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Given: a template admin to receive a notification. templateAdmin := dbgen.User(t, db, database.User{ @@ -1643,7 +1742,15 @@ func TestExpiredPrebuildsMultipleActions(t *testing.T) { fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) - controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + controller := prebuilds.NewStoreReconciler( + db, pubSub, cache, cfg, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Set up test environment with a template, version, and preset ownerID := uuid.New() @@ -2098,7 +2205,15 @@ func TestCancelPendingPrebuilds(t *testing.T) { registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, codersdk.PrebuildsConfig{}, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) owner := coderdtest.CreateFirstUser(t, client) // Given: a template with a version containing a preset with 1 prebuild instance @@ -2335,7 +2450,15 @@ func TestCancelPendingPrebuilds(t *testing.T) { registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, codersdk.PrebuildsConfig{}, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) owner := coderdtest.CreateFirstUser(t, client) // Given: template A with 2 versions @@ -2400,7 +2523,15 @@ func TestReconciliationStats(t *testing.T) { registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, codersdk.PrebuildsConfig{}, logger, + clock, + registry, + fakeEnqueuer, + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) owner := coderdtest.CreateFirstUser(t, client) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) @@ -2911,7 +3042,15 @@ func TestReconciliationRespectsPauseSetting(t *testing.T) { } logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) - reconciler := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr(), noop.NewTracerProvider()) + reconciler := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, logger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + 10, + ) // Setup a template with a preset that should create prebuilds org := dbgen.Organization(t, db, database.Organization{}) @@ -2972,3 +3111,425 @@ func TestReconciliationRespectsPauseSetting(t *testing.T) { require.NoError(t, err) require.Len(t, workspaces, 2, "should have recreated 2 prebuilds after resuming") } + +// BenchmarkReconcileAll_NoOps benchmarks the reconciliation loop with varying numbers +// of presets of inactive versions that require no reconciliation actions. +// +// This validates the performance benefit of the CanSkipReconciliation optimization, +// which avoids spawning goroutines for presets that don't need reconciliation actions. +// +// go test -bench='^BenchmarkReconcileAll_NoOps$' -run=^$ -benchtime=5x -count=2 ./enterprise/coderd/prebuilds/ +func BenchmarkReconcileAll_NoOps(b *testing.B) { + benchCases := []struct { + name string + presetCount int + }{ + {"100_presets", 100}, + {"1000_presets", 1000}, + {"5000_presets", 5000}, + } + + for _, bc := range benchCases { + b.Run(bc.name, func(b *testing.B) { + // Setup + ctx := context.Background() + logger := slog.Make() + db, ps, sqlDB := dbtestutil.NewDBWithSQLDB(b, dbtestutil.WithLogger(logger)) + + // Database configuration set per replica (see cli/server.go). + // Default value for CODER_PG_CONN_MAX_OPEN is 10. + maxOpenConns := 10 + sqlDB.SetMaxOpenConns(maxOpenConns) + sqlDB.SetMaxIdleConns(3) + + clock := quartz.NewMock(b).WithLogger(quartz.NoOpLogger) + cfg := codersdk.PrebuildsConfig{ + ReconciliationInterval: serpent.Duration(testutil.WaitLong), + } + prebuildsLogger := slogtest.Make(b, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelError) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, prebuildsLogger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + maxOpenConns, + ) + + org := dbgen.Organization(b, db, database.Organization{}) + user := dbgen.User(b, db, database.User{}) + + for i := 0; i < bc.presetCount; i++ { + template := dbgen.Template(b, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + + oldTV := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + }) + dbgen.Preset(b, db, database.InsertPresetParams{ + TemplateVersionID: oldTV.ID, + Name: "default", + DesiredInstances: sql.NullInt32{Int32: 2, Valid: true}, + }) + + // Create new version without preset and make it active + newTV := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + }) + err := db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ + ID: template.ID, + ActiveVersionID: newTV.ID, + }) + require.NoError(b, err) + } + + // Verify setup: all presets should be inactive with no work + // Get all presets from all templates + presets, err := db.GetTemplatePresetsWithPrebuilds(ctx, uuid.NullUUID{}) + require.NoError(b, err) + require.Len(b, presets, bc.presetCount) + + // Should have no prebuilt workspaces + workspaces, err := db.GetWorkspaces(ctx, database.GetWorkspacesParams{ + OwnerID: database.PrebuildsSystemUserID, + }) + require.NoError(b, err) + require.Empty(b, workspaces) + + // Benchmark the reconciliation loop + b.ResetTimer() + for i := 0; i < b.N; i++ { + stats, err := controller.ReconcileAll(ctx) + require.NoError(b, err) + _ = stats + } + }) + } +} + +// BenchmarkReconcileAll_ConnectionContention benchmarks the reconciliation loop with varying +// levels of database connection contention. +// +// This measures reconciliation time under heavy database load, where each preset +// needs to create multiple prebuilt workspaces. +// +// go test -bench='^BenchmarkReconcileAll_ConnectionContention$' -run=^$ -benchtime=5x -count=2 ./enterprise/coderd/prebuilds/ +func BenchmarkReconcileAll_ConnectionContention(b *testing.B) { + benchCases := []struct { + name string + presetsForReconciliation int + desiredInstances int32 + }{ + {"10_presets_5_instances", 10, 5}, // 50 creates + {"50_presets_5_instances", 50, 5}, // 250 creates + {"100_presets_5_instances", 100, 5}, // 500 creates + {"1000_presets_10_instances", 1000, 10}, // 10000 creates + } + + for _, bc := range benchCases { + b.Run(bc.name, func(b *testing.B) { + for i := 0; i < b.N; i++ { + b.StopTimer() + + // Setup: Create a fresh database for each iteration because ReconcileAll + // creates prebuilds on the first run. Subsequent runs would see those + // prebuilds as "in progress" and skip creating new ones, making the + // benchmark results inconsistent. + ctx := context.Background() + logger := slog.Make() + db, ps, sqlDB := dbtestutil.NewDBWithSQLDB(b, dbtestutil.WithLogger(logger)) + + // Database configuration set per replica (see cli/server.go). + // Default value for CODER_PG_CONN_MAX_OPEN is 10. + maxOpenConns := 10 + sqlDB.SetMaxOpenConns(maxOpenConns) + sqlDB.SetMaxIdleConns(3) + + clock := quartz.NewMock(b).WithLogger(quartz.NoOpLogger) + cfg := codersdk.PrebuildsConfig{ + ReconciliationInterval: serpent.Duration(testutil.WaitLong), + } + prebuildsLogger := slogtest.Make(b, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelError) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, prebuildsLogger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + maxOpenConns, + ) + + // Create presets from active template versions that need reconciliation actions + org := dbgen.Organization(b, db, database.Organization{}) + user := dbgen.User(b, db, database.User{}) + + for p := 0; p < bc.presetsForReconciliation; p++ { + template := dbgen.Template(b, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + + // Create a completed provisioner job for the template version. + // This is needed because workspace builds copy the StorageMethod and FileID + // from the template version's import job to know which Terraform files to use. + file := dbgen.File(b, db, database.File{ + CreatedBy: user.ID, + Hash: uuid.NewString(), // Generate unique hash for each file + }) + templateVersionJob := dbgen.ProvisionerJob(b, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + FileID: file.ID, + StorageMethod: database.ProvisionerStorageMethodFile, + Type: database.ProvisionerJobTypeTemplateVersionImport, + CompletedAt: sql.NullTime{Time: clock.Now(), Valid: true}, + }) + + tv := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + JobID: templateVersionJob.ID, + }) + + dbgen.Preset(b, db, database.InsertPresetParams{ + TemplateVersionID: tv.ID, + Name: "default", + DesiredInstances: sql.NullInt32{Int32: bc.desiredInstances, Valid: true}, + }) + + // Make this the active version + err := db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ + ID: template.ID, + ActiveVersionID: tv.ID, + }) + require.NoError(b, err) + } + + // Verify setup: all presets should require reconciliation + // Get all presets from all templates + presets, err := db.GetTemplatePresetsWithPrebuilds(ctx, uuid.NullUUID{}) + require.NoError(b, err) + require.Len(b, presets, bc.presetsForReconciliation) + + b.StartTimer() + + // Measure reconciliation + _, err = controller.ReconcileAll(ctx) + require.NoError(b, err) + + b.StopTimer() + } + }) + } +} + +// BenchmarkReconcileAll_Mix benchmarks reconciliation performance when there are +// many total presets in the database, but only a small subset are active and need reconciliation. +// +// This validates that the reconciler efficiently filters to only active template versions and +// doesn't slow down proportionally with the total number of inactive presets. +// +// go test -bench='^BenchmarkReconcileAll_Mix$' -run=^$ -benchtime=5x -count=2 ./enterprise/coderd/prebuilds/ +func BenchmarkReconcileAll_Mix(b *testing.B) { + benchCases := []struct { + name string + inactivePresetsCount int // Presets on inactive template versions (noise) + activePresetsCount int // Presets on active versions that need work + desiredInstances int32 // Desired prebuilds per preset + }{ + {"500_total_10_active", 490, 10, 2}, // 20 creates + {"1000_total_25_active", 975, 25, 2}, // 50 creates + {"5000_total_50_active", 4950, 50, 2}, // 100 creates + } + + for _, bc := range benchCases { + b.Run(bc.name, func(b *testing.B) { + for i := 0; i < b.N; i++ { + b.StopTimer() + + // Setup: Create a fresh database for each iteration because ReconcileAll + // creates prebuilds on the first run. Subsequent runs would see those + // prebuilds as "in progress" and skip creating new ones, making the + // benchmark results inconsistent. + ctx := context.Background() + logger := slog.Make() + db, ps, sqlDB := dbtestutil.NewDBWithSQLDB(b, dbtestutil.WithLogger(logger)) + + // Database configuration set per replica (see cli/server.go). + // Default value for CODER_PG_CONN_MAX_OPEN is 10. + maxOpenConns := 10 + sqlDB.SetMaxOpenConns(maxOpenConns) + sqlDB.SetMaxIdleConns(3) + + clock := quartz.NewMock(b).WithLogger(quartz.NoOpLogger) + cfg := codersdk.PrebuildsConfig{ + ReconciliationInterval: serpent.Duration(testutil.WaitLong), + } + prebuildsLogger := slogtest.Make(b, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelError) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler( + db, ps, cache, cfg, prebuildsLogger, + clock, + prometheus.NewRegistry(), + newNoopEnqueuer(), + newNoopUsageCheckerPtr(), + noop.NewTracerProvider(), + maxOpenConns, + ) + + org := dbgen.Organization(b, db, database.Organization{}) + user := dbgen.User(b, db, database.User{}) + + // Create inactive presets (noise that should be filtered out efficiently) + // These are on templates with inactive versions + for p := 0; p < bc.inactivePresetsCount; p++ { + template := dbgen.Template(b, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + + file := dbgen.File(b, db, database.File{ + CreatedBy: user.ID, + Hash: fmt.Sprintf("inactive-%d", p), + }) + + templateVersionJob := dbgen.ProvisionerJob(b, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + FileID: file.ID, + StorageMethod: database.ProvisionerStorageMethodFile, + Type: database.ProvisionerJobTypeTemplateVersionImport, + CompletedAt: sql.NullTime{Time: clock.Now(), Valid: true}, + }) + + inactiveVersion := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + JobID: templateVersionJob.ID, + Name: fmt.Sprintf("inactive-v%d", p), + }) + + // Create presets on this inactive version + dbgen.Preset(b, db, database.InsertPresetParams{ + TemplateVersionID: inactiveVersion.ID, + Name: "default", + DesiredInstances: sql.NullInt32{Int32: 2, Valid: true}, + }) + + // Create a newer active version (making the above version inactive) + newerFile := dbgen.File(b, db, database.File{ + CreatedBy: user.ID, + Hash: fmt.Sprintf("active-no-preset-%d", p), + }) + + newerJob := dbgen.ProvisionerJob(b, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + FileID: newerFile.ID, + StorageMethod: database.ProvisionerStorageMethodFile, + Type: database.ProvisionerJobTypeTemplateVersionImport, + CompletedAt: sql.NullTime{Time: clock.Now(), Valid: true}, + }) + + activeVersion := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + JobID: newerJob.ID, + Name: fmt.Sprintf("active-v%d", p), + }) + + // Make the newer version active (no presets = no reconciliation work) + err := db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ + ID: template.ID, + ActiveVersionID: activeVersion.ID, + }) + require.NoError(b, err) + } + + // Create active presets that need reconciliation (missing prebuilds) + for p := 0; p < bc.activePresetsCount; p++ { + template := dbgen.Template(b, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + Name: fmt.Sprintf("needs-work-%d", p), + }) + + file := dbgen.File(b, db, database.File{ + CreatedBy: user.ID, + Hash: fmt.Sprintf("needs-work-%d", p), + }) + + // Create a completed provisioner job for the template version. + // This is needed because workspace builds copy the StorageMethod and FileID + // from the template version's import job to know which Terraform files to use. + templateVersionJob := dbgen.ProvisionerJob(b, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + FileID: file.ID, + StorageMethod: database.ProvisionerStorageMethodFile, + Type: database.ProvisionerJobTypeTemplateVersionImport, + CompletedAt: sql.NullTime{Time: clock.Now(), Valid: true}, + }) + + tv := dbgen.TemplateVersion(b, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + JobID: templateVersionJob.ID, + }) + + dbgen.Preset(b, db, database.InsertPresetParams{ + TemplateVersionID: tv.ID, + Name: "default", + DesiredInstances: sql.NullInt32{Int32: bc.desiredInstances, Valid: true}, + }) + + // Make this the active version + err := db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ + ID: template.ID, + ActiveVersionID: tv.ID, + }) + require.NoError(b, err) + } + + // Verify setup + allPresets, err := db.GetTemplatePresetsWithPrebuilds(ctx, uuid.NullUUID{}) + require.NoError(b, err) + totalCount := bc.inactivePresetsCount + bc.activePresetsCount + require.Len(b, allPresets, totalCount, "total preset count should match") + + // Count how many are actually active + activeCount := 0 + for _, preset := range allPresets { + presetTemplate, err := db.GetTemplateByID(ctx, preset.TemplateID) + require.NoError(b, err) + if presetTemplate.ActiveVersionID == preset.TemplateVersionID { + activeCount++ + } + } + require.Equal(b, bc.activePresetsCount, activeCount, "active preset count should match") + + b.StartTimer() + + // Measure reconciliation: should only process the active presets + _, err = controller.ReconcileAll(ctx) + require.NoError(b, err) + + b.StopTimer() + } + }) + } +} diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index fe445ff749..d9ed713d24 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1987,6 +1987,7 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2110,6 +2111,7 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2233,6 +2235,7 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2378,6 +2381,7 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2524,6 +2528,7 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2970,6 +2975,7 @@ func TestWorkspaceProvisionerdServerMetrics(t *testing.T) { notifications.NewNoopEnqueuer(), api.AGPL.BuildUsageChecker, noop.NewTracerProvider(), + 10, ) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer)