Allow global resync & reconcilerImpl informers to be filtered via filterfunc #1909
Conversation
knative-prow-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lberk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## main #1909 +/- ##
==========================================
- Coverage 69.42% 69.08% -0.34%
==========================================
Files 207 209 +2
Lines 8739 8819 +80
==========================================
+ Hits 6067 6093 +26
- Misses 2402 2454 +52
- Partials 270 272 +2
Continue to review full report at Codecov.
|
Impl.globalResyncFilterFunc -> do not export (as is set by controller)
Ensure FilteredGlobalResync actually runs filterFunc over passed
objects
Fix descriptions/comments
Change passing func(interfcae{}) bool, to filterFunc
knative-prow-robot
added
size/L
|
thanks for the reviews @julz, should be fixed at this point. |
| @@ -203,6 +207,7 @@ func NewImpl(ctx {{.contextContext|raw}}, r Interface{{if .hasClass}}, classValu | |||
|
|
|||
| lister := {{.type|lowercaseSingular}}Informer.Lister() | |||
|
|
|||
| filterFunc := {{.controllerGetFilterFunc|raw}}(ctx) | |||
There was a problem hiding this comment.
I'd rather see this come through the controller.Options, but the ordering and capture make this awkward.
I think that if you have filterFunc := noFilter up here, and optionally overwrite it below the capture should work out correctly though?
There was a problem hiding this comment.
in fact the rest of the things already seem to be doing this, so you are pretty close 🤔
There was a problem hiding this comment.
#2170 rather 😄 . Just clicked your link and momentarily was very confused
|
@lberk: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
needs-rebase
|
@lberk: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Which state is this one in? Seems like it fell between the cracks @lberk ? |
|
@markusthoemmes this is still a problem in eventing. I was asked for further justification that this solves the root cause of the issue, I haven't had time to return to it since. |
|
I'm going to close this out since it's been a number of months - feel free reopen |
|
I’m pretty sure Options now has PromoteFilterFunc… |
related to #1764
There were two missing places where the informers from the shared cache we're not being filtered.
return trueLeaderAwareFuncs.PromoteFuncwhich was marked with a TODO to allow filteringThe former has been changed to allow for a
controllerImpl.GlobalResyncFilterFuncto be set (and if not, the former default ofreturn trueis passed to allow objects.The latter was complicated to pass as a function, if done as a
controller.Option, it would be too late for the closure in the initialreconcilerImpl. I've opted instead for apkg/controller/controller.govariable that will store via the shared context and again -- if not set -- pass a default filter ofreturn truefiltering over the objects./cc @vaikas @n3wscott @matzew