From 2f41f146a607f91e364d097482e040428420a6c4 Mon Sep 17 00:00:00 2001 From: Michalis Kargakis Date: Sat, 17 Oct 2026 21:42:47 +0200 Subject: [PATCH] Relax the hold on schedule when the surge itself fails --- docs/how-it-works.md | 5 +- internal/controller/understudy_controller.go | 22 ++++-- .../controller/understudy_controller_test.go | 73 +++++++++++++++++++ 3 files changed, 91 insertions(+), 9 deletions(-) diff --git a/docs/how-it-works.md b/docs/how-it-works.md index 9660036..5187f72 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -109,8 +109,9 @@ relaxed before the eviction is admitted. Voluntary disruption is held until the stand-in is ready. An involuntary deadline is compared with `minSurgeTimeSeconds`; if too little time remains, the hold is relaxed immediately while the surge continues. A hold is also -relaxed after `readinessDeadlineSeconds`, ensuring an unschedulable stand-in -cannot wedge a drain forever. +relaxed after `readinessDeadlineSeconds`, ensuring that neither an +unschedulable stand-in nor a surge the API server refuses (`SurgeReady=False`, +reason `SurgeFailed`) can wedge a drain forever. `holdMode` chooses which of those disruptions are held in the first place. `always` holds both classes, `voluntary-only` holds the unbounded diff --git a/internal/controller/understudy_controller.go b/internal/controller/understudy_controller.go index 82a0318..568b7d1 100644 --- a/internal/controller/understudy_controller.go +++ b/internal/controller/understudy_controller.go @@ -280,12 +280,13 @@ func (r *UnderstudyReconciler) reconcileDoomed(ctx context.Context, cr *appsv1al } metrics.TrackBlocked(req.String(), status.BlockedSince.Time) + // A failed surge still reaches the status patch below: the hold must be + // recorded for readinessDeadlineSeconds to be able to relax it. + var surgeErr error if !surgeActive || cr.Spec.BaselineReplicas != nil && target.Replicas() != base+cr.SurgeReplicasOrDefault() { - if err := target.PatchReplicas(ctx, r.Client, base+cr.SurgeReplicasOrDefault(), &base); err != nil { - return ctrl.Result{}, err - } + surgeErr = target.PatchReplicas(ctx, r.Client, base+cr.SurgeReplicasOrDefault(), &base) } - if !surgeActive { + if surgeErr == nil && !surgeActive { now := metav1.Now() status.LastSurgeTime = &now class := signal.VoluntaryUnbounded @@ -297,9 +298,11 @@ func (r *UnderstudyReconciler) reconcileDoomed(ctx context.Context, cr *appsv1al fmt.Sprintf("node doom (%s) for pod(s) of %s; surging %d -> %d", doomSource(a.worstDoom), target.GetName(), base, base+cr.SurgeReplicasOrDefault())) } - - if err := r.stampDeletionCosts(ctx, a.doomed); err != nil { - return ctrl.Result{}, err + if surgeErr == nil { + surgeErr = r.stampDeletionCosts(ctx, a.doomed) + } + if surgeErr != nil { + logf.FromContext(ctx).Error(surgeErr, "Failed to surge workload", "kind", target.Kind(), "name", target.GetName()) } previousPhase := status.Phase @@ -323,6 +326,11 @@ func (r *UnderstudyReconciler) reconcileDoomed(ctx context.Context, cr *appsv1al } r.eventOnce(cr, &status, corev1.EventTypeWarning, "HoldRelaxed", "Relax", fmt.Sprintf("eviction hold for %s relaxed (%s); eviction can proceed without a ready understudy", target.GetName(), relaxReason)) + case surgeErr != nil: + status.Phase = appsv1alpha1.PhaseSurging + message := fmt.Sprintf("cannot surge %s: %v", target.GetName(), surgeErr) + setCond(&status, cr, appsv1alpha1.ConditionSurgeReady, metav1.ConditionFalse, "SurgeFailed", message) + r.eventOnce(cr, &status, corev1.EventTypeWarning, "SurgeFailed", "Surge", message) default: status.Phase = appsv1alpha1.PhaseSurging setCond(&status, cr, appsv1alpha1.ConditionSurgeReady, metav1.ConditionFalse, diff --git a/internal/controller/understudy_controller_test.go b/internal/controller/understudy_controller_test.go index c505710..f0af203 100644 --- a/internal/controller/understudy_controller_test.go +++ b/internal/controller/understudy_controller_test.go @@ -23,6 +23,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" @@ -184,6 +185,56 @@ func taintNode(node *corev1.Node, key string) { }).Should(Succeed()) } +// freezeReplicas makes the API server refuse every replica change to the +// fixture's Deployment, as a policy engine or a missing permission would. +func freezeReplicas(f *fixture) { + policy := &admissionv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-freeze-replicas"}, + Spec: admissionv1.ValidatingAdmissionPolicySpec{ + FailurePolicy: ptr.To(admissionv1.Fail), + MatchConstraints: &admissionv1.MatchResources{ + ResourceRules: []admissionv1.NamedRuleWithOperations{{ + RuleWithOperations: admissionv1.RuleWithOperations{ + Operations: []admissionv1.OperationType{admissionv1.Update}, + Rule: admissionv1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + }, + }}, + }, + Validations: []admissionv1.Validation{{ + Expression: "object.spec.replicas == oldObject.spec.replicas", + Message: "replicas are frozen", + }}, + }, + } + binding := &admissionv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{Name: policy.Name}, + Spec: admissionv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: policy.Name, + ValidationActions: []admissionv1.ValidationAction{admissionv1.Deny}, + MatchResources: &admissionv1.MatchResources{ + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{corev1.LabelMetadataName: f.ns}, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + Expect(k8sClient.Create(ctx, binding)).To(Succeed()) + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, binding)).To(Succeed()) + Expect(k8sClient.Delete(ctx, policy)).To(Succeed()) + }) + Eventually(func() bool { + dep := getDeployment(f) + dep.Spec.Replicas = ptr.To(*dep.Spec.Replicas + 1) + return apierrors.IsInvalid(k8sClient.Update(ctx, dep, client.DryRunAll)) + }).Should(BeTrue(), "the API server should enforce the policy before the test relies on it") +} + func getDeployment(f *fixture) *appsv1.Deployment { dep := &appsv1.Deployment{} ExpectWithOffset(1, k8sClient.Get(ctx, client.ObjectKeyFromObject(f.dep), dep)).To(Succeed()) @@ -374,6 +425,28 @@ var _ = Describe("Understudy controller", func() { Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) }) + It("fires the never-wedge TTL even when the surge itself is refused", func() { + f := newFixture(func(cr *appsv1alpha1.Understudy) { + cr.Spec.ReadinessDeadlineSeconds = ptr.To(int32(3)) + }) + freezeReplicas(f) + cordonNode(f.node) + + By("recording the hold while the surge is refused") + Eventually(func() string { + if cond := findCond(getCR(f), appsv1alpha1.ConditionSurgeReady); cond != nil { + return cond.Reason + } + return "" + }).Should(Equal("SurgeFailed")) + Expect(getCR(f).Status.BlockedSince).NotTo(BeNil()) + + By("relaxing at the readiness deadline and admitting the eviction") + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseRelaxed)) + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) + Expect(*getDeployment(f).Spec.Replicas).To(Equal(int32(1))) + }) + It("releases immediately when an involuntary deadline is shorter than minSurgeTimeSeconds", func() { f := newFixture(func(cr *appsv1alpha1.Understudy) { cr.Spec.MinSurgeTimeSeconds = ptr.To(int32(300))