From e2d8e8777bd26ddb8fbbb0f266aec426738d6aa6 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 21 Apr 2025 17:17:12 +0200 Subject: [PATCH 1/8] Enable prebuilds Signed-off-by: Danny Kopping # Conflicts: # codersdk/deployment.go --- cli/testdata/coder_server_--help.golden | 6 +++ cli/testdata/server-config.yaml.golden | 12 +++++ coderd/apidoc/docs.go | 23 +++++++++ coderd/apidoc/swagger.json | 23 +++++++++ codersdk/deployment.go | 49 ++++++++++++++++++- docs/reference/api/general.md | 5 ++ docs/reference/api/schemas.md | 30 ++++++++++++ docs/reference/cli/server.md | 11 +++++ .../cli/testdata/coder_server_--help.golden | 6 +++ enterprise/coderd/coderd.go | 42 ++++++++++++++++ site/src/api/typesGenerated.ts | 4 ++ 11 files changed, 210 insertions(+), 1 deletion(-) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 1cefe8767f3b0..817093c17a9b0 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -670,6 +670,12 @@ workspaces stopping during the day due to template scheduling. must be *. Only one hour and minute can be specified (ranges or comma separated values are not supported). +WORKSPACE PREBUILDS OPTIONS: +Configure how workspace prebuilds behave. + + --workspace-prebuilds-reconciliation-interval duration, $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL (default: 15s) + How often to reconcile workspace prebuilds state. + ⚠️ DANGEROUS OPTIONS: --dangerous-allow-path-app-sharing bool, $CODER_DANGEROUS_ALLOW_PATH_APP_SHARING Allow workspace apps that are not served from subdomains to be shared. diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 911270a579457..e0ea86ef6432d 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -688,3 +688,15 @@ notifications: # How often to query the database for queued notifications. # (default: 15s, type: duration) fetchInterval: 15s +# Configure how workspace prebuilds behave. +workspace_prebuilds: + # How often to reconcile workspace prebuilds state. + # (default: 15s, type: duration) + reconciliation_interval: 15s + # Interval to increase reconciliation backoff by when unrecoverable errors occur. + # (default: 15s, type: duration) + reconciliation_backoff_interval: 15s + # Interval to look back to determine number of failed builds, which influences + # backoff. + # (default: 1h0m0s, type: duration) + reconciliation_backoff_lookback_period: 1h0m0s diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 62f91a858247d..c7c0eb3c40d65 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11926,6 +11926,9 @@ const docTemplate = `{ "workspace_hostname_suffix": { "type": "string" }, + "workspace_prebuilds": { + "$ref": "#/definitions/codersdk.PrebuildsConfig" + }, "write_config": { "type": "boolean" } @@ -12004,6 +12007,7 @@ const docTemplate = `{ "auto-fill-parameters", "notifications", "workspace-usage", + "workspace-prebuilds", "web-push", "dynamic-parameters" ], @@ -12013,6 +12017,7 @@ const docTemplate = `{ "ExperimentExample": "This isn't used for anything.", "ExperimentNotifications": "Sends notifications via SMTP and webhooks following certain events.", "ExperimentWebPush": "Enables web push notifications through the browser.", + "ExperimentWorkspacePrebuilds": "Enables the new workspace prebuilds feature.", "ExperimentWorkspaceUsage": "Enables the new workspace usage tracking." }, "x-enum-varnames": [ @@ -12020,6 +12025,7 @@ const docTemplate = `{ "ExperimentAutoFillParameters", "ExperimentNotifications", "ExperimentWorkspaceUsage", + "ExperimentWorkspacePrebuilds", "ExperimentWebPush", "ExperimentDynamicParameters" ] @@ -13654,6 +13660,23 @@ const docTemplate = `{ } } }, + "codersdk.PrebuildsConfig": { + "type": "object", + "properties": { + "reconciliation_backoff_interval": { + "description": "ReconciliationBackoffInterval specifies the amount of time to increase the backoff interval\nwhen errors occur during reconciliation.", + "type": "integer" + }, + "reconciliation_backoff_lookback": { + "description": "ReconciliationBackoffLookback determines the time window to look back when calculating\nthe number of failed prebuilds, which influences the backoff strategy.", + "type": "integer" + }, + "reconciliation_interval": { + "description": "ReconciliationInterval defines how often the workspace prebuilds state should be reconciled.", + "type": "integer" + } + } + }, "codersdk.Preset": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index e9e0470462b39..db0734eaefd72 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10684,6 +10684,9 @@ "workspace_hostname_suffix": { "type": "string" }, + "workspace_prebuilds": { + "$ref": "#/definitions/codersdk.PrebuildsConfig" + }, "write_config": { "type": "boolean" } @@ -10758,6 +10761,7 @@ "auto-fill-parameters", "notifications", "workspace-usage", + "workspace-prebuilds", "web-push", "dynamic-parameters" ], @@ -10767,6 +10771,7 @@ "ExperimentExample": "This isn't used for anything.", "ExperimentNotifications": "Sends notifications via SMTP and webhooks following certain events.", "ExperimentWebPush": "Enables web push notifications through the browser.", + "ExperimentWorkspacePrebuilds": "Enables the new workspace prebuilds feature.", "ExperimentWorkspaceUsage": "Enables the new workspace usage tracking." }, "x-enum-varnames": [ @@ -10774,6 +10779,7 @@ "ExperimentAutoFillParameters", "ExperimentNotifications", "ExperimentWorkspaceUsage", + "ExperimentWorkspacePrebuilds", "ExperimentWebPush", "ExperimentDynamicParameters" ] @@ -12346,6 +12352,23 @@ } } }, + "codersdk.PrebuildsConfig": { + "type": "object", + "properties": { + "reconciliation_backoff_interval": { + "description": "ReconciliationBackoffInterval specifies the amount of time to increase the backoff interval\nwhen errors occur during reconciliation.", + "type": "integer" + }, + "reconciliation_backoff_lookback": { + "description": "ReconciliationBackoffLookback determines the time window to look back when calculating\nthe number of failed prebuilds, which influences the backoff strategy.", + "type": "integer" + }, + "reconciliation_interval": { + "description": "ReconciliationInterval defines how often the workspace prebuilds state should be reconciled.", + "type": "integer" + } + } + }, "codersdk.Preset": { "type": "object", "properties": { diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 864a883330776..46c804c1b296d 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -81,6 +81,7 @@ const ( FeatureControlSharedPorts FeatureName = "control_shared_ports" FeatureCustomRoles FeatureName = "custom_roles" FeatureMultipleOrganizations FeatureName = "multiple_organizations" + FeatureWorkspacePrebuilds FeatureName = "workspace_prebuilds" ) // FeatureNames must be kept in-sync with the Feature enum above. @@ -103,6 +104,7 @@ var FeatureNames = []FeatureName{ FeatureControlSharedPorts, FeatureCustomRoles, FeatureMultipleOrganizations, + FeatureWorkspacePrebuilds, } // Humanize returns the feature name in a human-readable format. @@ -132,6 +134,7 @@ func (n FeatureName) AlwaysEnable() bool { FeatureHighAvailability: true, FeatureCustomRoles: true, FeatureMultipleOrganizations: true, + FeatureWorkspacePrebuilds: true, }[n] } @@ -393,6 +396,7 @@ type DeploymentValues struct { TermsOfServiceURL serpent.String `json:"terms_of_service_url,omitempty" typescript:",notnull"` Notifications NotificationsConfig `json:"notifications,omitempty" typescript:",notnull"` AdditionalCSPPolicy serpent.StringArray `json:"additional_csp_policy,omitempty" typescript:",notnull"` + Prebuilds PrebuildsConfig `json:"workspace_prebuilds,omitempty" typescript:",notnull"` WorkspaceHostnameSuffix serpent.String `json:"workspace_hostname_suffix,omitempty" typescript:",notnull"` Config serpent.YAMLConfigPath `json:"config,omitempty" typescript:",notnull"` @@ -1034,6 +1038,11 @@ func (c *DeploymentValues) Options() serpent.OptionSet { Parent: &deploymentGroupNotifications, YAML: "webhook", } + deploymentGroupPrebuilds = serpent.Group{ + Name: "Workspace Prebuilds", + YAML: "workspace_prebuilds", + Description: "Configure how workspace prebuilds behave.", + } deploymentGroupInbox = serpent.Group{ Name: "Inbox", Parent: &deploymentGroupNotifications, @@ -3029,6 +3038,41 @@ Write out the current server config as YAML to stdout.`, Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), Hidden: true, // Hidden because most operators should not need to modify this. }, + { + Name: "Reconciliation Interval", + Description: "How often to reconcile workspace prebuilds state.", + Flag: "workspace-prebuilds-reconciliation-interval", + Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL", + Value: &c.Prebuilds.ReconciliationInterval, + Default: (time.Second * 15).String(), + Group: &deploymentGroupPrebuilds, + YAML: "reconciliation_interval", + Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), + }, + { + Name: "Reconciliation Backoff Interval", + Description: "Interval to increase reconciliation backoff by when unrecoverable errors occur.", + Flag: "workspace-prebuilds-reconciliation-backoff-interval", + Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_INTERVAL", + Value: &c.Prebuilds.ReconciliationBackoffInterval, + Default: (time.Second * 15).String(), + Group: &deploymentGroupPrebuilds, + YAML: "reconciliation_backoff_interval", + Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), + Hidden: true, + }, + { + Name: "Reconciliation Backoff Lookback Period", + Description: "Interval to look back to determine number of failed builds, which influences backoff.", + Flag: "workspace-prebuilds-reconciliation-backoff-lookback-period", + Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_LOOKBACK_PERIOD", + Value: &c.Prebuilds.ReconciliationBackoffLookback, + Default: (time.Hour).String(), // TODO: use https://pkg.go.dev/github.com/jackc/pgtype@v1.12.0#Interval + Group: &deploymentGroupPrebuilds, + YAML: "reconciliation_backoff_lookback_period", + Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), + Hidden: true, + }, // Push notifications. } @@ -3254,6 +3298,7 @@ const ( ExperimentAutoFillParameters Experiment = "auto-fill-parameters" // This should not be taken out of experiments until we have redesigned the feature. ExperimentNotifications Experiment = "notifications" // Sends notifications via SMTP and webhooks following certain events. ExperimentWorkspaceUsage Experiment = "workspace-usage" // Enables the new workspace usage tracking. + ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds" // Enables the new workspace prebuilds feature. ExperimentWebPush Experiment = "web-push" // Enables web push notifications through the browser. ExperimentDynamicParameters Experiment = "dynamic-parameters" // Enables dynamic parameters when creating a workspace. ) @@ -3262,7 +3307,9 @@ const ( // users to opt-in to via --experimental='*'. // Experiments that are not ready for consumption by all users should // not be included here and will be essentially hidden. -var ExperimentsSafe = Experiments{} +var ExperimentsSafe = Experiments{ + ExperimentWorkspacePrebuilds, +} // Experiments is a list of experiments. // Multiple experiments may be enabled at the same time. diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 0db339a5baec9..3c27ddb6dea1d 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -519,6 +519,11 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "wgtunnel_host": "string", "wildcard_access_url": "string", "workspace_hostname_suffix": "string", + "workspace_prebuilds": { + "reconciliation_backoff_interval": 0, + "reconciliation_backoff_lookback": 0, + "reconciliation_interval": 0 + }, "write_config": true }, "options": [ diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index dd8ffd1971cb8..2389c7db33f64 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2170,6 +2170,11 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "wgtunnel_host": "string", "wildcard_access_url": "string", "workspace_hostname_suffix": "string", + "workspace_prebuilds": { + "reconciliation_backoff_interval": 0, + "reconciliation_backoff_lookback": 0, + "reconciliation_interval": 0 + }, "write_config": true }, "options": [ @@ -2650,6 +2655,11 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "wgtunnel_host": "string", "wildcard_access_url": "string", "workspace_hostname_suffix": "string", + "workspace_prebuilds": { + "reconciliation_backoff_interval": 0, + "reconciliation_backoff_lookback": 0, + "reconciliation_interval": 0 + }, "write_config": true } ``` @@ -2719,6 +2729,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `wgtunnel_host` | string | false | | | | `wildcard_access_url` | string | false | | | | `workspace_hostname_suffix` | string | false | | | +| `workspace_prebuilds` | [codersdk.PrebuildsConfig](#codersdkprebuildsconfig) | false | | | | `write_config` | boolean | false | | | ## codersdk.DisplayApp @@ -2815,6 +2826,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `auto-fill-parameters` | | `notifications` | | `workspace-usage` | +| `workspace-prebuilds` | | `web-push` | | `dynamic-parameters` | @@ -4659,6 +4671,24 @@ Git clone makes use of this by parsing the URL from: 'Username for "https://gith | `address` | [serpent.HostPort](#serpenthostport) | false | | | | `enable` | boolean | false | | | +## codersdk.PrebuildsConfig + +```json +{ + "reconciliation_backoff_interval": 0, + "reconciliation_backoff_lookback": 0, + "reconciliation_interval": 0 +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +|-----------------------------------|---------|----------|--------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `reconciliation_backoff_interval` | integer | false | | Reconciliation backoff interval specifies the amount of time to increase the backoff interval when errors occur during reconciliation. | +| `reconciliation_backoff_lookback` | integer | false | | Reconciliation backoff lookback determines the time window to look back when calculating the number of failed prebuilds, which influences the backoff strategy. | +| `reconciliation_interval` | integer | false | | Reconciliation interval defines how often the workspace prebuilds state should be reconciled. | + ## codersdk.Preset ```json diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index 1b4052e335e66..c01baa45a3f69 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -1603,3 +1603,14 @@ Enable Coder Inbox. | Default | 5 | The upper limit of attempts to send a notification. + +### --workspace-prebuilds-reconciliation-interval + +| | | +|-------------|-----------------------------------------------------------------| +| Type | duration | +| Environment | $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL | +| YAML | workspace_prebuilds.reconciliation_interval | +| Default | 15s | + +How often to reconcile workspace prebuilds state. diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index d11304742d974..344df781c3b7e 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -671,6 +671,12 @@ workspaces stopping during the day due to template scheduling. must be *. Only one hour and minute can be specified (ranges or comma separated values are not supported). +WORKSPACE PREBUILDS OPTIONS: +Configure how workspace prebuilds behave. + + --workspace-prebuilds-reconciliation-interval duration, $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL (default: 15s) + How often to reconcile workspace prebuilds state. + ⚠️ DANGEROUS OPTIONS: --dangerous-allow-path-app-sharing bool, $CODER_DANGEROUS_ALLOW_PATH_APP_SHARING Allow workspace apps that are not served from subdomains to be shared. diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 6b45bc65e2c3f..4ac24f0139b83 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -12,12 +12,15 @@ import ( "sync" "time" + "github.com/coder/quartz" + "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/idpsync" agplportsharing "github.com/coder/coder/v2/coderd/portsharing" + agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/enterprise/coderd/enidpsync" "github.com/coder/coder/v2/enterprise/coderd/portsharing" @@ -43,6 +46,7 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/dbauthz" "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/enterprise/coderd/prebuilds" "github.com/coder/coder/v2/enterprise/coderd/proxyhealth" "github.com/coder/coder/v2/enterprise/coderd/schedule" "github.com/coder/coder/v2/enterprise/dbcrypt" @@ -628,6 +632,8 @@ type API struct { licenseMetricsCollector *license.MetricsCollector tailnetService *tailnet.ClientService + + PrebuildsReconciler agplprebuilds.ReconciliationOrchestrator } // writeEntitlementWarningsHeader writes the entitlement warnings to the response header @@ -658,6 +664,13 @@ func (api *API) Close() error { if api.Options.CheckInactiveUsersCancelFunc != nil { api.Options.CheckInactiveUsersCancelFunc() } + + if api.PrebuildsReconciler != nil { + ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) + defer giveUp() + api.PrebuildsReconciler.Stop(ctx, xerrors.New("api closed")) // TODO: determine root cause (requires changes up the stack, though). + } + return api.AGPL.Close() } @@ -860,6 +873,20 @@ func (api *API) updateEntitlements(ctx context.Context) error { api.AGPL.PortSharer.Store(&ps) } + if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) || api.PrebuildsReconciler == nil { + reconciler, claimer := api.setupPrebuilds(enabled) + if api.PrebuildsReconciler != nil { + stopCtx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) + defer giveUp() + api.PrebuildsReconciler.Stop(stopCtx, xerrors.New("entitlements change")) + } + + api.PrebuildsReconciler = reconciler + go reconciler.RunLoop(context.Background()) + + api.AGPL.PrebuildsClaimer.Store(&claimer) + } + // External token encryption is soft-enforced featureExternalTokenEncryption := reloadedEntitlements.Features[codersdk.FeatureExternalTokenEncryption] featureExternalTokenEncryption.Enabled = len(api.ExternalTokenEncryption) > 0 @@ -1128,3 +1155,18 @@ func (api *API) runEntitlementsLoop(ctx context.Context) { func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Objecter) bool { return api.AGPL.HTTPAuth.Authorize(r, action, object) } + +func (api *API) setupPrebuilds(entitled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) { + enabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds) + if !enabled || !entitled { + api.Logger.Debug(context.Background(), "prebuilds not enabled", + slog.F("experiment_enabled", enabled), slog.F("entitled", entitled)) + + return agplprebuilds.NewNoopReconciler(), agplprebuilds.DefaultClaimer + } + + reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds, + api.Logger.Named("prebuilds"), quartz.NewReal()) + + return reconciler, prebuilds.EnterpriseClaimer{} +} diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 025ed9f1933cf..8f69954682032 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -690,6 +690,7 @@ export interface DeploymentValues { readonly terms_of_service_url?: string; readonly notifications?: NotificationsConfig; readonly additional_csp_policy?: string; + readonly workspace_prebuilds?: PrebuildsConfig; readonly workspace_hostname_suffix?: string; readonly config?: string; readonly write_config?: boolean; @@ -773,6 +774,7 @@ export type Experiment = | "example" | "notifications" | "web-push" + | "workspace-prebuilds" | "workspace-usage"; // From codersdk/deployment.go @@ -887,6 +889,7 @@ export type FeatureName = | "user_limit" | "user_role_management" | "workspace_batch_actions" + | "workspace_prebuilds" | "workspace_proxy"; export const FeatureNames: FeatureName[] = [ @@ -907,6 +910,7 @@ export const FeatureNames: FeatureName[] = [ "user_limit", "user_role_management", "workspace_batch_actions", + "workspace_prebuilds", "workspace_proxy", ]; From 9ea19df31f84e3a3390d1d38d77f6ad98473bb1b Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 23 Apr 2025 11:39:45 +0200 Subject: [PATCH 2/8] Improvements & tests Signed-off-by: Danny Kopping # Conflicts: # coderd/prebuilds/api.go # coderd/prebuilds/noop.go --- cli/testdata/server-config.yaml.golden | 5 +- coderd/apidoc/docs.go | 8 +-- coderd/apidoc/swagger.json | 8 +-- coderd/coderd.go | 17 +++-- coderd/prebuilds/noop.go | 31 +++------ codersdk/deployment.go | 12 ++-- docs/reference/api/schemas.md | 2 +- enterprise/coderd/coderd.go | 28 +++----- enterprise/coderd/coderd_test.go | 91 +++++++++++++++++++++++++- site/src/api/typesGenerated.ts | 2 +- 10 files changed, 141 insertions(+), 63 deletions(-) diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index e0ea86ef6432d..8f34ee8cbe7be 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -693,10 +693,11 @@ workspace_prebuilds: # How often to reconcile workspace prebuilds state. # (default: 15s, type: duration) reconciliation_interval: 15s - # Interval to increase reconciliation backoff by when unrecoverable errors occur. + # Interval to increase reconciliation backoff by when prebuilds fail, after which + # a retry attempt is made. # (default: 15s, type: duration) reconciliation_backoff_interval: 15s - # Interval to look back to determine number of failed builds, which influences + # Interval to look back to determine number of failed prebuilds, which influences # backoff. # (default: 1h0m0s, type: duration) reconciliation_backoff_lookback_period: 1h0m0s diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index c7c0eb3c40d65..daef10a90d422 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -12007,9 +12007,9 @@ const docTemplate = `{ "auto-fill-parameters", "notifications", "workspace-usage", - "workspace-prebuilds", "web-push", - "dynamic-parameters" + "dynamic-parameters", + "workspace-prebuilds" ], "x-enum-comments": { "ExperimentAutoFillParameters": "This should not be taken out of experiments until we have redesigned the feature.", @@ -12025,9 +12025,9 @@ const docTemplate = `{ "ExperimentAutoFillParameters", "ExperimentNotifications", "ExperimentWorkspaceUsage", - "ExperimentWorkspacePrebuilds", "ExperimentWebPush", - "ExperimentDynamicParameters" + "ExperimentDynamicParameters", + "ExperimentWorkspacePrebuilds" ] }, "codersdk.ExternalAuth": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index db0734eaefd72..3a7bc4c2c71ed 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10761,9 +10761,9 @@ "auto-fill-parameters", "notifications", "workspace-usage", - "workspace-prebuilds", "web-push", - "dynamic-parameters" + "dynamic-parameters", + "workspace-prebuilds" ], "x-enum-comments": { "ExperimentAutoFillParameters": "This should not be taken out of experiments until we have redesigned the feature.", @@ -10779,9 +10779,9 @@ "ExperimentAutoFillParameters", "ExperimentNotifications", "ExperimentWorkspaceUsage", - "ExperimentWorkspacePrebuilds", "ExperimentWebPush", - "ExperimentDynamicParameters" + "ExperimentDynamicParameters", + "ExperimentWorkspacePrebuilds" ] }, "codersdk.ExternalAuth": { diff --git a/coderd/coderd.go b/coderd/coderd.go index 4a9e3e61d9cf5..a29a9c74dc72f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -597,6 +597,7 @@ func New(options *Options) *API { api.AppearanceFetcher.Store(&f) api.PortSharer.Store(&portsharing.DefaultPortSharer) api.PrebuildsClaimer.Store(&prebuilds.DefaultClaimer) + api.PrebuildsReconciler.Store(&prebuilds.DefaultReconciler) buildInfo := codersdk.BuildInfoResponse{ ExternalURL: buildinfo.ExternalURL(), Version: buildinfo.Version(), @@ -1568,10 +1569,11 @@ type API struct { DERPMapper atomic.Pointer[func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap] // AccessControlStore is a pointer to an atomic pointer since it is // passed to dbauthz. - AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] - PortSharer atomic.Pointer[portsharing.PortSharer] - FileCache files.Cache - PrebuildsClaimer atomic.Pointer[prebuilds.Claimer] + AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] + PortSharer atomic.Pointer[portsharing.PortSharer] + FileCache files.Cache + PrebuildsClaimer atomic.Pointer[prebuilds.Claimer] + PrebuildsReconciler atomic.Pointer[prebuilds.ReconciliationOrchestrator] UpdatesProvider tailnet.WorkspaceUpdatesProvider @@ -1659,6 +1661,13 @@ func (api *API) Close() error { _ = api.AppSigningKeyCache.Close() _ = api.AppEncryptionKeyCache.Close() _ = api.UpdatesProvider.Close() + + if current := api.PrebuildsReconciler.Load(); current != nil { + ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) + defer giveUp() + (*current).Stop(ctx, nil) + } + return nil } diff --git a/coderd/prebuilds/noop.go b/coderd/prebuilds/noop.go index d122a61ebb9c6..2a617b43bdfc0 100644 --- a/coderd/prebuilds/noop.go +++ b/coderd/prebuilds/noop.go @@ -10,41 +10,28 @@ import ( type NoopReconciler struct{} -func NewNoopReconciler() *NoopReconciler { - return &NoopReconciler{} -} - -func (NoopReconciler) RunLoop(context.Context) {} - -func (NoopReconciler) Stop(context.Context, error) {} - -func (NoopReconciler) ReconcileAll(context.Context) error { - return nil -} - +func (NoopReconciler) RunLoop(context.Context) {} +func (NoopReconciler) Stop(context.Context, error) {} +func (NoopReconciler) ReconcileAll(context.Context) error { return nil } func (NoopReconciler) SnapshotState(context.Context, database.Store) (*GlobalSnapshot, error) { return &GlobalSnapshot{}, nil } - -func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error { - return nil -} - +func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error { return nil } func (NoopReconciler) CalculateActions(context.Context, PresetSnapshot) (*ReconciliationActions, error) { return &ReconciliationActions{}, nil } -var _ ReconciliationOrchestrator = NoopReconciler{} +var DefaultReconciler ReconciliationOrchestrator = NoopReconciler{} -type AGPLPrebuildClaimer struct{} +type NoopClaimer struct{} -func (AGPLPrebuildClaimer) Claim(context.Context, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) { +func (NoopClaimer) Claim(context.Context, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) { // Not entitled to claim prebuilds in AGPL version. return nil, ErrNoClaimablePrebuiltWorkspaces } -func (AGPLPrebuildClaimer) Initiator() uuid.UUID { +func (NoopClaimer) Initiator() uuid.UUID { return uuid.Nil } -var DefaultClaimer Claimer = AGPLPrebuildClaimer{} +var DefaultClaimer Claimer = NoopClaimer{} diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 46c804c1b296d..ec0f2851b9f2e 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -396,8 +396,8 @@ type DeploymentValues struct { TermsOfServiceURL serpent.String `json:"terms_of_service_url,omitempty" typescript:",notnull"` Notifications NotificationsConfig `json:"notifications,omitempty" typescript:",notnull"` AdditionalCSPPolicy serpent.StringArray `json:"additional_csp_policy,omitempty" typescript:",notnull"` - Prebuilds PrebuildsConfig `json:"workspace_prebuilds,omitempty" typescript:",notnull"` WorkspaceHostnameSuffix serpent.String `json:"workspace_hostname_suffix,omitempty" typescript:",notnull"` + Prebuilds PrebuildsConfig `json:"workspace_prebuilds,omitempty" typescript:",notnull"` Config serpent.YAMLConfigPath `json:"config,omitempty" typescript:",notnull"` WriteConfig serpent.Bool `json:"write_config,omitempty" typescript:",notnull"` @@ -3038,6 +3038,9 @@ Write out the current server config as YAML to stdout.`, Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), Hidden: true, // Hidden because most operators should not need to modify this. }, + // Push notifications. + + // Workspace Prebuilds Options { Name: "Reconciliation Interval", Description: "How often to reconcile workspace prebuilds state.", @@ -3051,7 +3054,7 @@ Write out the current server config as YAML to stdout.`, }, { Name: "Reconciliation Backoff Interval", - Description: "Interval to increase reconciliation backoff by when unrecoverable errors occur.", + Description: "Interval to increase reconciliation backoff by when prebuilds fail, after which a retry attempt is made.", Flag: "workspace-prebuilds-reconciliation-backoff-interval", Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_INTERVAL", Value: &c.Prebuilds.ReconciliationBackoffInterval, @@ -3063,7 +3066,7 @@ Write out the current server config as YAML to stdout.`, }, { Name: "Reconciliation Backoff Lookback Period", - Description: "Interval to look back to determine number of failed builds, which influences backoff.", + Description: "Interval to look back to determine number of failed prebuilds, which influences backoff.", Flag: "workspace-prebuilds-reconciliation-backoff-lookback-period", Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_LOOKBACK_PERIOD", Value: &c.Prebuilds.ReconciliationBackoffLookback, @@ -3073,7 +3076,6 @@ Write out the current server config as YAML to stdout.`, Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), Hidden: true, }, - // Push notifications. } return opts @@ -3298,9 +3300,9 @@ const ( ExperimentAutoFillParameters Experiment = "auto-fill-parameters" // This should not be taken out of experiments until we have redesigned the feature. ExperimentNotifications Experiment = "notifications" // Sends notifications via SMTP and webhooks following certain events. ExperimentWorkspaceUsage Experiment = "workspace-usage" // Enables the new workspace usage tracking. - ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds" // Enables the new workspace prebuilds feature. ExperimentWebPush Experiment = "web-push" // Enables web push notifications through the browser. ExperimentDynamicParameters Experiment = "dynamic-parameters" // Enables dynamic parameters when creating a workspace. + ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds" // Enables the new workspace prebuilds feature. ) // ExperimentsSafe should include all experiments that are safe for diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 2389c7db33f64..e2ba1373aa613 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2826,9 +2826,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `auto-fill-parameters` | | `notifications` | | `workspace-usage` | -| `workspace-prebuilds` | | `web-push` | | `dynamic-parameters` | +| `workspace-prebuilds` | ## codersdk.ExternalAuth diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 4ac24f0139b83..14c031b87e268 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -632,8 +632,6 @@ type API struct { licenseMetricsCollector *license.MetricsCollector tailnetService *tailnet.ClientService - - PrebuildsReconciler agplprebuilds.ReconciliationOrchestrator } // writeEntitlementWarningsHeader writes the entitlement warnings to the response header @@ -665,12 +663,6 @@ func (api *API) Close() error { api.Options.CheckInactiveUsersCancelFunc() } - if api.PrebuildsReconciler != nil { - ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) - defer giveUp() - api.PrebuildsReconciler.Stop(ctx, xerrors.New("api closed")) // TODO: determine root cause (requires changes up the stack, though). - } - return api.AGPL.Close() } @@ -873,15 +865,15 @@ func (api *API) updateEntitlements(ctx context.Context) error { api.AGPL.PortSharer.Store(&ps) } - if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) || api.PrebuildsReconciler == nil { + if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) { reconciler, claimer := api.setupPrebuilds(enabled) - if api.PrebuildsReconciler != nil { + if current := api.AGPL.PrebuildsReconciler.Load(); current != nil { stopCtx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) defer giveUp() - api.PrebuildsReconciler.Stop(stopCtx, xerrors.New("entitlements change")) + (*current).Stop(stopCtx, xerrors.New("entitlements change")) } - api.PrebuildsReconciler = reconciler + api.AGPL.PrebuildsReconciler.Store(&reconciler) go reconciler.RunLoop(context.Background()) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -1156,17 +1148,17 @@ func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Obj return api.AGPL.HTTPAuth.Authorize(r, action, object) } -func (api *API) setupPrebuilds(entitled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) { - enabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds) - if !enabled || !entitled { +// nolint:revive // featureEnabled is a legit control flag. +func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) { + experimentEnabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds) + if !experimentEnabled || !featureEnabled { api.Logger.Debug(context.Background(), "prebuilds not enabled", - slog.F("experiment_enabled", enabled), slog.F("entitled", entitled)) + slog.F("experiment_enabled", experimentEnabled), slog.F("feature_enabled", featureEnabled)) - return agplprebuilds.NewNoopReconciler(), agplprebuilds.DefaultClaimer + return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer } reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds, api.Logger.Named("prebuilds"), quartz.NewReal()) - return reconciler, prebuilds.EnterpriseClaimer{} } diff --git a/enterprise/coderd/coderd_test.go b/enterprise/coderd/coderd_test.go index 6b872f32591ca..4a3c47e56a671 100644 --- a/enterprise/coderd/coderd_test.go +++ b/enterprise/coderd/coderd_test.go @@ -28,10 +28,15 @@ import ( "github.com/coder/coder/v2/agent" "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/coderd/httpapi" + agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/enterprise/coderd/prebuilds" "github.com/coder/coder/v2/tailnet/tailnettest" + "github.com/coder/retry" + "github.com/coder/serpent" + agplaudit "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" @@ -50,8 +55,6 @@ import ( "github.com/coder/coder/v2/enterprise/dbcrypt" "github.com/coder/coder/v2/enterprise/replicasync" "github.com/coder/coder/v2/testutil" - "github.com/coder/retry" - "github.com/coder/serpent" ) func TestMain(m *testing.M) { @@ -253,6 +256,90 @@ func TestEntitlements_HeaderWarnings(t *testing.T) { }) } +func TestEntitlements_Prebuilds(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + experimentEnabled bool + featureEnabled bool + expectedEnabled bool + }{ + { + name: "Fully enabled", + featureEnabled: true, + experimentEnabled: true, + expectedEnabled: true, + }, + { + name: "Feature disabled", + featureEnabled: false, + experimentEnabled: true, + expectedEnabled: false, + }, + { + name: "Experiment disabled", + featureEnabled: true, + experimentEnabled: false, + expectedEnabled: false, + }, + { + name: "Fully disabled", + featureEnabled: false, + experimentEnabled: false, + expectedEnabled: false, + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var prebuildsEntitled int64 + if tc.featureEnabled { + prebuildsEntitled = 1 + } + + _, _, api, _ := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: coderdtest.DeploymentValues(t, func(values *codersdk.DeploymentValues) { + if tc.experimentEnabled { + values.Experiments = serpent.StringArray{string(codersdk.ExperimentWorkspacePrebuilds)} + } + }), + }, + + EntitlementsUpdateInterval: time.Second, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureWorkspacePrebuilds: prebuildsEntitled, + }, + }, + }) + + // The entitlements will need to refresh before the reconciler is set. + require.Eventually(t, func() bool { + return api.AGPL.PrebuildsReconciler.Load() != nil + }, testutil.WaitSuperLong, testutil.IntervalFast) + + reconciler := api.AGPL.PrebuildsReconciler.Load() + claimer := api.AGPL.PrebuildsClaimer.Load() + require.NotNil(t, reconciler) + require.NotNil(t, claimer) + + if tc.expectedEnabled { + require.IsType(t, &prebuilds.StoreReconciler{}, *reconciler) + require.IsType(t, prebuilds.EnterpriseClaimer{}, *claimer) + } else { + require.Equal(t, &agplprebuilds.DefaultReconciler, reconciler) + require.Equal(t, &agplprebuilds.DefaultClaimer, claimer) + } + }) + } +} + func TestAuditLogging(t *testing.T) { t.Parallel() t.Run("Enabled", func(t *testing.T) { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 8f69954682032..634c2da3f2bb1 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -690,8 +690,8 @@ export interface DeploymentValues { readonly terms_of_service_url?: string; readonly notifications?: NotificationsConfig; readonly additional_csp_policy?: string; - readonly workspace_prebuilds?: PrebuildsConfig; readonly workspace_hostname_suffix?: string; + readonly workspace_prebuilds?: PrebuildsConfig; readonly config?: string; readonly write_config?: boolean; readonly address?: string; From 0407a16deaa3b46baf4458f4da7f4f6708af3449 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 23 Apr 2025 13:59:45 +0200 Subject: [PATCH 3/8] Apply suggestions from code review Co-authored-by: Cian Johnston --- coderd/coderd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index a29a9c74dc72f..288671c6cb6e9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1663,7 +1663,7 @@ func (api *API) Close() error { _ = api.UpdatesProvider.Close() if current := api.PrebuildsReconciler.Load(); current != nil { - ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop")) + ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop before shutdown")) defer giveUp() (*current).Stop(ctx, nil) } From a8e1251fc1f3416817b3a4ebfddae2f611e48470 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 23 Apr 2025 14:01:38 +0200 Subject: [PATCH 4/8] Review comments Signed-off-by: Danny Kopping --- codersdk/deployment.go | 1 - enterprise/coderd/coderd.go | 8 +++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index ec0f2851b9f2e..d3613a6cf396c 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -3038,7 +3038,6 @@ Write out the current server config as YAML to stdout.`, Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), Hidden: true, // Hidden because most operators should not need to modify this. }, - // Push notifications. // Workspace Prebuilds Options { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 14c031b87e268..164d12b0bd87f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -1152,7 +1152,13 @@ func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Obj func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) { experimentEnabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds) if !experimentEnabled || !featureEnabled { - api.Logger.Debug(context.Background(), "prebuilds not enabled", + levelFn := api.Logger.Debug + // If the experiment is enabled but the license does not entitle the feature, operators should be warned. + if !featureEnabled { + levelFn = api.Logger.Warn + } + + levelFn(context.Background(), "prebuilds not enabled", slog.F("experiment_enabled", experimentEnabled), slog.F("feature_enabled", featureEnabled)) return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer From 4133ac0df3214928005b7abc285b503271c12182 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 23 Apr 2025 14:12:31 +0200 Subject: [PATCH 5/8] Making it clearer Signed-off-by: Danny Kopping --- enterprise/coderd/coderd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 164d12b0bd87f..1f2258a0aff12 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -1158,7 +1158,7 @@ func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.Reconciliatio levelFn = api.Logger.Warn } - levelFn(context.Background(), "prebuilds not enabled", + levelFn(context.Background(), "prebuilds not enabled; ensure you have a premium license and the 'workspace-prebuilds' experiment set", slog.F("experiment_enabled", experimentEnabled), slog.F("feature_enabled", featureEnabled)) return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer From a394838981b07841a2dfc38f39461dcf5c4bd856 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 24 Apr 2025 09:21:30 +0200 Subject: [PATCH 6/8] Hide CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL while experimental Signed-off-by: Danny Kopping --- codersdk/deployment.go | 1 + 1 file changed, 1 insertion(+) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index d3613a6cf396c..154d7f6cb92e4 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -3050,6 +3050,7 @@ Write out the current server config as YAML to stdout.`, Group: &deploymentGroupPrebuilds, YAML: "reconciliation_interval", Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), + Hidden: ExperimentsSafe.Enabled(ExperimentWorkspacePrebuilds), // Hide setting while this feature is experimental. }, { Name: "Reconciliation Backoff Interval", From 61b82ab56ff291dc0778b4956c5f1a24ea3e7207 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 24 Apr 2025 09:26:58 +0200 Subject: [PATCH 7/8] make gen Signed-off-by: Danny Kopping --- cli/testdata/coder_server_--help.golden | 6 ------ docs/reference/cli/server.md | 11 ----------- enterprise/cli/testdata/coder_server_--help.golden | 6 ------ 3 files changed, 23 deletions(-) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 817093c17a9b0..1cefe8767f3b0 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -670,12 +670,6 @@ workspaces stopping during the day due to template scheduling. must be *. Only one hour and minute can be specified (ranges or comma separated values are not supported). -WORKSPACE PREBUILDS OPTIONS: -Configure how workspace prebuilds behave. - - --workspace-prebuilds-reconciliation-interval duration, $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL (default: 15s) - How often to reconcile workspace prebuilds state. - ⚠️ DANGEROUS OPTIONS: --dangerous-allow-path-app-sharing bool, $CODER_DANGEROUS_ALLOW_PATH_APP_SHARING Allow workspace apps that are not served from subdomains to be shared. diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index c01baa45a3f69..1b4052e335e66 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -1603,14 +1603,3 @@ Enable Coder Inbox. | Default | 5 | The upper limit of attempts to send a notification. - -### --workspace-prebuilds-reconciliation-interval - -| | | -|-------------|-----------------------------------------------------------------| -| Type | duration | -| Environment | $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL | -| YAML | workspace_prebuilds.reconciliation_interval | -| Default | 15s | - -How often to reconcile workspace prebuilds state. diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index 344df781c3b7e..d11304742d974 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -671,12 +671,6 @@ workspaces stopping during the day due to template scheduling. must be *. Only one hour and minute can be specified (ranges or comma separated values are not supported). -WORKSPACE PREBUILDS OPTIONS: -Configure how workspace prebuilds behave. - - --workspace-prebuilds-reconciliation-interval duration, $CODER_WORKSPACE_PREBUILDS_RECONCILIATION_INTERVAL (default: 15s) - How often to reconcile workspace prebuilds state. - ⚠️ DANGEROUS OPTIONS: --dangerous-allow-path-app-sharing bool, $CODER_DANGEROUS_ALLOW_PATH_APP_SHARING Allow workspace apps that are not served from subdomains to be shared. From 9676b658d67b92e629223565ac1db0898b1c3a22 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 24 Apr 2025 11:57:59 +0200 Subject: [PATCH 8/8] Protect race on cancelFn by using running/stopped atomic bools Signed-off-by: Danny Kopping --- coderd/prebuilds/api.go | 4 +-- coderd/prebuilds/noop.go | 2 +- enterprise/coderd/coderd.go | 2 +- enterprise/coderd/prebuilds/reconcile.go | 26 ++++++++++++++----- enterprise/coderd/prebuilds/reconcile_test.go | 6 ++--- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/coderd/prebuilds/api.go b/coderd/prebuilds/api.go index ebc4c39c89b50..ba174d318d5fa 100644 --- a/coderd/prebuilds/api.go +++ b/coderd/prebuilds/api.go @@ -14,10 +14,10 @@ var ErrNoClaimablePrebuiltWorkspaces = xerrors.New("no claimable prebuilt worksp type ReconciliationOrchestrator interface { Reconciler - // RunLoop starts a continuous reconciliation loop that periodically calls ReconcileAll + // Run starts a continuous reconciliation loop that periodically calls ReconcileAll // to ensure all prebuilds are in their desired states. The loop runs until the context // is canceled or Stop is called. - RunLoop(ctx context.Context) + Run(ctx context.Context) // Stop gracefully shuts down the orchestrator with the given cause. // The cause is used for logging and error reporting. diff --git a/coderd/prebuilds/noop.go b/coderd/prebuilds/noop.go index 2a617b43bdfc0..e3dc0597b169b 100644 --- a/coderd/prebuilds/noop.go +++ b/coderd/prebuilds/noop.go @@ -10,7 +10,7 @@ import ( type NoopReconciler struct{} -func (NoopReconciler) RunLoop(context.Context) {} +func (NoopReconciler) Run(context.Context) {} func (NoopReconciler) Stop(context.Context, error) {} func (NoopReconciler) ReconcileAll(context.Context) error { return nil } func (NoopReconciler) SnapshotState(context.Context, database.Store) (*GlobalSnapshot, error) { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 1f2258a0aff12..1f468997ac220 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -874,7 +874,7 @@ func (api *API) updateEntitlements(ctx context.Context) error { } api.AGPL.PrebuildsReconciler.Store(&reconciler) - go reconciler.RunLoop(context.Background()) + go reconciler.Run(context.Background()) api.AGPL.PrebuildsClaimer.Store(&claimer) } diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index f74e019207c18..081b4223a93c4 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -38,6 +38,7 @@ type StoreReconciler struct { clock quartz.Clock cancelFn context.CancelCauseFunc + running atomic.Bool stopped atomic.Bool done chan struct{} } @@ -61,7 +62,7 @@ func NewStoreReconciler( } } -func (c *StoreReconciler) RunLoop(ctx context.Context) { +func (c *StoreReconciler) Run(ctx context.Context) { reconciliationInterval := c.cfg.ReconciliationInterval.Value() if reconciliationInterval <= 0 { // avoids a panic reconciliationInterval = 5 * time.Minute @@ -82,6 +83,11 @@ func (c *StoreReconciler) RunLoop(ctx context.Context) { ctx, cancel := context.WithCancelCause(dbauthz.AsPrebuildsOrchestrator(ctx)) c.cancelFn = cancel + // Everything is in place, reconciler can now be considered as running. + // + // NOTE: without this atomic bool, Stop might race with Run for the c.cancelFn above. + c.running.Store(true) + for { select { // TODO: implement pubsub listener to allow reconciling a specific template imperatively once it has been changed, @@ -107,16 +113,26 @@ func (c *StoreReconciler) RunLoop(ctx context.Context) { } func (c *StoreReconciler) Stop(ctx context.Context, cause error) { + defer c.running.Store(false) + if cause != nil { c.logger.Error(context.Background(), "stopping reconciler due to an error", slog.Error(cause)) } else { c.logger.Info(context.Background(), "gracefully stopping reconciler") } - if c.isStopped() { + // If previously stopped (Swap returns previous value), then short-circuit. + // + // NOTE: we need to *prospectively* mark this as stopped to prevent Stop being called multiple times and causing problems. + if c.stopped.Swap(true) { + return + } + + // If the reconciler is not running, there's nothing else to do. + if !c.running.Load() { return } - c.stopped.Store(true) + if c.cancelFn != nil { c.cancelFn(cause) } @@ -138,10 +154,6 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { } } -func (c *StoreReconciler) isStopped() bool { - return c.stopped.Load() -} - // ReconcileAll will attempt to resolve the desired vs actual state of all templates which have presets with prebuilds configured. // // NOTE: diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index a5bd4a728a4ea..5c1ffe993ec42 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -575,7 +575,7 @@ func TestRunLoop(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock) + reconciler := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -639,7 +639,7 @@ func TestRunLoop(t *testing.T) { // we need to wait until ticker is initialized, and only then use clock.Advance() // otherwise clock.Advance() will be ignored trap := clock.Trap().NewTicker() - go controller.RunLoop(ctx) + go reconciler.Run(ctx) // wait until ticker is initialized trap.MustWait(ctx).Release() // start 1st iteration of ReconciliationLoop @@ -681,7 +681,7 @@ func TestRunLoop(t *testing.T) { }, testutil.WaitShort, testutil.IntervalFast) // gracefully stop the reconciliation loop - controller.Stop(ctx, nil) + reconciler.Stop(ctx, nil) } func TestFailedBuildBackoff(t *testing.T) {