fix: canary scaledown with maxsurge

test/e2e/canary_test.go

@@ -112,6 +112,58 @@ func (s *CanarySuite) TestRolloutScalingWhenPaused() {
 		ExpectCanaryStablePodCount(1, 3)
 }
 
+
+// TestRolloutWithMaxSurgeScalingDuringUpdate verifies behavior when scaling a rollout up/down in middle of update and with maxSurge 100%
+func (s *CanarySuite) TestRolloutWithMaxSurgeScalingDuringUpdate() {
+	s.Given().
+		HealthyRollout(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: updatescaling
+spec:
+  replicas: 4
+  strategy:
+    canary:
+      maxSurge: 100
+  selector:
+    matchLabels:
+      app: updatescaling
+  template:
+    metadata:
+      labels:
+        app: updatescaling
+    spec:
+      containers:
+      - name: updatescaling
+        image: nginx:1.19-alpine
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 1m`).
+		When().
+		PatchSpec(`
+spec:
+  template:
+    spec:
+      containers:
+      - name: updatescaling
+        command: [/bad-command]`).
+		WaitForRolloutReplicas(7).
+		Then().
+		ExpectCanaryStablePodCount(4, 3).
+		When().
+		ScaleRollout(8).
+		WaitForRolloutReplicas(11).
+		Then().
+		ExpectCanaryStablePodCount(8, 3).
+		When().
+		ScaleRollout(4).
+		WaitForRolloutReplicas(7).
+		Then().
+		ExpectCanaryStablePodCount(4, 3)
+}
+
 // TestRolloutScalingDuringUpdate verifies behavior when scaling a rollout up/down in middle of update
 func (s *CanarySuite) TestRolloutScalingDuringUpdate() {
 	s.Given().
@@ -156,12 +208,12 @@ spec:
 		ScaleRollout(8).
 		WaitForRolloutReplicas(10).
 		Then().
-		// NOTE: the numbers below may change in the future.
-		// See: https://github.com/argoproj/argo-rollouts/issues/738
 		ExpectCanaryStablePodCount(6, 4).
 		When().
-		ScaleRollout(4)
-	// WaitForRolloutReplicas(4) // this doesn't work yet (bug)
+		ScaleRollout(4).
+	    WaitForRolloutReplicas(6).
+		Then().
+		ExpectCanaryStablePodCount(4, 2)
 }
 
 // TestReduceWeightAndHonorMaxUnavailable verifies we honor maxUnavailable when decreasing weight or aborting

utils/replicaset/canary.go

@@ -180,39 +180,44 @@ func CalculateReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS *appsv1.Re
 	// weight (e.g. we are aborting), then we can ignore pod availability of the canaryRS.
 	isIncreasing := newRS == nil || desiredNewRSReplicaCount >= *newRS.Spec.Replicas
 	replicasToScaleDown := GetReplicasForScaleDown(newRS, !isIncreasing) + GetReplicasForScaleDown(stableRS, isIncreasing)
-
 	if replicasToScaleDown <= minAvailableReplicaCount {
 		// Cannot scale down stableRS or newRS without going below min available replica count
 		return newRSReplicaCount, stableRSReplicaCount
 	}
 
 	scaleDownCount := replicasToScaleDown - minAvailableReplicaCount
+	// default to any value, will be calculated in later steps --> maxAllowed value - updated RS (to be within maxSurge)
+	allowedAvailableReplicaCount := int32(0)
+	if !isIncreasing {
+		// Skip scalingDown Stable replicaSet when Canary availability is not taken into calculation for scaleDown
+		newRSReplicaCount, allowedAvailableReplicaCount = calculateScaleDownReplicaCount(newRS, desiredNewRSReplicaCount, maxReplicaCountAllowed, scaleDownCount, newRSReplicaCount)
+		if allowedAvailableReplicaCount < stableRSReplicaCount {
+			// ScaleDown stableAvailableSet to allowed value to adhere to maxSurge
+			stableRSReplicaCount = allowedAvailableReplicaCount
+		}
+	} else {
+		// Skip scalingDown canary replicaSet when StableSet availability is not taken into calculation for scaleDown
+		stableRSReplicaCount, allowedAvailableReplicaCount = calculateScaleDownReplicaCount(stableRS, desiredStableRSReplicaCount, maxReplicaCountAllowed, scaleDownCount, stableRSReplicaCount)
+		if allowedAvailableReplicaCount < newRSReplicaCount {
+			// ScaleDown canaryAllowedSet to allowed value to adhere to maxSurge
+			newRSReplicaCount = allowedAvailableReplicaCount
+		}
+	}
+	return newRSReplicaCount, stableRSReplicaCount
+}
 
-	if newRS != nil && *newRS.Spec.Replicas > desiredNewRSReplicaCount {
+func calculateScaleDownReplicaCount(replicaSet *appsv1.ReplicaSet, desireRSReplicaCount int32, maxReplicaCountAllowed int32, scaleDownCount int32, updatedReplicaCount int32) (int32, int32) {
+	if replicaSet != nil && *replicaSet.Spec.Replicas > desireRSReplicaCount {
 		// if the controller doesn't have to use every replica to achieve the desired count, it only scales down to the
 		// desired count.
-		if *newRS.Spec.Replicas-scaleDownCount < desiredNewRSReplicaCount {
-			newRSReplicaCount = desiredNewRSReplicaCount
-			// Calculating how many replicas were used to scale down to the desired count
-			scaleDownCount = scaleDownCount - (*newRS.Spec.Replicas - desiredNewRSReplicaCount)
+		if *replicaSet.Spec.Replicas-scaleDownCount < desireRSReplicaCount {
+			updatedReplicaCount = desireRSReplicaCount
 		} else {
 			// The controller is using every replica it can to get closer to desired state.
-			newRSReplicaCount = *newRS.Spec.Replicas - scaleDownCount
-			scaleDownCount = 0
+			updatedReplicaCount = *replicaSet.Spec.Replicas - scaleDownCount
 		}
 	}
-
-	if scaleStableRS && *stableRS.Spec.Replicas > desiredStableRSReplicaCount {
-		// This follows the same logic as scaling down the newRS except with the stableRS and it does not need to
-		// set the scaleDownCount again since it's not used again
-		if *stableRS.Spec.Replicas-scaleDownCount < desiredStableRSReplicaCount {
-			stableRSReplicaCount = desiredStableRSReplicaCount
-		} else {
-			stableRSReplicaCount = *stableRS.Spec.Replicas - scaleDownCount
-		}
-	}
-
-	return newRSReplicaCount, stableRSReplicaCount
+	return updatedReplicaCount, maxReplicaCountAllowed - updatedReplicaCount
 }
 
 // BeforeStartingStep checks if canary rollout is at the starting step