🌱 Add a design document to filter cache by selectors #1419

qinqon · 2021-03-09T13:13:34Z

The controller-runtime is caching all the resource instance even if not
all of them end up being reconciled, this design document describe a
proposal to that problem by use selectors at the ListWatch informers
mechanism.

This is related to #1404

k8s-ci-robot · 2021-03-09T13:13:42Z

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.

alvaroaleman · 2021-03-09T14:52:49Z

@qinqon thanks for putting this up! Could you join the kubebuilder and controller-runtime biweekly meeting this Thursday 18 CET and show it briefly?

qinqon · 2021-03-09T15:03:54Z

@alvaroaleman sure, I will be there.

qinqon · 2021-03-11T14:24:25Z

@alvaroaleman do I have to add a bullet to the agenda or just rise a hand on the meeting ?

alvaroaleman · 2021-03-11T14:25:57Z

@qinqon a bullet would be great

qinqon · 2021-03-12T13:54:05Z

/cc @alvaroaleman @vincepri

Let me know if I have to pin someone else here, so we can discuss it.

designs/filter-cache-by-field.md

qinqon · 2021-03-16T09:35:43Z

force-push: Removed pkg/cache changes from this PR, converted map value to fields.Selector and added warning to proposal.

qinqon · 2021-03-16T11:34:49Z

force-push: Added an additional proposal to pass the selector using the builder instead of manager.Options as implemented in #1425

qinqon · 2021-03-17T09:17:39Z

force-push: Removed proposal 2 and adapterd former proposal to use NewCache using a builder function that will return a constructor.

qinqon · 2021-03-17T09:18:58Z

Does it make sense to include labels in this proposal ?, the interfaces to use labels are almost identicall to fields and we will be able to close the issue.

UPDATE: Created a PoC adding labels too #1435

alvaroaleman · 2021-04-05T18:53:18Z

/hold
for others

estroz · 2021-04-05T19:14:45Z

designs/use-selectors-at-cache.md

+// SelectorsByObject associate a GroupResource to a field/label selector
+type SelectorsByGVK map[schema.GVK]Selector


Nit

Suggested change

// SelectorsByObject associate a GroupResource to a field/label selector

type SelectorsByGVK map[schema.GVK]Selector

// SelectorsByGVK associate a GroupVersionKind to a field/label selector

type SelectorsByGVK map[schema.GroupVersionKind]Selector

estroz · 2021-04-05T19:17:07Z

designs/use-selectors-at-cache.md

+...
+
+// SelectorsByObject associate a client.Object to a field/label selector
+type SelectorsByObject map[client.Object]Selector


This should be restricted to runtime.Object so users don't get the idea that you can filter by object name/namespace via SetName() and SetNamespace().

Suggested change

type SelectorsByObject map[client.Object]Selector

type SelectorsByObject map[runtime.Object]Selector

@estroz why? It is a map, the values can not access the keys. Making this runtime.Object means we have to deal with stuff like ppl putting List types in there

@alvaroaleman don't we have the same problem with client.Object since it's embedding runtime.Object ?

@alvaroaleman it may be confusing to users to have all these object fields exposed by the metav1.Object, since they may assume the key can actually apply its own selector values, ex. by SetName(). Instead it would be better to constrain the contract to just GroupVersionKind().

There can be some internal validation step to make sure the object is not a list type.

Using GVK is inconvenient. Using the smaller interface (runtime.Object) doesn't prevent ppl from putting in objects that fulfill a lager interface (client.Object). It does however keep them from putting something like List types in there and makes the compiler verify that rather than getting a runtime error. Also, I think the idea that ppl might think the map key has any further relevance for filtering when the value is a Selector struct seems a bit far fetched to me.

We also have plenty of prior art where we use client.Object to only derive the type, e.G. in source.Kind or the clients delete method.

You could similarly argue that someone would not try to map a selector to a list type because that's not the type they add to For(), Owns(), or Watches().

However since there is prior art I am fine with client.Object. There can always be another selector map type added later for runtime.Object without breakage if we see bugs filed because of confusion. It's not worth worrying about too much now.

Going back to client.Object as proposed originally.

estroz · 2021-04-05T19:23:33Z

designs/use-selectors-at-cache.md

+}
+
+// FillInListOpts fill in ListOptions LabelSelector and FieldSelector if needed
+func (s Selector) FillInListOpts(listOpts *metav1.ListOptions) {


How about

Suggested change

func (s Selector) FillInListOpts(listOpts *metav1.ListOptions) {

func (s Selector) ApplyToList(listOpts *client.ListOptions) {

so it implements client.ListOption

Since we are receiving metav1.ListOptions at ListWatch functions I will keep the type so we don't have to do convertions, but changing the name to ApplyToList feels good.

estroz · 2021-04-05T19:27:07Z

designs/use-selectors-at-cache.md

+	Mapper            meta.RESTMapper
+	Resync            *time.Duration
+	Namespace         string
+	SelectorsByObject SelectorsByObject


It would be nice to generalize this to something like

Suggested change

SelectorsByObject SelectorsByObject

Selectors SelectorSet

Where SelectorSet is any set of selectors that implements

type SelectorSet interface { client.ListOption Select() }

where Select() is just an identifier method so users don't pass in other client.ListOptions.

We apply options per GVK so it does not make sense to implement ListOption at the map level.

Sure, get rid of client.ListOption from that interface. Regardless, I may want to construct a GVK myself instead of using a typed object or Unstructured, so why not also expose SelectorsByGVK?

As we have previously stated we are going to expose the map key as client.Object, internally will be GVK, in fact at the implementation PR I will remove SelectorsByObject from internal and use it only as a type to pass to the cache Options, at Options parsing it will be converted to the SelectorsByGVK.

The controller-runtime is caching all the resource instance even if not all of them end up being reconciled, this design document describe a proposal to that problem by use selectors at the ListWatch informers mechanism. Signed-off-by: Quique Llorente <ellorent@redhat.com>

qinqon · 2021-04-08T06:21:08Z

Before we merge this, can you confirm that we don't want to have something like client.ListOptions as the map value to allow users to filter with all the possible options not only selectors.

alvaroaleman · 2021-04-08T12:51:52Z

Before we merge this, can you confirm that we don't want to have something like client.ListOptions as the map value to allow users to filter with all the possible options not only selectors.

What other options exist that we could filter by? Only the namespace or not?

qinqon · 2021-04-08T13:27:18Z

Before we merge this, can you confirm that we don't want to have something like client.ListOptions as the map value to allow users to filter with all the possible options not only selectors.

What other options exist that we could filter by? Only the namespace or not?

The other options are more dangerous than useful, so yes.

alvaroaleman · 2021-04-08T13:31:21Z

The other options are more dangerous than useful, so yes.

Yeah, the namespace is a bit redundant as its also a global option. Generally, we can easily extend the Selector in the future in a backwards-compatible way if we see a reason, so I am not too concerned about missing something there.

qinqon · 2021-04-12T09:30:31Z

@estroz are you ok with the changes ?

estroz

/lgtm

k8s-ci-robot · 2021-04-12T20:47:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, estroz, qinqon

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

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

alvaroaleman · 2021-04-12T21:10:22Z

/hold cancel
Lets get this done!

alvaroaleman · 2021-04-12T21:34:19Z

/retest

qinqon · 2021-04-13T06:12:12Z

/retest

I think we can override the job clearly is not related

qinqon · 2021-04-13T06:12:57Z

/test pull-controller-runtime-test-master

alvaroaleman · 2021-04-13T12:38:17Z

/retest

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

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 9, 2021

k8s-ci-robot requested review from alexeldeib and joelanford March 9, 2021 13:13

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 9, 2021

qinqon mentioned this pull request Mar 9, 2021

✨ Add cache option to filter by field #1404

Closed

k8s-ci-robot requested review from alvaroaleman and vincepri March 12, 2021 13:54

alvaroaleman reviewed Mar 14, 2021

View reviewed changes

designs/filter-cache-by-field.md Outdated Show resolved Hide resolved

alvaroaleman reviewed Mar 14, 2021

View reviewed changes

designs/filter-cache-by-field.md Outdated Show resolved Hide resolved

qinqon force-pushed the add-cache-filter-field-option branch from 69c3cbf to e2bc322 Compare March 16, 2021 09:34

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2021

qinqon requested a review from alvaroaleman March 16, 2021 09:36

qinqon force-pushed the add-cache-filter-field-option branch from e2bc322 to 95dbc43 Compare March 16, 2021 11:31

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 16, 2021

qinqon force-pushed the add-cache-filter-field-option branch from 95dbc43 to abadf65 Compare March 17, 2021 09:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2021

k8s-ci-robot assigned alvaroaleman Apr 5, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 5, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2021

estroz reviewed Apr 5, 2021

View reviewed changes

qinqon force-pushed the add-cache-filter-field-option branch from e9f3b9d to 3c3313f Compare April 6, 2021 08:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2021

qinqon requested a review from estroz April 6, 2021 09:05

qinqon force-pushed the add-cache-filter-field-option branch from 3c3313f to 141ddd6 Compare April 8, 2021 06:19

estroz approved these changes Apr 12, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2021

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 12, 2021

k8s-ci-robot merged commit 2d6828d into kubernetes-sigs:master Apr 13, 2021

k8s-ci-robot added this to the v0.9.x milestone Apr 13, 2021

		// SelectorsByObject associate a GroupResource to a field/label selector
		type SelectorsByGVK map[schema.GVK]Selector

	type SelectorsByObject map[client.Object]Selector
	type SelectorsByObject map[runtime.Object]Selector

	func (s Selector) FillInListOpts(listOpts *metav1.ListOptions) {
	func (s Selector) ApplyToList(listOpts *client.ListOptions) {

🌱 Add a design document to filter cache by selectors #1419

🌱 Add a design document to filter cache by selectors #1419

Conversation

qinqon commented Mar 9, 2021 • edited Loading

k8s-ci-robot commented Mar 9, 2021

alvaroaleman commented Mar 9, 2021

qinqon commented Mar 9, 2021

qinqon commented Mar 11, 2021 • edited Loading

alvaroaleman commented Mar 11, 2021

qinqon commented Mar 12, 2021

qinqon commented Mar 16, 2021 • edited Loading

qinqon commented Mar 16, 2021

qinqon commented Mar 17, 2021

qinqon commented Mar 17, 2021 • edited Loading

alvaroaleman commented Apr 5, 2021

estroz Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

qinqon commented Apr 8, 2021 • edited Loading

alvaroaleman commented Apr 8, 2021

qinqon commented Apr 8, 2021

alvaroaleman commented Apr 8, 2021

qinqon commented Apr 12, 2021

estroz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 12, 2021

alvaroaleman commented Apr 12, 2021

alvaroaleman commented Apr 12, 2021

qinqon commented Apr 13, 2021

qinqon commented Apr 13, 2021

alvaroaleman commented Apr 13, 2021

qinqon commented Mar 9, 2021 •

edited

Loading

qinqon commented Mar 11, 2021 •

edited

Loading

qinqon commented Mar 16, 2021 •

edited

Loading

qinqon commented Mar 17, 2021 •

edited

Loading

estroz Apr 5, 2021 •

edited

Loading

estroz Apr 5, 2021 •

edited

Loading

qinqon Apr 6, 2021 •

edited

Loading

qinqon Apr 6, 2021 •

edited

Loading

qinqon Apr 8, 2021 •

edited

Loading

qinqon commented Apr 8, 2021 •

edited

Loading