Repository: argoproj/argo-rollouts

Issue:
when dynamicStableScale is enabled, the 2nd canary deployment (when the first one didn't finish) shifts the traffic to the stable 100% without waiting for it to scale up 
<!-- If you are trying to resolve an environment-specific issue or have a one-off question about the edge case that does not require a feature then please consider asking a question in argo rollouts slack [channel](https://argoproj.github.io/community/join-slack). -->

Checklist:

* [x] I've included steps to reproduce the bug.
* [x] I've included the version of argo rollouts.

**Describe the bug**
When using canary strategy and adding `dynamicStableScale` if a newer release were pushed (v3) while an unstable release (v2) is still being deployed, argo rollouts controller will shift all the traffic to the stable release (v1) without waiting for it to scale up.

**To Reproduce**
Just deploy a simple rollout with traffic routing and  `dynamicStableScale` enabled, then change the image version to `1.25.2` and apply the changes, now the v1 replicaset will have 1 replica while v2 will have 3, change the image to `1.25.3` and the issue will happen, it will immediately shift all traffic to v1 without waiting for it pods to start, since we are using nginx as an example it will be very short, but on production, this caused an outage

```
apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  name: test-argo-rollout
  labels:
    app: test-argo-rollout
spec:
  replicas: 4
  strategy:
    canary:
      canaryService: test-argo-rollout-canary
      stableService: test-argo-rollout
      dynamicStableScale: true
      abortScaleDownDelaySeconds: 300
      maxUnavailable: 0
      trafficRouting:
        nginx:
          stableIngress: test-argo-rollout-ingress
      steps:
        - setWeight: 90
        - pause: {}
  selector:
    matchLabels:
      app: test-argo-rollout
  template:
    metadata:
      labels:
        app: test-argo-rollout
    spec:
      containers:
        - name: nginx
          image: nginx:1.25.1
          imagePullPolicy: IfNotPresent
          ports:
            - containerPort: 80
              name: http
              protocol: TCP
          resources:
            requests:
              cpu: 100m
              memory: 100Mi

---

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test-argo-rollout-ingress
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  ingressClassName: nginx
  rules:
    - host:<HOST-HERE>
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: test-argo-rollout
                port:
                  number: 80

---

apiVersion: v1
kind: Service
metadata:
  name: test-argo-rollout
  labels:
    app: test-argo-rollout
spec:
  type: ClusterIP
  ports:
    - port: 80
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app: test-argo-rollout

---

apiVersion: v1
kind: Service
metadata:
  name: test-argo-rollout-canary
  labels:
    app: test-argo-rollout
spec:
  type: ClusterIP
  ports:
    - port: 80
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app: test-argo-rollout
```

**Expected behavior**

It should act exactly like an abort on v2 so the traffic shifting to v1 will be gradual, and then move to v3

**Version**

1.6.0

<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.


Discussion:
@zachaller: This might possibly be fixed by https://github.com/argoproj/argo-rollouts/pull/3077 can you test with the latest 1.6.6?@MarwanTukhta: > This might possibly be fixed by #3077 can you test with the latest 1.6.6?

I have tried it on 1.6.6, the bug is still there@Gabryel8818: I have same problem@psks1994: Facing same issue, 

initial rollout has below config with 6 pods as stable:  
```
      steps:
        - setWeight: 10
        - pause: {}
```

After executing below command:
```
argocd app set argocd/${APP_NAME} --helm-set strategy.canary.steps[0].setWeight=50
argocd app sync argocd/${APP_NAME}
```

I see, total 12 pods starts running and then after a while then we have 3 pods each in canary and stable. 

and same can be see if we set weight to 100 after that. first 12 pods then after a while 6 pods in canary.

We are trying to give value dynamically rather than having different fixed steps to promote to. 

Solution patch:
diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go
index eef25e1c..df00d70c 100644
--- a/rollout/trafficrouting.go
+++ b/rollout/trafficrouting.go
@@ -10,6 +10,8 @@ import (
 
 	"github.com/argoproj/argo-rollouts/rollout/trafficrouting/plugin"
 
+	appsv1 "k8s.io/api/apps/v1"
+
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	"github.com/argoproj/argo-rollouts/rollout/trafficrouting"
 	"github.com/argoproj/argo-rollouts/rollout/trafficrouting/alb"
@@ -133,6 +135,23 @@ func (c *Controller) NewTrafficRoutingReconciler(roCtx *rolloutContext) ([]traff
 	return nil, nil
 }
 
+func (c *rolloutContext) checkReplicasAvailable(rs *appsv1.ReplicaSet, desiredWeight int32) bool {
+	if rs == nil {
+		return false
+	}
+	availableReplicas := rs.Status.AvailableReplicas
+	totalReplicas := *c.rollout.Spec.Replicas
+
+	desiredReplicas := (desiredWeight * totalReplicas) / 100
+	if availableReplicas < desiredReplicas {
+		c.log.Infof("ReplicaSet '%s' has %d available replicas, waiting for %d", rs.Name, availableReplicas, desiredReplicas)
+		return false
+	}
+
+	return true
+
+}
+
 // this currently only be used in the canary strategy
 func (c *rolloutContext) reconcileTrafficRouting() error {
 	reconcilers, err := c.newTrafficRoutingReconciler(c)
@@ -243,6 +262,10 @@ func (c *rolloutContext) reconcileTrafficRouting() error {
 				desiredWeight = weightutil.MaxTrafficWeight(c.rollout)
 			}
 		}
+
+		if !c.checkReplicasAvailable(c.stableRS, weightutil.MaxTrafficWeight(c.rollout)-desiredWeight) {
+			return nil
+		}
 		// We need to check for revision > 1 because when we first install the rollout we run step 0 this prevents that.
 		// There is a bigger fix needed for the reasons on why we run step 0 on rollout install, that needs to be explored.
 		revision, revisionFound := annotations.GetRevisionAnnotation(c.rollout)



Test patch:
diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go
index 1022ed09..a5bb05af 100644
--- a/rollout/trafficrouting_test.go
+++ b/rollout/trafficrouting_test.go
@@ -1435,3 +1435,70 @@ func TestDontWeightOrHaveManagedRoutesDuringInterruptedUpdate(t *testing.T) {
 	f.fakeTrafficRouting.AssertCalled(t, "RemoveManagedRoutes", mock.Anything, mock.Anything)
 
 }
+
+// This test verifies that if we are shifting traffic to stable replicaset without the stable replicaset being available proportional to the weight, the traffic shouldn't be switched immediately to the stable replicaset.
+func TestCheckReplicaSetAvailable(t *testing.T) {
+	fix := newFixture(t)
+	defer fix.Close()
+
+	steps := []v1alpha1.CanaryStep{
+		{
+			SetWeight: pointer.Int32(60),
+		},
+		{
+			Pause: &v1alpha1.RolloutPause{},
+		},
+	}
+
+	rollout1 := newCanaryRollout("test-rollout", 10, nil, steps, pointer.Int32(1), intstr.FromInt(1), intstr.FromInt(1))
+	rollout1.Spec.Strategy.Canary.DynamicStableScale = true
+	rollout1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
+		SMI: &v1alpha1.SMITrafficRouting{},
+	}
+	rollout1.Spec.Strategy.Canary.CanaryService = "canary-service"
+	rollout1.Spec.Strategy.Canary.StableService = "stable-service"
+	rollout1.Status.ReadyReplicas = 10
+	rollout1.Status.AvailableReplicas = 10
+
+	rollout2 := bumpVersion(rollout1)
+
+	replicaSet1 := newReplicaSetWithStatus(rollout1, 1, 1)
+	replicaSet2 := newReplicaSetWithStatus(rollout2, 9, 9)
+
+	replicaSet1Hash := replicaSet1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	replicaSet2Hash := replicaSet2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	canarySelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: replicaSet2Hash}
+	stableSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: replicaSet1Hash}
+	canarySvc := newService("canary-service", 80, canarySelector, rollout1)
+	stableSvc := newService("stable-service", 80, stableSelector, rollout1)
+
+	rollout2.Spec = rollout1.Spec
+	rollout2.Status.StableRS = replicaSet1Hash
+	rollout2.Status.CurrentPodHash = replicaSet1Hash
+	rollout2.Status.Canary.Weights = &v1alpha1.TrafficWeights{
+		Canary: v1alpha1.WeightDestination{
+			Weight:          10,
+			ServiceName:     "canary-service",
+			PodTemplateHash: replicaSet2Hash,
+		},
+		Stable: v1alpha1.WeightDestination{
+			Weight:          90,
+			ServiceName:     "stable-service",
+			PodTemplateHash: replicaSet1Hash,
+		},
+	}
+
+	fix.kubeobjects = append(fix.kubeobjects, replicaSet1, replicaSet2, canarySvc, stableSvc)
+	fix.replicaSetLister = append(fix.replicaSetLister, replicaSet1, replicaSet2)
+
+	fix.rolloutLister = append(fix.rolloutLister, rollout2)
+	fix.objects = append(fix.objects, rollout2)
+
+	fix.expectUpdateReplicaSetAction(replicaSet1)
+	fix.expectUpdateRolloutAction(rollout2)
+	fix.expectUpdateReplicaSetAction(replicaSet1)
+	fix.expectPatchRolloutAction(rollout2)
+	fix.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler()
+	fix.fakeTrafficRouting.On("RemoveManagedRoutes", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+	fix.run(getKey(rollout1, t))
+}