fix!: improve basic canary approximation accuracy and honor maxSurge (#…

…1759)

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

rollout/analysis_test.go

@@ -735,7 +735,7 @@ func TestDoNothingWithAnalysisRunsWhileBackgroundAnalysisRunRunning(t *testing.T
 		SetWeight: pointer.Int32Ptr(10),
 	}}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 	r2.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
 		RolloutAnalysis: v1alpha1.RolloutAnalysis{
@@ -792,7 +792,7 @@ func TestDoNothingWhileStepBasedAnalysisRunRunning(t *testing.T) {
 		},
 	}}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 	ar := analysisRun(at, v1alpha1.RolloutTypeStepLabel, r2)
 	ar.Status.Phase = v1alpha1.AnalysisPhaseRunning
@@ -1067,7 +1067,7 @@ func TestPausedOnInconclusiveBackgroundAnalysisRun(t *testing.T) {
 		{SetWeight: pointer.Int32Ptr(30)},
 	}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 	ar := analysisRun(at, v1alpha1.RolloutTypeBackgroundRunLabel, r2)
 	r2.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
@@ -1447,7 +1447,7 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) {
 		{SetWeight: pointer.Int32Ptr(10)},
 	}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 	r2.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
 		RolloutAnalysis: v1alpha1.RolloutAnalysis{

rollout/experiment_test.go

@@ -322,7 +322,7 @@ func TestPauseRolloutAfterInconclusiveExperiment(t *testing.T) {
 		Experiment: &v1alpha1.RolloutExperimentStep{},
 	}}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 
 	rs1 := newReplicaSetWithStatus(r1, 1, 1)
@@ -373,7 +373,7 @@ func TestRolloutExperimentScaleDownExperimentFromPreviousStep(t *testing.T) {
 		{SetWeight: pointer.Int32Ptr(1)},
 	}
 
-	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(0), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1))
 	r2 := bumpVersion(r1)
 
 	rs1 := newReplicaSetWithStatus(r1, 1, 1)

utils/replicaset/canary.go

@@ -43,56 +43,6 @@ func AtDesiredReplicaCountsForCanary(ro *v1alpha1.Rollout, newRS, stableRS *apps
 	return true
 }
 
-/*
-// AtDesiredReplicaCountsForCanary indicates if the rollout is at the desired state for the current step
-func AtDesiredReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS, stableRS *appsv1.ReplicaSet, olderRSs []*appsv1.ReplicaSet) bool {
-	desiredNewRSReplicaCount, desiredStableRSReplicaCount := DesiredReplicaCountsForCanary(rollout, newRS, stableRS)
-	if newRS == nil || desiredNewRSReplicaCount != *newRS.Spec.Replicas || desiredNewRSReplicaCount != newRS.Status.AvailableReplicas {
-		return false
-	}
-	if stableRS == nil || desiredStableRSReplicaCount != *stableRS.Spec.Replicas || desiredStableRSReplicaCount != stableRS.Status.AvailableReplicas {
-		return false
-	}
-	if GetAvailableReplicaCountForReplicaSets(olderRSs) != int32(0) {
-		return false
-	}
-	return true
-}
-*/
-
-/*
-//DesiredReplicaCountsForCanary calculates the desired endstate replica count for the new and stable replicasets
-func DesiredReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS, stableRS *appsv1.ReplicaSet) (int32, int32) {
-	rolloutSpecReplica := defaults.GetReplicasOrDefault(rollout.Spec.Replicas)
-	replicas, weight := GetCanaryReplicasOrWeight(rollout)
-
-	desiredNewRSReplicaCount := int32(0)
-	desiredStableRSReplicaCount := int32(0)
-	if replicas != nil {
-		desiredNewRSReplicaCount = *replicas
-		desiredStableRSReplicaCount = rolloutSpecReplica
-	} else {
-		desiredNewRSReplicaCount = int32(math.Ceil(float64(rolloutSpecReplica) * (float64(weight) / 100)))
-		desiredStableRSReplicaCount = int32(math.Ceil(float64(rolloutSpecReplica) * (1 - (float64(weight) / 100))))
-	}
-
-	if !CheckStableRSExists(newRS, stableRS) {
-		// If there is no stableRS or it is the same as the newRS, then the rollout does not follow the canary steps.
-		// Instead the controller tries to get the newRS to 100% traffic.
-		desiredNewRSReplicaCount = rolloutSpecReplica
-		desiredStableRSReplicaCount = 0
-	}
-	// Unlike the ReplicaSet based weighted canary, a service mesh/ingress
-	// based canary leaves the stable as 100% scaled until the rollout completes.
-	if rollout.Spec.Strategy.Canary.TrafficRouting != nil {
-		desiredStableRSReplicaCount = rolloutSpecReplica
-	}
-
-	return desiredNewRSReplicaCount, desiredStableRSReplicaCount
-
-}
-*/
-
 // CalculateReplicaCountsForBasicCanary calculates the number of replicas for the newRS and the stableRS
 // when using the basic canary strategy. The function calculates the desired number of replicas for
 // the new and stable RS using the following equations:
@@ -131,9 +81,9 @@ func DesiredReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS, stableRS *a
 func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv1.ReplicaSet, oldRSs []*appsv1.ReplicaSet) (int32, int32) {
 	rolloutSpecReplica := defaults.GetReplicasOrDefault(rollout.Spec.Replicas)
 	_, desiredWeight := GetCanaryReplicasOrWeight(rollout)
+	maxSurge := MaxSurge(rollout)
 
-	desiredStableRSReplicaCount := int32(math.Ceil(float64(rolloutSpecReplica) * (1 - (float64(desiredWeight) / 100))))
-	desiredNewRSReplicaCount := int32(math.Ceil(float64(rolloutSpecReplica) * (float64(desiredWeight) / 100)))
+	desiredNewRSReplicaCount, desiredStableRSReplicaCount := approximateWeightedCanaryStableReplicaCounts(rolloutSpecReplica, desiredWeight, maxSurge)
 
 	stableRSReplicaCount := int32(0)
 	newRSReplicaCount := int32(0)
@@ -151,13 +101,6 @@ func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *apps
 		desiredStableRSReplicaCount = 0
 	}
 
-	maxSurge := MaxSurge(rollout)
-
-	if extraReplicaAdded(rolloutSpecReplica, desiredWeight) {
-		// In the case where the weight of the stable and canary replica counts cannot be divided evenly,
-		// the controller needs to surges by one to account for both replica counts being rounded up.
-		maxSurge = maxSurge + 1
-	}
 	maxReplicaCountAllowed := rolloutSpecReplica + maxSurge
 
 	allRSs := append(oldRSs, newRS)
@@ -223,6 +166,84 @@ func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *apps
 	return newRSReplicaCount, stableRSReplicaCount
 }
 
+// approximateWeightedCanaryStableReplicaCounts approximates the desired canary weight and returns
+// the closest replica count values for the canary and stable to reach the desired weight. The
+// canary/stable replica counts might sum to either spec.replicas or spec.replicas + 1 but will not
+// exceed spec.replicas if maxSurge is 0. If the canary weight is between 1-99, and spec.replicas is > 1,
+// we will always return a minimum of 1 for stable and canary as to not return 0.
+func approximateWeightedCanaryStableReplicaCounts(specReplicas, desiredWeight, maxSurge int32) (int32, int32) {
+	if specReplicas == 0 {
+		return 0, 0
+	}
+	// canaryOption is one potential return value of this function. We will evaluate multiple options
+	// for the canary count in order to best approximate the desired weight.
+	type canaryOption struct {
+		canary int32
+		total  int32
+	}
+	var options []canaryOption
+
+	ceilWeightedCanaryCount := int32(math.Ceil(float64(specReplicas*desiredWeight) / 100.0))
+	floorWeightedCanaryCount := int32(math.Floor(float64(specReplicas*desiredWeight) / 100.0))
+
+	tied := floorCeilingTied(desiredWeight, specReplicas)
+
+	// zeroAllowed indicates if are allowed to return the floored value if it is zero. We don't allow
+	// the value to be zero if when user has a weight from 1-99, and they run 2+ replicas (surge included)
+	zeroAllowed := desiredWeight == 100 || desiredWeight == 0 || (specReplicas == 1 && maxSurge == 0)
+
+	if ceilWeightedCanaryCount < specReplicas || zeroAllowed {
+		options = append(options, canaryOption{ceilWeightedCanaryCount, specReplicas})
+	}
+
+	if !tied && (floorWeightedCanaryCount != 0 || zeroAllowed) {
+		options = append(options, canaryOption{floorWeightedCanaryCount, specReplicas})
+	}
+
+	// check if we are allowed to surge. if we are, we can also consider rounding up to spec.replicas + 1
+	// in order to achieve a closer canary weight
+	if maxSurge > 0 {
+		options = append(options, canaryOption{ceilWeightedCanaryCount, specReplicas + 1})
+		surgeIsTied := floorCeilingTied(desiredWeight, specReplicas+1)
+		if !surgeIsTied && (floorWeightedCanaryCount != 0 || zeroAllowed) {
+			options = append(options, canaryOption{floorWeightedCanaryCount, specReplicas + 1})
+		}
+	}
+
+	if len(options) == 0 {
+		// should not get here
+		return 0, specReplicas
+	}
+
+	bestOption := options[0]
+	bestDelta := weightDelta(desiredWeight, bestOption.canary, bestOption.total)
+	for i := 1; i < len(options); i++ {
+		currOption := options[i]
+		currDelta := weightDelta(desiredWeight, currOption.canary, currOption.total)
+		if currDelta < bestDelta {
+			bestOption = currOption
+			bestDelta = currDelta
+		}
+	}
+	return bestOption.canary, bestOption.total - bestOption.canary
+}
+
+// floorCeilingTied indicates if the ceiling and floor values are equidistant from the desired weight
+// For example: replicas: 3, desiredWeight: 50%
+// A canary count of 1 (33.33%) or 2 (66.66%) are both equidistant from desired weight of 50%.
+// When this happens, we will pick the larger canary count
+func floorCeilingTied(desiredWeight, totalReplicas int32) bool {
+	_, frac := math.Modf(float64(totalReplicas) * (float64(desiredWeight) / 100))
+	return frac == 0.5
+}
+
+// weightDelta calculates the difference that the canary replicas will be from the desired weight
+// This is used to pick the closest approximation of canary counts.
+func weightDelta(desiredWeight, canaryReplicas, totalReplicas int32) float64 {
+	actualWeight := float64(canaryReplicas*100) / float64(totalReplicas)
+	return math.Abs(actualWeight - float64(desiredWeight))
+}
+
 // calculateScaleDownReplicaCount calculates drainRSReplicaCount
 // drainRSReplicaCount can be either stableRS count or canaryRS count
 // drainRSReplicaCount corresponds to RS whose availability is not considered in calculating replicasToScaleDown
@@ -397,14 +418,6 @@ func GetReplicasForScaleDown(rs *appsv1.ReplicaSet, ignoreAvailability bool) int
 	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.
-func extraReplicaAdded(replicas int32, setWeight int32) bool {
-	_, frac := math.Modf(float64(replicas) * (float64(setWeight) / 100))
-	return frac != 0.0
-}
-
 // GetCurrentCanaryStep returns the current canary step. If there are no steps or the rollout
 // has already executed the last step, the func returns nil
 func GetCurrentCanaryStep(rollout *v1alpha1.Rollout) (*v1alpha1.CanaryStep, *int32) {