Count a stand-in only once it is available, honouring minReadySeconds

docs/how-it-works.md

@@ -19,10 +19,11 @@ sentinel adapters normalize their observations into a `NodeDoom`.
 **3. Surge.** The controller increases the target workload's replica count and marks
 the old pod with a negative `controller.kubernetes.io/pod-deletion-cost`.
 
-**4. Wait for traffic readiness.** A stand-in counts only when it is Ready, is
-not deleting, and runs on neither the victim node nor another doomed node.
-Readiness gates therefore include load balancer registration. Once enough
-healthy stand-ins exist, the webhook admits the drainer's next retry.
+**4. Wait for traffic readiness.** A stand-in counts only when it has been Ready
+for the target's `minReadySeconds`, is not deleting, and runs on neither the
+victim node nor another doomed node. Readiness gates therefore include load
+balancer registration. Once enough healthy stand-ins exist, the webhook admits
+the drainer's next retry.
 
 **5. Scale back.** After the old pod leaves, the workload returns to its
 original replica count, or the latest requested `baselineReplicas` when capacity

internal/controller/evictionwebhook_test.go

@@ -176,6 +176,26 @@ var _ = Describe("Eviction webhook adapter", func() {
 		Eventually(func() error { return evict(f.pod, false) }).Should(Succeed())
 	})
 
+	It("counts a stand-in only once it has been ready for the target's minReadySeconds", func() {
+		f := newFixture(nil, func(dep *appsv1.Deployment) { dep.Spec.MinReadySeconds = 30 })
+		cordonNode(f.node)
+		expectSurged(f)
+
+		By("holding while the stand-in has been ready for less than minReadySeconds")
+		clearNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-clear"}}
+		Expect(k8sClient.Create(ctx, clearNode)).To(Succeed())
+		standIn := makePod(f.ns, "lead-understudy", f.dep.Name, clearNode.Name, true)
+		Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseSurging))
+		Consistently(func() bool {
+			return apierrors.IsTooManyRequests(evict(f.pod, false)) && getCR(f).Status.Phase == appsv1alpha1.PhaseSurging
+		}).Should(BeTrue())
+
+		By("releasing once the stand-in is available")
+		markPodReadySince(standIn, time.Now().Add(-31*time.Second))
+		Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseReleasing))
+		Eventually(func() error { return evict(f.pod, false) }).Should(Succeed())
+	})
+
 	It("never holds an unsupported target", func() {
 		f := newFixture(func(cr *appsv1alpha1.Understudy) {
 			cr.Spec.TargetRef.Kind = "StatefulSet"

internal/controller/understudy_controller.go

@@ -205,6 +205,7 @@ func (r *UnderstudyReconciler) assessPods(ctx context.Context, cr *appsv1alpha1.
 		return a, err
 	}
 	a.all = podList.Items
+	now := time.Now()
 	for i := range podList.Items {
 		pod := &podList.Items[i]
 		if !serving.Matches(labels.Set(pod.Labels)) || pod.Spec.NodeName == "" || pod.DeletionTimestamp != nil {
@@ -218,7 +219,7 @@ func (r *UnderstudyReconciler) assessPods(ctx context.Context, cr *appsv1alpha1.
 			}
 			continue
 		}
-		if podReady(pod) {
+		if target.PodAvailable(pod, now) {
 			a.viable++
 		}
 	}
@@ -493,15 +494,6 @@ func (r *UnderstudyReconciler) requestsInNamespace(ctx context.Context, namespac
 	return reqs
 }
 
-func podReady(pod *corev1.Pod) bool {
-	for _, c := range pod.Status.Conditions {
-		if c.Type == corev1.PodReady {
-			return c.Status == corev1.ConditionTrue
-		}
-	}
-	return false
-}
-
 func setCond(status *appsv1alpha1.UnderstudyStatus, cr *appsv1alpha1.Understudy,
 	condType string, condStatus metav1.ConditionStatus, reason, msg string) {
 	meta.SetStatusCondition(&status.Conditions, metav1.Condition{

internal/controller/understudy_controller_test.go

@@ -48,7 +48,7 @@ type fixture struct {
 	cr   *appsv1alpha1.Understudy
 }
 
-func newFixture(mutateCR func(*appsv1alpha1.Understudy)) *fixture {
+func newFixture(mutateCR func(*appsv1alpha1.Understudy), mutateDep ...func(*appsv1.Deployment)) *fixture {
 	nsCounter++
 	f := &fixture{ns: fmt.Sprintf("test-%d-%d", GinkgoRandomSeed(), nsCounter)}
 
@@ -60,6 +60,9 @@ func newFixture(mutateCR func(*appsv1alpha1.Understudy)) *fixture {
 	Expect(k8sClient.Create(ctx, f.node)).To(Succeed())
 
 	f.dep = makeDeployment(f.ns, "lead", 1)
+	for _, mutate := range mutateDep {
+		mutate(f.dep)
+	}
 	Expect(k8sClient.Create(ctx, f.dep)).To(Succeed())
 	settleDeployment(f.dep)
 
@@ -144,15 +147,20 @@ func makePod(ns, name, app, node string, ready bool) *corev1.Pod {
 }
 
 func markPodReady(pod *corev1.Pod) {
+	markPodReadySince(pod, time.Now())
+}
+
+func markPodReadySince(pod *corev1.Pod, since time.Time) {
 	Eventually(func() error {
 		current := &corev1.Pod{}
 		if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pod), current); err != nil {
 			return err
 		}
 		current.Status.Phase = corev1.PodRunning
 		current.Status.Conditions = []corev1.PodCondition{{
-			Type:   corev1.PodReady,
-			Status: corev1.ConditionTrue,
+			Type:               corev1.PodReady,
+			Status:             corev1.ConditionTrue,
+			LastTransitionTime: metav1.NewTime(since),
 		}}
 		return k8sClient.Status().Update(ctx, current)
 	}).Should(Succeed())

internal/signal/evictionwebhook/adapter.go

@@ -176,6 +176,7 @@ func (a *Adapter) standInReady(ctx context.Context, target *workload.Target, vic
 		return false
 	}
 	healthy := 0
+	now := time.Now()
 	for i := range pods.Items {
 		p := &pods.Items[i]
 		if p.DeletionTimestamp != nil || p.Spec.NodeName == "" {
@@ -187,11 +188,8 @@ func (a *Adapter) standInReady(ctx context.Context, target *workload.Target, vic
 		if _, doomed := a.registry.Lookup(p.Spec.NodeName); doomed {
 			continue
 		}
-		for _, c := range p.Status.Conditions {
-			if c.Type == corev1.PodReady && c.Status == corev1.ConditionTrue {
-				healthy++
-				break
-			}
+		if target.PodAvailable(p, now) {
+			healthy++
 		}
 	}
 	return healthy >= int(base)

internal/workload/target.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"strconv"
+	"time"
 
 	rolloutsv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	appsv1 "k8s.io/api/apps/v1"
@@ -91,6 +92,29 @@ func (t *Target) Replicas() int32 {
 	return *replicas
 }
 
+// PodAvailable applies the workload controller's own availability rule: Ready
+// for longer than the target's minReadySeconds.
+func (t *Target) PodAvailable(pod *corev1.Pod, now time.Time) bool {
+	minReady := time.Duration(t.minReadySeconds()) * time.Second
+	for _, c := range pod.Status.Conditions {
+		if c.Type == corev1.PodReady {
+			return c.Status == corev1.ConditionTrue &&
+				(minReady == 0 || !c.LastTransitionTime.IsZero() && c.LastTransitionTime.Add(minReady).Before(now))
+		}
+	}
+	return false
+}
+
+func (t *Target) minReadySeconds() int32 {
+	switch obj := t.Object.(type) {
+	case *appsv1.Deployment:
+		return obj.Spec.MinReadySeconds
+	case *rolloutsv1alpha1.Rollout:
+		return obj.Spec.MinReadySeconds
+	}
+	return 0
+}
+
 func (t *Target) revision() string {
 	if ro, ok := t.Object.(*rolloutsv1alpha1.Rollout); ok {
 		return ro.Status.CurrentPodHash

internal/workload/target_test.go

@@ -18,8 +18,11 @@ package workload
 
 import (
 	"testing"
+	"time"
 
 	rolloutsv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 )
@@ -81,3 +84,38 @@ func TestRolloutStandDown(t *testing.T) {
 		})
 	}
 }
+
+func TestPodAvailable(t *testing.T) {
+	now := time.Now()
+	ready := func(status corev1.ConditionStatus, since time.Time) *corev1.Pod {
+		return &corev1.Pod{Status: corev1.PodStatus{Conditions: []corev1.PodCondition{{
+			Type: corev1.PodReady, Status: status, LastTransitionTime: metav1.NewTime(since),
+		}}}}
+	}
+	tests := []struct {
+		name            string
+		pod             *corev1.Pod
+		minReadySeconds int32
+		want            bool
+	}{
+		{"no ready condition", &corev1.Pod{}, 0, false},
+		{"not ready", ready(corev1.ConditionFalse, now.Add(-time.Hour)), 0, false},
+		{"ready without minReadySeconds", ready(corev1.ConditionTrue, now), 0, true},
+		{"ready for less than minReadySeconds", ready(corev1.ConditionTrue, now.Add(-29*time.Second)), 30, false},
+		{"ready for exactly minReadySeconds", ready(corev1.ConditionTrue, now.Add(-30*time.Second)), 30, false},
+		{"ready for longer than minReadySeconds", ready(corev1.ConditionTrue, now.Add(-31*time.Second)), 30, true},
+		{"ready since an unknown time", ready(corev1.ConditionTrue, time.Time{}), 30, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for _, target := range []*Target{
+				{Object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{MinReadySeconds: tt.minReadySeconds}}},
+				{Object: &rolloutsv1alpha1.Rollout{Spec: rolloutsv1alpha1.RolloutSpec{MinReadySeconds: tt.minReadySeconds}}},
+			} {
+				if got := target.PodAvailable(tt.pod, now); got != tt.want {
+					t.Errorf("%s PodAvailable() = %v, want %v", target.Kind(), got, tt.want)
+				}
+			}
+		})
+	}
+}