Create TransformingInformer #104300

wojtek-t · 2021-08-11T10:28:24Z

TransformingInfomer is like a regular Informer, but allows for applying custom transform functions on the objects received via list/watch API calls.

This pattern was first used in CoreDNS couple years ago:
https://github.com/coredns/coredns/blob/9d5b8cd13d541cc3d2b7598ab255643d6298ba9d/plugin/kubernetes/object/informer.go#L30
Over time, with increasing amount of controllers and developers being more conscious about memory usage, I'm seeing more and more places where this could be potentially used. So it makes sense to actually make it supported by the client-go libraries natively.

NONE

/sig api-machinery
/kind feature
/priority important-longterm

wojtek-t · 2021-08-11T10:28:33Z

/assign @liggitt

wojtek-t · 2021-08-11T10:29:10Z

staging/src/k8s.io/client-go/tools/cache/controller.go

+	return clientState, newInformer(lw, objType, resyncPeriod, h, clientState, nil)
+}
+
+// TransformFunc allows for transforming an object before it will be processed


@liggitt - obviously tests are missing in this PR, but before I go ahead and add them I would appreciate some early feedback on this change.

@deads2k - would you mind providing some early feedback?

I'm ok with the idea, but I'd like to be sure that the layers above this one make it difficult to accidentally confuse your listers with different transformations.

make it difficult to accidentally confuse your listers with different transformations.

Can you clarify? Is the comment about naming mostly? If so, I'm happy to change if you have better suggestions.

make it difficult to accidentally confuse your listers with different transformations.

Can you clarify? Is the comment about naming mostly? If so, I'm happy to change if you have better suggestions.

Today we generate Informers and Listers for every type. I'm somewhat concerned that people will make a TransformingPodLister, but if the type is still PodLister, it could get improperly passed around. I think we can expect people making use of a transformed PodLister to accept a different type. Does that seem reasonable?

Got it [I though that this comment is related to Transform itself, whereas it's more related to the returned values of "NewTransformingInformer"], i.e. the fact that it returns (Store, Controller).

The Controller itself doesn't seem controversial to me.

The potential problem is Store, as (IIUC what you're saying) it may lead to people thinking it contains different things (i.e. whole objects) than it really contains.

[The Lister itself isn't (at least in the shape of this PR a problem, because it requires indexer which I don't create here, but the change would be too simple to add to ignore it.]

So IIUC, what you're saying is that instead of returning Store, we should consider returning TransformingStore (or sth like that). Do I understand correctly?

If so, this effectively would be the same interface, so it would be possible once to another, so I'm not sure it would really bring value. WDYT?

If so, this effectively would be the same interface, so it would be possible once to another, so I'm not sure it would really bring value. WDYT?

Duck typing never fails to disappoint. I think you're right.

k8s-ci-robot · 2021-08-11T10:29:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wojtek-t

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:

~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [wojtek-t]

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

caesarxuchao · 2021-08-12T20:09:29Z

/triage accepted

deads2k · 2021-08-24T12:01:02Z

staging/src/k8s.io/client-go/tools/cache/controller.go

@@ -393,24 +425,33 @@ func newInformer(
 		Process: func(obj interface{}) error {
 			// from oldest to newest
 			for _, d := range obj.(Deltas) {
+				obj := d.Object
+				if transformer != nil {


Have I correctly skimmed the code below and it computes the keys for any indexes after the transform, so the data to compute is always present in the returned instances?

and it computes the keys for any indexes after the transform,

Yes.

so the data to compute is always present in the returned instances?

I guess it depends on the indexes that are passed. But in the correct usage the index should only use data from transformed objects.

deads2k · 2021-08-24T12:03:18Z

TransformingInfomer is like a regular Informer, but allows for applying custom transform functions on the objects received via list/watch API calls.

I don't object in principle, but are we sure that we'd rather invest this way as opposed to making it easy to use the metadata only list/watch? I remember garbage collection tried, but I don't recall how successful it was.

wojtek-t · 2021-08-25T12:45:11Z

I don't object in principle, but are we sure that we'd rather invest this way as opposed to making it easy to use the metadata only list/watch? I remember garbage collection tried, but I don't recall how successful it was.

Thanks @deads2k .
The metadata-only listwatch is useful (and I though we're already doing it, but I'm not 100% sure - I will double check). But it doesn't solve many of the usecases, including the one by CoreDNS where they really want some non-metadata fields too, just not all of them, and they really want to be memory-efficient.

Given the above, I will go ahead and try to add tests in the upcoming weeks and will ask you for more detailed review then.

wojtek-t

I have added a test - this is now ready for a more formal review.
@deads2k - PTAL when you will have a chance.

wojtek-t · 2021-09-01T10:06:05Z

staging/src/k8s.io/client-go/tools/cache/controller.go

@@ -393,24 +425,33 @@ func newInformer(
 		Process: func(obj interface{}) error {
 			// from oldest to newest
 			for _, d := range obj.(Deltas) {
+				obj := d.Object
+				if transformer != nil {


and it computes the keys for any indexes after the transform,

Yes.

so the data to compute is always present in the returned instances?

I guess it depends on the indexes that are passed. But in the correct usage the index should only use data from transformed objects.

deads2k · 2021-09-22T17:18:36Z

staging/src/k8s.io/client-go/tools/cache/controller.go

+// The given transform function will be called on all objects before they will
+// put put into the Store and corresponding Add/Modify/Delete handlers will
+// be invokved for them.
+func NewTransformingInformer(


Will there be an indexer option? Why or why not?

I didn't initially add indexer option as neither of the examples of this pattern I saw was using indexes.

But in the end - adding it is just a boilerplate, so we should probably do that in one shot.
I added that in a second commit.

TransformingInfomer is like a regular Informer, but allows for applying custom transform functions on the objects received via list/watch API calls.

wojtek-t

@deads2k - PTAL

wojtek-t · 2021-09-23T07:15:18Z

staging/src/k8s.io/client-go/tools/cache/controller.go

+// The given transform function will be called on all objects before they will
+// put put into the Store and corresponding Add/Modify/Delete handlers will
+// be invokved for them.
+func NewTransformingInformer(


I didn't initially add indexer option as neither of the examples of this pattern I saw was using indexes.

But in the end - adding it is just a boilerplate, so we should probably do that in one shot.
I added that in a second commit.

deads2k · 2021-09-28T14:04:19Z

/lgtm

wojtek-t · 2021-09-28T15:03:00Z

/retest

wojtek-t · 2021-09-28T16:41:15Z

/retest

wojtek-t · 2021-09-28T18:35:57Z

/retest

jberkus · 2021-10-05T00:09:28Z

Given that this is a feature, shouldn't it have a release note?

wojtek-t · 2021-10-05T08:45:28Z

Given that this is a feature, shouldn't it have a release note?

I don't agree. This isn't a feature per-se. This isn't anything that impact users and/or cluster admins.
It's something that developers may use - but it's not what release notes are used for. It's just a library.

k8s-ci-robot assigned liggitt Aug 11, 2021

wojtek-t commented Aug 11, 2021

View reviewed changes

k8s-ci-robot requested review from lavalamp and ncdc August 11, 2021 10:29

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 12, 2021

wojtek-t assigned deads2k and unassigned liggitt Aug 23, 2021

deads2k reviewed Aug 24, 2021

View reviewed changes

wojtek-t force-pushed the converting_informer branch from 1e1e3ea to c629322 Compare September 1, 2021 10:03

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 Sep 1, 2021

wojtek-t commented Sep 1, 2021

View reviewed changes

wojtek-t changed the title ~~[WIP] Create TransformingInformer~~ Create TransformingInformer Sep 1, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2021

wojtek-t force-pushed the converting_informer branch from c629322 to 47551e8 Compare September 1, 2021 11:43

wojtek-t force-pushed the converting_informer branch from 47551e8 to 022469f Compare September 1, 2021 14:02

deads2k reviewed Sep 22, 2021

View reviewed changes

wojtek-t added 2 commits September 23, 2021 08:19

Create TransformingInformer

efd3490

TransformingInfomer is like a regular Informer, but allows for applying custom transform functions on the objects received via list/watch API calls.

Create TransformingIndexerInformer

56ffb4e

wojtek-t force-pushed the converting_informer branch from 022469f to 56ffb4e Compare September 23, 2021 07:15

wojtek-t commented Sep 23, 2021

View reviewed changes

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

k8s-ci-robot merged commit 66e1d27 into kubernetes:master Sep 28, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 28, 2021

dddddai mentioned this pull request Feb 22, 2022

Support for transforming objects in shared informers kubernetes-client/java#2146

Closed

aojea mentioned this pull request May 31, 2022

Implement advanced field selectors for Service resources #110185

Closed

vincepri mentioned this pull request Dec 8, 2022

Allow to create shared index informers with custom key functions #114321

Closed

aojea mentioned this pull request Feb 16, 2023

[FR] Add support in informers to filter objects for local cache #115803

Open

liggitt mentioned this pull request Mar 15, 2023

change where transformers are called #116623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create TransformingInformer #104300

Create TransformingInformer #104300

wojtek-t commented Aug 11, 2021

wojtek-t commented Aug 11, 2021

wojtek-t Aug 11, 2021

wojtek-t Aug 23, 2021

deads2k Aug 24, 2021

wojtek-t Aug 25, 2021

deads2k Aug 26, 2021

wojtek-t Aug 26, 2021

deads2k Sep 22, 2021

k8s-ci-robot commented Aug 11, 2021

caesarxuchao commented Aug 12, 2021

deads2k Aug 24, 2021

wojtek-t Sep 1, 2021

deads2k commented Aug 24, 2021

wojtek-t commented Aug 25, 2021

wojtek-t left a comment

wojtek-t Sep 1, 2021

deads2k Sep 22, 2021

wojtek-t Sep 23, 2021

wojtek-t left a comment

wojtek-t Sep 23, 2021

deads2k commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

jberkus commented Oct 5, 2021

wojtek-t commented Oct 5, 2021

Create TransformingInformer #104300

Create TransformingInformer #104300

Conversation

wojtek-t commented Aug 11, 2021

wojtek-t commented Aug 11, 2021

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

k8s-ci-robot commented Aug 11, 2021

caesarxuchao commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Aug 24, 2021

wojtek-t commented Aug 25, 2021

wojtek-t 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

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

wojtek-t commented Sep 28, 2021

jberkus commented Oct 5, 2021

wojtek-t commented Oct 5, 2021