Honor MaxSurge and MaxUnavailable after last step

rollout/canary.go

@@ -91,7 +91,7 @@ func (c *RolloutController) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*a
 func (c *RolloutController) reconcileStableRS(olderRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet, stableRS *appsv1.ReplicaSet, rollout *v1alpha1.Rollout) (bool, error) {
 	logCtx := logutil.WithRollout(rollout)
 	if !replicasetutil.CheckStableRSExists(newRS, stableRS) {
-		logCtx.Info("No StableRS exists to reconcile")
+		logCtx.Info("No StableRS exists to reconcile or matches newRS")
 		return false, nil
 	}
 	_, stableRSReplicaCount := replicasetutil.CalculateReplicaCountsForCanary(rollout, newRS, stableRS, olderRSs)
@@ -253,16 +253,22 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe
 	}
 
 	if stepCount == 0 {
-		logCtx.Info("Rollout has no steps so setting stableRS status to currentPodHash")
-		newStatus.Canary.StableRS = newStatus.CurrentPodHash
+		logCtx.Info("Rollout has no steps")
+		if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) {
+			logCtx.Info("New RS has successfully progressed")
+			newStatus.Canary.StableRS = newStatus.CurrentPodHash
+		}
 		newStatus = c.calculateRolloutConditions(r, newStatus, allRSs, newRS)
 		return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false))
 	}
 
 	if *currentStepIndex == stepCount {
 		logCtx.Info("Rollout has executed every step")
 		newStatus.CurrentStepIndex = &stepCount
-		newStatus.Canary.StableRS = newStatus.CurrentPodHash
+		if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) {
+			logCtx.Info("New RS has successfully progressed")
+			newStatus.Canary.StableRS = newStatus.CurrentPodHash
+		}
 		newStatus = c.calculateRolloutConditions(r, newStatus, allRSs, newRS)
 		return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false))
 	}
@@ -272,7 +278,6 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe
 		newStatus.CurrentStepIndex = currentStepIndex
 		if int(*currentStepIndex) == len(r.Spec.Strategy.CanaryStrategy.Steps) {
 			c.recorder.Event(r, corev1.EventTypeNormal, "SettingStableRS", "Completed all steps")
-			newStatus.Canary.StableRS = newStatus.CurrentPodHash
 		}
 		logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex)
 		c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex))

rollout/canary_test.go

@@ -220,9 +220,6 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) {
 	patch := f.getPatchedRollout(patchIndex)
 	expectedPatchTemplate := `{
 	"status":{
-		"canary": {
-			"stableRS":"` + rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `"
-		},
 		"pauseStartTime": null,
 		"conditions" : %s,
 		"currentStepIndex": 1
@@ -620,6 +617,49 @@ func TestRollBackToStable(t *testing.T) {
 	assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
 }
 
+func TestGradualShiftToNewStable(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	steps := []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}
+	r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(3), intstr.FromInt(0))
+
+	r2 := bumpVersion(r1)
+	rs2 := newReplicaSetWithStatus(r2, 4, 4)
+
+	rs1 := newReplicaSetWithStatus(r1, 9, 9)
+	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	f.kubeobjects = append(f.kubeobjects, rs1, rs2)
+	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+
+	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 13, 4, 13, false)
+	maxSurge := intstr.FromInt(3)
+	r2.Spec.Strategy.CanaryStrategy.MaxSurge = &maxSurge
+	r2.Status.CurrentPodHash = rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	f.rolloutLister = append(f.rolloutLister, r2)
+	f.objects = append(f.objects, r2)
+
+	updatedR2SIndex := f.expectUpdateReplicaSetAction(rs1)
+	patchIndex := f.expectPatchRolloutAction(r1)
+	f.run(getKey(r2, t))
+
+	updatedRS2 := f.getUpdatedReplicaSet(updatedR2SIndex)
+	assert.Equal(t, rs1.Name, updatedRS2.Name)
+	assert.Equal(t, int32(6), *updatedRS2.Spec.Replicas)
+
+	expectedPatchWithoutSub := `{
+		"status":{
+			"conditions": %s
+		}
+	}`
+	newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false)
+	expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newConditions)
+	patch := f.getPatchedRollout(patchIndex)
+	assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
+}
+
 func TestRollBackToStableAndStepChange(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
@@ -695,9 +735,6 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) {
 	patch := f.getPatchedRollout(patchIndex)
 	expectedPatch := `{
 		"status":{
-			"canary":{
-				"stableRS":"` + rs3.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `"
-			},
 			"currentStepIndex":1,
 			"conditions": %s
 		}

utils/replicaset/canary.go

@@ -140,15 +140,15 @@ func CalculateReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS *appsv1.Re
 	}
 
 	minAvailableReplicaCount := rolloutSpecReplica - MaxUnavailable(rollout)
-	totalCurrentAvailableReplicaCount := GetAvailableReplicaCountForReplicaSets(allRSs)
-	scaleDownCount := totalCurrentAvailableReplicaCount - minAvailableReplicaCount
+
+	totalAvailableOlderReplicaCount := GetAvailableReplicaCountForReplicaSets(oldRSs)
+	scaleDownCount := GetReplicasForScaleDown(newRS) + GetReplicasForScaleDown(stableRS) + totalAvailableOlderReplicaCount - minAvailableReplicaCount
+
 	if scaleDownCount <= 0 {
 		// Cannot scale down stableRS or newRS without going below min available replica count
 		return newRSReplicaCount, stableRSReplicaCount
 	}
 
-	totalAvailableOlderReplicaCount := GetAvailableReplicaCountForReplicaSets(oldRSs)
-
 	if scaleDownCount <= totalAvailableOlderReplicaCount {
 		//Need to scale down older replicas before scaling down the newRS or stableRS.
 		return newRSReplicaCount, stableRSReplicaCount
@@ -196,6 +196,22 @@ func CheckStableRSExists(newRS, stableRS *appsv1.ReplicaSet) bool {
 	return true
 }
 
+// GetReplicasForScaleDown returns the number of replicas to consider for scaling down.
+func GetReplicasForScaleDown(rs *appsv1.ReplicaSet) int32 {
+	if rs == nil {
+		return int32(0)
+	}
+	if *rs.Spec.Replicas < rs.Status.AvailableReplicas {
+		// The ReplicaSet is already going to scale down replicas since the availableReplica count is bigger
+		// than the spec count. The controller uses the .Spec.Replicas to prevent the controller from
+		// assuming the extra replicas (availableReplica - .Spec.Replicas) are going to remain available.
+		// Otherwise, the controller use those extra replicas to scale down more replicas and potentially
+		// violate the min available.
+		return *rs.Spec.Replicas
+	}
+	return rs.Status.AvailableReplicas
+}
+
 // extraReplicaAdded checks if an extra replica is added because the stable and canary replicas count are both
 // rounded up. The controller rounds both of the replica counts when the setWeight does not distribute evenly
 // in order to prevent either from having a 0 replica count.

utils/replicaset/canary_test.go

@@ -69,6 +69,22 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) {
 
 		olderRS *appsv1.ReplicaSet
 	}{
+		{
+			name:                "Do not add extra RSs in scaleDownCount when .Spec.Replica < AvailableReplicas",
+			rolloutSpecReplicas: 10,
+			setWeight:           100,
+			maxSurge:            intstr.FromInt(3),
+			maxUnavailable:      intstr.FromInt(0),
+
+			stableSpecReplica:      7,
+			stableAvailableReplica: 8,
+
+			canarySpecReplica:      6,
+			canaryAvailableReplica: 3,
+
+			expectedStableReplicaCount: 7,
+			expectedCanaryReplicaCount: 6,
+		},
 		{
 			name:                "Use max surge int to scale up canary",
 			rolloutSpecReplicas: 10,
@@ -296,7 +312,7 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) {
 			expectedCanaryReplicaCount: 3,
 		},
 		{
-			name:                "Do not scale down newRS or stable when older RS count >= minAvailable",
+			name:                "Do not scale down newRS or stable when older RS count >= scaleDownCount",
 			rolloutSpecReplicas: 10,
 			setWeight:           30,
 			maxSurge:            intstr.FromInt(0),