feat: improve docs and add validation for incompatible flags

Signed-off-by: Jesse Suen <[email protected]>
argoproj · Sep 20, 2021 · d8c081e · d8c081e
1 parent 1ebbd6a
commit d8c081e
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 8 deletions.
diff --git a/docs/features/canary.md b/docs/features/canary.md
@@ -109,11 +109,21 @@ spec:
           matchTrafficWeight: true
 ```
 
-If no `duration` is specified for a pause step, the rollout will be paused indefinitely. To unpause, use the [argo kubectl plugin](kubectl-plugin.md) `promote` command. 
+When using `setCanaryScale` with explicit values for either replicas or weight, one must be careful
+if used in conjunction with the `setWeight` step. If done incorrectly, an imbalanced amount of traffic
+may be directed to the canary (in proportion to the Rollout's scale). For example, the following set
+of steps would cause 90% of traffic to only be served by 10% of pods:
 
-```shell
-# promote to the next step
-kubectl argo rollouts promote <rollout>
+```yaml
+spec:
+  replicas: 10
+  strategy:
+    canary:
+      steps:
+      - setCanaryScale:
+          weight: 10
+      - setWeight: 90
+      - pause: {}
 ```
 
 ## Dynamic Stable Scale (with Traffic Routing)
@@ -141,14 +151,14 @@ spec:
 
 NOTE: that if `dynamicStableScale` is set, and the rollout is aborted, the canary ReplicaSet will
 automatically scale down as traffic shifts back to stable. If you wish to leave the canary ReplicaSet
-scaled up while aborting, an explicit value for `abortScaleDownDelay` can be set:
+scaled up while aborting, an explicit value for `abortScaleDownDelaySeconds` can be set:
 
 ```yaml
 spec:
   strategy:
     canary:
       dynamicStableScale: true
-      abortScaleDownDelay: 30
+      abortScaleDownDelaySeconds: 30
 ```
 
 

diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go
@@ -57,6 +57,10 @@ const (
 	InvalidAnalysisArgsMessage = "Analyses arguments must refer to valid object metadata supported by downwardAPI"
 	// InvalidCanaryScaleDownDelay indicates that canary.scaleDownDelaySeconds cannot be used
 	InvalidCanaryScaleDownDelay = "Canary scaleDownDelaySeconds can only be used with traffic routing"
+	// InvalidCanaryDynamicStableScale indicates that canary.dynamicStableScale cannot be used
+	InvalidCanaryDynamicStableScale = "Canary dynamicStableScale can only be used with traffic routing"
+	// InvalidCanaryDynamicStableScaleWithScaleDownDelay indicates that canary.dynamicStableScale cannot be used with scaleDownDelaySeconds
+	InvalidCanaryDynamicStableScaleWithScaleDownDelay = "Canary dynamicStableScale cannot be used with scaleDownDelaySeconds"
 )
 
 func ValidateRollout(rollout *v1alpha1.Rollout) field.ErrorList {
@@ -219,8 +223,17 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat
 		}
 	}
 
-	if canary.ScaleDownDelaySeconds != nil && canary.TrafficRouting == nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("scaleDownDelaySeconds"), *canary.ScaleDownDelaySeconds, InvalidCanaryScaleDownDelay))
+	if canary.TrafficRouting == nil {
+		if canary.ScaleDownDelaySeconds != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("scaleDownDelaySeconds"), *canary.ScaleDownDelaySeconds, InvalidCanaryScaleDownDelay))
+		}
+		if canary.DynamicStableScale {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("dynamicStableScale"), canary.DynamicStableScale, InvalidCanaryDynamicStableScale))
+		}
+	} else {
+		if canary.ScaleDownDelaySeconds != nil && canary.DynamicStableScale {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("dynamicStableScale"), canary.DynamicStableScale, InvalidCanaryDynamicStableScaleWithScaleDownDelay))
+		}
 	}
 
 	for i, step := range canary.Steps {

diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go
@@ -317,6 +317,50 @@ func TestCanaryScaleDownDelaySeconds(t *testing.T) {
 		allErrs := ValidateRollout(ro)
 		assert.Empty(t, allErrs)
 	})
+}
+
+func TestCanaryDynamicStableScale(t *testing.T) {
+	selector := &metav1.LabelSelector{
+		MatchLabels: map[string]string{"key": "value"},
+	}
+	ro := &v1alpha1.Rollout{
+		Spec: v1alpha1.RolloutSpec{
+			Selector: selector,
+			Strategy: v1alpha1.RolloutStrategy{
+				Canary: &v1alpha1.CanaryStrategy{
+					StableService:      "stable",
+					CanaryService:      "canary",
+					DynamicStableScale: true,
+				},
+			},
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: selector.MatchLabels,
+				},
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Resources: corev1.ResourceRequirements{},
+						Image:     "foo",
+						Name:      "image-name",
+					}},
+				},
+			},
+		},
+	}
+	t.Run("dynamicStableScale with basic canary", func(t *testing.T) {
+		ro := ro.DeepCopy()
+		allErrs := ValidateRollout(ro)
+		assert.EqualError(t, allErrs[0], fmt.Sprintf("spec.strategy.dynamicStableScale: Invalid value: true: %s", InvalidCanaryDynamicStableScale))
+	})
+	t.Run("dynamicStableScale with scaleDownDelaySeconds", func(t *testing.T) {
+		ro := ro.DeepCopy()
+		ro.Spec.Strategy.Canary.ScaleDownDelaySeconds = pointer.Int32Ptr(60)
+		ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
+			SMI: &v1alpha1.SMITrafficRouting{},
+		}
+		allErrs := ValidateRollout(ro)
+		assert.EqualError(t, allErrs[0], fmt.Sprintf("spec.strategy.dynamicStableScale: Invalid value: true: %s", InvalidCanaryDynamicStableScaleWithScaleDownDelay))
+	})
 
 }