fix: read the owner cap again right before the member's create marker, and check pool consent uncached whatever the demand

Change-Id: I24972f715031adeae241dbf3b92f6e975046cc59
Signed-off-by: Thomas Kosiewski <tk@coder.com>

Author: ThomasK33

internal/controller/coderworkspacepool_cap_test.go

@@ -1,12 +1,16 @@
 package controller_test
 
 import (
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"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"
 )
 
 // setOwnerCap sets maxWorkspaces of the control plane's only owner entry.
@@ -89,3 +93,43 @@ func TestPoolLoweredOwnerCapDeletesOnce(t *testing.T) {
 	require.Equal(t, 2, deleting, "one member stays at maxWorkspaces 1")
 	require.Len(t, e.poolMembers(t, e.pool.Name), 3, "no new member")
 }
+
+// firstReadOldCap answers the first control plane read with maxWorkspaces
+// 10, as before an admin lowered it, and later reads as stored.
+type firstReadOldCap struct {
+	client.Reader
+	reads *int
+}
+
+func (c firstReadOldCap) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if err := c.Reader.Get(ctx, key, obj, opts...); err != nil {
+		return err
+	}
+	if cp, ok := obj.(*coderv1alpha1.CoderControlPlane); ok && cp.Spec.WorkspacePools != nil {
+		if *c.reads++; *c.reads == 1 {
+			for i := range cp.Spec.WorkspacePools.Owners {
+				cp.Spec.WorkspacePools.Owners[i].MaxWorkspaces = new(int32(10))
+			}
+		}
+	}
+	return nil
+}
+
+// TestPoolMemberRechecksCapLast: the member reads the cap again after its
+// Coder reads, right before the marker, so a cap that an admin lowered
+// during those reads holds (Codex 4274565367).
+func TestPoolMemberRechecksCapLast(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.setOwnerCap(t, 1)
+	e.reconcileMember(t, e.createMember(t, nil), 2)
+	require.Equal(t, 1, e.fake.workspaceCount())
+	second := e.createMember(t, nil)
+	r := &controller.CoderWorkspacePoolMemberReconciler{Client: k8sClient, APIReader: firstReadOldCap{k8sClient, new(0)}, Scheme: scheme, Clock: e.clock}
+	for range 2 { // The finalizer, then the checks.
+		_, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: second})
+		require.NoError(t, err)
+	}
+	require.Equal(t, 1, e.fake.workspaceCount(), "no workspace above maxWorkspaces 1")
+	require.Equal(t, "OwnerWorkspaceLimit", e.reconcileMember(t, second, 0).Status.Reason)
+}

internal/controller/coderworkspacepool_controller.go

@@ -126,10 +126,18 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 		// its own object shows the new resolved inputs stored.
 		requeue = time.Second
 	default:
-		if err := r.trimToOwnerCap(ctx, pool, all.Items); err != nil {
+		var maxWorkspaces int32
+		if maxWorkspaces, step, err = r.freshOwnerCap(ctx, pool); err != nil {
 			return ctrl.Result{}, err
 		}
-		step, requeue, err = r.createMembers(ctx, pool, all.Items, &counts)
+		if step != nil {
+			requeue = poolResolvePoll
+			break
+		}
+		if err := r.trimToOwnerCap(ctx, pool, maxWorkspaces, all.Items); err != nil {
+			return ctrl.Result{}, err
+		}
+		step, requeue, err = r.createMembers(ctx, pool, maxWorkspaces, all.Items, &counts)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -243,13 +251,37 @@ func poolMemberFailedAt(m *coderv1alpha1.CoderWorkspacePoolMember) time.Time {
 	return m.CreationTimestamp.Time
 }
 
+// freshOwnerCap reads the pool's control plane without the cache, whatever
+// the pool's demand: a deletion or a consent that an admin just withdrew
+// holds even while the cache has the old object. It returns the owner's
+// maxWorkspaces from that read, or the wait.
+func (r *CoderWorkspacePoolReconciler) freshOwnerCap(ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool) (int32, *coderStep, error) {
+	controlPlane := &coderv1alpha1.CoderControlPlane{}
+	if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: pool.Namespace, Name: pool.Spec.ControlPlaneRef.Name}, controlPlane); err != nil {
+		return 0, nil, fmt.Errorf("get CoderControlPlane %s/%s: %w", pool.Namespace, pool.Spec.ControlPlaneRef.Name, err)
+	}
+	if !controlPlane.DeletionTimestamp.IsZero() {
+		return 0, coderWait("ControlPlaneDeleting", "CoderControlPlane %s is being deleted.", controlPlane.Name), nil
+	}
+	if step := workspacePoolConsent(controlPlane, pool.Status.Resolved.OwnerID, pool.Spec.Template); step != nil {
+		return 0, step, nil
+	}
+	owner := workspacePoolOwner(controlPlane, pool.Status.Resolved.OwnerID)
+	if owner.MaxWorkspaces == nil {
+		return 0, nil, fmt.Errorf("assertion failed: CoderControlPlane %s has no defaulted maxWorkspaces for Coder user %s", controlPlane.Name, pool.Status.Resolved.OwnerID)
+	}
+	return *owner.MaxWorkspaces, nil, nil
+}
+
 // createMembers creates current members while all caps hold (plan 6.3 step
 // 7): current unclaimed members fewer than spec.replicas, building members
 // fewer than spec.maxInFlight, the owner's live members on this control
-// plane fewer than maxWorkspaces, and the failure backoff passed. It returns
-// the wait that stops it, if any, and when to read again.
+// plane fewer than maxWorkspaces (from an uncached read), and the failure
+// backoff passed. It returns the wait that stops it, if any, and when to
+// read again.
 func (r *CoderWorkspacePoolReconciler) createMembers(
-	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts,
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, maxWorkspaces int32,
+	all []coderv1alpha1.CoderWorkspacePoolMember, counts *poolMemberCounts,
 ) (*coderStep, time.Duration, error) {
 	st, now := &pool.Status, r.Clock.Now()
 	need := *pool.Spec.Replicas - counts.current // Claims add their demand in PR 11.
@@ -265,25 +297,8 @@ func (r *CoderWorkspacePoolReconciler) createMembers(
 				last.Member, last.Reason, retryAt.UTC().Format(time.RFC3339)), retryAt.Sub(now), nil
 		}
 	}
-	// The cap and the count both come from uncached reads, so that neither
-	// a cap that an admin just lowered nor two pools on a stale cache pass it.
-	controlPlane := &coderv1alpha1.CoderControlPlane{}
-	if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: pool.Namespace, Name: pool.Spec.ControlPlaneRef.Name}, controlPlane); err != nil {
-		return nil, 0, fmt.Errorf("get CoderControlPlane %s/%s: %w", pool.Namespace, pool.Spec.ControlPlaneRef.Name, err)
-	}
-	if !controlPlane.DeletionTimestamp.IsZero() {
-		return coderWait("ControlPlaneDeleting", "CoderControlPlane %s is being deleted.", controlPlane.Name), poolResolvePoll, nil
-	}
-	// The consent check again, on the uncached control plane: a consent that
-	// an admin just withdrew holds even while the cache has the old one.
-	if step := workspacePoolConsent(controlPlane, st.Resolved.OwnerID, pool.Spec.Template); step != nil {
-		return step, poolResolvePoll, nil
-	}
-	owner := workspacePoolOwner(controlPlane, st.Resolved.OwnerID)
-	if owner.MaxWorkspaces == nil {
-		return nil, 0, fmt.Errorf("assertion failed: CoderControlPlane %s has no defaulted maxWorkspaces for Coder user %s", controlPlane.Name, st.Resolved.OwnerID)
-	}
-	maxWorkspaces := *owner.MaxWorkspaces
+	// The count comes from the uncached list, so two pools on a stale cache
+	// never pass the cap together.
 	live := poolOwnerLiveMembers(all, st.Resolved.OwnerID, pool.Spec.ControlPlaneRef.Name, nil)
 	n := min(need, *pool.Spec.MaxInFlight-counts.building, maxWorkspaces-live)
 	for range n {
@@ -329,17 +344,9 @@ func poolOwnerLiveMembers(
 // member is marked deleting in all, so the later steps of this reconcile
 // skip it.
 func (r *CoderWorkspacePoolReconciler) trimToOwnerCap(
-	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, all []coderv1alpha1.CoderWorkspacePoolMember,
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, maxWorkspaces int32, all []coderv1alpha1.CoderWorkspacePoolMember,
 ) error {
-	controlPlane := &coderv1alpha1.CoderControlPlane{}
-	if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: pool.Namespace, Name: pool.Spec.ControlPlaneRef.Name}, controlPlane); err != nil {
-		return fmt.Errorf("get CoderControlPlane %s/%s: %w", pool.Namespace, pool.Spec.ControlPlaneRef.Name, err)
-	}
-	owner := workspacePoolOwner(controlPlane, pool.Status.Resolved.OwnerID)
-	if !controlPlane.DeletionTimestamp.IsZero() || owner == nil || owner.MaxWorkspaces == nil {
-		return nil // createMembers reports these.
-	}
-	excess := poolOwnerLiveMembers(all, pool.Status.Resolved.OwnerID, pool.Spec.ControlPlaneRef.Name, nil) - *owner.MaxWorkspaces
+	excess := poolOwnerLiveMembers(all, pool.Status.Resolved.OwnerID, pool.Spec.ControlPlaneRef.Name, nil) - maxWorkspaces
 	if excess <= 0 {
 		return nil
 	}

internal/controller/coderworkspacepoolmember_controller.go

@@ -220,9 +220,6 @@ func (r *CoderWorkspacePoolMemberReconciler) checkCreate(
 	if step := workspacePoolConsent(fresh, spec.OwnerID, pool.Spec.Template); step != nil {
 		return nil, step, nil
 	}
-	if step, err := r.checkOwnerCap(ctx, member, fresh); step != nil || err != nil {
-		return nil, step, err
-	}
 	ownerUser, step, err := checkCoderWorkspaceOwner(ctx, sdk, uuid.MustParse(spec.OwnerID))
 	if step != nil || err != nil {
 		return nil, step, err
@@ -275,19 +272,35 @@ func (r *CoderWorkspacePoolMemberReconciler) checkCreate(
 	case !isCoderNotFound(err):
 		return nil, coderStepUnavailable("get workspace by name", err), nil
 	}
+	// Last, after the Coder reads: the owner cap and the consent again.
+	if step, err := r.checkOwnerCap(ctx, member, pool.Spec.Template); step != nil || err != nil {
+		return nil, step, err
+	}
 	return sdk, nil, nil
 }
 
-// checkOwnerCap counts, without the cache, the owner's other members on the
-// control plane that may have a workspace. At maxWorkspaces the member
+// checkOwnerCap reads the control plane again without the cache, right
+// before the create marker, and counts, also without the cache, the owner's
+// other members on it that may have a workspace. At maxWorkspaces the member
 // waits, so a member that the pool created under a higher cap creates
-// nothing above a cap that an admin lowered since (Codex 4271920249).
+// nothing above a cap that an admin lowered since (Codex 4271920249 and
+// 4274565367). The consent check runs again on the same read.
 func (r *CoderWorkspacePoolMemberReconciler) checkOwnerCap(
-	ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember, controlPlane *coderv1alpha1.CoderControlPlane,
+	ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember, template string,
 ) (*coderStep, error) {
+	controlPlane := &coderv1alpha1.CoderControlPlane{}
+	if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: member.Namespace, Name: member.Spec.ControlPlaneRef.Name}, controlPlane); err != nil {
+		return nil, fmt.Errorf("get CoderControlPlane %s/%s: %w", member.Namespace, member.Spec.ControlPlaneRef.Name, err)
+	}
+	if !controlPlane.DeletionTimestamp.IsZero() {
+		return coderWait("ControlPlaneDeleting", "CoderControlPlane %s is being deleted.", controlPlane.Name), nil
+	}
+	if step := workspacePoolConsent(controlPlane, member.Spec.OwnerID, template); step != nil {
+		return step, nil
+	}
 	owner := workspacePoolOwner(controlPlane, member.Spec.OwnerID)
-	if owner == nil || owner.MaxWorkspaces == nil {
-		return nil, fmt.Errorf("assertion failed: CoderControlPlane %s has no defaulted maxWorkspaces for Coder user %s after the consent check", controlPlane.Name, member.Spec.OwnerID)
+	if owner.MaxWorkspaces == nil {
+		return nil, fmt.Errorf("assertion failed: CoderControlPlane %s has no defaulted maxWorkspaces for Coder user %s", controlPlane.Name, member.Spec.OwnerID)
 	}
 	all := &coderv1alpha1.CoderWorkspacePoolMemberList{}
 	if err := r.APIReader.List(ctx, all, client.InNamespace(member.Namespace)); err != nil {