🤖 feat: create CoderWorkspacePool members on demand for waiting claims, keep claims of a missing pool, and mark a claim Lost when its workspace is deleted

internal/controller/coderworkspacepool_claim.go

@@ -20,12 +20,12 @@ import (
 // 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) {
+func (r *CoderWorkspacePoolReconciler) poolClaims(ctx context.Context, namespace, poolName string) ([]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)
+	if err := r.List(ctx, list, client.InNamespace(namespace)); err != nil {
+		return nil, fmt.Errorf("list workspace claims in namespace %s: %w", namespace, err)
 	}
-	claims := slices.DeleteFunc(list.Items, func(c coderv1alpha1.CoderWorkspaceClaim) bool { return c.Spec.PoolName != pool.Name })
+	claims := slices.DeleteFunc(list.Items, func(c coderv1alpha1.CoderWorkspaceClaim) bool { return c.Spec.PoolName != poolName })
 	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)
@@ -37,6 +37,41 @@ func (r *CoderWorkspacePoolReconciler) poolClaims(ctx context.Context, pool *cod
 	return claims, nil
 }
 
+// poolClaimWaits reports a claim that no member holds yet and that can
+// still bind.
+func poolClaimWaits(claim *coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember) bool {
+	return claim.DeletionTimestamp.IsZero() && !poolClaimEverBound(claim) && poolClaimHolder(claim, all) == nil
+}
+
+// poolWant is the number of current unclaimed members the pool wants:
+// spec.replicas plus the waiting claims (decision D4), so a pool at
+// replicas 0 still creates a member for each claim.
+func poolWant(pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim, all []coderv1alpha1.CoderWorkspacePoolMember) int32 {
+	want := *pool.Spec.Replicas
+	for i := range claims {
+		if poolClaimWaits(&claims[i], all) {
+			want++
+		}
+	}
+	return want
+}
+
+// reconcileOrphanClaims writes the status of the claims of a pool that is
+// gone or being deleted: a claim that a member holds stays as its member
+// says, so a bound claim stays Bound, and the others wait with
+// PoolNotFound.
+func (r *CoderWorkspacePoolReconciler) reconcileOrphanClaims(ctx context.Context, namespace, poolName string) error {
+	claims, err := r.poolClaims(ctx, namespace, poolName)
+	if err != nil || len(claims) == 0 {
+		return err
+	}
+	all := &coderv1alpha1.CoderWorkspacePoolMemberList{}
+	if err := r.APIReader.List(ctx, all, client.InNamespace(namespace)); err != nil {
+		return fmt.Errorf("list pool members in namespace %s: %w", namespace, err)
+	}
+	return r.writeClaimStatuses(ctx, poolName, true, claims, all.Items, "")
+}
+
 // 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
@@ -134,7 +169,7 @@ func (r *CoderWorkspacePoolReconciler) bindClaims(
 		if len(free) == 0 {
 			return nil
 		}
-		if !claim.DeletionTimestamp.IsZero() || poolClaimEverBound(claim) || poolClaimHolder(claim, all) != nil {
+		if !poolClaimWaits(claim, all) {
 			continue
 		}
 		if claim.Status.OwnerName != ownerName {
@@ -173,10 +208,12 @@ func (r *CoderWorkspacePoolReconciler) bindClaims(
 }
 
 // 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
+// member that holds it (plan 6.3 step 10). A claim whose member's workspace
+// is deleted becomes Lost, which is final. ownerName is the owner's Coder
 // username from this reconcile, or "" when the owner read did not run.
+// poolMissing marks a pool that is gone or being deleted.
 func (r *CoderWorkspacePoolReconciler) writeClaimStatuses(
-	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, claims []coderv1alpha1.CoderWorkspaceClaim,
+	ctx context.Context, poolName string, poolMissing bool, claims []coderv1alpha1.CoderWorkspaceClaim,
 	all []coderv1alpha1.CoderWorkspacePoolMember, ownerName string,
 ) error {
 	now := metav1.NewTime(r.Clock.Now())
@@ -187,7 +224,10 @@ func (r *CoderWorkspacePoolReconciler) writeClaimStatuses(
 		}
 		before := claim.Status.DeepCopy()
 		st := &claim.Status
-		ready, lost, reason, message := false, false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", pool.Name)
+		ready, lost, reason, message := false, false, "WaitingForReadyWorkspace", fmt.Sprintf("CoderWorkspacePool %s has no ready workspace for this claim yet.", poolName)
+		if poolMissing {
+			reason, message = "PoolNotFound", fmt.Sprintf("CoderWorkspacePool %s does not exist or is being deleted.", poolName)
+		}
 		m := poolClaimHolder(claim, all)
 		if m == nil && poolClaimEverBound(claim) {
 			// Lost is final, so check the holder on an uncached list first:
@@ -216,6 +256,10 @@ func (r *CoderWorkspacePoolReconciler) writeClaimStatuses(
 			if !ready {
 				reason, message = cmp.Or(m.Status.Reason, "WorkspaceNotReady"), truncateTemplateTestMessage(m.Status.Message)
 			}
+			if meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted) {
+				st.Phase, lost = coderv1alpha1.CoderWorkspaceClaimPhaseLost, true
+				reason, message = "WorkspaceDeleted", fmt.Sprintf("Workspace %s was deleted. The claim never binds again.", m.Status.WorkspaceName)
+			}
 		default:
 			st.Phase = coderv1alpha1.CoderWorkspaceClaimPhasePending
 		}

internal/controller/coderworkspacepool_claim_lifecycle_test.go

@@ -0,0 +1,168 @@
+package controller_test
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/stretchr/testify/require"
+	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"
+)
+
+// TestPoolCreatesOnDemand: a pool at replicas 0 creates a member for each
+// waiting claim (decision D4: want is replicas plus the waiting claims),
+// binds it once it is ready, and creates no more after that.
+func TestPoolCreatesOnDemand(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(0)) })
+	e.reconcilePool(t, 1)
+	require.Empty(t, e.poolMembers(t, e.pool.Name), "no claim, no member")
+
+	claimKey := e.createClaim(t, "build-1")
+	e.reconcilePool(t, 2)
+	members := e.poolMembers(t, e.pool.Name)
+	require.Len(t, members, 1, "one member for the one waiting claim")
+	requireClaim(t, e.claim(t, claimKey), coderv1alpha1.CoderWorkspaceClaimPhasePending, "WaitingForReadyWorkspace", false)
+
+	e.setMemberStatus(t, memberKey(members[0]), readyWithWorkspace("kwp-on-demand"))
+	e.reconcilePool(t, 2)
+	e.requireBound(t, memberKey(members[0]), claimKey)
+	requireClaim(t, e.claim(t, claimKey), coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true)
+	require.Len(t, e.poolMembers(t, e.pool.Name), 1, "no member after the claim is bound")
+}
+
+// TestPoolDeleteKeepsBoundClaims: the claim controls a bound member, so
+// deleting the pool does not reach it, and its claim stays Bound. A claim
+// that no member holds waits with PoolNotFound, as does a claim on a pool
+// that never existed.
+func TestPoolDeleteKeepsBoundClaims(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	member := e.createMember(t, nil)
+	e.setMemberStatus(t, member, readyWithWorkspace("kwp-kept"))
+	bound := e.createClaim(t, "bound")
+	time.Sleep(1100 * time.Millisecond) // creationTimestamp has second precision.
+	waiting := e.createClaim(t, "waiting")
+	e.reconcilePool(t, 1)
+	e.requireBound(t, member, bound)
+
+	require.NoError(t, k8sClient.Delete(e.ctx, e.reconcilePool(t, 0))) // No finalizer: gone at once.
+	r := &controller.CoderWorkspacePoolReconciler{Client: k8sClient, 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)
+	e.requireBound(t, member, bound)
+	claim := e.claim(t, bound)
+	requireClaim(t, claim, coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true)
+	require.Equal(t, "kwp-kept", claim.Status.WorkspaceName)
+	require.Equal(t, "tester", claim.Status.OwnerName, "kept from the bind")
+	requireClaim(t, e.claim(t, waiting), coderv1alpha1.CoderWorkspaceClaimPhasePending, "PoolNotFound", false)
+
+	stray := &coderv1alpha1.CoderWorkspaceClaim{}
+	stray.Name, stray.Namespace, stray.Spec.PoolName = "stray", e.ns, "no-such-pool"
+	require.NoError(t, k8sClient.Create(e.ctx, stray))
+	_, err = r.Reconcile(e.ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: e.ns, Name: "no-such-pool"}})
+	require.NoError(t, err)
+	requireClaim(t, e.claim(t, types.NamespacedName{Namespace: e.ns, Name: "stray"}), coderv1alpha1.CoderWorkspaceClaimPhasePending, "PoolNotFound", false)
+}
+
+// TestPoolClaimLostWhenWorkspaceDeleted: after the one extension at bind, a
+// bound member keeps reading its workspace every 60 s. Builds by the
+// claimant, a stop included, change nothing. A deleted workspace sets
+// WorkspaceDeleted=True, and the claim becomes Lost, which is final: it
+// never binds again (plan 6.2 step 11 rule 3).
+func TestPoolClaimLostWhenWorkspaceDeleted(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)
+	claimKey := e.createClaim(t, "build-1")
+	e.reconcilePool(t, 1)
+	e.requireBound(t, member, claimKey)
+	m := e.reconcileMember(t, member, 1)
+	require.True(t, m.Status.BindExtended)
+	require.Equal(t, time.Minute, e.lastStep.RequeueAfter, "a bound member keeps watching")
+
+	stop := e.fake.addExternalBuild(workspaceID, e.tester, codersdk.WorkspaceTransitionStop)
+	e.fake.setBuildJob(stop.ID, codersdk.ProvisionerJobSucceeded)
+	m = e.reconcileMember(t, member, 1)
+	require.Equal(t, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, m.Status.Phase, "a stop by the claimant changes nothing")
+
+	e.fake.markDeleted(workspaceID)
+	m = e.reconcileMember(t, member, 1)
+	requireMemberFailed(t, m, "WorkspaceDeletedExternally", "deleted", metav1.ConditionTrue)
+	e.reconcilePool(t, 1)
+	claim := e.claim(t, claimKey)
+	require.Equal(t, coderv1alpha1.CoderWorkspaceClaimPhaseLost, claim.Status.Phase)
+	require.Equal(t, "WorkspaceDeleted", claim.Status.Reason)
+
+	// The failed member goes, and the cache still shows the claim without
+	// its Lost status: a Lost claim is no demand, so no member is created
+	// for it.
+	e.deleteMember(t, member)
+	require.Nil(t, e.reconcileMember(t, member, 2), "the workspace is already deleted")
+	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.Empty(t, e.poolMembers(t, e.pool.Name), "no member for a Lost claim")
+
+	// A ready member appears: the Lost claim never binds again.
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(1)) })
+	spare := e.createMember(t, nil)
+	e.setMemberStatus(t, spare, readyWithWorkspace("kwp-spare"))
+	// Again on the stale cache, now with a ready member to bind.
+	_, 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)
+	e.reconcilePool(t, 1)
+	require.Nil(t, e.reconcileMember(t, spare, 0).Spec.ClaimRef)
+	require.Equal(t, coderv1alpha1.CoderWorkspaceClaimPhaseLost, e.claim(t, claimKey).Status.Phase)
+}
+
+// crashAfterBind lets the first member update land, then reports an
+// error, as a crash right after the bind would end the reconcile.
+type crashAfterBind struct{ client.Client }
+
+func (c crashAfterBind) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	if err := c.Client.Update(ctx, obj, opts...); err != nil {
+		return err
+	}
+	if _, ok := obj.(*coderv1alpha1.CoderWorkspacePoolMember); ok {
+		return errors.New("crash after the bind")
+	}
+	return nil
+}
+
+// TestPoolOrphanClaimKeepsOwnerName: the pool crashes right after a bind,
+// before the claim status, and the pool is deleted before the next
+// reconcile. The claim still shows Bound with the owner's Coder username,
+// which the pool stored in the claim status before the bind.
+func TestPoolOrphanClaimKeepsOwnerName(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	member := e.createMember(t, nil)
+	e.setMemberStatus(t, member, readyWithWorkspace("kwp-crash"))
+	claimKey := e.createClaim(t, "build-1")
+	key := types.NamespacedName{Namespace: e.ns, Name: e.pool.Name}
+	r := &controller.CoderWorkspacePoolReconciler{Client: crashAfterBind{k8sClient}, APIReader: k8sClient, Scheme: scheme, Clock: e.clock}
+	_, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key})
+	require.ErrorContains(t, err, "crash after the bind")
+	e.requireBound(t, member, claimKey)
+
+	require.NoError(t, k8sClient.Delete(e.ctx, e.reconcilePool(t, 0)))
+	r = &controller.CoderWorkspacePoolReconciler{Client: k8sClient, APIReader: k8sClient, Scheme: scheme, Clock: e.clock}
+	_, err = r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key})
+	require.NoError(t, err)
+	claim := e.claim(t, claimKey)
+	requireClaim(t, claim, coderv1alpha1.CoderWorkspaceClaimPhaseBound, "WorkspaceReady", true)
+	require.Equal(t, "tester", claim.Status.OwnerName)
+	require.Equal(t, "kwp-crash", claim.Status.WorkspaceName)
+}

internal/controller/coderworkspacepool_claim_test.go

@@ -97,7 +97,7 @@ func TestPoolBindsOldestClaimFirst(t *testing.T) {
 	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")
+	require.Len(t, e.poolMembers(t, e.pool.Name), 3, "the pool replaces the bound member and adds one for the waiting claim")
 }
 
 // TestPoolBindsCurrentBeforeOutdated: a claim takes a current member before

internal/controller/coderworkspacepool_controller.go

@@ -11,6 +11,7 @@ import (
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/google/uuid"
 	"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"
 	"k8s.io/apimachinery/pkg/labels"
@@ -65,8 +66,10 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 	// and a reconcile that a member delete starts must see the status
 	// written right before that delete.
 	pool := &coderv1alpha1.CoderWorkspacePool{}
-	if err := r.APIReader.Get(ctx, req.NamespacedName, pool); err != nil {
-		return ctrl.Result{}, client.IgnoreNotFound(err)
+	if err := r.APIReader.Get(ctx, req.NamespacedName, pool); apierrors.IsNotFound(err) {
+		return ctrl.Result{}, r.reconcileOrphanClaims(ctx, req.Namespace, req.Name)
+	} else if err != nil {
+		return ctrl.Result{}, fmt.Errorf("get pool %s: %w", req, err)
 	}
 	if pool.Name != req.Name || pool.Namespace != req.Namespace {
 		return ctrl.Result{}, fmt.Errorf("assertion failed: fetched pool %s/%s does not match request %s", pool.Namespace, pool.Name, req)
@@ -76,8 +79,8 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 	}
 	if !pool.DeletionTimestamp.IsZero() {
 		// Garbage collection deletes the unclaimed members. Their finalizers
-		// run the last use check.
-		return ctrl.Result{}, nil
+		// run the last use check. Bound members belong to their claims.
+		return ctrl.Result{}, r.reconcileOrphanClaims(ctx, pool.Namespace, pool.Name)
 	}
 
 	before := pool.Status.DeepCopy()
@@ -114,7 +117,7 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 			members = append(members, m)
 		}
 	}
-	claims, err := r.poolClaims(ctx, pool)
+	claims, err := r.poolClaims(ctx, pool.Namespace, pool.Name)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -152,16 +155,16 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 		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)
+		step, requeue, err = r.createMembers(ctx, pool, poolWant(pool, claims, all.Items), maxWorkspaces, all.Items, &counts)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 	// The namespace list, where trimToOwnerCap marked its deletes.
-	if err := r.deleteUnclaimedMembers(ctx, pool, all.Items, step); err != nil {
+	if err := r.deleteUnclaimedMembers(ctx, pool, poolWant(pool, claims, all.Items), all.Items, step); err != nil {
 		return ctrl.Result{}, err
 	}
-	if err := r.writeClaimStatuses(ctx, pool, claims, all.Items, ownerName); err != nil {
+	if err := r.writeClaimStatuses(ctx, pool.Name, false, claims, all.Items, ownerName); err != nil {
 		return ctrl.Result{}, err
 	}
 	r.applyPoolStatus(pool, counts, step)
@@ -310,13 +313,12 @@ var poolConsentLostReasons = []string{"OwnerNotAllowed", "TemplateNotAllowed", "
 // go here. After a consent removal or an owner refusal, all other unclaimed
 // members go. Otherwise, new before old: outdated members that are not ready
 // go at once, and outdated ready members go oldest first, down to what the
-// current ready members do not cover yet. Current members above
-// spec.replicas go in this order: those without a create request, those not
+// current ready members do not cover yet. Current members above want
+// (spec.replicas plus the waiting claims) go in this order: those without a create request, those not
 // ready (newest first), then ready ones (oldest first).
 func (r *CoderWorkspacePoolReconciler) deleteUnclaimedMembers(
-	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, members []coderv1alpha1.CoderWorkspacePoolMember, step *coderStep,
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, want int32, members []coderv1alpha1.CoderWorkspacePoolMember, step *coderStep,
 ) error {
-	want := *pool.Spec.Replicas // Claims add their demand in PR 11.
 	var current, outdatedReady, doomed []*coderv1alpha1.CoderWorkspacePoolMember
 	var readyCurrent int32
 	for i := range members {
@@ -419,17 +421,18 @@ func (r *CoderWorkspacePoolReconciler) freshOwnerCap(ctx context.Context, pool *
 }
 
 // createMembers creates current members while all caps hold (plan 6.3 step
-// 7): current unclaimed members fewer than spec.replicas, building members
+// 7): current unclaimed members fewer than want (spec.replicas plus the
+// waiting claims, decision D4), building members
 // fewer than spec.maxInFlight, the owner's live members on this control
 // 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, maxWorkspaces int32,
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, want, 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.
+	need := want - counts.current
 	if need <= 0 {
 		return nil, 0, nil
 	}