feat: delete unwanted CoderWorkspacePool members new before old, release used members, and delete unclaimed members when consent goes

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

Author: ThomasK33

internal/controller/coderworkspacepool_controller.go

@@ -1,6 +1,7 @@
 package controller
 
 import (
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
@@ -112,6 +113,9 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 			members = append(members, m)
 		}
 	}
+	if err := r.releaseUsedMembers(ctx, pool, members); err != nil {
+		return ctrl.Result{}, err
+	}
 	if err := r.deleteFailedMembers(ctx, pool, before, members); err != nil {
 		return ctrl.Result{}, err
 	}
@@ -134,6 +138,9 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 			return ctrl.Result{}, err
 		}
 	}
+	if err := r.deleteUnclaimedMembers(ctx, pool, members, step); err != nil {
+		return ctrl.Result{}, err
+	}
 	r.applyPoolStatus(pool, counts, step)
 	if err := r.writePoolStatus(ctx, pool, before); err != nil {
 		return ctrl.Result{}, err
@@ -229,6 +236,118 @@ func (r *CoderWorkspacePoolReconciler) deleteFailedMembers(
 	return errors.Join(errs...)
 }
 
+// releaseUsedMembers removes the pool's controller reference from its
+// unclaimed members that someone used (plan 6.3 step 4, decision D6). A
+// used member then never counts as unclaimed again, and neither the pool
+// nor its deletion deletes it. It keeps the pool label for
+// status.usedReplicas. The update carries the listed resourceVersion, and
+// the listed members stay as they were, so the later steps decide on the
+// list.
+func (r *CoderWorkspacePoolReconciler) releaseUsedMembers(
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, members []coderv1alpha1.CoderWorkspacePoolMember,
+) error {
+	for i := range members {
+		m := &members[i]
+		controller := metav1.GetControllerOf(m)
+		if controller == nil || controller.UID != pool.UID || m.Spec.ClaimRef != nil || !m.DeletionTimestamp.IsZero() ||
+			!meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) {
+			continue
+		}
+		released := m.DeepCopy()
+		released.OwnerReferences = slices.DeleteFunc(released.OwnerReferences, func(o metav1.OwnerReference) bool { return o.UID == pool.UID })
+		if err := r.Update(ctx, released); err != nil {
+			return fmt.Errorf("release used pool member %s/%s: %w", m.Namespace, m.Name, err)
+		}
+	}
+	return nil
+}
+
+// poolConsentLostReasons are the waits after which no unclaimed member may
+// keep its workspace: each one holds the owner's token from its start build.
+var poolConsentLostReasons = []string{"OwnerNotAllowed", "TemplateNotAllowed", "OwnerNotEligible"}
+
+// deleteUnclaimedMembers deletes the pool's unclaimed members that it no
+// longer wants (plan 6.3 steps 8 and 9). The deletes carry the listed UID and
+// resourceVersion, so a member that its reconciler marked used after the
+// list answers 409 and stays. Bound, used, failed, and deleting members never
+// 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
+// ready (newest first), then ready ones (oldest first).
+func (r *CoderWorkspacePoolReconciler) deleteUnclaimedMembers(
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, 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 {
+		m := &members[i]
+		controller := metav1.GetControllerOf(m)
+		if controller == nil || controller.UID != pool.UID || m.Spec.ClaimRef != nil || !m.DeletionTimestamp.IsZero() ||
+			m.Status.Phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseFailed ||
+			meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) {
+			continue
+		}
+		switch ready := poolMemberReady(m); {
+		case step != nil && slices.Contains(poolConsentLostReasons, step.reason):
+			doomed = append(doomed, m)
+		case poolMemberInputMismatch(m, pool) == "":
+			current = append(current, m)
+			if ready {
+				readyCurrent++
+			}
+		case ready:
+			outdatedReady = append(outdatedReady, m)
+		default:
+			doomed = append(doomed, m)
+		}
+	}
+	readyAt := func(m *coderv1alpha1.CoderWorkspacePoolMember) time.Time {
+		return cmp.Or(m.Status.ReadyTime, &m.CreationTimestamp).Time
+	}
+	// Newest ready first: the members to keep come first.
+	slices.SortStableFunc(outdatedReady, func(a, b *coderv1alpha1.CoderWorkspacePoolMember) int { return readyAt(b).Compare(readyAt(a)) })
+	doomed = append(doomed, outdatedReady[min(int(max(want-readyCurrent, 0)), len(outdatedReady)):]...)
+	if extra := len(current) - int(want); extra > 0 {
+		rank := func(m *coderv1alpha1.CoderWorkspacePoolMember) int {
+			switch {
+			case !poolMemberMayHaveWorkspace(m):
+				return 0
+			case !poolMemberReady(m):
+				return 1
+			}
+			return 2
+		}
+		slices.SortStableFunc(current, func(a, b *coderv1alpha1.CoderWorkspacePoolMember) int {
+			if c := cmp.Compare(rank(a), rank(b)); c != 0 {
+				return c
+			}
+			if rank(a) == 2 {
+				return readyAt(a).Compare(readyAt(b)) // Ready: oldest first.
+			}
+			return b.CreationTimestamp.Compare(a.CreationTimestamp.Time) // Newest first.
+		})
+		doomed = append(doomed, current[:extra]...)
+	}
+	var errs []error
+	for _, m := range doomed {
+		uid, version := m.UID, m.ResourceVersion
+		if err := r.Delete(ctx, m, client.Preconditions{UID: &uid, ResourceVersion: &version}); client.IgnoreNotFound(err) != nil {
+			errs = append(errs, fmt.Errorf("delete unclaimed pool member %s/%s: %w", m.Namespace, m.Name, err))
+		}
+	}
+	return errors.Join(errs...)
+}
+
+// poolMemberReady reports a member that is ready to claim: phase Ready and
+// its Ready condition True.
+func poolMemberReady(m *coderv1alpha1.CoderWorkspacePoolMember) bool {
+	return m.Status.Phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseReady &&
+		meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady)
+}
+
 // poolMemberFailedAt is when a failed member failed. The member sets its
 // WorkspaceDeleted condition first when it fails, so its transition time is
 // the failure time. Ready is already False while a member builds, so its
@@ -486,8 +605,7 @@ func countPoolMembers(pool *coderv1alpha1.CoderWorkspacePool, members []coderv1a
 		if m.Status.Phase != coderv1alpha1.CoderWorkspacePoolMemberPhaseReady {
 			c.building++
 		}
-		ready := m.Status.Phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseReady &&
-			meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady)
+		ready := poolMemberReady(m)
 		current := poolMemberInputMismatch(m, pool) == ""
 		switch {
 		case current && ready:

internal/controller/coderworkspacepool_create_test.go

@@ -172,7 +172,7 @@ func TestPoolOwnerCapUncached(t *testing.T) {
 	cp.Spec.WorkspacePools.Owners[0].MaxWorkspaces, cp.Spec.WorkspacePools.Owners[0].Templates = new(int32(10)), []string{"default.other"}
 	require.NoError(t, k8sClient.Update(e.ctx, cp))
 	reconcileStale(t, func(o *coderv1alpha1.WorkspacePoolOwner) { o.Templates = []string{"default.docker"} })
-	require.Len(t, e.poolMembers(t, e.pool.Name), 1, "no member after consent is withdrawn")
+	require.Empty(t, e.poolMembers(t, e.pool.Name), "no create after consent is withdrawn, and the unclaimed member is deleted")
 	require.Equal(t, "TemplateNotAllowed", e.reconcilePool(t, 0).Status.Reason)
 }
 
@@ -205,11 +205,13 @@ func TestPoolMemberDiscovery(t *testing.T) {
 	e.reconcilePool(t, 1)
 	members := e.poolMembers(t, e.pool.Name)
 	require.Len(t, members, 1)
+	e.setMemberStatus(t, memberKey(members[0]), readyStatus)
 	member := e.reconcileMember(t, memberKey(members[0]), 0)
 	member.Labels = nil
 	require.NoError(t, k8sClient.Update(e.ctx, member))
 	// The member no longer matches the pool's inputs, so the pool counts it
-	// as outdated and builds one current member, but never more.
+	// as outdated, keeps it while it is ready, and builds one current
+	// member, but never more.
 	pool := e.reconcilePool(t, 3)
 	require.Equal(t, int32(2), pool.Status.Replicas, "the controlled member still counts")
 	require.Equal(t, int32(1), pool.Status.UpdatedReplicas)

internal/controller/coderworkspacepool_delete_test.go

@@ -0,0 +1,192 @@
+package controller_test
+
+import (
+	"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"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+	"github.com/coder/coder-k8s/internal/controller"
+)
+
+// withWorkspace marks a member as one that sent its create request.
+func withWorkspace(edit func(*coderv1alpha1.CoderWorkspacePoolMemberStatus)) func(*coderv1alpha1.CoderWorkspacePoolMemberStatus) {
+	return func(s *coderv1alpha1.CoderWorkspacePoolMemberStatus) {
+		if edit != nil {
+			edit(s)
+		}
+		s.WorkspaceID = uuid.NewString()
+	}
+}
+
+// usedStatus is a ready member that someone used without a claim.
+var usedStatus = memberPhase(coderv1alpha1.CoderWorkspacePoolMemberPhaseReady,
+	memberCondition(coderv1alpha1.CoderWorkspacePoolMemberConditionReady, metav1.ConditionTrue, "WorkspaceReady"),
+	memberCondition(coderv1alpha1.CoderWorkspacePoolMemberConditionUsed, metav1.ConditionTrue, "Used"))
+
+// gone reports whether the member no longer exists.
+func (e *poolMemberEnv) gone(t *testing.T, key types.NamespacedName) bool {
+	t.Helper()
+	return e.reconcileMember(t, key, 0) == nil
+}
+
+// TestPoolDeleteOrder: on a scale-down the pool deletes current members
+// without a create request first, then members that are not ready (newest
+// first), then ready members (oldest first).
+func TestPoolDeleteOrder(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(5)) })
+	now := e.clock.Now()
+	readyNew := e.createMember(t, nil)
+	e.setMemberStatus(t, readyNew, withWorkspace(e.readyAt(now.Add(-time.Minute))))
+	readyOld := e.createMember(t, nil)
+	e.setMemberStatus(t, readyOld, withWorkspace(e.readyAt(now.Add(-time.Hour))))
+	building := e.createMember(t, nil)
+	e.setMemberStatus(t, building, withWorkspace(nil))
+	time.Sleep(1100 * time.Millisecond) // creationTimestamp has second precision.
+	buildingNew := e.createMember(t, nil)
+	e.setMemberStatus(t, buildingNew, withWorkspace(nil))
+	pending := e.createMember(t, nil)
+	e.reconcilePool(t, 1)
+	require.Len(t, e.poolMembers(t, e.pool.Name), 5)
+
+	for _, step := range []struct {
+		replicas int32
+		deleted  types.NamespacedName
+	}{{4, pending}, {3, buildingNew}, {2, building}, {1, readyOld}} {
+		e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(step.replicas) })
+		e.reconcilePool(t, 1)
+		require.True(t, e.gone(t, step.deleted), "replicas %d deletes %s", step.replicas, step.deleted.Name)
+		require.Len(t, e.poolMembers(t, e.pool.Name), int(step.replicas))
+	}
+	require.False(t, e.gone(t, readyNew))
+}
+
+// TestPoolNewBeforeOld: after a version change, outdated members that are
+// not ready go at once, and outdated ready members stay claimable until
+// current members are ready, oldest first.
+func TestPoolNewBeforeOld(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) {
+		s.Replicas, s.MaxInFlight = new(int32(2)), new(int32(2))
+	})
+	v0 := e.fake.addVersion(e.tplID, "v0", codersdk.ProvisionerJobSucceeded).String()
+	outdated := func(m *coderv1alpha1.CoderWorkspacePoolMember) { m.Spec.TemplateVersionID = v0 }
+	now := e.clock.Now()
+	oldest := e.createMember(t, outdated)
+	e.setMemberStatus(t, oldest, withWorkspace(e.readyAt(now.Add(-2*time.Hour))))
+	older := e.createMember(t, outdated)
+	e.setMemberStatus(t, older, withWorkspace(e.readyAt(now.Add(-time.Hour))))
+	building := e.createMember(t, outdated)
+	e.setMemberStatus(t, building, withWorkspace(nil))
+
+	e.reconcilePool(t, 2)
+	require.True(t, e.gone(t, building), "an outdated member that is not ready goes at once")
+	require.False(t, e.gone(t, oldest))
+	require.False(t, e.gone(t, older))
+	var current []types.NamespacedName
+	for _, m := range e.poolMembers(t, e.pool.Name) {
+		if m.Spec.TemplateVersionID != v0 {
+			current = append(current, memberKey(m))
+		}
+	}
+	require.Len(t, current, 2)
+
+	e.setMemberStatus(t, current[0], withWorkspace(e.readyAt(now)))
+	e.reconcilePool(t, 1)
+	require.True(t, e.gone(t, oldest), "one current ready member replaces the oldest outdated one")
+	require.False(t, e.gone(t, older))
+	e.setMemberStatus(t, current[1], withWorkspace(e.readyAt(now)))
+	e.reconcilePool(t, 1)
+	require.True(t, e.gone(t, older))
+	requirePool(t, e.reconcilePool(t, 1), "Ready", true, false)
+}
+
+// TestPoolReleasesUsedMember: the pool releases a member that someone used
+// and never deletes it, also not on a scale to zero (mutation check 5). A
+// member that its reconciler marks used after the pool's list answers 409
+// to the delete and stays.
+func TestPoolReleasesUsedMember(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(2)) })
+	used := e.createMember(t, nil)
+	e.setMemberStatus(t, used, withWorkspace(usedStatus))
+	ready := e.createMember(t, nil)
+	e.setMemberStatus(t, ready, withWorkspace(readyStatus))
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(0)) })
+
+	pool := e.reconcilePool(t, 2)
+	require.True(t, e.gone(t, ready))
+	released := e.reconcileMember(t, used, 0)
+	require.NotNil(t, released, "a used member is never deleted")
+	require.Nil(t, metav1.GetControllerOf(released), "the pool released it")
+	require.Equal(t, string(pool.UID), released.Labels[coderv1alpha1.CoderWorkspacePoolSelectorLabel])
+	require.Equal(t, int32(1), pool.Status.UsedReplicas)
+	require.Zero(t, pool.Status.Replicas)
+
+	// The member reconciler marks the member used between the list and the
+	// delete: the resourceVersion precondition refuses the delete.
+	late := e.createMember(t, nil)
+	e.setMemberStatus(t, late, withWorkspace(readyStatus))
+	hook := deleteHookClient{Client: k8sClient, deleted: new(false), hook: func(m *coderv1alpha1.CoderWorkspacePoolMember) {
+		e.setMemberStatus(t, memberKey(*m), usedStatus)
+	}}
+	r := &controller.CoderWorkspacePoolReconciler{Client: hook, 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.True(t, apierrors.IsConflict(err), "the delete conflicts: %v", err)
+	e.reconcilePool(t, 1)
+	require.Nil(t, metav1.GetControllerOf(e.reconcileMember(t, late, 0)), "the next reconcile releases it")
+}
+
+// TestPoolConsentLossDeletesUnclaimed: when the admin removes the owner or
+// the template, or the owner gains a role, the pool deletes every unclaimed
+// member, because each one holds the owner's token. Bound and used members
+// stay.
+func TestPoolConsentLossDeletesUnclaimed(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []struct {
+		name, reason string
+		lose         func(t *testing.T, e *poolMemberEnv)
+	}{
+		{"template removed", "TemplateNotAllowed", func(t *testing.T, e *poolMemberEnv) { e.allowOwner(t, e.tester.String(), "default.other") }},
+		{"owner removed", "OwnerNotAllowed", func(t *testing.T, e *poolMemberEnv) { e.allowOwner(t, uuid.NewString(), "default.docker") }},
+		{"owner gains an admin role", "OwnerNotEligible", func(_ *testing.T, e *poolMemberEnv) {
+			e.fake.updateUser(e.tester, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{{Name: codersdk.RoleOwner}} })
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			e := newPoolMemberEnv(t)
+			e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Replicas = new(int32(4)) })
+			ready := e.createMember(t, nil)
+			e.setMemberStatus(t, ready, withWorkspace(readyStatus))
+			pending := e.createMember(t, nil)
+			used := e.createMember(t, nil)
+			e.setMemberStatus(t, used, withWorkspace(usedStatus))
+			bound := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) {
+				m.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: "build-1", UID: uuid.NewString()}
+			})
+			e.setMemberStatus(t, bound, withWorkspace(readyStatus))
+			tc.lose(t, e)
+
+			pool := e.reconcilePool(t, 1)
+			require.Equal(t, tc.reason, pool.Status.Reason, pool.Status.Message)
+			require.True(t, e.gone(t, ready))
+			require.True(t, e.gone(t, pending))
+			require.False(t, e.gone(t, used))
+			require.False(t, e.gone(t, bound))
+			require.False(t, meta.IsStatusConditionTrue(pool.Status.Conditions, coderv1alpha1.CoderWorkspacePoolConditionReady))
+		})
+	}
+}