Skip to content

Commit b56243c

Browse files
authored
🤖 feat: hold CoderWorkspacePool members at a lowered owner cap, and keep the failure run across a rescale (#379)
## Summary This PR closes Codex security finding 4271920249 from #359, and it keeps the failure run of a `CoderWorkspacePool` across a rescale. Both changes are in the dormant pool and member reconcilers. Nothing registers them yet, so no user sees a change. 1. **A lowered `maxWorkspaces` holds.** When an admin lowers the owner's `maxWorkspaces`, the pool can already have members that it created under the old cap and that have not sent their create request yet. Before this PR, those members still created workspaces above the new cap. Now: - The member checks the cap again right before it writes its create marker, after all its Coder reads, on a new uncached read of the control plane. It lists the namespace's members without the cache and counts the owner's other members on the same control plane that may have a workspace (a create marker or a workspace ID) and whose workspace is not deleted. At the cap, it waits with `OwnerWorkspaceLimit` and sends no create request. - The pool deletes its own members that sent no create request yet, newest first, while the owner has more live members than the cap allows. Otherwise these members would wait at their cap check until their deadline fails them, and each such failure would count toward the failure limit. Each delete carries the listed UID and `resourceVersion`, so a member that wrote its marker after the list answers 409 and stays. 2. **A rescale keeps the failure run.** Before this PR, any spec change (`metadata.generation`) started a new run of failures. So a pool that an HPA rescales through `/scale` could get past the 5-failure limit. Now only a change of `status.resolved` starts a new run: the template, the version, the preset, the parameters, the owner, or `readyTimeoutSeconds`. A change of `replicas` or `maxInFlight` keeps the run. `maxWorkspaces` still caps creation either way. Refs #153 ## Background This is PR 10a of the `CoderWorkspacePool` stack (#153). PR 10b (delete order, new before old, used-member release, consent removal) stacks on this branch. Decision on the run reset: the maintainer asked that a replicas-only change does not reset the run, and that only input changes that can fix a failure do. I added `readyTimeoutSeconds` to the list, because a longer timeout can fix a `DeadlineExceeded` failure. Without it, a pool that stalled on deadline failures could only leave `MemberFailureLimit` through a template, version, or parameter change. To detect that change without a stored copy of the spec, `status.resolved` now holds `readyTimeoutSeconds`, and new members pin it from there. The member input check does not compare it, so a timeout change does not make ready members outdated. The first resolve (no stored `status.resolved`) does not start a run. This also removes the case where the failure write before the deletes stores the run but not `observedGeneration`: the reset no longer reads `observedGeneration`. I recorded the remaining effect (that write publishes an old `observedGeneration` until the final write) in the plan for PR 12, as asked. API: one new optional status field, `status.resolved.readyTimeoutSeconds`. The CRD is still in `config/crd/dormant/`, and the pool has no API reference page yet, so `make docs-reference` changes nothing. ## Validation Red, then green. I wrote the two tests first and ran them on `main` (`fb42486a`): ```text --- FAIL: TestPoolFailureRunSurvivesRescale coderworkspacepool_failure_test.go:223: a rescale keeps the run (expected: 1, actual: 0) --- FAIL: TestPoolLoweredOwnerCap coderworkspacepool_cap_test.go:44: one workspace at maxWorkspaces 1 (expected: 1, actual: 3) ``` With this PR, both pass. - `TestPoolLoweredOwnerCap` (new, `coderworkspacepool_cap_test.go`): 3 pending members, then `maxWorkspaces` goes from 10 to 1. Every member reconciles before the pool sees the new cap: Coder gets exactly 1 workspace, and the other two wait with `OwnerWorkspaceLimit` without a marker. The pool then deletes exactly those two, keeps the member with the workspace, and creates no new member (`OwnerWorkspaceLimit`). - `TestPoolFailureRunSurvivesRescale` (new): after one failure, a `/scale` write to 2 replicas, a `maxInFlight` change, and a new name of the same version keep `consecutiveFailures: 1` and the backoff. A `readyTimeoutSeconds` change starts a new run, a failure of a member with the old timeout does not count in it, and the new members pin the new timeout. - Changed tests: `TestPoolFailureBackoffAndLimit` and `TestPoolFailureRunResets` now lift the limit with a `readyTimeoutSeconds` change instead of a `maxInFlight` change. `TestPoolResolvesInputs` expects the new field. Codex review round 1 (all fixed in `ee015d1b`): - 4274233487: a deleting member that sent no create request no longer counts as live, so the next reconcile deletes no more members (`TestPoolLoweredOwnerCapDeletesOnce`). - 4274233497: a new name of the same version no longer starts a new run (`poolRunInputsChanged` ignores `templateVersionName`). - 4274233505: a failed member with an old `readyTimeoutSeconds` no longer counts in the new run. It is still deleted. Codex review round 2 (fixed in `007545e9`): - 4274565367: the member's cap check now runs last, after all Coder reads, on a new uncached control plane read (deletion, consent, and cap). `TestPoolMemberRechecksCapLast` answers the first control plane read with the old cap and failed on the previous code. - For 4274557698 on #380: the pool now reads the control plane without the cache in every reconcile that resolved its inputs (`freshOwnerCap`), before the cap trim and the creates, whatever the demand. Before, a pool that needed no create skipped that read. Codex review round 3 (fixed in `4612ad65`): - 4274801753: while the pool's inputs do not resolve (any wait or stall of the resolve step), failed members are deleted without counting. A bad spec edit then leaves no failures in the run, and a revert to the same inputs needs no reset (`TestPoolFailuresUncountedWhileUnresolved`). Trade-off: failures during a Coder outage do not count either. - 4274801768: the `MemberFailureLimit` message names only inputs that a user can change (`spec.template` is immutable). Mutation checks: I removed one control at a time, and each removal made a named test fail (10 of 10): the member's cap check, the pool's deletes above the cap, keeping members with a workspace out of those deletes, counting only members that may have a workspace in the member's check, the reset on a generation change, the resolved timeout, not counting deleting pending members as live, ignoring a version rename, not counting failures at an old timeout, and not counting failures while the inputs do not resolve. The final cap read has a red run instead: `TestPoolMemberRechecksCapLast` failed on the code before that fix. Gates on the final commit: drift (codegen, manifests, installer, API reference: no diff), `make docs-check`, dormant-CRD guard, docs lint and spelling, `make verify-vendor`, `make build`, `make lint`, actionlint, `make test`, `make test-integration`, `make vuln`, race. All passed. ## Size 468 hand-written lines (230 production, 238 test, counting removals). Generated: 15 lines of the dormant CRD. ## Risks None for users. The reconcilers are dormant. One limit, recorded in the plan for PR 12: the member's cap check is exact while member reconciles run one at a time (the controller-runtime default). With parallel member reconciles, two members can pass the check together. --- _Generated with `mux` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$154.26`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=154.26 --> --------- Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent d56051f commit b56243c

9 files changed

Lines changed: 420 additions & 63 deletions

‎api/v1alpha1/coderworkspacepool_types.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,10 @@ type CoderWorkspacePoolResolved struct {
232232
// current members from outdated ones without storing values twice.
233233
// +optional
234234
ParametersHash string `json:"parametersHash,omitempty"`
235+
// ReadyTimeoutSeconds is spec.readyTimeoutSeconds. A change of any of
236+
// these values starts a new run of failures, because it can fix one.
237+
// +optional
238+
ReadyTimeoutSeconds int32 `json:"readyTimeoutSeconds,omitempty"`
235239
// ResolvedTime is when the controller last changed these values.
236240
// +optional
237241
ResolvedTime *metav1.Time `json:"resolvedTime,omitempty"`
@@ -293,16 +297,17 @@ type CoderWorkspacePoolStatus struct {
293297
// +optional
294298
Resolved *CoderWorkspacePoolResolved `json:"resolved,omitempty"`
295299
// ConsecutiveFailures is the number of member failures in a row: the
296-
// length of RunFailures. A ready member, a spec change, and a new
297-
// version reset it.
300+
// length of RunFailures. A ready member and a change of status.resolved
301+
// (template, version, preset, parameters, owner, or readyTimeoutSeconds)
302+
// reset it. A change of replicas or maxInFlight does not.
298303
// +optional
299304
ConsecutiveFailures int32 `json:"consecutiveFailures,omitempty"`
300305
// LastFailure is the newest member failure.
301306
// +optional
302307
LastFailure *CoderWorkspacePoolFailure `json:"lastFailure,omitempty"`
303308
// FailureRunStart is the start of the current run of failures: the
304-
// latest of the last spec or version change and the newest ready time
305-
// of a member. Failures at or before it do not count.
309+
// latest of the last change of status.resolved and the newest ready
310+
// time of a member. Failures at or before it do not count.
306311
// +optional
307312
FailureRunStart *metav1.Time `json:"failureRunStart,omitempty"`
308313
// RunFailures are the member failures of the current run, oldest first,

‎config/crd/dormant/coder.com_coderworkspacepools.yaml‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,15 +307,16 @@ spec:
307307
consecutiveFailures:
308308
description: |-
309309
ConsecutiveFailures is the number of member failures in a row: the
310-
length of RunFailures. A ready member, a spec change, and a new
311-
version reset it.
310+
length of RunFailures. A ready member and a change of status.resolved
311+
(template, version, preset, parameters, owner, or readyTimeoutSeconds)
312+
reset it. A change of replicas or maxInFlight does not.
312313
format: int32
313314
type: integer
314315
failureRunStart:
315316
description: |-
316317
FailureRunStart is the start of the current run of failures: the
317-
latest of the last spec or version change and the newest ready time
318-
of a member. Failures at or before it do not count.
318+
latest of the last change of status.resolved and the newest ready
319+
time of a member. Failures at or before it do not count.
319320
format: date-time
320321
type: string
321322
lastFailure:
@@ -386,6 +387,12 @@ spec:
386387
presetID:
387388
description: PresetID is the UUID of the preset named by spec.presetName.
388389
type: string
390+
readyTimeoutSeconds:
391+
description: |-
392+
ReadyTimeoutSeconds is spec.readyTimeoutSeconds. A change of any of
393+
these values starts a new run of failures, because it can fix one.
394+
format: int32
395+
type: integer
389396
resolvedTime:
390397
description: ResolvedTime is when the controller last changed
391398
these values.
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
package controller_test
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
"k8s.io/apimachinery/pkg/types"
9+
ctrl "sigs.k8s.io/controller-runtime"
10+
"sigs.k8s.io/controller-runtime/pkg/client"
11+
12+
coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
13+
"github.com/coder/coder-k8s/internal/controller"
14+
)
15+
16+
// setOwnerCap sets maxWorkspaces of the control plane's only owner entry.
17+
func (e *poolMemberEnv) setOwnerCap(t *testing.T, maxWorkspaces int32) {
18+
t.Helper()
19+
cp := &coderv1alpha1.CoderControlPlane{}
20+
require.NoError(t, k8sClient.Get(e.ctx, types.NamespacedName{Namespace: e.ns, Name: "coder"}, cp))
21+
cp.Spec.WorkspacePools.Owners[0].MaxWorkspaces = &maxWorkspaces
22+
require.NoError(t, k8sClient.Update(e.ctx, cp))
23+
}
24+
25+
// gone reports whether the member no longer exists.
26+
func (e *poolMemberEnv) gone(t *testing.T, key types.NamespacedName) bool {
27+
t.Helper()
28+
return e.reconcileMember(t, key, 0) == nil
29+
}
30+
31+
// TestPoolLoweredOwnerCap: an admin lowers maxWorkspaces while members of
32+
// the pool wait for their create (Codex 4271920249). No member creates a
33+
// workspace above the new cap: each member checks the cap again, without
34+
// the cache, right before its create marker. The pool then deletes the
35+
// members that sent no create request, down to the cap.
36+
func TestPoolLoweredOwnerCap(t *testing.T) {
37+
t.Parallel()
38+
e := newPoolMemberEnv(t)
39+
e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) {
40+
s.Replicas, s.MaxInFlight = new(int32(3)), new(int32(3))
41+
})
42+
e.reconcilePool(t, 2)
43+
members := e.poolMembers(t, e.pool.Name)
44+
require.Len(t, members, 3)
45+
e.setOwnerCap(t, 1)
46+
47+
// Every member reconciles before the pool sees the new cap.
48+
waiting := map[string]bool{}
49+
for _, m := range members {
50+
got := e.reconcileMember(t, memberKey(m), 2) // The finalizer, then the create.
51+
if got.Status.Reason == "OwnerWorkspaceLimit" {
52+
require.Nil(t, got.Status.CreateAttemptTime, "a waiting member writes no marker")
53+
waiting[got.Name] = true
54+
}
55+
}
56+
require.Equal(t, 1, e.fake.workspaceCount(), "one workspace at maxWorkspaces 1")
57+
require.Len(t, waiting, 2)
58+
59+
// The pool deletes the two members without a create request and keeps
60+
// the member that created its workspace.
61+
e.reconcilePool(t, 1)
62+
for _, m := range e.poolMembers(t, e.pool.Name) {
63+
require.Equal(t, waiting[m.Name], !m.DeletionTimestamp.IsZero(), "member %s", m.Name)
64+
}
65+
requirePool(t, e.reconcilePool(t, 1), "OwnerWorkspaceLimit", false, false)
66+
require.Len(t, e.poolMembers(t, e.pool.Name), 3, "no new member: the deleting members wait for their finalizers")
67+
require.Equal(t, 1, e.fake.workspaceCount())
68+
}
69+
70+
// TestPoolLoweredOwnerCapDeletesOnce: members that the pool deleted above
71+
// the cap count no longer while their finalizers run, so the next reconcile
72+
// deletes no more of them.
73+
func TestPoolLoweredOwnerCapDeletesOnce(t *testing.T) {
74+
t.Parallel()
75+
e := newPoolMemberEnv(t)
76+
e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) {
77+
s.Replicas, s.MaxInFlight = new(int32(3)), new(int32(3))
78+
})
79+
e.reconcilePool(t, 2)
80+
members := e.poolMembers(t, e.pool.Name)
81+
require.Len(t, members, 3)
82+
for _, m := range members {
83+
e.reconcileMember(t, memberKey(m), 1) // The finalizer only.
84+
}
85+
e.setOwnerCap(t, 1)
86+
e.reconcilePool(t, 2)
87+
deleting := 0
88+
for _, m := range e.poolMembers(t, e.pool.Name) {
89+
if !m.DeletionTimestamp.IsZero() {
90+
deleting++
91+
}
92+
}
93+
require.Equal(t, 2, deleting, "one member stays at maxWorkspaces 1")
94+
require.Len(t, e.poolMembers(t, e.pool.Name), 3, "no new member")
95+
}
96+
97+
// firstReadOldCap answers the first control plane read with maxWorkspaces
98+
// 10, as before an admin lowered it, and later reads as stored.
99+
type firstReadOldCap struct {
100+
client.Reader
101+
reads *int
102+
}
103+
104+
func (c firstReadOldCap) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
105+
if err := c.Reader.Get(ctx, key, obj, opts...); err != nil {
106+
return err
107+
}
108+
if cp, ok := obj.(*coderv1alpha1.CoderControlPlane); ok && cp.Spec.WorkspacePools != nil {
109+
if *c.reads++; *c.reads == 1 {
110+
for i := range cp.Spec.WorkspacePools.Owners {
111+
cp.Spec.WorkspacePools.Owners[i].MaxWorkspaces = new(int32(10))
112+
}
113+
}
114+
}
115+
return nil
116+
}
117+
118+
// TestPoolMemberRechecksCapLast: the member reads the cap again after its
119+
// Coder reads, right before the marker, so a cap that an admin lowered
120+
// during those reads holds (Codex 4274565367).
121+
func TestPoolMemberRechecksCapLast(t *testing.T) {
122+
t.Parallel()
123+
e := newPoolMemberEnv(t)
124+
e.setOwnerCap(t, 1)
125+
e.reconcileMember(t, e.createMember(t, nil), 2)
126+
require.Equal(t, 1, e.fake.workspaceCount())
127+
second := e.createMember(t, nil)
128+
r := &controller.CoderWorkspacePoolMemberReconciler{Client: k8sClient, APIReader: firstReadOldCap{k8sClient, new(0)}, Scheme: scheme, Clock: e.clock}
129+
for range 2 { // The finalizer, then the checks.
130+
_, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: second})
131+
require.NoError(t, err)
132+
}
133+
require.Equal(t, 1, e.fake.workspaceCount(), "no workspace above maxWorkspaces 1")
134+
require.Equal(t, "OwnerWorkspaceLimit", e.reconcileMember(t, second, 0).Status.Reason)
135+
}

0 commit comments

Comments
 (0)