feat(controller): multiple TrafficRoutingReconciler #1472
Conversation
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
|
We face similar scenarios, we built a custom controller for this to sync these two.. Hi @jessesuen Appreciate your efforts! Thank you |
|
@xavipanda |
Codecov Report
@@ Coverage Diff @@
## master #1472 +/- ##
==========================================
- Coverage 81.77% 81.57% -0.20%
==========================================
Files 112 112
Lines 15071 15142 +71
==========================================
+ Hits 12324 12352 +28
- Misses 2103 2134 +31
- Partials 644 656 +12
Continue to review full report at Codecov.
|
|
@fblgit can you fix lint errors? |
|
@khhirani can you review the PR? |
|
sure, let me work on it :) thanks |
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
|
Hi @khhirani e2e-tests:
Note: Added a few func's to get Ingress data. Let me know if u want a decoupled e2e just for the nginx, should be quickly doable. lint: unit-tests:
coverage:
Hope next time I can nail the PR it at the first attempt, sorry for the trouble. Thank you |
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
|
@fblgit Thanks for the contribution. Can you fix the indentation issue? |
Signed-off-by: fblgit <[email protected]>
|
@harikrongali Regarding your comments: indent:
documentation:
example:
Again thanks for your time, let me know if anything else is needed. |
Signed-off-by: fblgit <[email protected]>
|
quite strange... But when I run this one locally: I believe the problem is unrelated to the PR itself, can someone confirm? thank you |
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
|
@khhirani / @harikrongali |
|
Hi @fblgit ! Thank you for this contribution! Regarding “merge order”, theres no real order per-se. It's affected by release timing, or if/when the PR is ready. This past week, I had been prioritizing and only merging things which were required for the v1.1 release candidate, since we are running behind on v1.1. I actually didn't realize @harikrongali had targeted this for v1.1 otherwise I would have paid closer attention. Sorry about that. Generally speaking, we try to avoid producing unnecessary work for external contributors if it can be avoided, and instead place the burden on project maintainers. This includes holding off on merging big sweeping changes (until impacted smaller PRs get in first), so that outside contributors don't have to deal with the conflicts. Other times we'll contribute to the PR itself to fix conflicts ourselves. In any case, this PR now on my radar and based on feedback, appears close to complete. So if you rebase now, I think you won't have to deal with conflicts again. |
|
Noted and thank you, will be ready before Monday. Regards |
|
@fblgit It is from my end. Sorry for that, I wanted to get in as we have delayed release. But as there are review comments on changes, it didn't cut the 1.1 release. I should have updated it last week. We wanted to get in the changes as soon as possible after the review process is done to avoid merge conflicts. |
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
|
@jessesuen ready to merge. tests and e2e looks good. |
Signed-off-by: fblgit <[email protected]>
Signed-off-by: fblgit <[email protected]>
fblgit
force-pushed
the
rollouts_multitrafficreconciler
branch
from
f87f8ab to
a0bfa56
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| if reconcilers == nil { | ||
| // Not using traffic routing | ||
| c.newStatus.Canary.Weights = nil | ||
| return nil |
There was a problem hiding this comment.
This is dead code because if len(reconcilers) == 0 will cover the case where reconcilers == nil.
|
Great improvement! Can fix the minor issue later. |
Signed-off-by: fblgit <[email protected]>
Current Scenario
You can add multiple
trafficRoutingon rollouts spec. Unfortunately, only the first is used (alphabetically sorted). example:In that case, only
nginxwill be used andsmiwill be ignored.Feature multiple TrafficRoutingReconciler
With the same spec:
In this case, both
trafficRoutingdefinitions are used and they are kept in sync as well.This is performed with the minimum impact by just mutating the structure into a list that is processed one by one sequentially.
Scenarios
A) existing fleet that can't transition from nginx to VirtualGateways or similars
B) adding sidecars to large throughput nginx controllers is not viable or can have a negative impact
C) adoption of full N-S and W-E canary with zero-downtime on large fleets
D) nginx-service-mesh (nginxinc) ++ ingress-nginx (kubernetes) doesn't work so well together
E) nginx service mesh N/S W/E without using nginxinc ingress.
F) no host header annotations to circumvent routing issues when adding nginx-ingress to a mesh
Despite facing both A+B+C+D+E+F Scenarios, there are a few thoughts behind this:
This will be the first to allow the use of multiple traffic-shift mechanisms among the other existing solutions that enables features such as canary around :)
This PR enables the requested feature #514 that was gonna be part of 1.1 but ultimately was removed.
Checklist:
"fix(controller): Updates such and such. Fixes #1234".