Allow global resync & reconcilerImpl informers to be filtered #2176

benmoss · 2021-06-30T16:16:44Z

Changes

A revised implementation of #2170 and #1909, updated and now using options as per @mattmoor's ancient code review.

Also switched our reconcilers to use controller.NewImplFull since NewImpl is deprecated, and this makes setting the GlobalResyncFilterFunc easier too.

This came up again because of knative/eventing#5543, so looking to reintroduce @lberk's changes.

/cc @markusthoemmes

…terfunc

google-cla · 2021-06-30T16:16:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

knative-prow-robot · 2021-06-30T16:16:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: benmoss
To complete the pull request process, please assign mattmoor after the PR has been reviewed.
You can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

benmoss · 2021-06-30T16:18:39Z

codegen/cmd/injection-gen/generators/reconciler_controller.go

+	options := {{.controllerControllerOptions|raw}}{
+		Logger: logger,
+		WorkQueueName: ctrTypeName,
+		GlobalResyncFilterFunc: func(o interface{}) bool { filterFunc(o) },


a hacky trick here, wrapping the filterFunc in another function closes over the value so we can mutate it down below, passing filterFunc directly would pass by value: https://play.golang.org/p/lfNHOKu_yPZ

google-cla · 2021-06-30T16:24:58Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Users can just set the impl FilterFunc option and it will percolate down to the controller.ControllerOptions GlobalResyncFilterFunc as well.

google-cla · 2021-06-30T16:30:38Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

codecov · 2021-06-30T16:35:46Z

Codecov Report

Merging #2176 (f4bed1c) into main (51cfaab) will decrease coverage by 0.04%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##             main    #2176      +/-   ##
==========================================
- Coverage   66.05%   66.00%   -0.05%     
==========================================
  Files         224      224              
  Lines        9426     9437      +11     
==========================================
+ Hits         6226     6229       +3     
- Misses       2920     2928       +8     
  Partials      280      280

Impacted Files	Coverage Δ
.../injection-gen/generators/reconciler_controller.go	`0.00% <0.00%> (ø)`
controller/controller.go	`86.71% <100.00%> (+0.15%)`	⬆️
test/gcs/mock/mock.go	`91.39% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51cfaab...f4bed1c. Read the comment docs.

n3wscott · 2021-06-30T16:41:02Z

codegen/cmd/injection-gen/generators/reconciler_controller.go

@@ -225,11 +234,12 @@ func NewImpl(ctx {{.contextContext|raw}}, r Interface{{if .hasClass}}, classValu
 					return err
 				}
 				for _, elt := range all {
-					// TODO: Consider letting users specify a filter in options.


I love it when we find a need for the TODO and then implement the TODO 😄

n3wscott

This LGTM, can I ask you to add a small note in the docs for it so someone reading the docs might realize they can use this option: https://github.com/knative/pkg/blob/main/injection/README.md

/lgtm

benmoss · 2021-07-01T14:14:31Z

/hold

benmoss · 2021-07-06T13:57:44Z

Opened #2180 instead

Allow global resync & reconcilerImpl informers to be filtered via fil…

a80eafc

…terfunc

knative-prow-robot requested a review from markusthoemmes June 30, 2021 16:16

google-cla bot added the cla: no Indicates the PR's author has not signed the CLA. label Jun 30, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 30, 2021

benmoss commented Jun 30, 2021

View reviewed changes

benmoss changed the title ~~Filter func controller informers~~ Allow global resync & reconcilerImpl informers to be filtered Jun 30, 2021

benmoss force-pushed the filterFunc-controller-informers branch from 8973f78 to 3ec10ea Compare June 30, 2021 16:24

Set filter func via the same option

f4bed1c

Users can just set the impl FilterFunc option and it will percolate down to the controller.ControllerOptions GlobalResyncFilterFunc as well.

benmoss force-pushed the filterFunc-controller-informers branch from 3ec10ea to f4bed1c Compare June 30, 2021 16:30

n3wscott reviewed Jun 30, 2021

View reviewed changes

knative-prow-robot assigned n3wscott Jun 30, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2021

benmoss mentioned this pull request Jul 6, 2021

Add ability to filter objects on injection controller promotion #2180

Merged

benmoss closed this Jul 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow global resync & reconcilerImpl informers to be filtered #2176

Allow global resync & reconcilerImpl informers to be filtered #2176

benmoss commented Jun 30, 2021 •

edited

Loading

google-cla bot commented Jun 30, 2021

knative-prow-robot commented Jun 30, 2021

benmoss Jun 30, 2021 •

edited

Loading

google-cla bot commented Jun 30, 2021

google-cla bot commented Jun 30, 2021

codecov bot commented Jun 30, 2021 •

edited

Loading

n3wscott Jun 30, 2021

n3wscott left a comment

benmoss commented Jul 1, 2021

benmoss commented Jul 6, 2021

Allow global resync & reconcilerImpl informers to be filtered #2176

Allow global resync & reconcilerImpl informers to be filtered #2176

Conversation

benmoss commented Jun 30, 2021 • edited Loading

Changes

google-cla bot commented Jun 30, 2021

knative-prow-robot commented Jun 30, 2021

benmoss Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

google-cla bot commented Jun 30, 2021

google-cla bot commented Jun 30, 2021

codecov bot commented Jun 30, 2021 • edited Loading

Codecov Report

n3wscott Jun 30, 2021

Choose a reason for hiding this comment

n3wscott left a comment

Choose a reason for hiding this comment

benmoss commented Jul 1, 2021

benmoss commented Jul 6, 2021

benmoss commented Jun 30, 2021 •

edited

Loading

benmoss Jun 30, 2021 •

edited

Loading

codecov bot commented Jun 30, 2021 •

edited

Loading