-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Add cache option to filter by field #1404
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qinqon 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 |
250a741
to
c1948c8
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.
Hi @qinqon, thanks for the contribution. Left a few comments, let's please reduce the PR to the absolute minimum set of changes needed for the requested feature addition. Also this doesn't seem to be a 🐛, but rather a new feature ✨
pkg/cache/internal/informers_map.go
Outdated
// Scheme maps runtime.Objects to GroupVersionKinds | ||
Scheme *runtime.Scheme |
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.
This change seems outside the scope of this PR?
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.
Done
pkg/cache/internal/informers_map.go
Outdated
// mapper maps GroupVersionKinds to Resources | ||
mapper meta.RESTMapper | ||
|
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.
Same as above, could we keep these?
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.
Done
pkg/cache/internal/informers_map.go
Outdated
// resync is the base frequency the informers are resynced | ||
// a 10 percent jitter will be added to the resync period between informers | ||
// so that all informers will not send list requests simultaneously. | ||
resync time.Duration | ||
|
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.
Same as above, could we keep these?
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.
Done
pkg/cache/internal/informers_map.go
Outdated
// namespace is the namespace that all ListWatches are restricted to | ||
// default or empty string means all namespaces | ||
namespace string |
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.
Same as above, could we keep these?
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.
Done
pkg/cache/cache.go
Outdated
|
||
// FieldSelectorByResource restricts the cache's ListWatch to the desired | ||
// fields per resource | ||
FieldSelectorByResource map[string]string |
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.
What's the key and value expected to look like here? Would it better to use a GroupKind as key instead?
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.
Or GroupResource?
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.
Used GroupResource so we can match all the versions.
pkg/cache/internal/options.go
Outdated
// Options are the optional arguments for creating a new InformersMap object | ||
type Options struct { | ||
Scheme *runtime.Scheme | ||
Mapper meta.RESTMapper | ||
Resync *time.Duration | ||
Namespace string | ||
FieldSelectorByResource map[string]string | ||
} |
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.
Given that this is all internal, how are users supposed to use this new feature?
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.
Removed the internal Options
/ok-to-test |
f5be2b0
to
2dd1373
Compare
Remove the commit that encapsulate some internal attributes to an internal Options struct and create a small helper to find the fieldselector on the map using the resource from mapper, @vincepri you can take a look now |
2dd1373
to
6edc30b
Compare
/test pull-controller-runtime-test-master |
pkg/cache/cache.go
Outdated
|
||
// FieldSelectorByResource restricts the cache's ListWatch to the desired | ||
// fields per resource | ||
FieldSelectorByResource map[schema.GroupResource]string |
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.
Please add a sample field selector here and link to the kube docs about field selectors, many ppl are not aware that they only work on a very limited set of fields and not at all for CRDs
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.
Done
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.
its medadata.name
, not meta.name
or not?
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.
Done, correct value is metadata.name
.
pkg/cache/internal/informers_map.go
Outdated
@@ -231,6 +235,15 @@ func (ip *specificInformersMap) addInformerToMap(gvk schema.GroupVersionKind, ob | |||
return i, ip.started, nil | |||
} | |||
|
|||
func (ip *specificInformersMap) findFieldSelectorByGVR(gvr schema.GroupVersionResource) string { | |||
gr := schema.GroupResource{Group: gvr.Group, Resource: gvr.Resource} | |||
fieldSelector, ok := ip.fieldSelectorByResource[gr] |
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.
Nit: This is an empty string if there isn't a match in the map, so you do not need the ok
check
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.
Done
6edc30b
to
cf2e7dc
Compare
cf2e7dc
to
948d09c
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.
Looks good except for the doc sample.
I can try to add labels filtering at a follow up PR to keep this small.
If you have the time for that, that would be great but I didn't mean to say that you are in any way obligated to that, just that it would be premature to close the issue
pkg/cache/cache.go
Outdated
|
||
// FieldSelectorByResource restricts the cache's ListWatch to the desired | ||
// fields per resource | ||
FieldSelectorByResource map[schema.GroupResource]string |
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.
its medadata.name
, not meta.name
or not?
948d09c
to
03473b4
Compare
@vincepri do you know if Is the issue at pull-controller-runtime-apidiff-master important ? |
03473b4
to
a4e89ae
Compare
force-push: Rebased |
@qinqon The one thing this change ignores entirely is what impact this has for the cache-backed client. Could you write a small design doc in a different PR that:
Doesn't have to be anything big, but we asked for that multiple times in #244 (comment) and declined other PRs because of that. I personally like the simplicity of just saying "We allow this on the cache and if you use a custom cache on your manager, you imply that you know better" but its a bigger design question and I want to be sure there is agreement about that. |
@alvaroaleman done a PR with the proposal #1419 |
a4e89ae
to
b6217c6
Compare
force-push: Rebased and converting the map value to |
As a follow up of [1] this change instruct the ListWatcher from the informers to select by label/field but instead of using a manager/cache Options it pass the select using the builder. [1] kubernetes-sigs#1404 Signed-off-by: Quique Llorente <ellorent@redhat.com>
All instance for a same resources are being cached by controller-runtime, for some use cases this consumes a lot of memory and CPU. This change add a option to the cache so resources can be filtered by field. Signed-off-by: Quique Llorente <ellorent@redhat.com>
b6217c6
to
ff97b8f
Compare
@qinqon: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
All instance for a same resources are being cached by
controller-runtime, for some use cases this consumes a lot of memory and
CPU. This change add a option to the cache so resources can be filtered
by field.