fix: remove condition where header routes can stay directed at empty service in preemption #3898
Conversation
…service in preemption Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com>
schlags
force-pushed
the
schlags-removeManagedRoutesInPreemption
branch
from
b54f980 to
cb43cc5
Compare
Published E2E Test Results 4 files 4 suites 3h 16m 2s ⏱️ For more details on these failures, see this check. Results for commit 5ca1d5e. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 279 tests 2 279 ✅ 2m 59s ⏱️ Results for commit 5ca1d5e. ♻️ This comment has been updated with latest results. |
|
We had some downtime related to this so would be sweet to get a look! |
|
Can we get a unit test for this? |
Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com>
Added one just now, was able to verify that this test fails without the presence of that function call! |
|
| f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ingress)) | ||
|
|
||
| f.expectPatchRolloutAction(r3) | ||
| f.run(getKey(r3, t)) |
There was a problem hiding this comment.
This first reconciliation shows that it skips scaling down the old RS since it is still referenced.
| r3.Status.Canary.Weights = &v1alpha1.TrafficWeights{ | ||
| Canary: v1alpha1.WeightDestination{ | ||
| PodTemplateHash: replicasetutil.GetPodTemplateHash(rs3), | ||
| }, | ||
| } | ||
|
|
||
| f.expectUpdateReplicaSetAction(rs3) | ||
| f.run(getKey(r3, t)) |
There was a problem hiding this comment.
We swap that reference here, and reconcile again to observe the scaling down intermediate RS log - where the else if c.newRS == nil || c.newRS.Status.AvailableReplicas == 0 condition is met while reconciling the traffic routing.
Adding the removal of the managed traffic routes here allows us to ensure that no traffic will be directed to that intermediate RS.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3898 +/- ##
==========================================
- Coverage 83.92% 82.72% -1.20%
==========================================
Files 163 163
Lines 18564 22895 +4331
==========================================
+ Hits 15579 18939 +3360
- Misses 2116 3084 +968
- Partials 869 872 +3 ☔ View full report in Codecov by Sentry. |
…service in preemption (argoproj#3898) * fix: remove condition where header routes can stay directed at empty service in preemption Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com> * add unit test Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com> * lint Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com> --------- Signed-off-by: Dylan Schlager <dylan.schlager@lattice.com>
Attempts to address some concerns with behavior in:
It seems like we are forgetting to remove the managed routes when the following occurs:
It does not seem like any managed routes are considered between steps 4-6. I believe this code that I've made an addition to is where this would need to occur, but would love some feedback.
NOTE: I still don't think this fixes another situation where the service selector is swapped to the new replicaset before the traffic can be reconciled back at the 0% weight, but this at least allows the managed routes to fall back to the stable V1 version, instead of the behavior in the graph below:
Checklist:
"fix(controller): Updates such and such. Fixes #1234".