feat: hold CoderWorkspacePool members at a lowered owner cap, and keep the failure run across a rescale

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

Author: ThomasK33

api/v1alpha1/coderworkspacepool_types.go

@@ -232,6 +232,10 @@ type CoderWorkspacePoolResolved struct {
 	// current members from outdated ones without storing values twice.
 	// +optional
 	ParametersHash string `json:"parametersHash,omitempty"`
+	// ReadyTimeoutSeconds is spec.readyTimeoutSeconds. A change of any of
+	// these values starts a new run of failures, because it can fix one.
+	// +optional
+	ReadyTimeoutSeconds int32 `json:"readyTimeoutSeconds,omitempty"`
 	// ResolvedTime is when the controller last changed these values.
 	// +optional
 	ResolvedTime *metav1.Time `json:"resolvedTime,omitempty"`
@@ -293,16 +297,17 @@ type CoderWorkspacePoolStatus struct {
 	// +optional
 	Resolved *CoderWorkspacePoolResolved `json:"resolved,omitempty"`
 	// ConsecutiveFailures is the number of member failures in a row: the
-	// length of RunFailures. A ready member, a spec change, and a new
-	// version reset it.
+	// length of RunFailures. A ready member and a change of status.resolved
+	// (template, version, preset, parameters, owner, or readyTimeoutSeconds)
+	// reset it. A change of replicas or maxInFlight does not.
 	// +optional
 	ConsecutiveFailures int32 `json:"consecutiveFailures,omitempty"`
 	// LastFailure is the newest member failure.
 	// +optional
 	LastFailure *CoderWorkspacePoolFailure `json:"lastFailure,omitempty"`
 	// FailureRunStart is the start of the current run of failures: the
-	// latest of the last spec or version change and the newest ready time
-	// of a member. Failures at or before it do not count.
+	// latest of the last change of status.resolved and the newest ready
+	// time of a member. Failures at or before it do not count.
 	// +optional
 	FailureRunStart *metav1.Time `json:"failureRunStart,omitempty"`
 	// RunFailures are the member failures of the current run, oldest first,

config/crd/dormant/coder.com_coderworkspacepools.yaml

@@ -307,15 +307,16 @@ spec:
               consecutiveFailures:
                 description: |-
                   ConsecutiveFailures is the number of member failures in a row: the
-                  length of RunFailures. A ready member, a spec change, and a new
-                  version reset it.
+                  length of RunFailures. A ready member and a change of status.resolved
+                  (template, version, preset, parameters, owner, or readyTimeoutSeconds)
+                  reset it. A change of replicas or maxInFlight does not.
                 format: int32
                 type: integer
               failureRunStart:
                 description: |-
                   FailureRunStart is the start of the current run of failures: the
-                  latest of the last spec or version change and the newest ready time
-                  of a member. Failures at or before it do not count.
+                  latest of the last change of status.resolved and the newest ready
+                  time of a member. Failures at or before it do not count.
                 format: date-time
                 type: string
               lastFailure:
@@ -386,6 +387,12 @@ spec:
                   presetID:
                     description: PresetID is the UUID of the preset named by spec.presetName.
                     type: string
+                  readyTimeoutSeconds:
+                    description: |-
+                      ReadyTimeoutSeconds is spec.readyTimeoutSeconds. A change of any of
+                      these values starts a new run of failures, because it can fix one.
+                    format: int32
+                    type: integer
                   resolvedTime:
                     description: ResolvedTime is when the controller last changed
                       these values.

internal/controller/coderworkspacepool_cap_test.go

@@ -0,0 +1,58 @@
+package controller_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/types"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+)
+
+// setOwnerCap sets maxWorkspaces of the control plane's only owner entry.
+func (e *poolMemberEnv) setOwnerCap(t *testing.T, maxWorkspaces int32) {
+	t.Helper()
+	cp := &coderv1alpha1.CoderControlPlane{}
+	require.NoError(t, k8sClient.Get(e.ctx, types.NamespacedName{Namespace: e.ns, Name: "coder"}, cp))
+	cp.Spec.WorkspacePools.Owners[0].MaxWorkspaces = &maxWorkspaces
+	require.NoError(t, k8sClient.Update(e.ctx, cp))
+}
+
+// TestPoolLoweredOwnerCap: an admin lowers maxWorkspaces while members of
+// the pool wait for their create (Codex 4271920249). No member creates a
+// workspace above the new cap: each member checks the cap again, without
+// the cache, right before its create marker. The pool then deletes the
+// members that sent no create request, down to the cap.
+func TestPoolLoweredOwnerCap(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) {
+		s.Replicas, s.MaxInFlight = new(int32(3)), new(int32(3))
+	})
+	e.reconcilePool(t, 2)
+	members := e.poolMembers(t, e.pool.Name)
+	require.Len(t, members, 3)
+	e.setOwnerCap(t, 1)
+
+	// Every member reconciles before the pool sees the new cap.
+	waiting := map[string]bool{}
+	for _, m := range members {
+		got := e.reconcileMember(t, memberKey(m), 2) // The finalizer, then the create.
+		if got.Status.Reason == "OwnerWorkspaceLimit" {
+			require.Nil(t, got.Status.CreateAttemptTime, "a waiting member writes no marker")
+			waiting[got.Name] = true
+		}
+	}
+	require.Equal(t, 1, e.fake.workspaceCount(), "one workspace at maxWorkspaces 1")
+	require.Len(t, waiting, 2)
+
+	// The pool deletes the two members without a create request and keeps
+	// the member that created its workspace.
+	e.reconcilePool(t, 1)
+	for _, m := range e.poolMembers(t, e.pool.Name) {
+		require.Equal(t, waiting[m.Name], !m.DeletionTimestamp.IsZero(), "member %s", m.Name)
+	}
+	requirePool(t, e.reconcilePool(t, 1), "OwnerWorkspaceLimit", false, false)
+	require.Len(t, e.poolMembers(t, e.pool.Name), 3, "no new member: the deleting members wait for their finalizers")
+	require.Equal(t, 1, e.fake.workspaceCount())
+}

internal/controller/coderworkspacepool_controller.go

@@ -89,9 +89,12 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 		pool.Status.Resolved = resolved
 	}
 	resolvedChanged := !poolResolvedEqual(before.Resolved, pool.Status.Resolved)
-	if pool.Generation != before.ObservedGeneration ||
-		(before.Resolved != nil && pool.Status.Resolved != nil && before.Resolved.TemplateVersionID != pool.Status.Resolved.TemplateVersionID) {
-		// A spec change or a new version starts a new run of failures.
+	if resolvedChanged && before.Resolved != nil {
+		// Only an input that can fix a failure starts a new run of failures:
+		// the template, the version, the preset, the parameters, the owner,
+		// or readyTimeoutSeconds. A rescale, for example by an HPA through
+		// /scale, keeps the run, so a pool that keeps rescaling still stops
+		// at the failure limit.
 		pool.Status.FailureRunStart = &metav1.Time{Time: r.Clock.Now()}
 	}
 
@@ -123,6 +126,9 @@ 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 {
+			return ctrl.Result{}, err
+		}
 		step, requeue, err = r.createMembers(ctx, pool, all.Items, &counts)
 		if err != nil {
 			return ctrl.Result{}, err
@@ -147,8 +153,8 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R
 
 // deleteFailedMembers records the failures of the pool's failed unclaimed
 // members in the current run, stores them, and then deletes all those
-// members (plan 6.3 step 6). The run starts at the latest spec or version
-// change or member ready time. A failure counts once, and only when it
+// members (plan 6.3 step 6). The run starts at the latest change of
+// status.resolved or member ready time. A failure counts once, and only when it
 // happened after the run start; a failure in the same second as the start
 // does not count. A failed member with old inputs never counts.
 func (r *CoderWorkspacePoolReconciler) deleteFailedMembers(
@@ -248,7 +254,7 @@ func (r *CoderWorkspacePoolReconciler) createMembers(
 		return nil, 0, nil
 	}
 	if n := len(st.RunFailures); n >= poolFailureLimit {
-		return coderFail("MemberFailureLimit", "%d members failed in a row. Change the pool or wait for a new version.", n), 0, nil
+		return coderFail("MemberFailureLimit", "%d members failed in a row. Change the template, version, preset, parameters, or readyTimeoutSeconds, or wait for a new version.", n), 0, nil
 	} else if n > 0 {
 		last := st.RunFailures[n-1]
 		if retryAt := last.Time.Add(min(poolFailureBackoffBase<<(n-1), poolFailureBackoffCap)); now.Before(retryAt) {
@@ -275,16 +281,7 @@ func (r *CoderWorkspacePoolReconciler) createMembers(
 		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
-	ownerID := uuid.MustParse(st.Resolved.OwnerID) // resolve parsed it.
-	var live int32
-	for i := range all {
-		m := &all[i]
-		id, err := uuid.Parse(m.Spec.OwnerID)
-		if err == nil && id == ownerID && m.Spec.ControlPlaneRef.Name == pool.Spec.ControlPlaneRef.Name &&
-			!meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted) {
-			live++
-		}
-	}
+	live := poolOwnerLiveMembers(all, st.Resolved.OwnerID, pool.Spec.ControlPlaneRef.Name, nil)
 	n := min(need, *pool.Spec.MaxInFlight-counts.building, maxWorkspaces-live)
 	for range n {
 		if err := r.createMember(ctx, pool); err != nil {
@@ -301,6 +298,67 @@ func (r *CoderWorkspacePoolReconciler) createMembers(
 	return nil, 0, nil
 }
 
+// poolOwnerLiveMembers counts the members of one owner on one control plane
+// whose workspace is not deleted, and that match, if match is set.
+func poolOwnerLiveMembers(
+	all []coderv1alpha1.CoderWorkspacePoolMember, ownerID, controlPlane string, match func(*coderv1alpha1.CoderWorkspacePoolMember) bool,
+) int32 {
+	want := uuid.MustParse(ownerID) // Callers pass a checked owner ID.
+	var live int32
+	for i := range all {
+		m := &all[i]
+		id, err := uuid.Parse(m.Spec.OwnerID)
+		if err == nil && id == want && m.Spec.ControlPlaneRef.Name == controlPlane && (match == nil || match(m)) &&
+			!meta.IsStatusConditionTrue(m.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted) {
+			live++
+		}
+	}
+	return live
+}
+
+// trimToOwnerCap deletes the pool's members that sent no create request yet,
+// newest first, while the owner has more live members than maxWorkspaces
+// allows, because an admin lowered it (Codex 4271920249). Such a member only
+// waits at its own cap check until its deadline fails it. The preconditions
+// keep a member that wrote its create marker after the list.
+func (r *CoderWorkspacePoolReconciler) trimToOwnerCap(
+	ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, 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
+	if excess <= 0 {
+		return nil
+	}
+	var pending []*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() &&
+			m.Status.Phase != coderv1alpha1.CoderWorkspacePoolMemberPhaseFailed && !poolMemberMayHaveWorkspace(m) {
+			pending = append(pending, m)
+		}
+	}
+	slices.SortStableFunc(pending, func(a, b *coderv1alpha1.CoderWorkspacePoolMember) int {
+		return b.CreationTimestamp.Compare(a.CreationTimestamp.Time)
+	})
+	pending = pending[:min(int(excess), len(pending))]
+	var errs []error
+	for _, m := range pending {
+		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 pool member %s/%s above the owner cap: %w", m.Namespace, m.Name, err))
+		}
+	}
+	return errors.Join(errs...)
+}
+
 // createMember creates one member that pins the pool's resolved inputs.
 func (r *CoderWorkspacePoolReconciler) createMember(ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool) error {
 	resolved := pool.Status.Resolved
@@ -317,7 +375,7 @@ func (r *CoderWorkspacePoolReconciler) createMember(ctx context.Context, pool *c
 			ControlPlaneRef: pool.Spec.ControlPlaneRef, PoolName: pool.Name,
 			OwnerID: resolved.OwnerID, OrganizationID: resolved.OrganizationID, TemplateID: resolved.TemplateID,
 			TemplateVersionID: resolved.TemplateVersionID, PresetID: resolved.PresetID,
-			Parameters: params, ParametersHash: resolved.ParametersHash, ReadyTimeoutSeconds: *pool.Spec.ReadyTimeoutSeconds,
+			Parameters: params, ParametersHash: resolved.ParametersHash, ReadyTimeoutSeconds: resolved.ReadyTimeoutSeconds,
 		},
 	}
 	if err := controllerutil.SetControllerReference(pool, member, r.Scheme); err != nil {
@@ -387,7 +445,7 @@ func (r *CoderWorkspacePoolReconciler) resolve(
 	return &coderv1alpha1.CoderWorkspacePoolResolved{
 		OrganizationID: template.OrganizationID.String(), TemplateID: template.ID.String(),
 		TemplateVersionID: version.ID.String(), TemplateVersionName: version.Name, PresetID: presetID,
-		OwnerID: ownerID.String(), ParametersHash: workspacePoolParametersHash(params),
+		OwnerID: ownerID.String(), ParametersHash: workspacePoolParametersHash(params), ReadyTimeoutSeconds: *spec.ReadyTimeoutSeconds,
 	}, nil, nil
 }
 

internal/controller/coderworkspacepool_controller_test.go

@@ -96,8 +96,8 @@ func TestPoolResolvesInputs(t *testing.T) {
 	require.Equal(t, coderv1alpha1.CoderWorkspacePoolResolved{
 		OrganizationID: e.orgID.String(), TemplateID: e.tplID.String(), TemplateVersionID: e.v1.String(), TemplateVersionName: "v1",
 		PresetID: presetID.String(), OwnerID: e.tester.String(),
-		ParametersHash: parametersHash(coderv1alpha1.CoderWorkspacePoolMemberParameter{Name: "region", Value: "eu"}),
-		ResolvedTime:   resolved.ResolvedTime,
+		ParametersHash:      parametersHash(coderv1alpha1.CoderWorkspacePoolMemberParameter{Name: "region", Value: "eu"}),
+		ReadyTimeoutSeconds: 900, ResolvedTime: resolved.ResolvedTime,
 	}, *resolved)
 	require.True(t, resolved.ResolvedTime.Time.Equal(e.clock.Now()))
 	// The same inputs keep the time.

internal/controller/coderworkspacepool_failure_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/stretchr/testify/require"
+	autoscalingv1 "k8s.io/api/autoscaling/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -48,7 +49,7 @@ func (e *poolMemberEnv) readyAt(at time.Time) func(*coderv1alpha1.CoderWorkspace
 func (e *poolMemberEnv) tick() { e.clock.SetTime(e.clock.Now().Add(time.Second)) }
 
 // TestPoolFailureBackoffAndLimit: the pool counts each failed member once,
-// deletes it, backs off, and stops after 5 failures in a row until its spec
+// deletes it, backs off, and stops after 5 failures in a row until an input
 // changes. A member that becomes ready resets the count.
 func TestPoolFailureBackoffAndLimit(t *testing.T) {
 	t.Parallel()
@@ -88,9 +89,9 @@ func TestPoolFailureBackoffAndLimit(t *testing.T) {
 	e.reconcilePool(t, 1)
 	require.Empty(t, e.poolMembers(t, e.pool.Name), "the limit holds")
 
-	// A spec change lifts the limit, and a ready member resets the count.
-	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.MaxInFlight = new(int32(4)) })
-	pool := e.reconcilePool(t, 1)
+	// An input change lifts the limit, and a ready member resets the count.
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.ReadyTimeoutSeconds = new(int32(1800)) })
+	pool := e.reconcilePool(t, 2) // Publishes the new inputs, then creates.
 	require.Zero(t, pool.Status.ConsecutiveFailures)
 	failNewest(t)
 	require.Equal(t, int32(1), e.reconcilePool(t, 1).Status.ConsecutiveFailures)
@@ -159,7 +160,7 @@ func TestPoolFailureRunOrder(t *testing.T) {
 	require.Zero(t, pool.Status.ConsecutiveFailures)
 }
 
-// TestPoolFailureRunResets: failures from before a spec change or a new
+// TestPoolFailureRunResets: failures from before an input change or a new
 // version never count, also when their members are still there at the
 // change, and a failed member with old inputs never counts.
 func TestPoolFailureRunResets(t *testing.T) {
@@ -171,9 +172,9 @@ func TestPoolFailureRunResets(t *testing.T) {
 		e.setMemberStatus(t, e.createMember(t, nil), e.failedStatus("BuildFailed"))
 	}
 	e.tick()
-	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.MaxInFlight = new(int32(3)) })
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.ReadyTimeoutSeconds = new(int32(1800)) })
 	pool := e.reconcilePool(t, 1)
-	require.Zero(t, pool.Status.ConsecutiveFailures, "the spec change started a new run")
+	require.Zero(t, pool.Status.ConsecutiveFailures, "the input change started a new run")
 
 	e.tick()
 	outdated := e.createMember(t, func(m *coderv1alpha1.CoderWorkspacePoolMember) {
@@ -198,6 +199,43 @@ func TestPoolFailureRunResets(t *testing.T) {
 	require.Zero(t, e.reconcilePool(t, 1).Status.ConsecutiveFailures, "the new version started a new run")
 }
 
+// TestPoolFailureRunSurvivesRescale: only an input that can fix a failure
+// starts a new run. A replicas change through /scale, as an HPA sends it,
+// and a maxInFlight change keep the run, so a pool that keeps rescaling
+// still stops at the failure limit. A new readyTimeoutSeconds can fix a
+// DeadlineExceeded failure, so it starts a new run.
+func TestPoolFailureRunSurvivesRescale(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	e.reconcilePool(t, 1)
+	e.tick()
+	e.setMemberStatus(t, memberKey(e.poolMembers(t, e.pool.Name)[0]), e.failedStatus("DeadlineExceeded"))
+	require.Equal(t, int32(1), e.reconcilePool(t, 1).Status.ConsecutiveFailures)
+
+	scale := &autoscalingv1.Scale{}
+	require.NoError(t, k8sClient.SubResource("scale").Get(e.ctx, e.pool, scale))
+	scale.Spec.Replicas = 2
+	require.NoError(t, k8sClient.SubResource("scale").Update(e.ctx, e.pool, client.WithSubResourceBody(scale)))
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.MaxInFlight = new(int32(4)) })
+	e.tick()
+	pool := e.reconcilePool(t, 1)
+	require.Equal(t, pool.Generation, pool.Status.ObservedGeneration)
+	require.Equal(t, int32(1), pool.Status.ConsecutiveFailures, "a rescale keeps the run")
+	requirePool(t, pool, "FailureBackoff", false, false)
+
+	e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.ReadyTimeoutSeconds = new(int32(1800)) })
+	e.tick()
+	pool = e.reconcilePool(t, 1)
+	require.Zero(t, pool.Status.ConsecutiveFailures, "a new readyTimeoutSeconds starts a new run")
+	require.Equal(t, int32(1800), pool.Status.Resolved.ReadyTimeoutSeconds)
+	e.reconcilePool(t, 1)
+	members := e.poolMembers(t, e.pool.Name)
+	require.Len(t, members, 2)
+	for _, m := range members {
+		require.Equal(t, int32(1800), m.Spec.ReadyTimeoutSeconds, "new members pin the resolved timeout")
+	}
+}
+
 // deleteHookClient runs hook before each member delete and fails the pool
 // status writes after a member delete when failStatus is set.
 type deleteHookClient struct {