fix(controller): prevent negative vsvc weights on a replica scaledown…

… following a canary abort for istio trafficrouting (#3467) * chore(deps): bump github.com/aws/aws-sdk-go-v2/service/cloudwatch from 1.36.1 to 1.36.3 (#3452) chore(deps): bump github.com/aws/aws-sdk-go-v2/service/cloudwatch Bumps [github.com/aws/aws-sdk-go-v2/service/cloudwatch](https://github.com/aws/aws-sdk-go-v2) from 1.36.1 to 1.36.3. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Commits](aws/aws-sdk-go-v2@service/ec2/v1.36.1...service/ssm/v1.36.3) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/service/cloudwatch dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * controller: Fixed negative canary weight bug for istio trafficrouting Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * Added e2e test to validate the fix Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * Removed local changes Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * linted as per golangci-lint run Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * docs: typo in BlueGreen (#3463) Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * chore(deps): bump slsa-framework/slsa-github-generator from 1.9.1 to 1.10.0 (#3462) chore(deps): bump slsa-framework/slsa-github-generator Bumps [slsa-framework/slsa-github-generator](https://github.com/slsa-framework/slsa-github-generator) from 1.9.1 to 1.10.0. - [Release notes](https://github.com/slsa-framework/slsa-github-generator/releases) - [Changelog](https://github.com/slsa-framework/slsa-github-generator/blob/main/CHANGELOG.md) - [Commits](slsa-framework/slsa-github-generator@v1.9.1...v1.10.0) --- updated-dependencies: - dependency-name: slsa-framework/slsa-github-generator dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * chore(deps): bump github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 from 1.30.3 to 1.30.4 (#3461) chore(deps): bump github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 Bumps [github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2](https://github.com/aws/aws-sdk-go-v2) from 1.30.3 to 1.30.4. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Commits](aws/aws-sdk-go-v2@service/s3/v1.30.3...service/s3/v1.30.4) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * Removed sleep and waited for 1 replicas Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> * Moved up canary weight test to match PR title Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: shubhajyoti-bagchi-groww <shubhajyoti.bagchi@groww.in> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Thanapat Chotipun <66824385+PatrickChoDev@users.noreply.github.com>
argoproj · Mar 26, 2024 · 4b1cc3b · 4b1cc3b
1 parent 3bd955a
commit 4b1cc3b
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 1 deletion.
diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go
@@ -305,7 +305,7 @@ func (c *rolloutContext) calculateDesiredWeightOnAbortOrStableRollback() int32 {
 	}
 	// When using dynamic stable scaling, we must dynamically decreasing the weight to the canary
 	// according to the availability of the stable (whatever it can support).
-	desiredWeight := weightutil.MaxTrafficWeight(c.rollout) - ((weightutil.MaxTrafficWeight(c.rollout) * c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas)
+	desiredWeight := maxInt(0, weightutil.MaxTrafficWeight(c.rollout)-((weightutil.MaxTrafficWeight(c.rollout)*c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas))
 	if c.rollout.Status.Canary.Weights != nil {
 		// This ensures that if we are already at a lower weight, then we will not
 		// increase the weight because stable availability is flapping (e.g. pod restarts)

diff --git a/test/e2e/istio/istio-subset-split-in-stable-downscale-after-canary-abort.yaml b/test/e2e/istio/istio-subset-split-in-stable-downscale-after-canary-abort.yaml
@@ -0,0 +1,101 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: istio-subset-split-in-stable-downscale-after-canary-abort
+spec:
+  ports:
+  - port: 80
+    targetPort: http
+    protocol: TCP
+    name: http
+  selector:
+    app: istio-subset-split-in-stable-downscale-after-canary-abort
+
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: VirtualService
+metadata:
+  name: istio-subset-split-in-stable-downscale-after-canary-abort-vsvc
+spec:
+  hosts:
+  - istio-subset-split-in-stable-downscale-after-canary-abort
+  http:
+  - route:
+    - destination:
+        host: istio-subset-split-in-stable-downscale-after-canary-abort
+        subset: stable
+      weight: 100
+    - destination:
+        host: istio-subset-split-in-stable-downscale-after-canary-abort
+        subset: canary
+      weight: 0
+
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: DestinationRule
+metadata:
+  name: istio-subset-split-in-stable-downscale-after-canary-abort-destrule
+spec:
+  host: istio-subset-split-in-stable-downscale-after-canary-abort
+  subsets:
+  - name: stable
+    labels:
+      app: istio-subset-split-in-stable-downscale-after-canary-abort
+  - name: canary
+    labels:
+      app: istio-subset-split-in-stable-downscale-after-canary-abort
+
+---
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: istio-subset-split-in-stable-downscale-after-canary-abort
+spec:
+  replicas: 4
+  strategy:
+    canary:
+      dynamicStableScale: true
+      trafficRouting:
+        istio:
+          virtualService: 
+            name: istio-subset-split-in-stable-downscale-after-canary-abort-vsvc
+          destinationRule:
+            name: istio-subset-split-in-stable-downscale-after-canary-abort-destrule
+            canarySubsetName: canary
+            stableSubsetName: stable
+      canaryMetadata:
+        labels:
+          role: canary
+      stableMetadata:
+        labels:
+          role: stable
+      maxSurge: "25%"
+      maxUnavailable: "20%"
+      steps:
+        - setWeight: 10
+        - pause: {}
+        - setWeight: 20
+        - pause: {}
+        - setWeight: 30
+        - pause: {}          
+        - setWeight: 50
+        - pause: {}
+  selector:
+    matchLabels:
+      app: istio-subset-split-in-stable-downscale-after-canary-abort
+  template:
+    metadata:
+      labels:
+        app: istio-subset-split-in-stable-downscale-after-canary-abort
+    spec:
+      containers:
+      - name: istio-subset-split-in-stable-downscale-after-canary-abort
+        image: nginx:1.19-alpine
+        ports:
+        - name: http
+          containerPort: 80
+          protocol: TCP
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 5m
diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go
@@ -492,3 +492,29 @@ func (s *IstioSuite) TestIstioSubsetSplitExperimentStep() {
 
 	s.TearDownSuite()
 }
+
+func (s *IstioSuite) TestIstioSubsetSplitInStableDownscaleAfterCanaryAbort() {
+	s.Given().
+		RolloutObjects("@istio/istio-subset-split-in-stable-downscale-after-canary-abort.yaml").
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		PromoteRolloutFull().
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		AbortRollout().
+		WaitForRolloutStatus("Degraded").
+		ScaleRollout(1).
+		WaitForRolloutReplicas(1).
+		Then().
+		Assert(func(t *fixtures.Then) {
+			vsvc := t.GetVirtualService()
+			stableWeight := vsvc.Spec.HTTP[0].Route[0].Weight
+			canaryWeight := vsvc.Spec.HTTP[0].Route[1].Weight
+
+			assert.Equal(s.T(), int64(0), canaryWeight)
+			assert.Equal(s.T(), int64(100), stableWeight)
+		})
+
+	s.TearDownSuite()
+}