Fully Promoting a 0-Replica Canary Rollout with Traffic Routing Causes a Panic #3686

Describe the bug

My team uses Argo Rollouts with the canary strategy and traffic routing. Our strategy typically looks something like this:

canary:
  canaryService: example-service-canary
  stableService: example-service
  dynamicStableScale: true
  steps:
    - setCanaryScale:
        replicas: 1
    - pause: {}
    - setCanaryScale:
        matchTrafficWeight: true
    - setWeight: 20
    - setWeight: 40
    - setWeight: 60
    - setWeight: 80
    - setWeight: 100
  trafficRouting:
    istio:
      virtualServices:
        - name: example-service-backend
          routes:
            - example-service-stable
        - name: example-service-ingress
          routes:
            - example-service-stable

We also have a case where we occasionally have to scale down the number of replicas to 0. When this is done, and a change is made to the Rollout spec, a new ReplicaSet is created containing the changes with 1 replica because of setCanaryScale.replicas: 1. This is not ideal, but is not a bug. IMO it would be nice if there was a feature to disable the canary behavior when a Rollout has its replica count set to 0, or otherwise annotated with some sort of suspended boolean. But that is not my reason for creating this issue.

If you then Resume the Rollout steps, the Rollout completes successfully and then scales the replicas back down to zero.

However, if you instead Promote-Full the Rollout, an integer divide by zero exception is thrown. The exception is caught, but the Rollout effectively gets permanently stuck in this progressing state. And the Rollout Controller logs continuously log the caught exceptions.

Sync begins

time="2024-06-28T17:10:15Z" level=info msg="Started syncing rollout" generation=9 namespace=example-app resourceVersion=1218236008 rollout=example-service
time="2024-06-28T17:10:15Z" level=info msg="delaying service switch from 588865bc5f to f45bd9bbb: ReplicaSet has zero availability" namespace=example-app rollout=example-service service=example-service
time="2024-06-28T17:10:15Z" level=info msg="Found 1 TrafficRouting Reconcilers" namespace=example-app rollout=example-service
time="2024-06-28T17:10:15Z" level=info msg="Reconciling TrafficRouting with type 'Istio'" namespace=example-app rollout=example-service
time="2024-06-28T17:10:15Z" level=info msg="Reconciliation completed" generation=9 namespace=example-app resourceVersion=1218236008 rollout=example-service time_ms=1.3598189999999999

Exception occurs

time="2024-06-28T17:10:15Z" level=error msg="Recovered from panic: runtime error: integer divide by zero\ngoroutine 43159 [running]:\nruntime/debug.Stack()\n\t/usr/local/go/src/runtime/debug/stack.go:24 +0x5e\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1.1()\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:151 +0x49\npanic({0x2c65f20?, 0x5358630?})\n\t/usr/local/go/src/runtime/panic.go:914 +0x21f\ngithub.com/argoproj/argo-rollouts/rollout.(*rolloutContext).reconcileTrafficRouting(0xc003fb2400)\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/trafficrouting.go:206 +0x113d\ngithub.com/argoproj/argo-rollouts/rollout.(*rolloutContext).rolloutCanary(0xc003fb2400)\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/canary.go:57 +0x1d3\ngithub.com/argoproj/argo-rollouts/rollout.(*rolloutContext).reconcile(0xc003fb2400)\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/context.go:86 +0xd4\ngithub.com/argoproj/argo-rollouts/rollout.(*Controller).syncHandler(0xc0006c8380, {0x39446d0, 0xc0004a0780}, {0xc00467c0c0, 0x1b})\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:437 +0x4d3\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1()\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:155 +0x7d\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1({0x3957450?, 0xc00046ebe0}, {0x31bf658, 0x7}, 0xc0035dfe70, {0x39446d0?, 0xc0004a0780}, 0xc000a31860?, {0x2a9bfa0, 0xc00112a930})\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:159 +0x3ed\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem({0x39446d0, 0xc0004a0780}, {0x3957450, 0xc00046ebe0}, {0x31bf658, 0x7}, 0x0?, 0xc0013ab860?)\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:178 +0xb5\ngithub.com/argoproj/argo-rollouts/utils/controller.RunWorker(...)\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:106\ngithub.com/argoproj/argo-rollouts/rollout.(*Controller).Run.func1()\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:358 +0xb2\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:226 +0x33\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xa01223201?, {0x3912ba0, 0xc001abd170}, 0x1, 0xc000915260)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:227 +0xaf\nk8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc000ab6fb0?, 0x3b9aca00, 0x0, 0x0?, 0x0?)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:204 +0x7f\nk8s.io/apimachinery/pkg/util/wait.Until(0x8dc6c5?, 0x0?, 0xc000ab6f10?)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:161 +0x1e\ncreated by github.com/argoproj/argo-rollouts/rollout.(*Controller).Run in goroutine 43032\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:357 +0xa7\n" namespace=example-app rollout=example-service

Exception is recovered

time="2024-06-28T17:10:15Z" level=error msg="rollout syncHandler error: Recovered from Panic" namespace=example-app rollout=example-service
time="2024-06-28T17:10:15Z" level=info msg="rollout syncHandler queue retries: 2 : key "example-app/example-service"" namespace=example-app rollout=example-service
time="2024-06-28T17:10:15Z" level=error msg="Recovered from Panic" error=""

After reading the trace, it is clear that the cause for the exception occurs on this line when desiredWeight is calculated and one is using dynamicStableScale. It attempts to divide by zero when the number of replicas is 0. Likely there should be another nested conditional or some sort of error handling that manages this case. Presumably the desiredWeight should then instead be set to 100? Or just not modified from the existing state?

} else if c.rollout.Status.PromoteFull {
    // on a promote full, desired stable weight should be 0 (100% to canary),
    // But we can only increase canary weight according to available replica counts of the canary.
    // we will need to set the desiredWeight to 0 when the newRS is not available.
    if c.rollout.Spec.Strategy.Canary.DynamicStableScale {
        desiredWeight = (weightutil.MaxTrafficWeight(c.rollout) * c.newRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas
    } else if c.rollout.Status.Canary.Weights != nil {
        desiredWeight = c.rollout.Status.Canary.Weights.Canary.Weight
    }

    err := reconciler.RemoveManagedRoutes()
    if err != nil {
        return err
    }

To Reproduce

1. Configure a canary Rollout to use Traffic Routing with Istio and dynamicStableScale
2. Set the number of Replicas for the Rollout to 0
3. Modify an aspect of the Rollout spec to trigger a Rollout
4. Promote-Full the Rollout

Expected behavior

The Rollout would simply promote the change and probably not modify Istio weights at all.

Version

v1.7.0

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.