k8s/informers: centralize informers to simplify code #4801

keithmattix · 2022-06-09T05:02:22Z

Instead of creating different objects (from 5 different packages) that
each manage their own informer collections, create a single object that
runs a set of informers. A pointer to this object is then shared with
all objects that need it.

Addresses #4551

This PR only modifies the k8s client; once this is merged, the other 4 clients (SMI, config, configurator, and policy) will be modified to use InformerCollection as well

Description:

Testing done:

Affected area:

Functional Area
Control Plane	[X]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project? no
- Did you notify the maintainers and provide attribution? N/A
Is this a breaking change? no
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? N/A

Instead of creating different objects (from 5 different packages) that each manage their own informer collections, create a single object that runs a set of informers. A pointer to this object is then shared with all objects that need it Signed-off-by: Keith Mattix II <[email protected]>

Signed-off-by: Keith Mattix II <[email protected]>

steeling

A lot of questions intended as thought experiment, with some concrete suggestions as well

pkg/k8s/informers/informers.go

steeling · 2022-06-09T13:13:31Z

pkg/k8s/informers/informers.go

+		InformerKeyRetry:                  ic.initRetryMonitor,
+	}
+
+	if len(ic.selectedInformers) == 0 {


Consider allowing no informers to run, and defaulting to the raw client calls

I feel like the informer collection can probably assume that the informers should be used rather than raw client calls.

One of the issues is that (I think) calling Add on the store, only adds the object to the local store, and not to the k8s master. IDK a scenario in which we would actually want to expose that. Removing footguns would be a benefit

So with this proposed implementation, the store is never actually exposed unless you opt-in by passing in a custom store. The previous implementations kept the store exposed for testing purposes (which I definitely think was the right tradeoff given the complexity of timing some of these tests with fake informers), but I agree that removing that possible footgun is preferable which is why I added that custom store option

pkg/k8s/informers/informers.go

steeling · 2022-06-09T13:17:50Z

pkg/k8s/informers/init.go

+	ic.informers[InformerKeyPod] = informer
+}
+
+func (ic *InformerCollection) initEndpointMonitor() {


A lot of boilerplate in these functions. Could we clean this up with a map?

ie: for key, client := range ic.informers {
... init and cache informer.
}

You would need a getter on the informer factory though. Again just trying to provoke the train of thought :)

We essentially do that in the NewInformerCollection function; the reason why it's difficult to de-duplicate further is because of the informer factory initialization (e.g. informerFactory.Access().V1alpha3().TrafficTargets().Informer()).

pkg/k8s/informers/types.go

steeling · 2022-06-09T13:22:48Z

pkg/k8s/informers/informers.go

+// WithCustomStore sets the shared store for the informerCollection to reference.
+// This store will be used instead of each informer's store. This should typically
+// only be used in tests
+func WithCustomStores(stores map[InformerKey]cache.Store) InformerCollectionOpt {


Can you provide an example when we would want to use this in tests?

Does this have improvements over the fakeclient stuff?

If not, I'd say let's take it out, and refactor out the customStore stuff in the informer type below

Yeah this was actually necessary to prevent us from having to run informers and deal with timing issues in tests. Check out client_test.go. GitHub hides it by default (large diff)

Can you expand on those issues? Maybe write a comment on why it's there including technical details.

Added the comment here since GitHub won't update the code showed in the PR comment:

WithCustomStores provides the InformerCollection an set of cache.Stores indexed
by InformerKey. This functionality was added for the express purpose of testing
flexibility since the alternative often leads to flaky tests and race conditions
between the time an object is added to a fake clientset and when that object
is actually added to the informer cache.Store.

Signed-off-by: Keith Mattix II <[email protected]>

steeling · 2022-06-09T16:25:43Z

pkg/k8s/informers/informers.go

+}
+
+// WithKubeClient sets the kubeClient for the InformerCollection
+func WithKubeClient(kubeClient kubernetes.Interface) InformerCollectionOption {


Is there ever a place where we pass in a client but don't run the informer? If not, I'd say this could be the signal.

If there is, maybe we could add a bool here, runInformer? Simplifying the API otherwise you could specify an informer to run where the client was never passed in, which would be a bug.

Good call; I've implemented that pattern 👍🏾

a further suggestion: put the init methods in here and get rid of the initInformerMap

I think the map makes initialization cleaner; the alternatives are to re-declare a subset of the map and loop through the keys again or to have each method called sequentially. Happy to discuss further, but I feel like it's probably a preference

steeling · 2022-06-09T16:27:26Z

pkg/k8s/informers/informers.go

+		InformerKeyRetry:                  ic.initRetryMonitor,
+	}
+
+	if len(ic.selectedInformers) == 0 {


One of the issues is that (I think) calling Add on the store, only adds the object to the local store, and not to the k8s master. IDK a scenario in which we would actually want to expose that. Removing footguns would be a benefit

Signed-off-by: Keith Mattix II <[email protected]>

codecov-commenter · 2022-06-09T16:42:40Z

Codecov Report

Merging #4801 (a2b6ed3) into main (1e73ba3) will decrease coverage by 0.34%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #4801      +/-   ##
==========================================
- Coverage   68.89%   68.54%   -0.35%     
==========================================
  Files         223      223              
  Lines       16399    16225     -174     
==========================================
- Hits        11298    11122     -176     
- Misses       5049     5052       +3     
+ Partials       52       51       -1

Flag	Coverage Δ
unittests	`68.54% <50.00%> (-0.35%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/osm-controller/osm-controller.go	`13.28% <0.00%> (-0.60%)`	⬇️
pkg/k8s/types.go	`100.00% <ø> (ø)`
pkg/k8s/client.go	`95.81% <100.00%> (+1.73%)`	⬆️
pkg/identity/types.go	`76.92% <0.00%> (-23.08%)`	⬇️
pkg/service/types.go	`62.50% <0.00%> (-4.17%)`	⬇️
pkg/ticker/ticker.go	`83.33% <0.00%> (-3.85%)`	⬇️
pkg/envoy/lds/rbac.go	`80.35% <0.00%> (-2.36%)`	⬇️
pkg/trafficpolicy/trafficpolicy.go	`95.68% <0.00%> (-1.79%)`	⬇️
pkg/catalog/ingress.go	`96.29% <0.00%> (-0.23%)`	⬇️
pkg/errcode/errcode.go	`100.00% <0.00%> (ø)`
... and 5 more

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 1e73ba3...a2b6ed3. Read the comment docs.

shashankram · 2022-06-09T16:42:16Z

pkg/k8s/client.go

 	// Add event handler to informer
 	nsEventTypes := EventTypes{
 		Add:    announcements.NamespaceAdded,
 		Update: announcements.NamespaceUpdated,
 		Delete: announcements.NamespaceDeleted,
 	}
-	c.informers[Namespaces].AddEventHandler(GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))
+	c.informers.AddEventHandler(osminformers.InformerKeyNamespace, GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))


nit: we can drop the prefix InformerKey from the variable name. Same applies to other informer keys.

Suggested change

c.informers.AddEventHandler(osminformers.InformerKeyNamespace, GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))

c.informers.AddEventHandler(osminformers.NamespaceKey, GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))

This prefix is actually a naming convention used in client-go and other packages when creating variables for a type alias. It helps remove ambiguity when searching for ambiguous or overloaded vars (like namespace or pod)

Signed-off-by: Keith Mattix II <[email protected]>

steeling · 2022-06-09T17:05:57Z

pkg/k8s/client_test.go

 			a.Nil(err)
-			_ = c.informers[Services].GetStore().Add(tc.service)
+			c := newClient(ic, nil, nil)
+			_ = store.Add(tc.service)


So the real store doesn't return the value immediately when doing an add followed by a get?

I thought the only time it wasn't immediate is when doing an add through the actual client, which triggers an async add.

I believe this direct add is synchronous

The direct add is synchronous, but recall that the underlying map of informers is unexported; the tests in k8s/client_test.go don't have access to it (or their stores by extension).

but if it's synchronous why are we instantiating a fake store here?

Because we can't access the real store from this package

I see. adding all of this extra stuff to get around this testing seems like a bit of a code smell IMO. I would recommend 1 of 3 things:

Add all the objects to the k8s client, ie testclient.NewSimpleClientset(tc.namespace, ...), then use the internals to ensure that the informer HasSynced.

Expose an add method on the store itself, so you don't need this fake store.

Don't separate these into 2 different packages if you need to rely so heavily on internal implementation details

pkg/k8s/informers/init.go

pkg/k8s/informers/informers.go

Signed-off-by: Keith Mattix II <[email protected]>

keithmattix added 2 commits June 8, 2022 23:55

Update changes from main to use informercollection

cd24fa8

Signed-off-by: Keith Mattix II <[email protected]>

keithmattix requested review from shashankram, snehachhabria, nojnhuh, draychev, jaellio and trstringer as code owners June 9, 2022 05:02

steeling suggested changes Jun 9, 2022

View reviewed changes

Fix PR comments and startup errors

45436de

Signed-off-by: Keith Mattix II <[email protected]>

keithmattix requested a review from steeling June 9, 2022 16:05

Fix nil opt and linting errors

d71fa54

Signed-off-by: Keith Mattix II <[email protected]>

steeling reviewed Jun 9, 2022

View reviewed changes

Add comment describing WithCustomStores functionality

ceb522c

Signed-off-by: Keith Mattix II <[email protected]>

shashankram approved these changes Jun 9, 2022

View reviewed changes

shashankram changed the title ~~Refactor/centralize informers~~ k8s/informers: centralize informers to simplify code Jun 9, 2022

Infer selected informers based on client options

be516de

Signed-off-by: Keith Mattix II <[email protected]>

steeling reviewed Jun 9, 2022

View reviewed changes

jaellio reviewed Jun 9, 2022

View reviewed changes

nojnhuh reviewed Jun 9, 2022

View reviewed changes

pkg/k8s/informers/informers.go Outdated Show resolved Hide resolved

keithmattix requested a review from steeling June 9, 2022 17:33

Resolve PR comments

a2b6ed3

Signed-off-by: Keith Mattix II <[email protected]>

nojnhuh approved these changes Jun 9, 2022

View reviewed changes

Fix linting errors

7653159

Signed-off-by: Keith Mattix II <[email protected]>

keithmattix requested a review from jaellio June 9, 2022 18:27

jaellio approved these changes Jun 9, 2022

View reviewed changes

jaellio merged commit 47c06ab into openservicemesh:main Jun 9, 2022

keithmattix deleted the refactor/centralize-informers branch June 9, 2022 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k8s/informers: centralize informers to simplify code #4801

k8s/informers: centralize informers to simplify code #4801

keithmattix commented Jun 9, 2022 •

edited

Loading

steeling left a comment

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022 •

edited

Loading

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

codecov-commenter commented Jun 9, 2022 •

edited

Loading

shashankram Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

keithmattix Jun 9, 2022

steeling Jun 9, 2022

	c.informers.AddEventHandler(osminformers.InformerKeyNamespace, GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))
	c.informers.AddEventHandler(osminformers.NamespaceKey, GetEventHandlerFuncs(nil, nsEventTypes, c.msgBroker))

k8s/informers: centralize informers to simplify code #4801

k8s/informers: centralize informers to simplify code #4801

Conversation

keithmattix commented Jun 9, 2022 • edited Loading

steeling left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithmattix Jun 9, 2022 • 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

codecov-commenter commented Jun 9, 2022 • edited Loading

Codecov Report

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

keithmattix commented Jun 9, 2022 •

edited

Loading

keithmattix Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading