fix(trafficrouting): Fix downtime on initial deployment using Istio DestinationRule Subsets. Fixes #2507 #3602
Conversation
wmuizelaar
changed the title
wmuizelaar
force-pushed
the
fix_istio_desination_rule_readiness
branch
from
bde071e
to
25f0dc8
Compare
Go Published Test Results2 166 tests 2 166 ✅ 2m 55s ⏱️ Results for commit 582f087. ♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3602 +/- ##
==========================================
- Coverage 83.87% 83.81% -0.07%
==========================================
Files 162 162
Lines 18524 18529 +5
==========================================
- Hits 15537 15530 -7
- Misses 2119 2125 +6
- Partials 868 874 +6 ☔ View full report in Codecov by Sentry. |
E2E Tests Published Test Results 4 files 4 suites 3h 28m 0s ⏱️ For more details on these failures, see this check. Results for commit 582f087. ♻️ This comment has been updated with latest results. |
wmuizelaar
changed the title
wmuizelaar
changed the title
wmuizelaar
force-pushed
the
fix_istio_desination_rule_readiness
branch
2 times, most recently
from
54a2ab8
to
3704456
Compare
|
|
LGTM |
|
@zachaller are you able to take a look at this? It would be much appreciated! 🙏 |
|
Yea, at first glance I think I would like to try and not change the traffic router interface, this will cause all plugins to have to update etc etc. I think an ok alternative is to pass the replicasetInformer/Lister/k8s client or just add roCtx.allRSs into the istio reconciler context and use that to get the replicasets. If this where to be a plugin they would have to do something similar as well. |
|
Thanks for your remark, I agree that it would be really nice if the interface could stay the same. Let me experiment if I can get it passed to the Istio Reconciler in a different way. |
wmuizelaar
force-pushed
the
fix_istio_desination_rule_readiness
branch
2 times, most recently
from
2c40d9e
to
582f087
Compare
|
@zachaller can you take a look again please? This feels indeed a lot simpler and easier way of implementation. |
|
LGTM, working on my test project. |
|
@zachaller Gentle bump. Looking to see if this also resolves #3681. |
|
Sorry, it's been a while I will take a look at this soon. |
| // We need to check if the replicasets are ready here as well if we didn't define any services in the rollout | ||
| // See: https://github.com/argoproj/argo-rollouts/issues/2507 | ||
| if r.rollout.Spec.Strategy.Canary.CanaryService == "" && r.rollout.Spec.Strategy.Canary.StableService == "" { |
There was a problem hiding this comment.
Do we know for sure this is being properly checked when services are defined? In the example in #3681 I am defining both a canary service and stable service but still experiencing a brief downtime/outage after the rollout ends.
There was a problem hiding this comment.
Aaah, if you explicitly define the services, the existing code should actually be working - the fix I built here is explicitly for when you are using the Istio DestionationRule-method to update your traffic (https://argo-rollouts.readthedocs.io/en/stable/features/traffic-management/istio/#subset-level-traffic-splitting to be precise).
If you define the services (I did not see those in your example, so that's why I thought it might be related to this), there are other codepaths that actually check service-readiness. There might be a big/issue in there, but then your issue is at least definitely NOT related to mine.
…estinationRule Subsets. Fixes argoproj#2507 Signed-off-by: Wietse Muizelaar <wmuizelaar@bol.com>
zachaller
force-pushed
the
fix_istio_desination_rule_readiness
branch
from
582f087
to
1dcdfcd
Compare
|
Published E2E Test Results 4 files 4 suites 3h 22m 20s ⏱️ For more details on these failures, see this check. Results for commit 1dcdfcd. |
Published Unit Test Results2 264 tests 2 264 ✅ 2m 59s ⏱️ Results for commit 1dcdfcd. |
Checklist:
"fix(controller): Updates such and such. Fixes #1234".Fixes #2507