-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[provider] refactoring kubernetes provider to single reconciler #702
Conversation
534db41
to
c864a40
Compare
Codecov Report
@@ Coverage Diff @@
## main #702 +/- ##
==========================================
- Coverage 63.67% 61.47% -2.20%
==========================================
Files 47 46 -1
Lines 5949 5649 -300
==========================================
- Hits 3788 3473 -315
- Misses 1921 1949 +28
+ Partials 240 227 -13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
oh god all the tests passed :') |
so brute force single controller is working (all based on existing tests) |
hey @chauhanshubham did you get a chance to review the comments ? |
4ef5b14
to
63756e0
Compare
f96a71f
to
583b7e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chauhanshubham thanks for all your work on this PR. I have a few notes and questions that I would like your feedback on.
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
9b1e601
to
d120c23
Compare
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chauhanshubham for taking this on, just a couple comments/questions so far. Still working my way through things.
57715a9
to
eecaf96
Compare
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Adds Gateway Deletion Support to Controller
@chauhanshubham I found a regression when testing this PR. Reproducer:
When I add a log statement to the end of the reconcile() method, it appears that duplicate httproute entries exist in the watchable map. From reviewing the contents of the watchable map, it appears the same issue exists for services. |
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging since this PR is blocking several other PRs. |
This commit removes separate controllers for gateway API objects, and
works on only a single controller that now watches for all related objects.
Prior to this we had
n
controllers each watching>=1
resources and pushingaccording to the resource map. These pushes happened from these
n
controllers.For instance;
Now:
We have a single gatewayAPIController that has a single gatewayAPIReconciler, which reconciles
the gatewayClass object while watching for ALL gateway API objects. Any CRUD in these related
objects, triggers the same reconciliation logic, where the resource structure is created from scratch,
and pushed as is to the resource map.
Any error during reconciliation - particularly around any object not being found (non-existant/deleted resource),
skips the reconciliation around that particular update, and therefore does not lead to a push to
the resource map. Therefore, the push ONLY happens when all related objects exist -
once per reconciliation
.A few tests are not required so those have been removed - we could work towards adding more
meaningful ones subsequently. Conformance tests remain as is, with two new additions as part of
this PR (commented out reference grant ones)
Signed-off-by: Shubham Chauhan shubham@tetrate.io
Resolves #413
Resolves #539