Skip to content

Commit 81da6be

Browse files
dannykoppingdannykopping
committed
Improvements & tests
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent 72b6c42 commit 81da6be

File tree

8 files changed

+134
-59
lines changed

8 files changed

+134
-59
lines changed

‎cli/testdata/server-config.yaml.golden

+3-2
Original file line numberDiff line numberDiff line change
@@ -693,10 +693,11 @@ workspace_prebuilds:
693693
# How often to reconcile workspace prebuilds state.
694694
# (default: 15s, type: duration)
695695
reconciliation_interval: 15s
696-
# Interval to increase reconciliation backoff by when unrecoverable errors occur.
696+
# Interval to increase reconciliation backoff by when prebuilds fail, after which
697+
# a retry attempt is made.
697698
# (default: 15s, type: duration)
698699
reconciliation_backoff_interval: 15s
699-
# Interval to look back to determine number of failed builds, which influences
700+
# Interval to look back to determine number of failed prebuilds, which influences
700701
# backoff.
701702
# (default: 1h0m0s, type: duration)
702703
reconciliation_backoff_lookback_period: 1h0m0s

‎coderd/coderd.go

+13-4
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,7 @@ func New(options *Options) *API {
597597
api.AppearanceFetcher.Store(&f)
598598
api.PortSharer.Store(&portsharing.DefaultPortSharer)
599599
api.PrebuildsClaimer.Store(&prebuilds.DefaultClaimer)
600+
api.PrebuildsReconciler.Store(&prebuilds.DefaultReconciler)
600601
buildInfo := codersdk.BuildInfoResponse{
601602
ExternalURL: buildinfo.ExternalURL(),
602603
Version: buildinfo.Version(),
@@ -1568,10 +1569,11 @@ type API struct {
15681569
DERPMapper atomic.Pointer[func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap]
15691570
// AccessControlStore is a pointer to an atomic pointer since it is
15701571
// passed to dbauthz.
1571-
AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore]
1572-
PortSharer atomic.Pointer[portsharing.PortSharer]
1573-
FileCache files.Cache
1574-
PrebuildsClaimer atomic.Pointer[prebuilds.Claimer]
1572+
AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore]
1573+
PortSharer atomic.Pointer[portsharing.PortSharer]
1574+
FileCache files.Cache
1575+
PrebuildsClaimer atomic.Pointer[prebuilds.Claimer]
1576+
PrebuildsReconciler atomic.Pointer[prebuilds.ReconciliationOrchestrator]
15751577

15761578
UpdatesProvider tailnet.WorkspaceUpdatesProvider
15771579

@@ -1659,6 +1661,13 @@ func (api *API) Close() error {
16591661
_ = api.AppSigningKeyCache.Close()
16601662
_ = api.AppEncryptionKeyCache.Close()
16611663
_ = api.UpdatesProvider.Close()
1664+
1665+
if current := api.PrebuildsReconciler.Load(); current != nil {
1666+
ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
1667+
defer giveUp()
1668+
(*current).Stop(ctx, nil)
1669+
}
1670+
16621671
return nil
16631672
}
16641673

‎coderd/prebuilds/api.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ package prebuilds
22

33
import (
44
"context"
5-
"errors"
65

76
"github.com/google/uuid"
7+
"golang.org/x/xerrors"
88

99
"github.com/coder/coder/v2/coderd/database"
1010
)
1111

12-
var ErrNoClaimablePrebuiltWorkspaces = errors.New("no claimable prebuilt workspaces found")
12+
var ErrNoClaimablePrebuiltWorkspaces = xerrors.New("no claimable prebuilt workspaces found")
1313

1414
// ReconciliationOrchestrator manages the lifecycle of prebuild reconciliation.
1515
// It runs a continuous loop to check and reconcile prebuild states, and can be stopped gracefully.

‎coderd/prebuilds/noop.go

+9-25
Original file line numberDiff line numberDiff line change
@@ -10,41 +10,25 @@ import (
1010

1111
type NoopReconciler struct{}
1212

13-
func NewNoopReconciler() *NoopReconciler {
14-
return &NoopReconciler{}
15-
}
16-
17-
func (NoopReconciler) RunLoop(context.Context) {}
18-
19-
func (NoopReconciler) Stop(context.Context, error) {}
20-
21-
func (NoopReconciler) ReconcileAll(context.Context) error {
22-
return nil
23-
}
24-
13+
func (NoopReconciler) RunLoop(context.Context) {}
14+
func (NoopReconciler) Stop(context.Context, error) {}
15+
func (NoopReconciler) ReconcileAll(context.Context) error { return nil }
2516
func (NoopReconciler) SnapshotState(context.Context, database.Store) (*GlobalSnapshot, error) {
2617
return &GlobalSnapshot{}, nil
2718
}
28-
29-
func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error {
30-
return nil
31-
}
32-
19+
func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error { return nil }
3320
func (NoopReconciler) CalculateActions(context.Context, PresetSnapshot) (*ReconciliationActions, error) {
3421
return &ReconciliationActions{}, nil
3522
}
3623

37-
var _ ReconciliationOrchestrator = NoopReconciler{}
24+
var DefaultReconciler ReconciliationOrchestrator = NoopReconciler{}
3825

39-
type AGPLPrebuildClaimer struct{}
26+
type NoopClaimer struct{}
4027

41-
func (c AGPLPrebuildClaimer) Claim(context.Context, database.Store, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) {
28+
func (NoopClaimer) Claim(context.Context, database.Store, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) {
4229
// Not entitled to claim prebuilds in AGPL version.
4330
return nil, ErrNoClaimablePrebuiltWorkspaces
4431
}
32+
func (NoopClaimer) Initiator() uuid.UUID { return uuid.Nil }
4533

46-
func (c AGPLPrebuildClaimer) Initiator() uuid.UUID {
47-
return uuid.Nil
48-
}
49-
50-
var DefaultClaimer Claimer = AGPLPrebuildClaimer{}
34+
var DefaultClaimer Claimer = NoopClaimer{}

‎codersdk/deployment.go

+7-5
Original file line numberDiff line numberDiff line change
@@ -396,8 +396,8 @@ type DeploymentValues struct {
396396
TermsOfServiceURL serpent.String `json:"terms_of_service_url,omitempty" typescript:",notnull"`
397397
Notifications NotificationsConfig `json:"notifications,omitempty" typescript:",notnull"`
398398
AdditionalCSPPolicy serpent.StringArray `json:"additional_csp_policy,omitempty" typescript:",notnull"`
399-
Prebuilds PrebuildsConfig `json:"workspace_prebuilds,omitempty" typescript:",notnull"`
400399
WorkspaceHostnameSuffix serpent.String `json:"workspace_hostname_suffix,omitempty" typescript:",notnull"`
400+
Prebuilds PrebuildsConfig `json:"workspace_prebuilds,omitempty" typescript:",notnull"`
401401

402402
Config serpent.YAMLConfigPath `json:"config,omitempty" typescript:",notnull"`
403403
WriteConfig serpent.Bool `json:"write_config,omitempty" typescript:",notnull"`
@@ -3038,6 +3038,9 @@ Write out the current server config as YAML to stdout.`,
30383038
Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
30393039
Hidden: true, // Hidden because most operators should not need to modify this.
30403040
},
3041+
// Push notifications.
3042+
3043+
// Workspace Prebuilds Options
30413044
{
30423045
Name: "Reconciliation Interval",
30433046
Description: "How often to reconcile workspace prebuilds state.",
@@ -3051,7 +3054,7 @@ Write out the current server config as YAML to stdout.`,
30513054
},
30523055
{
30533056
Name: "Reconciliation Backoff Interval",
3054-
Description: "Interval to increase reconciliation backoff by when unrecoverable errors occur.",
3057+
Description: "Interval to increase reconciliation backoff by when prebuilds fail, after which a retry attempt is made.",
30553058
Flag: "workspace-prebuilds-reconciliation-backoff-interval",
30563059
Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_INTERVAL",
30573060
Value: &c.Prebuilds.ReconciliationBackoffInterval,
@@ -3063,7 +3066,7 @@ Write out the current server config as YAML to stdout.`,
30633066
},
30643067
{
30653068
Name: "Reconciliation Backoff Lookback Period",
3066-
Description: "Interval to look back to determine number of failed builds, which influences backoff.",
3069+
Description: "Interval to look back to determine number of failed prebuilds, which influences backoff.",
30673070
Flag: "workspace-prebuilds-reconciliation-backoff-lookback-period",
30683071
Env: "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_LOOKBACK_PERIOD",
30693072
Value: &c.Prebuilds.ReconciliationBackoffLookback,
@@ -3073,7 +3076,6 @@ Write out the current server config as YAML to stdout.`,
30733076
Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
30743077
Hidden: true,
30753078
},
3076-
// Push notifications.
30773079
}
30783080

30793081
return opts
@@ -3298,9 +3300,9 @@ const (
32983300
ExperimentAutoFillParameters Experiment = "auto-fill-parameters" // This should not be taken out of experiments until we have redesigned the feature.
32993301
ExperimentNotifications Experiment = "notifications" // Sends notifications via SMTP and webhooks following certain events.
33003302
ExperimentWorkspaceUsage Experiment = "workspace-usage" // Enables the new workspace usage tracking.
3301-
ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds" // Enables the new workspace prebuilds feature.
33023303
ExperimentWebPush Experiment = "web-push" // Enables web push notifications through the browser.
33033304
ExperimentDynamicParameters Experiment = "dynamic-parameters" // Enables dynamic parameters when creating a workspace.
3305+
ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds" // Enables the new workspace prebuilds feature.
33043306
)
33053307

33063308
// ExperimentsSafe should include all experiments that are safe for

‎enterprise/coderd/coderd.go

+10-18
Original file line numberDiff line numberDiff line change
@@ -632,8 +632,6 @@ type API struct {
632632

633633
licenseMetricsCollector *license.MetricsCollector
634634
tailnetService *tailnet.ClientService
635-
636-
PrebuildsReconciler agplprebuilds.ReconciliationOrchestrator
637635
}
638636

639637
// writeEntitlementWarningsHeader writes the entitlement warnings to the response header
@@ -665,12 +663,6 @@ func (api *API) Close() error {
665663
api.Options.CheckInactiveUsersCancelFunc()
666664
}
667665

668-
if api.PrebuildsReconciler != nil {
669-
ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
670-
defer giveUp()
671-
api.PrebuildsReconciler.Stop(ctx, xerrors.New("api closed")) // TODO: determine root cause (requires changes up the stack, though).
672-
}
673-
674666
return api.AGPL.Close()
675667
}
676668

@@ -873,15 +865,15 @@ func (api *API) updateEntitlements(ctx context.Context) error {
873865
api.AGPL.PortSharer.Store(&ps)
874866
}
875867

876-
if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) || api.PrebuildsReconciler == nil {
868+
if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) {
877869
reconciler, claimer := api.setupPrebuilds(enabled)
878-
if api.PrebuildsReconciler != nil {
870+
if current := api.AGPL.PrebuildsReconciler.Load(); current != nil {
879871
stopCtx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
880872
defer giveUp()
881-
api.PrebuildsReconciler.Stop(stopCtx, xerrors.New("entitlements change"))
873+
(*current).Stop(stopCtx, xerrors.New("entitlements change"))
882874
}
883875

884-
api.PrebuildsReconciler = reconciler
876+
api.AGPL.PrebuildsReconciler.Store(&reconciler)
885877
go reconciler.RunLoop(context.Background())
886878

887879
api.AGPL.PrebuildsClaimer.Store(&claimer)
@@ -1156,17 +1148,17 @@ func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Obj
11561148
return api.AGPL.HTTPAuth.Authorize(r, action, object)
11571149
}
11581150

1159-
func (api *API) setupPrebuilds(entitled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) {
1160-
enabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds)
1161-
if !enabled || !entitled {
1151+
// nolint:revive // featureEnabled is a legit control flag.
1152+
func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) {
1153+
experimentEnabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds)
1154+
if !experimentEnabled || !featureEnabled {
11621155
api.Logger.Debug(context.Background(), "prebuilds not enabled",
1163-
slog.F("experiment_enabled", enabled), slog.F("entitled", entitled))
1156+
slog.F("experiment_enabled", experimentEnabled), slog.F("feature_enabled", featureEnabled))
11641157

1165-
return agplprebuilds.NewNoopReconciler(), agplprebuilds.DefaultClaimer
1158+
return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer
11661159
}
11671160

11681161
reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds,
11691162
api.Logger.Named("prebuilds"), quartz.NewReal())
1170-
11711163
return reconciler, prebuilds.EnterpriseClaimer{}
11721164
}

‎enterprise/coderd/coderd_test.go

+89-2
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,15 @@ import (
2828
"github.com/coder/coder/v2/agent"
2929
"github.com/coder/coder/v2/agent/agenttest"
3030
"github.com/coder/coder/v2/coderd/httpapi"
31+
agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds"
3132
"github.com/coder/coder/v2/coderd/rbac/policy"
3233
"github.com/coder/coder/v2/coderd/util/ptr"
34+
"github.com/coder/coder/v2/enterprise/coderd/prebuilds"
3335
"github.com/coder/coder/v2/tailnet/tailnettest"
3436

37+
"github.com/coder/retry"
38+
"github.com/coder/serpent"
39+
3540
agplaudit "github.com/coder/coder/v2/coderd/audit"
3641
"github.com/coder/coder/v2/coderd/coderdtest"
3742
"github.com/coder/coder/v2/coderd/database"
@@ -50,8 +55,6 @@ import (
5055
"github.com/coder/coder/v2/enterprise/dbcrypt"
5156
"github.com/coder/coder/v2/enterprise/replicasync"
5257
"github.com/coder/coder/v2/testutil"
53-
"github.com/coder/retry"
54-
"github.com/coder/serpent"
5558
)
5659

5760
func TestMain(m *testing.M) {
@@ -253,6 +256,90 @@ func TestEntitlements_HeaderWarnings(t *testing.T) {
253256
})
254257
}
255258

259+
func TestEntitlements_Prebuilds(t *testing.T) {
260+
t.Parallel()
261+
262+
cases := []struct {
263+
name string
264+
experimentEnabled bool
265+
featureEnabled bool
266+
expectedEnabled bool
267+
}{
268+
{
269+
name: "Fully enabled",
270+
featureEnabled: true,
271+
experimentEnabled: true,
272+
expectedEnabled: true,
273+
},
274+
{
275+
name: "Feature disabled",
276+
featureEnabled: false,
277+
experimentEnabled: true,
278+
expectedEnabled: false,
279+
},
280+
{
281+
name: "Experiment disabled",
282+
featureEnabled: true,
283+
experimentEnabled: false,
284+
expectedEnabled: false,
285+
},
286+
{
287+
name: "Fully disabled",
288+
featureEnabled: false,
289+
experimentEnabled: false,
290+
expectedEnabled: false,
291+
},
292+
}
293+
294+
for _, tc := range cases {
295+
tc := tc
296+
297+
t.Run(tc.name, func(t *testing.T) {
298+
t.Parallel()
299+
300+
var prebuildsEntitled int64
301+
if tc.featureEnabled {
302+
prebuildsEntitled = 1
303+
}
304+
305+
_, _, api, _ := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
306+
Options: &coderdtest.Options{
307+
DeploymentValues: coderdtest.DeploymentValues(t, func(values *codersdk.DeploymentValues) {
308+
if tc.experimentEnabled {
309+
values.Experiments = serpent.StringArray{string(codersdk.ExperimentWorkspacePrebuilds)}
310+
}
311+
}),
312+
},
313+
314+
EntitlementsUpdateInterval: time.Second,
315+
LicenseOptions: &coderdenttest.LicenseOptions{
316+
Features: license.Features{
317+
codersdk.FeatureWorkspacePrebuilds: prebuildsEntitled,
318+
},
319+
},
320+
})
321+
322+
// The entitlements will need to refresh before the reconciler is set.
323+
require.Eventually(t, func() bool {
324+
return api.AGPL.PrebuildsReconciler.Load() != nil
325+
}, testutil.WaitSuperLong, testutil.IntervalFast)
326+
327+
reconciler := api.AGPL.PrebuildsReconciler.Load()
328+
claimer := api.AGPL.PrebuildsClaimer.Load()
329+
require.NotNil(t, reconciler)
330+
require.NotNil(t, claimer)
331+
332+
if tc.expectedEnabled {
333+
require.IsType(t, &prebuilds.StoreReconciler{}, *reconciler)
334+
require.IsType(t, prebuilds.EnterpriseClaimer{}, *claimer)
335+
} else {
336+
require.Equal(t, &agplprebuilds.DefaultReconciler, reconciler)
337+
require.Equal(t, &agplprebuilds.DefaultClaimer, claimer)
338+
}
339+
})
340+
}
341+
}
342+
256343
func TestAuditLogging(t *testing.T) {
257344
t.Parallel()
258345
t.Run("Enabled", func(t *testing.T) {

‎site/src/api/typesGenerated.ts

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)