feat: Implement Issue #1779: add rollout.Spec.Strategy.Canary.MinPods…

…PerReplicaSet (#2448) * add rollout.Spec.Strategy.Canary.MinPodsPerRS (for TrafficRoutedCanary only) Signed-off-by: Shlomo Sanders <[email protected]> * fix lint and test Signed-off-by: Shlomo Sanders <[email protected]> * fix codegen Signed-off-by: Shlomo Sanders <[email protected]> * rename MinPodsPerRS to MinPodsPerReplicaSet Signed-off-by: Shlomo Sanders <[email protected]> * fix lint and codegen Signed-off-by: Shlomo Sanders <[email protected]> * update specification.md Signed-off-by: Shlomo Sanders <[email protected]> * codegen Signed-off-by: zachaller <[email protected]> * codegen missed a file Signed-off-by: zachaller <[email protected]> Signed-off-by: Shlomo Sanders <[email protected]> Signed-off-by: zachaller <[email protected]> Co-authored-by: Shlomo Sanders <[email protected]> Co-authored-by: zachaller <[email protected]>
argoproj · Dec 19, 2022 · 4874b94 · 4874b94
1 parent 91e491c
commit 4874b94
Show file tree

Hide file tree

Showing 13 changed files with 598 additions and 488 deletions.
diff --git a/docs/features/specification.md b/docs/features/specification.md
@@ -212,6 +212,11 @@ spec:
       # traffic routing.
       scaleDownDelaySeconds: 30
 
+      # The minimum number of pods that will be requested for each ReplicaSet
+      # when using traffic routed canary. This is to ensure high availability
+      # of each ReplicaSet. Defaults to 1. +optional
+      minPodsPerReplicaSet: 2
+
       # Limits the number of old RS that can run at one time before getting
       # scaled down. Defaults to nil
       scaleDownDelayRevisionLimit: 2

diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml
@@ -393,6 +393,9 @@ spec:
                         - type: integer
                         - type: string
                         x-kubernetes-int-or-string: true
+                      minPodsPerReplicaSet:
+                        format: int32
+                        type: integer
                       pingPong:
                         properties:
                           pingService:

diff --git a/manifests/install.yaml b/manifests/install.yaml
@@ -11403,6 +11403,9 @@ spec:
                         - type: integer
                         - type: string
                         x-kubernetes-int-or-string: true
+                      minPodsPerReplicaSet:
+                        format: int32
+                        type: integer
                       pingPong:
                         properties:
                           pingService:

diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml
@@ -11403,6 +11403,9 @@ spec:
                         - type: integer
                         - type: string
                         x-kubernetes-int-or-string: true
+                      minPodsPerReplicaSet:
+                        format: int32
+                        type: integer
                       pingPong:
                         properties:
                           pingService:

diff --git a/pkg/apiclient/rollout/rollout.swagger.json b/pkg/apiclient/rollout/rollout.swagger.json
@@ -927,6 +927,11 @@
         "pingPong": {
           "$ref": "#/definitions/github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.PingPongSpec",
           "title": "PingPongSpec holds the ping and pong services"
+        },
+        "minPodsPerReplicaSet": {
+          "type": "integer",
+          "format": "int32",
+          "title": "Assuming the desired number of pods in a stable or canary ReplicaSet is not zero, then make sure it is at least\nMinPodsPerReplicaSet for High Availability. Only applicable for TrafficRoutedCanary"
         }
       },
       "title": "CanaryStrategy defines parameters for a Replica Based Canary"

diff --git a/pkg/apis/rollouts/v1alpha1/generated.pb.go b/pkg/apis/rollouts/v1alpha1/generated.pb.go
diff --git a/pkg/apis/rollouts/v1alpha1/generated.proto b/pkg/apis/rollouts/v1alpha1/generated.proto
diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go
diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go
@@ -310,6 +310,9 @@ type CanaryStrategy struct {
 	DynamicStableScale bool `json:"dynamicStableScale,omitempty" protobuf:"varint,14,opt,name=dynamicStableScale"`
 	// PingPongSpec holds the ping and pong services
 	PingPong *PingPongSpec `json:"pingPong,omitempty" protobuf:"varint,15,opt,name=pingPong"`
+	// Assuming the desired number of pods in a stable or canary ReplicaSet is not zero, then make sure it is at least
+	// MinPodsPerReplicaSet for High Availability. Only applicable for TrafficRoutedCanary
+	MinPodsPerReplicaSet *int32 `json:"minPodsPerReplicaSet,omitempty" protobuf:"varint,16,opt,name=minPodsPerReplicaSet"`
 }
 
 // PingPongSpec holds the ping and pong service name.

diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go
diff --git a/ui/src/models/rollout/generated/api.ts b/ui/src/models/rollout/generated/api.ts
@@ -665,6 +665,12 @@ export interface GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1CanaryStrat
      * @memberof GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1CanaryStrategy
      */
     pingPong?: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1PingPongSpec;
+    /**
+     * 
+     * @type {number}
+     * @memberof GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1CanaryStrategy
+     */
+    minPodsPerReplicaSet?: number;
 }
 /**
  * DryRun defines the settings for running the analysis in Dry-Run mode.

diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go
@@ -312,9 +312,22 @@ func maxValue(countA int32, countB int32) int32 {
 	return countA
 }
 
+// CheckMinPodsPerReplicaSet ensures that if the desired number of pods in a stable or canary ReplicaSet is not zero,
+// then it is at least MinPodsPerReplicaSet for High Availability. Only applicable if using TrafficRouting
+func CheckMinPodsPerReplicaSet(rollout *v1alpha1.Rollout, count int32) int32 {
+	if count == 0 {
+		return count
+	}
+	if rollout.Spec.Strategy.Canary == nil || rollout.Spec.Strategy.Canary.MinPodsPerReplicaSet == nil || rollout.Spec.Strategy.Canary.TrafficRouting == nil {
+		return count
+	}
+	return max(count, *rollout.Spec.Strategy.Canary.MinPodsPerReplicaSet)
+}
+
 // CalculateReplicaCountsForTrafficRoutedCanary calculates the canary and stable replica counts
 // when using canary with traffic routing. If current traffic weights are supplied, we factor the
 // those weights into the and return the higher of current traffic scale vs. desired traffic scale
+// If MinPodsPerReplicaSet is defined and the number of replicas in either RS is not 0, then return at least MinPodsPerReplicaSet
 func CalculateReplicaCountsForTrafficRoutedCanary(rollout *v1alpha1.Rollout, weights *v1alpha1.TrafficWeights) (int32, int32) {
 	var canaryCount, stableCount int32
 	rolloutSpecReplica := defaults.GetReplicasOrDefault(rollout.Spec.Replicas)
@@ -323,7 +336,7 @@ func CalculateReplicaCountsForTrafficRoutedCanary(rollout *v1alpha1.Rollout, wei
 		// a canary count was explicitly set
 		canaryCount = *setCanaryScaleReplicas
 	} else {
-		canaryCount = trafficWeightToReplicas(rolloutSpecReplica, desiredWeight)
+		canaryCount = CheckMinPodsPerReplicaSet(rollout, trafficWeightToReplicas(rolloutSpecReplica, desiredWeight))
 	}
 
 	if !rollout.Spec.Strategy.Canary.DynamicStableScale {
@@ -354,7 +367,7 @@ func CalculateReplicaCountsForTrafficRoutedCanary(rollout *v1alpha1.Rollout, wei
 			canaryCount = max(trafficWeightReplicaCount, canaryCount)
 		}
 	}
-	return canaryCount, stableCount
+	return CheckMinPodsPerReplicaSet(rollout, canaryCount), CheckMinPodsPerReplicaSet(rollout, stableCount)
 }
 
 // trafficWeightToReplicas returns the appropriate replicas given the full spec.replicas and a weight

diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go
@@ -97,6 +97,7 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) {
 
 		abortScaleDownDelaySeconds *int32
 		statusAbort                bool
+		minPodsPerReplicaSet       *int32
 	}{
 		{
 			name:                "Do not add extra RSs in scaleDownCount when .Spec.Replica < AvailableReplicas",
@@ -674,6 +675,23 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) {
 			expectedStableReplicaCount: 1,
 			expectedCanaryReplicaCount: 0,
 		},
+		{
+			name:                "Honor MinPodsPerReplicaSet when using trafficRouting and starting canary",
+			rolloutSpecReplicas: 10,
+			setWeight:           5,
+
+			stableSpecReplica:      10,
+			stableAvailableReplica: 10,
+
+			canarySpecReplica:      0,
+			canaryAvailableReplica: 0,
+
+			trafficRouting:       &v1alpha1.RolloutTrafficRouting{},
+			minPodsPerReplicaSet: intPnt(2),
+
+			expectedStableReplicaCount: 10,
+			expectedCanaryReplicaCount: 2,
+		},
 	}
 	for i := range tests {
 		test := tests[i]
@@ -684,6 +702,9 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) {
 			stableRS := newRS("stable", test.stableSpecReplica, test.stableAvailableReplica)
 			canaryRS := newRS("canary", test.canarySpecReplica, test.canaryAvailableReplica)
 			rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds = test.abortScaleDownDelaySeconds
+			if test.minPodsPerReplicaSet != nil {
+				rollout.Spec.Strategy.Canary.MinPodsPerReplicaSet = test.minPodsPerReplicaSet
+			}
 			var newRSReplicaCount, stableRSReplicaCount int32
 			if test.trafficRouting != nil {
 				newRSReplicaCount, stableRSReplicaCount = CalculateReplicaCountsForTrafficRoutedCanary(rollout, nil)