diff --git a/api/v1alpha1/coderworkspacepool_types.go b/api/v1alpha1/coderworkspacepool_types.go index d02fdf17..35f5eaee 100644 --- a/api/v1alpha1/coderworkspacepool_types.go +++ b/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, diff --git a/config/crd/dormant/coder.com_coderworkspacepools.yaml b/config/crd/dormant/coder.com_coderworkspacepools.yaml index 9ed28e11..3a56a2df 100644 --- a/config/crd/dormant/coder.com_coderworkspacepools.yaml +++ b/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. diff --git a/internal/controller/coderworkspacepool_cap_test.go b/internal/controller/coderworkspacepool_cap_test.go new file mode 100644 index 00000000..db35590e --- /dev/null +++ b/internal/controller/coderworkspacepool_cap_test.go @@ -0,0 +1,135 @@ +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. +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)) +} + +// 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 +} + +// 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()) +} + +// TestPoolLoweredOwnerCapDeletesOnce: members that the pool deleted above +// the cap count no longer while their finalizers run, so the next reconcile +// deletes no more of them. +func TestPoolLoweredOwnerCapDeletesOnce(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) + for _, m := range members { + e.reconcileMember(t, memberKey(m), 1) // The finalizer only. + } + e.setOwnerCap(t, 1) + e.reconcilePool(t, 2) + deleting := 0 + for _, m := range e.poolMembers(t, e.pool.Name) { + if !m.DeletionTimestamp.IsZero() { + deleting++ + } + } + 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) +} diff --git a/internal/controller/coderworkspacepool_controller.go b/internal/controller/coderworkspacepool_controller.go index f87acf44..50383a6d 100644 --- a/internal/controller/coderworkspacepool_controller.go +++ b/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 before.Resolved != nil && pool.Status.Resolved != nil && poolRunInputsChanged(before.Resolved, pool.Status.Resolved) { + // 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()} } @@ -109,7 +112,7 @@ func (r *CoderWorkspacePoolReconciler) Reconcile(ctx context.Context, req ctrl.R members = append(members, m) } } - if err := r.deleteFailedMembers(ctx, pool, before, members); err != nil { + if err := r.deleteFailedMembers(ctx, pool, before, members, step == nil); err != nil { return ctrl.Result{}, err } counts := countPoolMembers(pool, members) @@ -123,7 +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: - step, requeue, err = r.createMembers(ctx, pool, all.Items, &counts) + var maxWorkspaces int32 + if maxWorkspaces, step, err = r.freshOwnerCap(ctx, pool); err != nil { + return ctrl.Result{}, err + } + 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 } @@ -147,13 +161,18 @@ 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. +// +// While the pool's inputs do not resolve (count is false), failures do not +// count either: the members then fail on inputs that the pool no longer +// selects, or on a Coder that does not answer, and a revert of a bad spec +// edit resolves to the same inputs, which starts no new run. func (r *CoderWorkspacePoolReconciler) deleteFailedMembers( ctx context.Context, pool *coderv1alpha1.CoderWorkspacePool, before *coderv1alpha1.CoderWorkspacePoolStatus, - members []coderv1alpha1.CoderWorkspacePoolMember, + members []coderv1alpha1.CoderWorkspacePoolMember, count bool, ) error { st := &pool.Status var runStart time.Time @@ -184,7 +203,10 @@ func (r *CoderWorkspacePoolReconciler) deleteFailedMembers( }) for _, m := range failed { at := poolMemberFailedAt(m) - if poolMemberInputMismatch(m, pool) != "" || !at.After(runStart) || + // A member with an old readyTimeoutSeconds is current for claims, but + // its failure says nothing about the new timeout. + oldTimeout := st.Resolved != nil && m.Spec.ReadyTimeoutSeconds != st.Resolved.ReadyTimeoutSeconds + if !count || poolMemberInputMismatch(m, pool) != "" || oldTimeout || !at.After(runStart) || slices.ContainsFunc(run, func(f coderv1alpha1.CoderWorkspacePoolFailure) bool { return f.Member == m.Name }) { continue } @@ -234,13 +256,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. @@ -248,7 +294,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 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) { @@ -256,35 +302,9 @@ 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 - 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++ - } - } + // 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 { if err := r.createMember(ctx, pool); err != nil { @@ -301,6 +321,65 @@ 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. A +// deleting member that sent no create request never gets a workspace. +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)) && + (m.DeletionTimestamp.IsZero() || poolMemberMayHaveWorkspace(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. A deleted +// 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, maxWorkspaces int32, all []coderv1alpha1.CoderWorkspacePoolMember, +) error { + excess := poolOwnerLiveMembers(all, pool.Status.Resolved.OwnerID, pool.Spec.ControlPlaneRef.Name, nil) - 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)) + continue + } + m.DeletionTimestamp = &metav1.Time{Time: r.Clock.Now()} + } + 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 +396,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,10 +466,19 @@ 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 } +// poolRunInputsChanged reports a change of the resolved inputs that a build +// uses. A new name of the same version (Coder can rename a version) changes +// no build, so it does not start a new run of failures. +func poolRunInputsChanged(a, b *coderv1alpha1.CoderWorkspacePoolResolved) bool { + a, b = a.DeepCopy(), b.DeepCopy() + a.TemplateVersionName, b.TemplateVersionName = "", "" + return !poolResolvedEqual(a, b) +} + // poolResolvedEqual compares two resolved inputs without their time. func poolResolvedEqual(a, b *coderv1alpha1.CoderWorkspacePoolResolved) bool { if a == nil || b == nil { diff --git a/internal/controller/coderworkspacepool_controller_test.go b/internal/controller/coderworkspacepool_controller_test.go index b0cfe9ac..ff32d5dc 100644 --- a/internal/controller/coderworkspacepool_controller_test.go +++ b/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. diff --git a/internal/controller/coderworkspacepool_failure_test.go b/internal/controller/coderworkspacepool_failure_test.go index 05164fd5..50e8c60b 100644 --- a/internal/controller/coderworkspacepool_failure_test.go +++ b/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,74 @@ 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, +// a maxInFlight change, and a new name of the same version 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, and a member that still has the old timeout does not count in it. +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, s.Version = new(int32(4)), coderv1alpha1.CoderWorkspacePoolVersion{ID: e.v1.String()} + }) + 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.fake.renameVersion(e.v1, "v1-renamed") + e.tick() + pool = e.reconcilePool(t, 1) + require.Equal(t, "v1-renamed", pool.Status.Resolved.TemplateVersionName) + require.Equal(t, int32(1), pool.Status.ConsecutiveFailures, "a new name of the same version keeps the run") + + building := e.createMember(t, nil) // Pins the old timeout of 900 s. + 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.tick() + e.setMemberStatus(t, building, e.failedStatus("DeadlineExceeded")) + require.Zero(t, e.reconcilePool(t, 1).Status.ConsecutiveFailures, "a failure at the old timeout does not count") + require.True(t, e.gone(t, building)) + 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") + } +} + +// TestPoolFailuresUncountedWhileUnresolved: a spec edit that does not +// resolve leaves status.resolved as it was. Members that fail meanwhile are +// deleted without counting, so reverting the edit, which starts no new run, +// leaves no failures behind. +func TestPoolFailuresUncountedWhileUnresolved(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + pending := e.createMember(t, nil) // The one replica. + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Version.Name = "nope" }) + requirePool(t, e.reconcilePool(t, 1), "TemplateVersionNotFound", false, false) + e.tick() + e.setMemberStatus(t, pending, e.failedStatus("DeadlineExceeded")) + require.Zero(t, e.reconcilePool(t, 1).Status.ConsecutiveFailures) + require.True(t, e.gone(t, pending), "deleted without counting") + e.editPool(t, func(s *coderv1alpha1.CoderWorkspacePoolSpec) { s.Version.Name = "v1" }) + require.Zero(t, e.reconcilePool(t, 1).Status.ConsecutiveFailures) +} + // deleteHookClient runs hook before each member delete and fails the pool // status writes after a member delete when failStatus is set. type deleteHookClient struct { diff --git a/internal/controller/coderworkspacepoolmember_controller.go b/internal/controller/coderworkspacepoolmember_controller.go index 60589111..91ef4033 100644 --- a/internal/controller/coderworkspacepoolmember_controller.go +++ b/internal/controller/coderworkspacepoolmember_controller.go @@ -272,9 +272,50 @@ 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 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 and +// 4274565367). The consent check runs again on the same read. +func (r *CoderWorkspacePoolMemberReconciler) checkOwnerCap( + 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.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 { + return nil, fmt.Errorf("list pool members in namespace %s: %w", member.Namespace, err) + } + created := poolOwnerLiveMembers(all.Items, member.Spec.OwnerID, controlPlane.Name, func(m *coderv1alpha1.CoderWorkspacePoolMember) bool { + return m.UID != member.UID && poolMemberMayHaveWorkspace(m) + }) + if created >= *owner.MaxWorkspaces { + return coderWait("OwnerWorkspaceLimit", "Coder user %s has %d of at most %d workspaces on CoderControlPlane %s.", + member.Spec.OwnerID, created, *owner.MaxWorkspaces, controlPlane.Name), nil + } + return nil, nil +} + // poolMemberInputMismatch names the first pinned input that differs from the // pool, or returns "". A member can only build what its pool builds. func poolMemberInputMismatch(member *coderv1alpha1.CoderWorkspacePoolMember, pool *coderv1alpha1.CoderWorkspacePool) string { diff --git a/internal/controller/coderworkspacepoolmember_controller_test.go b/internal/controller/coderworkspacepoolmember_controller_test.go index c22b1b72..75b3a0c3 100644 --- a/internal/controller/coderworkspacepoolmember_controller_test.go +++ b/internal/controller/coderworkspacepoolmember_controller_test.go @@ -1,6 +1,7 @@ package controller_test import ( + "cmp" "crypto/sha256" "encoding/hex" "fmt" @@ -75,7 +76,7 @@ func (e *poolMemberEnv) resolve(t *testing.T, edit func(*coderv1alpha1.CoderWork require.NoError(t, k8sClient.Get(e.ctx, types.NamespacedName{Namespace: e.ns, Name: e.pool.Name}, e.pool)) e.pool.Status.Resolved = &coderv1alpha1.CoderWorkspacePoolResolved{ OrganizationID: e.orgID.String(), TemplateID: e.tplID.String(), TemplateVersionID: e.v1.String(), TemplateVersionName: "v1", - OwnerID: e.tester.String(), ParametersHash: parametersHash(), + OwnerID: e.tester.String(), ParametersHash: parametersHash(), ReadyTimeoutSeconds: *e.pool.Spec.ReadyTimeoutSeconds, } if edit != nil { edit(e.pool.Status.Resolved) @@ -114,7 +115,7 @@ func (e *poolMemberEnv) createMember(t *testing.T, edit func(*coderv1alpha1.Code ControlPlaneRef: e.pool.Spec.ControlPlaneRef, PoolName: e.pool.Name, OwnerID: resolved.OwnerID, OrganizationID: resolved.OrganizationID, TemplateID: resolved.TemplateID, TemplateVersionID: resolved.TemplateVersionID, PresetID: resolved.PresetID, ParametersHash: resolved.ParametersHash, - ReadyTimeoutSeconds: 900, + ReadyTimeoutSeconds: cmp.Or(resolved.ReadyTimeoutSeconds, 900), }, } if edit != nil { diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 01824e4a..9bcea52c 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -347,6 +347,17 @@ func (f *fakeCoder) archiveVersion(versionID uuid.UUID) { f.versions[versionID] = v } +// renameVersion gives a version a new name, as PATCH /templateversions/{id} +// does. +func (f *fakeCoder) renameVersion(versionID uuid.UUID, name string) { + f.mu.Lock() + defer f.mu.Unlock() + v, ok := f.versions[versionID] + require.True(f.t, ok, "assertion failed: unknown version %s", versionID) + v.Name = name + f.versions[versionID] = v +} + // updateUser edits a user, for example to suspend it. func (f *fakeCoder) updateUser(userID uuid.UUID, edit func(*codersdk.User)) { f.mu.Lock()