feat: support dynamic scaling of stable ReplicaSet as inverse of canary weight

[jessesuen]

Fixes #1029

1. Introduces a canary.dynamicStableScale flag which allows the stable ReplicaSet to dynamically scale down/up depending on canary traffic weight. This feature is needed to run less pods during an update, but at the cost of slower aborts (since the stable will need to scale back up).

2. The feature introduces a new weights field into the rollout status status.canary.weights. e.g.:

status:
  canary:
    weights:
      canary:
        podTemplateHash: 68978bfc96
        serviceName: dynamic-stable-scale-canary
        weight: 0
      stable:
        podTemplateHash: 5cc54cc5f4
        serviceName: dynamic-stable-scale-stable
        weight: 100

The tracking of canary weights into status is needed for us to understand the weight which was set in the traffic router. This allows us to calculate the amount of replicas which we can safely reduce the scale the stable (or canary during abort). For example, it is not safe to scale down the stable, until we know traffic has shifted away from it. It also avoids us to have all traffic routers implement a GetCurrentWeight() function because we instead remember what we set last.

We could also use this information to show in the Rollout dashboard in the future, or the CLI. Currently the actual weight field in the CLI is not very accurate.

3. During an abort, we scale stable to 100% immediately. As the stable pods become available, we start shifting traffic to the stable ReplicaSet (whatever is available), and start scaling down the canary. Users may opt-out of scaling down the canary upon abort by explicitly setting a value for canary,abortScaleDownDelaySeconds.

Signed-off-by: Jesse Suen jessesuen@gmail.com

[codecov]

Codecov Report

Merging #1430 (0e01d6b) into master (d9ba36a) will increase coverage by 0.06%.
The diff coverage is 76.26%.

@@            Coverage Diff             @@
##           master    #1430      +/-   ##
==========================================
+ Coverage   81.67%   81.73%   +0.06%     
==========================================
  Files         110      112       +2     
  Lines       14798    15070     +272     
==========================================
+ Hits        12086    12318     +232     
- Misses       2078     2107      +29     
- Partials      634      645      +11     

Impacted Files	Coverage Δ
utils/conditions/conditions.go	80.76% <ø> (ø)
utils/replicaset/replicaset.go	90.04% <12.50%> (-0.83%)	⬇️
rollout/trafficrouting/istio/istio.go	80.96% <33.33%> (ø)
rollout/trafficrouting/nginx/nginx.go	85.81% <33.33%> (ø)
rollout/replicaset.go	67.59% <59.37%> (-3.09%)	⬇️
utils/defaults/defaults.go	88.02% <62.50%> (-3.64%)	⬇️
rollout/trafficrouting/alb/alb.go	84.86% <63.63%> (ø)
utils/replicaset/canary.go	89.42% <69.56%> (+4.23%)	⬆️
rollout/trafficrouting/smi/smi.go	94.90% <71.42%> (ø)
rollout/canary.go	76.52% <73.33%> (+0.06%)	⬆️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c54090...0e01d6b. Read the comment docs.

[harikrongali]

@perenesenko can you please review and validate

[perenesenko]

I tested different scenarios of promotion and abortion with Istio and Nginx. All works fine
Just have two questions...

[perenesenko]

Q1 what about using the setCanaryScale. Previously we discussed with @harikrongali Probably we should prevent to use of dynamicStableScale and setCanaryScale together.

In case of steps

      steps:
        - setCanaryScale:
            weight: 20
        - setWeight: 80
        - pause: {}

After the setCanaryScale the canary no longer be autoscaling by the dynamicStableScale flag.
Also we will have a situation when 20% of canary pods serving 80% of traffic

[perenesenko]

Q2 The scaleDownDelaySeconds is not working with the dynamicStableScaling as once we scale canary to 100 the stable will have 0 pods. So the scaleDownDelaySeconds will be ignored. Should we mention this somewhere?

[harikrongali]

For Q1, it is confirmed that #1382 (comment) setCanaryScale and dynamicStableScale will work together. I would like to see what is the expected behavior if both values set. @jessesuen can you please provide us the behavior when both are configured. @perenesenko when you validated, 20%of pods are taking 80% of traffic? and can you provide what is the stable replica set count at that time?

[perenesenko]

@harikrongali
Yes, my configuration has 5 pods. During update the canary was set to 20% which is 1 pod and after sending 80 to canary it still has 1 pod.

[jessesuen]

Q1 what about using the setCanaryScale. Previously we discussed with @harikrongali Probably we should prevent to use of dynamicStableScale and setCanaryScale together.

In case of steps

      steps:
        - setCanaryScale:
            weight: 20
        - setWeight: 80
        - pause: {}

After the setCanaryScale the canary no longer be autoscaling by the dynamicStableScale flag.
Also we will have a situation when 20% of canary pods serving 80% of traffic

Yes, this is a risk with the setCanaryScale feature. But that is a risk unrelated to the dynamicStableScale and existed before this feature. I'm adding some documentation to highlight the concern.

I would like to see what is the expected behavior if both values set. @jessesuen can you please provide us the behavior when both are configured.

They independent choices. Both can be configured at the same time. dynamicStableScaling can be simply understood as: "whatever the current canary traffic weight is, the stable will be scaled to be:
inverse canary weight * rollout.spec.replicas

So if canaryWeight is set to 80, and there are 10 spec.replicas, then stable scale will be 2 irrespective of how the canary was scaled.

Q2 The scaleDownDelaySeconds is not working with the dynamicStableScaling as once we scale canary to 100 the stable will have 0 pods. So the scaleDownDelaySeconds will be ignored. Should we mention this somewhere?

correct. scaleDownDelaySeconds is not expected to work with dynamicStableScaling and is ignored.

[jessesuen]

Previously we discussed with @harikrongali Probably we should prevent to use of dynamicStableScale and setCanaryScale together.

Just to clarify. Theres no harm in using setCanaryScale and dynamicStableScale together. Setting the canary scale does not affect traffic weight. That is what setWeight is for. DynamicStableScale will always follow the change in canary weight, and so is only affected by setWeight.

correct. scaleDownDelaySeconds is not expected to work with dynamicStableScaling and is ignored.

I added validation to make sure these two options are not used together.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
5.7% Duplication

[alexmt]

LGTM