From 3f5e8707adb56dec9435642b8cc9261e6aa91bea Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 22:41:29 +0000 Subject: [PATCH 1/4] feat: bind CoderWorkspaceClaims to ready CoderWorkspacePool members and write the claim status Change-Id: Ia47cdccaaad51b9a7cfa069a5e5de45de22be757 Signed-off-by: Thomas Kosiewski --- .../controller/coderworkspacepool_claim.go | 173 +++++++++++ .../coderworkspacepool_claim_test.go | 286 ++++++++++++++++++ .../coderworkspacepool_controller.go | 18 +- 3 files changed, 475 insertions(+), 2 deletions(-) create mode 100644 internal/controller/coderworkspacepool_claim.go create mode 100644 internal/controller/coderworkspacepool_claim_test.go diff --git a/internal/controller/coderworkspacepool_claim.go b/internal/controller/coderworkspacepool_claim.go new file mode 100644 index 00000000..b03e1b00 --- /dev/null +++ b/internal/controller/coderworkspacepool_claim.go @@ -0,0 +1,173 @@ +package controller + +import ( + "cmp" + "context" + "fmt" + "slices" + "time" + + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// poolClaims lists the claims of the pool in its namespace, oldest first. +// The list can come from the cache: the member list, which decides the +// binds, is uncached. +func (r *CoderWorkspacePoolReconciler) poolClaims(ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool) ([]coderv1alpha1.CoderWorkspaceClaim, error) { + list := &coderv1alpha1.CoderWorkspaceClaimList{} + if err := r.List(ctx, list, client.InNamespace(pool.Namespace)); err != nil { + return nil, fmt.Errorf("list workspace claims in namespace %s: %w", pool.Namespace, err) + } + claims := slices.DeleteFunc(list.Items, func(c coderv1alpha1.CoderWorkspaceClaim) bool { return c.Spec.PoolName != pool.Name }) + for i := range claims { + if claims[i].UID == "" { + return nil, fmt.Errorf("assertion failed: workspace claim %s/%s has no UID", claims[i].Namespace, claims[i].Name) + } + } + slices.SortStableFunc(claims, func(a, b coderv1alpha1.CoderWorkspaceClaim) int { + return cmp.Or(a.CreationTimestamp.Compare(b.CreationTimestamp.Time), cmp.Compare(a.Name, b.Name)) + }) + return claims, nil +} + +// poolClaimHolder returns the member that holds the claim, or nil. The +// member is the source of truth: after a crash between the bind and the +// claim status, the next reconcile finds the claim's member here. +func poolClaimHolder(claim *coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember) *coderv1alpha1.CoderWorkspacePoolMember { + i := slices.IndexFunc(all, func(m coderv1alpha1.CoderWorkspacePoolMember) bool { + return m.Spec.ClaimRef != nil && m.Spec.ClaimRef.UID == string(claim.UID) + }) + if i < 0 { + return nil + } + return &all[i] +} + +// bindClaims binds the pool's waiting claims, oldest first, to its ready +// unclaimed members (plan 6.3 step 5): current before outdated, then the +// oldest ready time. Ready means what the counts use: phase Ready, +// Ready=True, and not used. One Update per bind sets spec.claimRef and +// moves the controller reference from the pool to the claim, with the +// listed resourceVersion, so a member that changed after the list (for +// example, someone used it) answers 409 and stays. A conflict ends the +// reconcile. The bound members change in all, so the later steps see them +// bound. +func (r *CoderWorkspacePoolReconciler) bindClaims( + ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim, + all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts, +) error { + var free []*coderv1alpha1.CoderWorkspacePoolMember + for i := range all { + m := &all[i] + controller := metav1.GetControllerOf(m) + if controller != nil && controller.UID == pool.UID && m.Spec.ClaimRef == nil && m.DeletionTimestamp.IsZero() && + poolMemberReady(m) && !meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) { + free = append(free, m) + } + } + readyAt := func(m *coderv1alpha1.CoderWorkspacePoolMember) time.Time { + return cmp.Or(m.Status.ReadyTime, &m.CreationTimestamp).Time + } + slices.SortStableFunc(free, func(a, b *coderv1alpha1.CoderWorkspacePoolMember) int { + aOld, bOld := poolMemberInputMismatch(a, pool) != "", poolMemberInputMismatch(b, pool) != "" + if aOld != bOld { + if aOld { + return 1 + } + return -1 + } + return readyAt(a).Compare(readyAt(b)) + }) + for i := range claims { + claim := &claims[i] + if len(free) == 0 { + return nil + } + if !claim.DeletionTimestamp.IsZero() || claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseLost || poolClaimHolder(claim, all) != nil { + continue + } + m := free[0] + current := poolMemberInputMismatch(m, pool) == "" + bound := m.DeepCopy() + bound.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: claim.Name, UID: string(claim.UID)} + bound.OwnerReferences = slices.DeleteFunc(bound.OwnerReferences, func(o metav1.OwnerReference) bool { return o.UID == pool.UID }) + if err := controllerutil.SetControllerReference(claim, bound, r.Scheme); err != nil { + return fmt.Errorf("assertion failed: set claim %s as controller of member %s: %w", claim.Name, m.Name, err) + } + if err := r.Update(ctx, bound); err != nil { + return fmt.Errorf("bind pool member %s/%s to claim %s: %w", m.Namespace, m.Name, claim.Name, err) + } + *m, free = *bound, free[1:] + counts.unclaimed-- + counts.ready-- + counts.bound++ + if current { + counts.current-- + counts.readyCurrent-- + } else { + counts.outdated-- + } + } + return nil +} + +// writeClaimStatuses writes the status of every claim of the pool from the +// member that holds it (plan 6.3 step 10). ownerName is the owner's Coder +// username from this reconcile, or "" when the owner read did not run. +func (r *CoderWorkspacePoolReconciler) writeClaimStatuses( + ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim, + all []coderv1alpha1.CoderWorkspacePoolMember, ownerName string, +) error { + now := metav1.NewTime(r.Clock.Now()) + for i := range claims { + claim := &claims[i] + if claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseLost { + continue // Final. + } + before := claim.Status.DeepCopy() + st := &claim.Status + ready, reason, message := false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", pool.Name) + if m := poolClaimHolder(claim, all); m != nil { + st.Phase, st.MemberName = coderv1alpha1.CoderWorkspaceClaimPhaseBound, m.Name + st.WorkspaceID, st.WorkspaceName = m.Status.WorkspaceID, m.Status.WorkspaceName + st.OwnerName = cmp.Or(ownerName, st.OwnerName) + if st.BoundTime == nil { + st.BoundTime = &now + } + ready = poolMemberReady(m) + reason, message = "WorkspaceReady", fmt.Sprintf("Workspace %s is ready.", m.Status.WorkspaceName) + if !ready { + reason, message = cmp.Or(m.Status.Reason, "WorkspaceNotReady"), truncateTemplateTestMessage(m.Status.Message) + } + } else { + st.Phase = coderv1alpha1.CoderWorkspaceClaimPhasePending + } + st.Reason, st.Message = reason, message + for conditionType, ok := range map[string]bool{ + coderv1alpha1.CoderWorkspaceClaimConditionReady: ready, + coderv1alpha1.CoderWorkspaceClaimConditionReconciling: !ready, + coderv1alpha1.CoderWorkspaceClaimConditionStalled: false, + } { + status := metav1.ConditionFalse + if ok { + status = metav1.ConditionTrue + } + meta.SetStatusCondition(&st.Conditions, metav1.Condition{ + Type: conditionType, Status: status, ObservedGeneration: claim.Generation, Reason: reason, Message: message, LastTransitionTime: now, + }) + } + if equality.Semantic.DeepEqual(before, st) { + continue + } + if err := r.Status().Update(ctx, claim); err != nil { + return fmt.Errorf("update status of workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) + } + } + return nil +} diff --git a/internal/controller/coderworkspacepool_claim_test.go b/internal/controller/coderworkspacepool_claim_test.go new file mode 100644 index 00000000..d63d8fd7 --- /dev/null +++ b/internal/controller/coderworkspacepool_claim_test.go @@ -0,0 +1,286 @@ +package controller_test + +import ( + "context" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// createClaim creates a claim on the env's pool. +func (e *poolMemberEnv) createClaim(t *testing.T, name string) types.NamespacedName { + t.Helper() + claim := &coderv1alpha1.CoderWorkspaceClaim{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: e.ns}, + Spec: coderv1alpha1.CoderWorkspaceClaimSpec{PoolName: e.pool.Name}, + } + require.NoError(t, k8sClient.Create(e.ctx, claim)) + return types.NamespacedName{Namespace: e.ns, Name: name} +} + +func (e *poolMemberEnv) claim(t *testing.T, key types.NamespacedName) *coderv1alpha1.CoderWorkspaceClaim { + t.Helper() + claim := &coderv1alpha1.CoderWorkspaceClaim{} + require.NoError(t, k8sClient.Get(e.ctx, key, claim)) + return claim +} + +// readyWithWorkspace is a ready member with a workspace ID and name. +func readyWithWorkspace(name string) func(*coderv1alpha1.CoderWorkspacePoolMemberStatus) { + return withWorkspace(func(s *coderv1alpha1.CoderWorkspacePoolMemberStatus) { + readyStatus(s) + s.WorkspaceName = name + }) +} + +// requireBound checks that the member holds the claim: claimRef, the claim +// as the only controller, no reference to the pool, and the pool label. +func (e *poolMemberEnv) requireBound(t *testing.T, member, claimKey types.NamespacedName) { + t.Helper() + m, claim := e.reconcileMember(t, member, 0), e.claim(t, claimKey) + require.NotNil(t, m.Spec.ClaimRef, "member %s is not bound", member.Name) + require.Equal(t, coderv1alpha1.CoderWorkspaceClaimReference{Name: claim.Name, UID: string(claim.UID)}, *m.Spec.ClaimRef) + controllerRef := metav1.GetControllerOf(m) + require.NotNil(t, controllerRef) + require.Equal(t, claim.UID, controllerRef.UID) + require.Equal(t, "CoderWorkspaceClaim", controllerRef.Kind) + require.True(t, *controllerRef.BlockOwnerDeletion, "a claim delete waits for the member's cleanup") + for _, o := range m.OwnerReferences { + require.NotEqual(t, e.pool.UID, o.UID, "garbage collection of the pool never reaches a bound member") + } + require.Equal(t, string(e.pool.UID), m.Labels[coderv1alpha1.CoderWorkspacePoolSelectorLabel]) +} + +func requireClaim(t *testing.T, claim *coderv1alpha1.CoderWorkspaceClaim, phase, reason string, ready bool) { + t.Helper() + require.Equal(t, phase, claim.Status.Phase, claim.Status.Message) + require.Equal(t, reason, claim.Status.Reason, claim.Status.Message) + require.Equal(t, ready, meta.IsStatusConditionTrue(claim.Status.Conditions, coderv1alpha1.CoderWorkspaceClaimConditionReady)) + require.Equal(t, !ready, meta.IsStatusConditionTrue(claim.Status.Conditions, coderv1alpha1.CoderWorkspaceClaimConditionReconciling)) + require.False(t, meta.IsStatusConditionTrue(claim.Status.Conditions, coderv1alpha1.CoderWorkspaceClaimConditionStalled)) +} + +// TestPoolBindsOldestClaimFirst: with one ready member and two claims, the +// older claim gets the member. Its status names the workspace, and the +// other claim waits. +func TestPoolBindsOldestClaimFirst(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + member := e.createMember(t, nil) + e.setMemberStatus(t, member, readyWithWorkspace("kwp-ready")) + older := e.createClaim(t, "older") + time.Sleep(1100 * time.Millisecond) // creationTimestamp has second precision. + newer := e.createClaim(t, "newer") + + pool := e.reconcilePool(t, 1) + e.requireBound(t, member, older) + claim := e.claim(t, older) + requireClaim(t, claim, coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true) + m := e.reconcileMember(t, member, 0) + require.Equal(t, m.Name, claim.Status.MemberName) + require.Equal(t, m.Status.WorkspaceID, claim.Status.WorkspaceID) + require.Equal(t, "kwp-ready", claim.Status.WorkspaceName) + require.Equal(t, "tester", claim.Status.OwnerName, "coder ssh tester/kwp-ready reaches it") + require.NotNil(t, claim.Status.BoundTime) + requireClaim(t, e.claim(t, newer), coderv1alpha1.CoderWorkspaceClaimPhasePending, "WaitingForReadyWorkspace", false) + require.Equal(t, int32(1), pool.Status.BoundReplicas) + require.Zero(t, pool.Status.ReadyReplicas, "a bound member is not ready to claim") + require.Len(t, e.poolMembers(t, e.pool.Name), 2, "the pool replaces the bound member") +} + +// TestPoolBindsCurrentBeforeOutdated: a claim takes a current member before +// an outdated one, even an older one. A used member and a member whose +// agent is not ready are never bound (plan 16, PR 9 advisor item (a)). +func TestPoolBindsCurrentBeforeOutdated(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(4)) }) + v0 := e.fake.addVersion(e.tplID, "v0", codersdk.ProvisionerJobSucceeded).String() + now := e.clock.Now() + outdated := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { m.Spec.TemplateVersionID = v0 }) + e.setMemberStatus(t, outdated, withWorkspace(e.readyAt(now.Add(-time.Hour)))) + current := e.createMember(t, nil) + e.setMemberStatus(t, current, withWorkspace(e.readyAt(now))) + used := e.createMember(t, nil) + e.setMemberStatus(t, used, withWorkspace(usedStatus)) + notReady := e.createMember(t, nil) + e.setMemberStatus(t, notReady, withWorkspace(memberPhase(coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, + memberCondition(coderv1alpha1.CoderWorkspacePoolMemberConditionReady, metav1.ConditionFalse, "AgentNotReady")))) + first := e.createClaim(t, "first") + time.Sleep(1100 * time.Millisecond) + second := e.createClaim(t, "second") + time.Sleep(1100 * time.Millisecond) + third := e.createClaim(t, "third") + + e.reconcilePool(t, 1) + e.requireBound(t, current, first) + e.requireBound(t, outdated, second) + requireClaim(t, e.claim(t, third), coderv1alpha1.CoderWorkspaceClaimPhasePending, "WaitingForReadyWorkspace", false) + require.Nil(t, e.reconcileMember(t, used, 0).Spec.ClaimRef) + require.Nil(t, e.reconcileMember(t, notReady, 0).Spec.ClaimRef) +} + +// bindHookClient runs hook once, right before the first member update. +type bindHookClient struct { + client.Client + hook func(m *coderv1alpha1.CoderWorkspacePoolMember) + done *bool +} + +func (c bindHookClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + if m, ok := obj.(*coderv1alpha1.CoderWorkspacePoolMember); ok && !*c.done { + *c.done = true + c.hook(m) + } + return c.Client.Update(ctx, obj, opts...) +} + +// afterMemberList runs hook once, right after the first member list. +type afterMemberList struct { + client.Reader + hook func() + done *bool +} + +func (c afterMemberList) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if err := c.Reader.List(ctx, list, opts...); err != nil { + return err + } + if _, ok := list.(*coderv1alpha1.CoderWorkspacePoolMemberList); ok && !*c.done { + *c.done = true + c.hook() + } + return nil +} + +// onlyClaim lists only the named claim, as a cache that has not seen the +// others yet. +type onlyClaim struct { + client.Client + name string +} + +func (c onlyClaim) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if err := c.Client.List(ctx, list, opts...); err != nil { + return err + } + if claims, ok := list.(*coderv1alpha1.CoderWorkspaceClaimList); ok { + var keep []coderv1alpha1.CoderWorkspaceClaim + for _, c2 := range claims.Items { + if c2.Name == c.name { + keep = append(keep, c2) + } + } + claims.Items = keep + } + return nil +} + +// TestPoolBindRace is mutation check 4: a bind carries the listed +// resourceVersion. Two reconciles that race for one member (for example two +// operator replicas during a leader change, one with a cache that has not +// seen the older claim) bind exactly one claim. A member that its +// reconciler marks used between the list and the bind is not bound. +func TestPoolBindRace(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + key := types.NamespacedName{Namespace: e.ns, Name: e.pool.Name} + member := e.createMember(t, nil) + e.setMemberStatus(t, member, readyWithWorkspace("kwp-race")) + a := e.createClaim(t, "a") + time.Sleep(1100 * time.Millisecond) + b := e.createClaim(t, "b") + other := &controller.CoderWorkspacePoolReconciler{Client: onlyClaim{k8sClient, "b"}, APIReader: k8sClient, Scheme: scheme, Clock: e.clock} + racing := bindHookClient{Client: k8sClient, done: new(false), hook: func(*coderv1alpha1.CoderWorkspacePoolMember) { + _, err := other.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + }} + r := &controller.CoderWorkspacePoolReconciler{Client: racing, APIReader: k8sClient, Scheme: scheme, Clock: e.clock} + _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + require.Error(t, err, "the second bind of the member must fail") + e.requireBound(t, member, b) + e.reconcilePool(t, 1) + requireClaim(t, e.claim(t, a), coderv1alpha1.CoderWorkspaceClaimPhasePending, "WaitingForReadyWorkspace", false) + bound := 0 + for _, m := range e.poolMembers(t, e.pool.Name) { + if m.Spec.ClaimRef != nil { + bound++ + } + } + require.Equal(t, 1, bound, "exactly one bind") + + // Someone uses a ready member right after the pool's member list. + e2 := newPoolMemberEnv(t) + used := e2.createMember(t, nil) + e2.setMemberStatus(t, used, readyWithWorkspace("kwp-used")) + e2.createClaim(t, "c") + marking := afterMemberList{Reader: k8sClient, done: new(false), hook: func() { e2.setMemberStatus(t, used, usedStatus) }} + r = &controller.CoderWorkspacePoolReconciler{Client: k8sClient, APIReader: marking, Scheme: scheme, Clock: e2.clock} + _, err = r.Reconcile(e2.ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: e2.ns, Name: e2.pool.Name}}) + require.True(t, apierrors.IsConflict(err), "the resourceVersion refuses the bind: %v", err) + require.Nil(t, e2.reconcileMember(t, used, 0).Spec.ClaimRef, "a used member is never bound") +} + +// TestPoolFinishesClaimAfterCrash: the pool crashed after the member bind, +// before the claim status. The next reconcile finds the member that holds +// the claim, finishes the claim status, and binds no second member. +func TestPoolFinishesClaimAfterCrash(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(2)) }) + claimKey := e.createClaim(t, "build-1") + claim := e.claim(t, claimKey) + held := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { + m.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: claim.Name, UID: string(claim.UID)} + m.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: coderv1alpha1.GroupVersion.String(), Kind: "CoderWorkspaceClaim", Name: claim.Name, UID: claim.UID, + Controller: new(true), BlockOwnerDeletion: new(true), + }} + }) + e.setMemberStatus(t, held, readyWithWorkspace("kwp-held")) + spare := e.createMember(t, nil) + e.setMemberStatus(t, spare, readyWithWorkspace("kwp-spare")) + + e.reconcilePool(t, 1) + claim = e.claim(t, claimKey) + requireClaim(t, claim, coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true) + require.Equal(t, held.Name, claim.Status.MemberName) + require.Equal(t, "kwp-held", claim.Status.WorkspaceName) + require.Nil(t, e.reconcileMember(t, spare, 0).Spec.ClaimRef, "no second member for the claim") +} + +// TestPoolClaimDeleteDeletesWorkspace: the claim controls its member, with +// blockOwnerDeletion, so a claim delete makes garbage collection delete the +// member. envtest runs no garbage collector, so the test deletes the member +// as the collector would. The member's cleanup then deletes the workspace, +// also after the claimant used it. +func TestPoolClaimDeleteDeletesWorkspace(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(0)) }) + member, workspaceID := e.readyMember(t) + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(1)) }) + claimKey := e.createClaim(t, "build-1") + e.reconcilePool(t, 1) + e.requireBound(t, member, claimKey) + + e.fake.markUsed(workspaceID, e.clock.Now().Add(time.Minute)) + require.NoError(t, k8sClient.Delete(e.ctx, e.claim(t, claimKey))) + e.deleteMember(t, member) + m := e.reconcileMember(t, member, 1) + requireMember(t, m, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build") + e.fake.setBuildJob(uuid.MustParse(m.Status.DeleteBuildID), codersdk.ProvisionerJobSucceeded) + require.Nil(t, e.reconcileMember(t, member, 1), "the workspace is deleted, then the finalizer goes") +} diff --git a/internal/controller/coderworkspacepool_controller.go b/internal/controller/coderworkspacepool_controller.go index 1b4ea72c..caf2bb3f 100644 --- a/internal/controller/coderworkspacepool_controller.go +++ b/internal/controller/coderworkspacepool_controller.go @@ -81,7 +81,8 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R } before := pool.Status.DeepCopy() - resolved, step, err := r.resolve(ctx, pool) + var ownerName string + resolved, step, err := r.resolve(ctx, pool, &ownerName) if step, err = memberWaitOnWrongAnswer(ctx, step, err); err != nil { return ctrl.Result{}, err } @@ -113,6 +114,10 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R members = append(members, m) } } + claims, err := r.poolClaims(ctx, pool) + if err != nil { + return ctrl.Result{}, err + } if err := r.releaseUsedMembers(ctx, pool, members); err != nil { return ctrl.Result{}, err } @@ -141,6 +146,9 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.trimToOwnerCap(ctx, pool, maxWorkspaces, all.Items); err != nil { return ctrl.Result{}, err } + if err := r.bindClaims(ctx, pool, claims, all.Items, &counts); err != nil { + return ctrl.Result{}, err + } step, requeue, err = r.createMembers(ctx, pool, maxWorkspaces, all.Items, &counts) if err != nil { return ctrl.Result{}, err @@ -150,6 +158,9 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.deleteUnclaimedMembers(ctx, pool, all.Items, step); err != nil { return ctrl.Result{}, err } + if err := r.writeClaimStatuses(ctx, pool, claims, all.Items, ownerName); err != nil { + return ctrl.Result{}, err + } r.applyPoolStatus(pool, counts, step) if err := r.writePoolStatus(ctx, pool, before); err != nil { return ctrl.Result{}, err @@ -537,8 +548,10 @@ func (r *CoderWorkspacePoolReconciler) createMember(ctx context.Context, pool *c // resolve reads the pool's inputs from Coder (plan 6.3 step 2) with the // rules that the member checks again before it creates. A nil step means // that new members can pin the returned values. +// It also stores the owner's Coder username in ownerName, once the owner +// read succeeded. func (r *CoderWorkspacePoolReconciler) resolve( - ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, + ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, ownerName *string, ) (*coderv1alpha1.CoderWorkspacePoolResolved, *coderStep, error) { spec := pool.Spec sdk, controlPlane, wait, err := controlPlaneCoderClient(ctx, r.Client, pool.Namespace, spec.ControlPlaneRef.Name) @@ -559,6 +572,7 @@ func (r *CoderWorkspacePoolReconciler) resolve( if step != nil || err != nil { return nil, step, err } + *ownerName = owner.Username template, step, err := resolveCoderTemplate(ctx, sdk, spec.Template, owner) if step != nil || err != nil { return nil, step, err From c482469109bf1fefcf78f9a1c9031a80b1e5f894 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 23:18:11 +0000 Subject: [PATCH 2/4] fix: never bind a CoderWorkspaceClaim twice, scope its holder to its pool, repair the pool label, and store the owner name before the bind Change-Id: Id61727ee0e9898f2def86bd569dbc814c6b44fa4 Signed-off-by: Thomas Kosiewski --- .../controller/coderworkspacepool_claim.go | 67 +++++++++++++++---- .../coderworkspacepool_claim_test.go | 56 +++++++++++++++- .../coderworkspacepool_controller.go | 2 +- 3 files changed, 111 insertions(+), 14 deletions(-) diff --git a/internal/controller/coderworkspacepool_claim.go b/internal/controller/coderworkspacepool_claim.go index b03e1b00..378b29f7 100644 --- a/internal/controller/coderworkspacepool_claim.go +++ b/internal/controller/coderworkspacepool_claim.go @@ -8,6 +8,7 @@ import ( "time" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,12 +37,14 @@ func (r *CoderWorkspacePoolReconciler) poolClaims(ctx context.Context, pool *cod return claims, nil } -// poolClaimHolder returns the member that holds the claim, or nil. The -// member is the source of truth: after a crash between the bind and the -// claim status, the next reconcile finds the claim's member here. +// poolClaimHolder returns the member of the claim's pool that holds the +// claim, or nil. The member is the source of truth: after a crash between +// the bind and the claim status, the next reconcile finds the claim's +// member here. spec.poolName is immutable, so a member of another pool +// never counts. func poolClaimHolder(claim *coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember) *coderv1alpha1.CoderWorkspacePoolMember { i := slices.IndexFunc(all, func(m coderv1alpha1.CoderWorkspacePoolMember) bool { - return m.Spec.ClaimRef != nil && m.Spec.ClaimRef.UID == string(claim.UID) + return m.Spec.ClaimRef != nil && m.Spec.ClaimRef.UID == string(claim.UID) && m.Spec.PoolName == claim.Spec.PoolName }) if i < 0 { return nil @@ -49,6 +52,13 @@ func poolClaimHolder(claim *coderv1alpha1.CoderWorkspaceClaim, all []coderv1alph return &all[i] } +// poolClaimEverBound reports a claim that a member held once. Such a claim +// never binds again: when its member is gone, it becomes Lost. +func poolClaimEverBound(claim *coderv1alpha1.CoderWorkspaceClaim) bool { + return claim.Status.BoundTime != nil || claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseBound || + claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseLost +} + // bindClaims binds the pool's waiting claims, oldest first, to its ready // unclaimed members (plan 6.3 step 5): current before outdated, then the // oldest ready time. Ready means what the counts use: phase Ready, @@ -57,11 +67,18 @@ func poolClaimHolder(claim *coderv1alpha1.CoderWorkspaceClaim, all []coderv1alph // listed resourceVersion, so a member that changed after the list (for // example, someone used it) answers 409 and stays. A conflict ends the // reconcile. The bound members change in all, so the later steps see them -// bound. +// bound. Right before each bind, the pool reads the claim again without +// the cache, so a claim whose Bound or Lost status the cache has not shown +// yet never binds a second member, and it stores ownerName in the claim +// status, so the name survives a crash between the bind and the status. +// It also sets the pool label again, in case it drifted. func (r *CoderWorkspacePoolReconciler) bindClaims( ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim, - all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts, + all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts, ownerName string, ) error { + if ownerName == "" { + return fmt.Errorf("assertion failed: pool %s/%s binds claims without the owner's Coder username", pool.Namespace, pool.Name) + } var free []*coderv1alpha1.CoderWorkspacePoolMember for i := range all { m := &all[i] @@ -89,14 +106,35 @@ func (r *CoderWorkspacePoolReconciler) bindClaims( if len(free) == 0 { return nil } - if !claim.DeletionTimestamp.IsZero() || claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseLost || poolClaimHolder(claim, all) != nil { + if !claim.DeletionTimestamp.IsZero() || poolClaimEverBound(claim) || poolClaimHolder(claim, all) != nil { continue } + fresh := &coderv1alpha1.CoderWorkspaceClaim{} + if err := r.APIReader.Get(ctx, client.ObjectKeyFromObject(claim), fresh); apierrors.IsNotFound(err) { + continue + } else if err != nil { + return fmt.Errorf("get workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) + } + if fresh.UID != claim.UID || !fresh.DeletionTimestamp.IsZero() || poolClaimEverBound(fresh) { + *claim = *fresh + continue + } + if fresh.Status.OwnerName != ownerName { + fresh.Status.OwnerName = ownerName + if err := r.Status().Update(ctx, fresh); err != nil { + return fmt.Errorf("update status of workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) + } + } + *claim = *fresh m := free[0] current := poolMemberInputMismatch(m, pool) == "" bound := m.DeepCopy() bound.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: claim.Name, UID: string(claim.UID)} bound.OwnerReferences = slices.DeleteFunc(bound.OwnerReferences, func(o metav1.OwnerReference) bool { return o.UID == pool.UID }) + if bound.Labels == nil { + bound.Labels = map[string]string{} + } + bound.Labels[coderv1alpha1.CoderWorkspacePoolSelectorLabel] = string(pool.UID) if err := controllerutil.SetControllerReference(claim, bound, r.Scheme); err != nil { return fmt.Errorf("assertion failed: set claim %s as controller of member %s: %w", claim.Name, m.Name, err) } @@ -132,8 +170,13 @@ func (r *CoderWorkspacePoolReconciler) writeClaimStatuses( } before := claim.Status.DeepCopy() st := &claim.Status - ready, reason, message := false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", pool.Name) - if m := poolClaimHolder(claim, all); m != nil { + ready, lost, reason, message := false, false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", pool.Name) + m := poolClaimHolder(claim, all) + switch { + case m == nil && poolClaimEverBound(claim): + st.Phase, lost = coderv1alpha1.CoderWorkspaceClaimPhaseLost, true + reason, message = "MemberDeleted", "The member that held this claim is gone. The claim never binds again." + case m != nil: st.Phase, st.MemberName = coderv1alpha1.CoderWorkspaceClaimPhaseBound, m.Name st.WorkspaceID, st.WorkspaceName = m.Status.WorkspaceID, m.Status.WorkspaceName st.OwnerName = cmp.Or(ownerName, st.OwnerName) @@ -145,14 +188,14 @@ func (r *CoderWorkspacePoolReconciler) writeClaimStatuses( if !ready { reason, message = cmp.Or(m.Status.Reason, "WorkspaceNotReady"), truncateTemplateTestMessage(m.Status.Message) } - } else { + default: st.Phase = coderv1alpha1.CoderWorkspaceClaimPhasePending } st.Reason, st.Message = reason, message for conditionType, ok := range map[string]bool{ coderv1alpha1.CoderWorkspaceClaimConditionReady: ready, - coderv1alpha1.CoderWorkspaceClaimConditionReconciling: !ready, - coderv1alpha1.CoderWorkspaceClaimConditionStalled: false, + coderv1alpha1.CoderWorkspaceClaimConditionReconciling: !ready && !lost, + coderv1alpha1.CoderWorkspaceClaimConditionStalled: lost, } { status := metav1.ConditionFalse if ok { diff --git a/internal/controller/coderworkspacepool_claim_test.go b/internal/controller/coderworkspacepool_claim_test.go index d63d8fd7..3016dd99 100644 --- a/internal/controller/coderworkspacepool_claim_test.go +++ b/internal/controller/coderworkspacepool_claim_test.go @@ -109,7 +109,9 @@ func TestPoolBindsCurrentBeforeOutdated(t *testing.T) { e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(4)) }) v0 := e.fake.addVersion(e.tplID, "v0", codersdk.ProvisionerJobSucceeded).String() now := e.clock.Now() - outdated := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { m.Spec.TemplateVersionID = v0 }) + // The outdated member's label drifted: the pool finds it as its + // controller, and the bind sets the label again. + outdated := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { m.Spec.TemplateVersionID, m.Labels = v0, nil }) e.setMemberStatus(t, outdated, withWorkspace(e.readyAt(now.Add(-time.Hour)))) current := e.createMember(t, nil) e.setMemberStatus(t, current, withWorkspace(e.readyAt(now))) @@ -258,6 +260,7 @@ func TestPoolFinishesClaimAfterCrash(t *testing.T) { requireClaim(t, claim, coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true) require.Equal(t, held.Name, claim.Status.MemberName) require.Equal(t, "kwp-held", claim.Status.WorkspaceName) + require.Equal(t, "tester", claim.Status.OwnerName) require.Nil(t, e.reconcileMember(t, spare, 0).Spec.ClaimRef, "no second member for the claim") } @@ -284,3 +287,54 @@ func TestPoolClaimDeleteDeletesWorkspace(t *testing.T) { e.fake.setBuildJob(uuid.MustParse(m.Status.DeleteBuildID), codersdk.ProvisionerJobSucceeded) require.Nil(t, e.reconcileMember(t, member, 1), "the workspace is deleted, then the finalizer goes") } + +// staleClaims answers claim lists with the claims as the cache first saw +// them: without status. +type staleClaims struct{ client.Client } + +func (c staleClaims) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if err := c.Client.List(ctx, list, opts...); err != nil { + return err + } + if claims, ok := list.(*coderv1alpha1.CoderWorkspaceClaimList); ok { + for i := range claims.Items { + claims.Items[i].Status = coderv1alpha1.CoderWorkspaceClaimStatus{} + } + } + return nil +} + +// TestPoolNeverRebindsClaim: a claim that a member held once never binds +// again, also when its member is gone and the cache still shows the claim +// without status: the pool reads the claim again without the cache right +// before a bind. The claim becomes Lost with MemberDeleted. A member of +// another pool that names the claim never counts as its holder. +func TestPoolNeverRebindsClaim(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(2)) }) + first := e.createMember(t, nil) + e.setMemberStatus(t, first, readyWithWorkspace("kwp-first")) + claimKey := e.createClaim(t, "build-1") + claim := e.claim(t, claimKey) + // A member of another pool with a claimRef to this claim. + e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { + m.Spec.PoolName, m.OwnerReferences, m.Labels = "other", nil, nil + m.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: claim.Name, UID: string(claim.UID)} + }) + e.reconcilePool(t, 1) + e.requireBound(t, first, claimKey) + require.Equal(t, first.Name, e.claim(t, claimKey).Status.MemberName) + + require.NoError(t, k8sClient.Delete(e.ctx, e.reconcileMember(t, first, 0))) + spare := e.createMember(t, nil) + e.setMemberStatus(t, spare, readyWithWorkspace("kwp-spare")) + stale := &controller.CoderWorkspacePoolReconciler{Client: staleClaims{k8sClient}, APIReader: k8sClient, Scheme: scheme, Clock: e.clock} + _, err := stale.Reconcile(e.ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: e.ns, Name: e.pool.Name}}) + require.NoError(t, err) + require.Nil(t, e.reconcileMember(t, spare, 0).Spec.ClaimRef, "no second member for the claim") + claim = e.claim(t, claimKey) + require.Equal(t, coderv1alpha1.CoderWorkspaceClaimPhaseLost, claim.Status.Phase) + require.Equal(t, "MemberDeleted", claim.Status.Reason) + require.True(t, meta.IsStatusConditionTrue(claim.Status.Conditions, coderv1alpha1.CoderWorkspaceClaimConditionStalled)) +} diff --git a/internal/controller/coderworkspacepool_controller.go b/internal/controller/coderworkspacepool_controller.go index caf2bb3f..96ce399a 100644 --- a/internal/controller/coderworkspacepool_controller.go +++ b/internal/controller/coderworkspacepool_controller.go @@ -146,7 +146,7 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.trimToOwnerCap(ctx, pool, maxWorkspaces, all.Items); err != nil { return ctrl.Result{}, err } - if err := r.bindClaims(ctx, pool, claims, all.Items, &counts); err != nil { + if err := r.bindClaims(ctx, pool, claims, all.Items, &counts, ownerName); err != nil { return ctrl.Result{}, err } step, requeue, err = r.createMembers(ctx, pool, maxWorkspaces, all.Items, &counts) From c8c6095d9594aa9cd5cc9d6fd5f2ee16972f5257 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 23:38:24 +0000 Subject: [PATCH 3/4] fix: refresh waiting CoderWorkspaceClaims without the cache, skip a replaced claim, and check the holder again before Lost Change-Id: I536b0015fc519c2ffd84a81fb4b055c5f0a0b871 Signed-off-by: Thomas Kosiewski --- .../controller/coderworkspacepool_claim.go | 64 +++++++++++++------ .../coderworkspacepool_claim_test.go | 62 ++++++++++++++++++ .../coderworkspacepool_controller.go | 3 + 3 files changed, 110 insertions(+), 19 deletions(-) diff --git a/internal/controller/coderworkspacepool_claim.go b/internal/controller/coderworkspacepool_claim.go index 378b29f7..37a4b9c1 100644 --- a/internal/controller/coderworkspacepool_claim.go +++ b/internal/controller/coderworkspacepool_claim.go @@ -59,6 +59,33 @@ func poolClaimEverBound(claim *coderv1alpha1.CoderWorkspaceClaim) bool { claim.Status.Phase == coderv1alpha1.CoderWorkspaceClaimPhaseLost } +// refreshWaitingClaims reads each claim that waits by the (cached) list +// again without the cache, so a claim whose Bound or Lost status the cache +// has not shown yet neither binds a second member nor counts as demand. A +// claim that is gone, or whose name a new claim took, leaves the list. +func (r *CoderWorkspacePoolReconciler) refreshWaitingClaims( + ctx context.Context, claims []coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember, +) ([]coderv1alpha1.CoderWorkspaceClaim, error) { + kept := claims[:0] + for _, claim := range claims { + if !claim.DeletionTimestamp.IsZero() || poolClaimEverBound(&claim) || poolClaimHolder(&claim, all) != nil { + kept = append(kept, claim) + continue + } + fresh := &coderv1alpha1.CoderWorkspaceClaim{} + if err := r.APIReader.Get(ctx, client.ObjectKeyFromObject(&claim), fresh); apierrors.IsNotFound(err) { + continue + } else if err != nil { + return nil, fmt.Errorf("get workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) + } + if fresh.UID != claim.UID { + continue // A new claim with the same name, maybe of another pool. + } + kept = append(kept, *fresh) + } + return kept, nil +} + // bindClaims binds the pool's waiting claims, oldest first, to its ready // unclaimed members (plan 6.3 step 5): current before outdated, then the // oldest ready time. Ready means what the counts use: phase Ready, @@ -67,11 +94,10 @@ func poolClaimEverBound(claim *coderv1alpha1.CoderWorkspaceClaim) bool { // listed resourceVersion, so a member that changed after the list (for // example, someone used it) answers 409 and stays. A conflict ends the // reconcile. The bound members change in all, so the later steps see them -// bound. Right before each bind, the pool reads the claim again without -// the cache, so a claim whose Bound or Lost status the cache has not shown -// yet never binds a second member, and it stores ownerName in the claim -// status, so the name survives a crash between the bind and the status. -// It also sets the pool label again, in case it drifted. +// bound. The claims come from refreshWaitingClaims. Before each bind, the +// pool stores ownerName in the claim status, so the name survives a crash +// between the bind and the status. It also sets the pool label again, in +// case it drifted. func (r *CoderWorkspacePoolReconciler) bindClaims( ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts, ownerName string, @@ -109,23 +135,12 @@ func (r *CoderWorkspacePoolReconciler) bindClaims( if !claim.DeletionTimestamp.IsZero() || poolClaimEverBound(claim) || poolClaimHolder(claim, all) != nil { continue } - fresh := &coderv1alpha1.CoderWorkspaceClaim{} - if err := r.APIReader.Get(ctx, client.ObjectKeyFromObject(claim), fresh); apierrors.IsNotFound(err) { - continue - } else if err != nil { - return fmt.Errorf("get workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) - } - if fresh.UID != claim.UID || !fresh.DeletionTimestamp.IsZero() || poolClaimEverBound(fresh) { - *claim = *fresh - continue - } - if fresh.Status.OwnerName != ownerName { - fresh.Status.OwnerName = ownerName - if err := r.Status().Update(ctx, fresh); err != nil { + if claim.Status.OwnerName != ownerName { + claim.Status.OwnerName = ownerName + if err := r.Status().Update(ctx, claim); err != nil { return fmt.Errorf("update status of workspace claim %s/%s: %w", claim.Namespace, claim.Name, err) } } - *claim = *fresh m := free[0] current := poolMemberInputMismatch(m, pool) == "" bound := m.DeepCopy() @@ -172,6 +187,17 @@ func (r *CoderWorkspacePoolReconciler) writeClaimStatuses( st := &claim.Status ready, lost, reason, message := false, false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", pool.Name) m := poolClaimHolder(claim, all) + if m == nil && poolClaimEverBound(claim) { + // Lost is final, so check the holder on an uncached list first: + // another reconcile can have bound a member after this one's list. + fresh := &coderv1alpha1.CoderWorkspacePoolMemberList{} + if err := r.APIReader.List(ctx, fresh, client.InNamespace(claim.Namespace)); err != nil { + return fmt.Errorf("list pool members in namespace %s: %w", claim.Namespace, err) + } + if h := poolClaimHolder(claim, fresh.Items); h != nil { + m = h.DeepCopy() + } + } switch { case m == nil && poolClaimEverBound(claim): st.Phase, lost = coderv1alpha1.CoderWorkspaceClaimPhaseLost, true diff --git a/internal/controller/coderworkspacepool_claim_test.go b/internal/controller/coderworkspacepool_claim_test.go index 3016dd99..55d5cc81 100644 --- a/internal/controller/coderworkspacepool_claim_test.go +++ b/internal/controller/coderworkspacepool_claim_test.go @@ -338,3 +338,65 @@ func TestPoolNeverRebindsClaim(t *testing.T) { require.Equal(t, "MemberDeleted", claim.Status.Reason) require.True(t, meta.IsStatusConditionTrue(claim.Status.Conditions, coderv1alpha1.CoderWorkspaceClaimConditionStalled)) } + +// fixedClaims answers claim lists with a saved copy, as a cache that still +// holds a claim that is gone. +type fixedClaims struct { + client.Client + items []coderv1alpha1.CoderWorkspaceClaim +} + +func (c fixedClaims) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if claims, ok := list.(*coderv1alpha1.CoderWorkspaceClaimList); ok { + claims.Items = append([]coderv1alpha1.CoderWorkspaceClaim(nil), c.items...) + return nil + } + return c.Client.List(ctx, list, opts...) +} + +// TestPoolSkipsReplacedClaim: the cache still holds a deleted claim, and a +// new claim of another pool took its name. The pool neither binds the new +// claim nor writes its status. +func TestPoolSkipsReplacedClaim(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + member := e.createMember(t, nil) + e.setMemberStatus(t, member, readyWithWorkspace("kwp-ready")) + key := e.createClaim(t, "build-1") + old := e.claim(t, key) + require.NoError(t, k8sClient.Delete(e.ctx, old)) + other := &coderv1alpha1.CoderWorkspaceClaim{} + other.Name, other.Namespace, other.Spec.PoolName = key.Name, key.Namespace, "other" + require.NoError(t, k8sClient.Create(e.ctx, other)) + + r := &controller.CoderWorkspacePoolReconciler{Client: fixedClaims{k8sClient, []coderv1alpha1.CoderWorkspaceClaim{*old}}, APIReader: k8sClient, Scheme: scheme, Clock: e.clock} + _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: e.ns, Name: e.pool.Name}}) + require.NoError(t, err) + require.Nil(t, e.reconcileMember(t, member, 0).Spec.ClaimRef) + require.Empty(t, e.claim(t, key).Status.Phase, "the other pool's claim has no status from this pool") +} + +// TestPoolClaimNotLostOnOverlap: another reconcile binds the claim after +// this reconcile listed members. This reconcile sees the claim Bound +// without a holder in its own list, checks the holder again without the +// cache, and keeps the claim Bound instead of marking it Lost. +func TestPoolClaimNotLostOnOverlap(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + member := e.createMember(t, nil) + e.setMemberStatus(t, member, readyWithWorkspace("kwp-ready")) + claimKey := e.createClaim(t, "build-1") + key := types.NamespacedName{Namespace: e.ns, Name: e.pool.Name} + inner := &controller.CoderWorkspacePoolReconciler{Client: k8sClient, APIReader: k8sClient, Scheme: scheme, Clock: e.clock} + overlap := afterMemberList{Reader: k8sClient, done: new(false), hook: func() { + _, err := inner.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + }} + r := &controller.CoderWorkspacePoolReconciler{Client: staleClaims{k8sClient}, APIReader: overlap, Scheme: scheme, Clock: e.clock} + _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + if err != nil { + require.True(t, apierrors.IsConflict(err), "only a conflict may end the overlapping reconcile: %v", err) + } + e.requireBound(t, member, claimKey) + requireClaim(t, e.claim(t, claimKey), coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true) +} diff --git a/internal/controller/coderworkspacepool_controller.go b/internal/controller/coderworkspacepool_controller.go index 96ce399a..d0d99a7f 100644 --- a/internal/controller/coderworkspacepool_controller.go +++ b/internal/controller/coderworkspacepool_controller.go @@ -146,6 +146,9 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.trimToOwnerCap(ctx, pool, maxWorkspaces, all.Items); err != nil { return ctrl.Result{}, err } + if claims, err = r.refreshWaitingClaims(ctx, claims, all.Items); err != nil { + return ctrl.Result{}, err + } if err := r.bindClaims(ctx, pool, claims, all.Items, &counts, ownerName); err != nil { return ctrl.Result{}, err } From 366c494d0b9f0109793bae546058159c09c1c555 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 23:54:13 +0000 Subject: [PATCH 4/4] fix: bind only members whose spec.poolName names the pool Change-Id: I06dd83d03cfd1e061e4d3a03940ac6e5a2be9934 Signed-off-by: Thomas Kosiewski --- internal/controller/coderworkspacepool_claim.go | 4 +++- internal/controller/coderworkspacepool_claim_test.go | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/controller/coderworkspacepool_claim.go b/internal/controller/coderworkspacepool_claim.go index 37a4b9c1..34c90724 100644 --- a/internal/controller/coderworkspacepool_claim.go +++ b/internal/controller/coderworkspacepool_claim.go @@ -109,7 +109,9 @@ func (r *CoderWorkspacePoolReconciler) bindClaims( for i := range all { m := &all[i] controller := metav1.GetControllerOf(m) - if controller != nil && controller.UID == pool.UID && m.Spec.ClaimRef == nil && m.DeletionTimestamp.IsZero() && + // spec.poolName is immutable and decides the holder, so a member that + // names another pool is never bound here. + if controller != nil && controller.UID == pool.UID && m.Spec.PoolName == pool.Name && m.Spec.ClaimRef == nil && m.DeletionTimestamp.IsZero() && poolMemberReady(m) && !meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) { free = append(free, m) } diff --git a/internal/controller/coderworkspacepool_claim_test.go b/internal/controller/coderworkspacepool_claim_test.go index 55d5cc81..d45832d7 100644 --- a/internal/controller/coderworkspacepool_claim_test.go +++ b/internal/controller/coderworkspacepool_claim_test.go @@ -102,7 +102,8 @@ func TestPoolBindsOldestClaimFirst(t *testing.T) { // TestPoolBindsCurrentBeforeOutdated: a claim takes a current member before // an outdated one, even an older one. A used member and a member whose -// agent is not ready are never bound (plan 16, PR 9 advisor item (a)). +// agent is not ready are never bound (plan 16, PR 9 advisor item (a)), nor +// is a member whose spec.poolName names another pool. func TestPoolBindsCurrentBeforeOutdated(t *testing.T) { t.Parallel() e := newPoolMemberEnv(t) @@ -120,6 +121,9 @@ func TestPoolBindsCurrentBeforeOutdated(t *testing.T) { notReady := e.createMember(t, nil) e.setMemberStatus(t, notReady, withWorkspace(memberPhase(coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, memberCondition(coderv1alpha1.CoderWorkspacePoolMemberConditionReady, metav1.ConditionFalse, "AgentNotReady")))) + // The pool controls this member, but it names another pool. + misnamed := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) { m.Spec.PoolName = "other" }) + e.setMemberStatus(t, misnamed, withWorkspace(e.readyAt(now.Add(-2*time.Hour)))) first := e.createClaim(t, "first") time.Sleep(1100 * time.Millisecond) second := e.createClaim(t, "second") @@ -132,6 +136,7 @@ func TestPoolBindsCurrentBeforeOutdated(t *testing.T) { requireClaim(t, e.claim(t, third), coderv1alpha1.CoderWorkspaceClaimPhasePending, "WaitingForReadyWorkspace", false) require.Nil(t, e.reconcileMember(t, used, 0).Spec.ClaimRef) require.Nil(t, e.reconcileMember(t, notReady, 0).Spec.ClaimRef) + require.Nil(t, e.reconcileMember(t, misnamed, 0).Spec.ClaimRef) } // bindHookClient runs hook once, right before the first member update.