fix: replicaset count would flap when interrupting update with new pod spec (argoproj#1479)

Signed-off-by: Jesse Suen <[email protected]>

Author: jessesuen

rollout/canary.go

@@ -185,10 +185,33 @@ func (c *rolloutContext) scaleDownOldReplicaSetsForCanary(oldRSs []*appsv1.Repli
 				desiredReplicaCount = *targetRS.Spec.Replicas - maxScaleDown
 			}
 		} else {
-			// For traffic shaped canary, we leave the old ReplicaSets up until scaleDownDelaySeconds
-			annotationedRSs, desiredReplicaCount, err = c.scaleDownDelayHelper(targetRS, annotationedRSs, rolloutReplicas)
-			if err != nil {
-				return totalScaledDown, err
+			if rolloututil.IsFullyPromoted(c.rollout) || replicasetutil.HasScaleDownDeadline(targetRS) {
+				// If we are fully promoted and we encounter an old ReplicaSet, we can infer that
+				// this ReplicaSet is likely the previous stable and should follow scaleDownDelaySeconds
+				annotationedRSs, desiredReplicaCount, err = c.scaleDownDelayHelper(targetRS, annotationedRSs, rolloutReplicas)
+				if err != nil {
+					return totalScaledDown, err
+				}
+			} else {
+				// If we get here, we are *not* fully promoted and are in the middle of an update.
+				// We just encountered a scaled up ReplicaSet which is neither the stable or canary
+				// and doesn't yet have scale down deadline. This happens when a user changes their
+				// mind in the middle of an V1 -> V2 update, and then applies a V3. We are deciding
+				// what to do with the defunct, intermediate V2 ReplicaSet right now.
+				if replicasetutil.IsReplicaSetReady(c.newRS) && replicasetutil.IsReplicaSetReady(c.stableRS) {
+					// If the both new and old RS are available, we can infer that it is safe to
+					// scale down this ReplicaSet, since traffic should have shifted away from this RS.
+					// TODO: instead of checking availability of canary/stable, a better way to determine
+					// if it is safe to scale this down, is to check if traffic is directed to the RS.
+					// But to do so, we need the new status.canary.weights field in PR:
+					// https://github.com/argoproj/argo-rollouts/pull/1430
+					c.log.Infof("scaling down intermediate RS '%s'", targetRS.Name)
+				} else {
+					// The current and stable ReplicaSets have not reached readiness. This implies
+					// we might not have shifted traffic away from this ReplicaSet so we need to
+					// keep this scaled up.
+					continue
+				}
 			}
 		}
 		if *targetRS.Spec.Replicas == desiredReplicaCount {

rollout/canary_test.go

@@ -706,6 +706,93 @@ func TestCanaryRolloutScaleDownOldRsDontScaleDownTooMuch(t *testing.T) {
 
 }
 
+// TestCanaryDontScaleDownOldRsDuringInterruptedUpdate tests when we need to prevent scale down an
+// intermediate V2 ReplicaSet when applying a V3 spec in the middle of updating a traffic routed
+// canary going from V1 -> V2 (i.e. because we haven't shifted traffic away from V2 yet).
+func TestCanaryDontScaleDownOldRsDuringInterruptedUpdate(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	steps := []v1alpha1.CanaryStep{
+		{
+			SetWeight: pointer.Int32Ptr(100),
+		},
+		{
+			Pause: &v1alpha1.RolloutPause{},
+		},
+	}
+	r1 := newCanaryRollout("foo", 5, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0))
+	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
+		SMI: &v1alpha1.SMITrafficRouting{},
+	}
+	r1.Spec.Strategy.Canary.StableService = "stable-svc"
+	r1.Spec.Strategy.Canary.CanaryService = "canary-svc"
+	r2 := bumpVersion(r1)
+	r3 := bumpVersion(r2)
+
+	stableSvc := newService("stable-svc", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: r1.Status.CurrentPodHash}, r1)
+	canarySvc := newService("canary-svc", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: r3.Status.CurrentPodHash}, r3)
+	r3.Status.StableRS = r1.Status.CurrentPodHash
+
+	rs1 := newReplicaSetWithStatus(r1, 5, 5)
+	rs2 := newReplicaSetWithStatus(r2, 5, 5)
+	rs3 := newReplicaSetWithStatus(r3, 5, 0)
+
+	f.objects = append(f.objects, r3)
+	f.kubeobjects = append(f.kubeobjects, rs1, rs2, rs3, canarySvc, stableSvc)
+	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2, rs3)
+	f.serviceLister = append(f.serviceLister, canarySvc, stableSvc)
+
+	f.expectPatchRolloutAction(r3)
+	f.run(getKey(r3, t))
+}
+
+// TestCanaryScaleDownOldRsDuringInterruptedUpdate tests that we proceed with scale down of an
+// intermediate V2 ReplicaSet when applying a V3 spec in the middle of updating a traffic routed
+// canary going from V1 -> V2 (i.e. after we have shifted traffic away from V2). This test is the
+// same as TestCanaryDontScaleDownOldRsDuringUpdate but rs3 is fully available
+func TestCanaryScaleDownOldRsDuringInterruptedUpdate(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	steps := []v1alpha1.CanaryStep{
+		{
+			SetWeight: pointer.Int32Ptr(100),
+		},
+		{
+			Pause: &v1alpha1.RolloutPause{},
+		},
+	}
+	r1 := newCanaryRollout("foo", 5, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0))
+	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
+		SMI: &v1alpha1.SMITrafficRouting{},
+	}
+	r1.Spec.Strategy.Canary.StableService = "stable-svc"
+	r1.Spec.Strategy.Canary.CanaryService = "canary-svc"
+	r2 := bumpVersion(r1)
+	r3 := bumpVersion(r2)
+
+	stableSvc := newService("stable-svc", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: r1.Status.CurrentPodHash}, r1)
+	canarySvc := newService("canary-svc", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: r3.Status.CurrentPodHash}, r3)
+	r3.Status.StableRS = r1.Status.CurrentPodHash
+
+	rs1 := newReplicaSetWithStatus(r1, 5, 5)
+	rs2 := newReplicaSetWithStatus(r2, 5, 5)
+	rs3 := newReplicaSetWithStatus(r3, 5, 5)
+
+	f.objects = append(f.objects, r3)
+	f.kubeobjects = append(f.kubeobjects, rs1, rs2, rs3, canarySvc, stableSvc)
+	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2, rs3)
+	f.serviceLister = append(f.serviceLister, canarySvc, stableSvc)
+
+	f.expectPatchRolloutAction(r3)
+	updatedRSIndex := f.expectUpdateReplicaSetAction(rs2)
+	f.run(getKey(r3, t))
+
+	updatedRs2 := f.getUpdatedReplicaSet(updatedRSIndex)
+	assert.Equal(t, int32(0), *updatedRs2.Spec.Replicas)
+}
+
 func TestRollBackToStable(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()

rollout/service.go

@@ -242,7 +242,7 @@ func (c *rolloutContext) reconcileStableAndCanaryService() error {
 		return err
 	}
 
-	if c.newRSReady() {
+	if replicasetutil.IsReplicaSetReady(c.newRS) {
 		err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.newRS)
 		if err != nil {
 			return err
@@ -267,12 +267,3 @@ func (c *rolloutContext) ensureSVCTargets(svcName string, rs *appsv1.ReplicaSet)
 	}
 	return nil
 }
-
-func (c *rolloutContext) newRSReady() bool {
-	if c.newRS == nil {
-		return false
-	}
-	replicas := c.newRS.Spec.Replicas
-	readyReplicas := c.newRS.Status.ReadyReplicas
-	return replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas
-}

utils/replicaset/replicaset.go

@@ -616,3 +616,13 @@ func GetPodsOwnedByReplicaSet(ctx context.Context, client kubernetes.Interface,
 	}
 	return podOwnedByRS, nil
 }
+
+// IsReplicaSetReady returns if a ReplicaSet is scaled up and its ready count is >= desired count
+func IsReplicaSetReady(rs *appsv1.ReplicaSet) bool {
+	if rs == nil {
+		return false
+	}
+	replicas := rs.Spec.Replicas
+	readyReplicas := rs.Status.ReadyReplicas
+	return replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas
+}

utils/replicaset/replicaset_test.go

@@ -1247,3 +1247,54 @@ spec:
 
 	assert.True(t, PodTemplateEqualIgnoreHash(&live, &desired))
 }
+
+func TestIsReplicaSetReady(t *testing.T) {
+	{
+		assert.False(t, IsReplicaSetReady(nil))
+	}
+	{
+		rs := appsv1.ReplicaSet{
+			Spec: appsv1.ReplicaSetSpec{
+				Replicas: pointer.Int32Ptr(1),
+			},
+			Status: appsv1.ReplicaSetStatus{
+				ReadyReplicas: 0,
+			},
+		}
+		assert.False(t, IsReplicaSetReady(&rs))
+	}
+	{
+		rs := appsv1.ReplicaSet{
+			Spec: appsv1.ReplicaSetSpec{
+				Replicas: pointer.Int32Ptr(1),
+			},
+			Status: appsv1.ReplicaSetStatus{
+				ReadyReplicas: 1,
+			},
+		}
+		assert.True(t, IsReplicaSetReady(&rs))
+	}
+	{
+		rs := appsv1.ReplicaSet{
+			Spec: appsv1.ReplicaSetSpec{
+				Replicas: pointer.Int32Ptr(1),
+			},
+			Status: appsv1.ReplicaSetStatus{
+				ReadyReplicas: 2,
+			},
+		}
+		assert.True(t, IsReplicaSetReady(&rs))
+	}
+	{
+		rs := appsv1.ReplicaSet{
+			Spec: appsv1.ReplicaSetSpec{
+				Replicas: pointer.Int32Ptr(0),
+			},
+			Status: appsv1.ReplicaSetStatus{
+				ReadyReplicas: 0,
+			},
+		}
+		// NOTE: currently consider scaled down replicas as not ready
+		assert.False(t, IsReplicaSetReady(&rs))
+	}
+}