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

🌱 Refactor internal cache/informers map impl #2103

Merged

k8s-ci-robot merged 2 commits into kubernetes-sigs:master from vincepri:cleanup-informer-map

Dec 22, 2022

Member

vincepri commented Dec 16, 2022 •

edited

Loading

This is a first of a series of cleanups of our internal libraries, which has been long overdue. There are two commits in here, which we can also separate in different PRs if needed. The first pass simplifies w/ adding options to the implementation, the second refactors the map by removing the delegating map logic, which had lots of repetition and created confusion in the past.

Signed-off-by: Vince Prignano vince@prigna.com

k8s-ci-robot added the cncf-cla: yes label

Contributor

k8s-ci-robot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

k8s-ci-robot requested review from alvaroaleman and joelanford

December 16, 2022 20:29

k8s-ci-robot added approved size/L labels

vincepri force-pushed the cleanup-informer-map branch 2 times, most recently from f637164 to 4a1bb11 Compare

December 16, 2022 20:33

Member Author

vincepri commented Dec 16, 2022

/assign @alvaroaleman @sbueringer

k8s-ci-robot assigned alvaroaleman and sbueringer

k8s-ci-robot added size/XL and removed size/L labels

vincepri changed the title ~~🌱 Cleanup internal cache/informers map impl~~ 🌱 Refactor internal cache/informers map impl

Member Author

vincepri commented Dec 17, 2022

/retest

alvaroaleman reviewed

View reviewed changes

pkg/cache/internal/informers_map.go Outdated Show resolved Hide resolved

pkg/cache/internal/informers_map.go Show resolved Hide resolved

vincepri force-pushed the cleanup-informer-map branch from ac5c3d7 to f4563b1 Compare

December 18, 2022 22:50

sbueringer reviewed

View reviewed changes

Member

sbueringer left a comment

Just some nits, overall lgtm

pkg/cache/internal/informers_map.go Outdated

+              // in an InformerMap.
+              type InformersMapOptionsByGVK struct {
+              	Selectors       SelectorsByGVK
+              	Transformers    TransformFuncByObject

Member

sbueringer Dec 21, 2022

No strong objection but it seems a bit odd to have "ByGVK" and then have TransformFuncByObject.

Looking at TransformFuncByObject it seems to essentially also use GVK

	Set(runtime.Object, *runtime.Scheme, cache.TransformFunc) error
	Get(schema.GroupVersionKind) cache.TransformFunc

Get directly works with GVK and only Set takes an Object but only uses it to infer the GVK.

Wondering if we could/should rename TransformFuncByObject to TransformFuncByGVK especially as I would say the main functionality is mapping from "something" to TransformFunc in Get and that is done by GVK

Member Author

vincepri Dec 21, 2022

Renamed, it was all internal functions fortunately

pkg/cache/internal/informers_map.go

    
            @@ -145,7 +152,7 @@ type specificInformersMap struct {
          
              // Start calls Run on each of the informers and sets started to true.  Blocks on the context.

              // It doesn't return start because it can't return an error, and it's not a runnable directly.

              func (ip *specificInformersMap) Start(ctx context.Context) {

              func (ip *InformersMap) Start(ctx context.Context) error {

Member

sbueringer Dec 21, 2022

Q: Was the error return parameter just added in case we want to return errors in the future? (as of now we only return nil)

Member Author

vincepri Dec 21, 2022

It was added to satisfy the Runnable interface, the same (nil error returned in any case) was before in the deleg_map file

Member

sbueringer Dec 21, 2022

Makes sense, thx for the info!

pkg/cache/internal/informers_map.go Outdated Show resolved Hide resolved

pkg/cache/internal/informers_map.go

Comment on lines +273 to +284

+              		ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
+              			ip.selectors(gvk).ApplyToList(&opts)
+              			return lw.ListFunc(opts)
+              		},
+              		WatchFunc: func(opts metav1.ListOptions) (watch.Interface, error) {
+              			ip.selectors(gvk).ApplyToList(&opts)
+              			opts.Watch = true // Watch needs to be set to true separately
+              			return lw.WatchFunc(opts)
+              		},

Member

sbueringer Dec 21, 2022

Q: would it make sense to do this in makeListWatcher so that makeListWatcher alredy creates the final ListWatch vs having "layered" ListWatches?

Member Author

vincepri Dec 21, 2022

Originally it was in each list watcher creation function, although the code was duplicate between each watch/listfunc, which was a bit repetitive. Do you see another way to do this that doesn't end up in repetition?

Member

sbueringer Dec 21, 2022

I see the following options:

write the listFunc and watchFunc in makeListWatcher inside the switch in variables and then build the ListWatch like it is done here at the end of the makeListWatcher func
writing the ListWatch'es in makeListWatcher in a var and then add this code at the end of the makeListWatcher func
defining inline modifyListOptions modifyWatchOptions func in makeListWatcher and calling them in each case

I would probably go with option 1, but also fine to just keep it as is if you want to

pkg/cache/internal/informers_map.go Outdated Show resolved Hide resolved

vincepri force-pushed the cleanup-informer-map branch from f4563b1 to aff10e3 Compare

December 21, 2022 15:36


          🌱 Cleanup internal cache/informers map impl

67c79f0

Signed-off-by: Vince Prignano <vince@prigna.com>

vincepri force-pushed the cleanup-informer-map branch 2 times, most recently from ccd3222 to 24812a3 Compare

December 21, 2022 15:42


          Remove delegating map impl, and simplify InformersMap

384ffbc

Signed-off-by: Vince Prignano <vince@prigna.com>

vincepri force-pushed the cleanup-informer-map branch from 24812a3 to 384ffbc Compare

December 21, 2022 15:47

sbueringer reviewed

View reviewed changes

pkg/cache/internal/informers_map.go

+              				return list, err
+              			},
+              			// Setup the watch function
+              			WatchFunc: func(opts metav1.ListOptions) (watcher watch.Interface, err error) {

Member

sbueringer Dec 21, 2022

Suggested change

      
            			WatchFunc: func(opts metav1.ListOptions) (watcher watch.Interface, err error) {
          
            			WatchFunc: func(opts metav1.ListOptions) (watch.Interface, error) {

nit: maybe get rid of the named return parameters if we don't really need them anymore
(fine if you want to keep it)

pkg/cache/internal/informers_map.go

    
            @@ -145,7 +152,7 @@ type specificInformersMap struct {
          
              // Start calls Run on each of the informers and sets started to true.  Blocks on the context.

              // It doesn't return start because it can't return an error, and it's not a runnable directly.

              func (ip *specificInformersMap) Start(ctx context.Context) {

              func (ip *InformersMap) Start(ctx context.Context) error {

Member

sbueringer Dec 21, 2022

Makes sense, thx for the info!

pkg/cache/internal/informers_map.go

Comment on lines +273 to +284

+              		ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
+              			ip.selectors(gvk).ApplyToList(&opts)
+              			return lw.ListFunc(opts)
+              		},
+              		WatchFunc: func(opts metav1.ListOptions) (watch.Interface, error) {
+              			ip.selectors(gvk).ApplyToList(&opts)
+              			opts.Watch = true // Watch needs to be set to true separately
+              			return lw.WatchFunc(opts)
+              		},

Member

sbueringer Dec 21, 2022

I see the following options:

write the listFunc and watchFunc in makeListWatcher inside the switch in variables and then build the ListWatch like it is done here at the end of the makeListWatcher func
writing the ListWatch'es in makeListWatcher in a var and then add this code at the end of the makeListWatcher func
defining inline modifyListOptions modifyWatchOptions func in makeListWatcher and calling them in each case

I would probably go with option 1, but also fine to just keep it as is if you want to

Member Author

vincepri commented Dec 21, 2022

/retest

Member

sbueringer commented Dec 22, 2022

/lgtm

k8s-ci-robot added the lgtm label

k8s-ci-robot merged commit 3c4deba into kubernetes-sigs:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm size/XL