Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Add SelectorsByObject option to cache #1435

Merged
merged 1 commit into from Apr 27, 2021
Merged

✨ Add SelectorsByObject option to cache #1435

merged 1 commit into from Apr 27, 2021

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Mar 17, 2021 •
edited

Controller-Runtime controllers use a cache to subscribe to events from
Kubernetes objects and to read those objects more efficiently by avoiding
to call out to the API. This cache is backed by Kubernetes informers.

The only way to filter this cache is by namespace and resource type.
In cases where a controller is only interested in a small subset of objects
(for example all pods on a node), this might end up not being efficient enough.

This change increase the "pkg/cache" interface adding the
"BuildWithOptins" function and the "Options.SelectorsByObject" option.

The SelectorsByObject restricts the cache's ListWatch to the desired
fields per GVK at the specified object, the map's value must implement
Selector [1] using for example a Set [2]

This is the implementation of the design document [3]

[1] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Selector
[2] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Set
[3] https://github.com/kubernetes-sigs/controller-runtime/blob/master/designs/use-selectors-at-cache.md

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @qinqon. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 17, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 17, 2021
@qinqon qinqon mentioned this pull request Mar 17, 2021
Merged
@visheshtanksale
Copy link

@qinqon Thanks for the PR. This will be very helpful..

@qinqon
Copy link
Contributor Author

qinqon commented Mar 23, 2021

@visheshtanksale this is just a PoC for the design document here #1419

@qinqon qinqon force-pushed the add-cache-filter-option-labels-and-fields branch 2 times, most recently from 551ced4 to 48fe9c7 Compare April 5, 2021 09:22
@qinqon qinqon force-pushed the add-cache-filter-option-labels-and-fields branch 6 times, most recently from 2ff9e18 to 6d1af5b Compare April 15, 2021 11:11
@qinqon
Copy link
Contributor Author

qinqon commented Apr 15, 2021

/cc @alvaroaleman @estroz

The PR to implement #1419 is ready.

@qinqon qinqon changed the title ✨ Add cache filter option labels and fields ✨ Add SelectorsByObject option to cache Apr 15, 2021
@qinqon qinqon force-pushed the add-cache-filter-option-labels-and-fields branch from 6d1af5b to 79f3347 Compare April 15, 2021 11:48
timebertt
timebertt reviewed Apr 16, 2021
View reviewed changes
Copy link
Contributor

@timebertt timebertt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, looking forward to the feature!
I have a few suggestions, see inline.

pkg/cache/internal/informers_map.go Show resolved Hide resolved
pkg/cache/internal/selector.go Show resolved Hide resolved
pkg/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/cache/cache_test.go Outdated Show resolved Hide resolved
@timebertt timebertt mentioned this pull request Apr 16, 2021
Closed
@qinqon qinqon force-pushed the add-cache-filter-option-labels-and-fields branch from 79f3347 to 37fe04e Compare April 16, 2021 09:13
@qinqon qinqon requested a review from estroz April 16, 2021 09:14
@qinqon
Copy link
Contributor Author

qinqon commented Apr 16, 2021

@timebertt can you re-review ?

alvaroaleman
alvaroaleman reviewed Apr 18, 2021
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ok-to-test

pkg/cache/cache.go Outdated Show resolved Hide resolved
pkg/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/cache/cache.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 18, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2021
@estroz
Copy link
Contributor

estroz commented Apr 27, 2021

Nope lets get this in!

/lgtm
/hold cancel
/override pull-controller-runtime-apidiff-master

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2021
@estroz
Copy link
Contributor

estroz commented Apr 27, 2021

@alvaroaleman you will have to override apidiff, I don't have perms to do so

@k8s-ci-robot k8s-ci-robot merged commit 4477c71 into kubernetes-sigs:master Apr 27, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.9.x milestone Apr 27, 2021
@estroz
Copy link
Contributor

estroz commented Apr 27, 2021

Nevermind, that worked!

@qinqon
Copy link
Contributor Author

qinqon commented Apr 27, 2021

@estroz @alvaroaleman can this be cherry picked to 0.8 and do a release with it ?

@alvaroaleman
Copy link
Member

@alvaroaleman you will have to override apidiff, I don't have perms to do so

It's optional :)

@estroz
Copy link
Contributor

estroz commented Apr 27, 2021

Since this is a feature and not a bugfix, wouldn't it be best to just release in v0.9.0?

@timebertt timebertt mentioned this pull request May 4, 2021
Merged
This was referenced Jun 7, 2021
Closed
Open
@timebertt timebertt mentioned this pull request Jun 16, 2021
Closed
@roee88 roee88 mentioned this pull request Jun 29, 2021
Merged
@johscheuer johscheuer mentioned this pull request Jul 10, 2021
Closed
@phoracek phoracek mentioned this pull request Aug 23, 2021
Closed
@ialidzhikov ialidzhikov mentioned this pull request Sep 7, 2021
Closed
@zhujian7 zhujian7 mentioned this pull request Oct 14, 2021
Merged
@amisevsk amisevsk mentioned this pull request Oct 15, 2021
Closed
@johscheuer johscheuer mentioned this pull request Nov 15, 2021
Closed
@thbkrkr thbkrkr mentioned this pull request Dec 1, 2021
Closed
@moolen moolen mentioned this pull request Feb 15, 2022
Closed
@fedepaol fedepaol mentioned this pull request Mar 25, 2022
Merged
@larsks larsks mentioned this pull request Jun 19, 2022
Open
@flavio flavio mentioned this pull request Apr 13, 2023
Merged
flavio added a commit to flavio/kubewarden-controller that referenced this pull request Apr 13, 2023
@flavio
fix: reduce RBAC access granted to controller service account
4394600 
Reduce the access to the cluster given to the ServiceAccount
used by the controller.

This leverages the changes introduced by kubernetes-sigs/controller-runtime#1435
to allow controllers to reduce the scope of watches.

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@karlkfi karlkfi mentioned this pull request Nov 14, 2023
Merged
@czeslavo czeslavo mentioned this pull request Apr 17, 2024
Open
6 tasks
@czeslavo czeslavo mentioned this pull request Apr 25, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timebertt timebertt timebertt left review comments

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@gerred gerred Awaiting requested review from gerred

@mengqiy mengqiy Awaiting requested review from mengqiy

@estroz estroz Awaiting requested review from estroz

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@alvaroaleman alvaroaleman

@estroz estroz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9.x
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@qinqon @k8s-ci-robot @visheshtanksale @estroz @alvaroaleman @timebertt