Add Informer examples #30

wfarr · 2016-11-04T21:21:59Z

Demonstrates a basic controller watching Pod events in a cluster.

Tested against kubernetes 1.4.5 on GKE.

attn @caesarxuchao
cc @mkobetic @ibawt @lxfontes

This change is

caesarxuchao

Sorry for the delay. Generally looks good. Made a few suggestions. Thank you, @wfarr!

caesarxuchao · 2016-11-16T19:15:02Z

examples/informer/README.md

+
+## Use Cases
+
+* Capturing resource events for logging to external systems (eg. monitor non-Normal Events and publish metrics to a time series database)


I'm not a native speaker, why do you use "non-Normal" instead of "abnormal" here?

Folks may want to report events that currently have the Warning type (eg. pod create failures) to external error aggregation services or time-series databases. "Normal" is one of the Types possible on an Event (the other being Warning right now, but the typedef stipulates more may be added later). I'll try and think of some better wording for this.

I see. How about putting quotes around "Normal"?

@wfarr are you still on it , its been a couple months old now

I'm addressing some of the remaining code-review comments in a new branch against kubernetes/kubernetes, and fixed this in kubernetes/kubernetes@9645bbf

caesarxuchao · 2016-11-16T19:23:52Z

examples/informer/README.md

+## Recommendations
+
+The design of `cache.NewInformer` lends itself towards writing simple, idempotent operations.
+It includes its own reconciliation loop which refreshes resources every resync period in addition to on-the-fly event handlers.


"refreshes resources" is a little bit vague. Every resync period, the informer just sends all the locally cached resources to the ResourceEventHandlerFuncs. So perhaps change to "loops through resources in the local cache..."?

Also, the list/watch mechanism guarantees the controller won't miss any event, so in most cases, resync is not necessary.

Is there any sort of retry behavior in the list/watch if a handler on the Informer should fail?

Is there any sort of retry behavior in the list/watch if a handler on the Informer should fail?

Yes. The informer is backed by a controller which keeps a known cache for its backing reflector, which allow the reflector to retry list/watch operations indefinitely while keeping the notifications coherent.

caesarxuchao · 2016-11-16T19:27:30Z

examples/informer/README.md

+
+> If you write a controller that for a ThirdPartyResource that manages the lifecycle of Database objects, you might use `{{.Metadata.Name}}-{{.Metadata.UID}}` as the name of the remote database instance.
+
+Some resources may require more than UID to guarantee uniqueness (for example `v1.Event`). 


I didn't get why event is special?

I called out v1.Event as during the course of writing some tools, I came across https://github.com/kubernetes/kubernetes/blob/master/docs/design/event_compression.md, which covers some interesting uniqueness keys in use for that type specifically.

UID still guarantees the uniqueness of a v1.Event, because you can only get v1.Event object for each UID.

Though one v1.Event could be a compression of a bunch of similar events that exist in the memory by a controller.

caesarxuchao · 2016-11-16T19:32:41Z

examples/informer/main.go

+		panic(err)
+	}
+
+	stopchan := make(chan struct{}, 1)


nit: consider just call it stop. stopchan looks like Hungarian notation.

caesarxuchao · 2016-11-16T19:35:14Z

examples/informer/main.go

+			DeleteFunc: delete,
+		})
+
+	// store can be used to List and Get, never to write directly


It's worth mentioning one should not modify the object acquired from the store.

caesarxuchao · 2016-11-16T19:36:03Z

examples/informer/main.go

+	fmt.Println("listing pods from store:")
+	for _, obj := range store.List() {
+		pod := obj.(*v1.Pod)
+		fmt.Printf("%#v\n", pod)


I guess this will be empty?

caesarxuchao · 2016-11-16T19:37:12Z

examples/informer/main.go

+
+func update(old, new interface{}) {
+	oldpod := old.(*v1.Pod)
+	// newpod := new.(*v1.Pod)


Why not printing the newpod as well?

nit: s/oldPod/oldpod

Suggest set the resync period to 0 in this example, otherwise you will get duplicate updates every resync period.

Tested against kubernetes 1.4.5 on GKE.

caesarxuchao · 2016-12-03T00:39:27Z

examples/informer/README.md

+
+Example:
+
+> If you write a controller that for a ThirdPartyResource that manages the lifecycle of Database objects, you might use `{{.Metadata.Name}}-{{.Metadata.UID}}` as the name of the remote database instance.


Hmm, when reading this again, I don't follow the example anymore. Do you mind elaborating it or chatting on slack (I might not be able to reply immediately)?

caesarxuchao · 2016-12-03T00:43:38Z

examples/informer/README.md

+
+> If you write a controller that for a ThirdPartyResource that manages the lifecycle of Database objects, you might use `{{.Metadata.Name}}-{{.Metadata.UID}}` as the name of the remote database instance.
+
+Some resources may require more than UID to guarantee uniqueness (for example `v1.Event`). 


UID still guarantees the uniqueness of a v1.Event, because you can only get v1.Event object for each UID.

Though one v1.Event could be a compression of a bunch of similar events that exist in the memory by a controller.

wfarr · 2016-12-03T20:24:26Z

Sorry for the delay on revisiting this. All the comments/review should be addressed at this point I think.

caesarxuchao · 2016-12-15T00:09:11Z

examples/informer/main.go

+		pod := obj.(*v1.Pod)
+
+		// This will likely be empty the first run, but may not
+		fmt.Printf("%#v\n", pod)


When will it not be empty? I guess I don't see why the example tries to list the store at this point.

I think the loop should be moved below

go controller.Run(stop)

and consider adding this:

# This way you can wait until a store finished its initial synchonization cache.WaitForCacheSync(stop, store.HasSynced)

That is very nice to know, when you want to populate secondary caches.
After that the store.List() makes more sense in my eyes.

That's a good point. This doc has more details.

Fixed in kubernetes/kubernetes@1d601f8

caesarxuchao · 2016-12-15T00:11:51Z

Thanks @wfarr. Sorry, I didn't get time reviewing it earlier.
I only two nits left. Could you squash though? I'll merge it then. Thanks again.

mkobetic · 2016-12-15T13:35:57Z

examples/informer/main.go

+	signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM)
+	s := <-signals
+	fmt.Printf("received signal %#v, exiting...\n", s)
+	close(stop)


I would highlight here that the controller will likely not get to actually react to the closed channel and get preempted by the os.Exit on the next line. Alternatively include your trick exploiting the fact that controller.Run is blocking and passing a stopped channel into the controller goroutine that is closed when Run returns.

Hey @mkobetic I think I understand the problem you're describing here, but I don't understand the trick you suggested. I'm new to Golang. Please can you provide some further explanation.

ideahitme · 2017-02-15T14:45:26Z

sorry for the intrusion but is there any plan on getting this merged ? IMO it will be useful to have these examples in the repo

ideahitme · 2017-02-16T00:58:44Z

example is no longer correct as per the latest code. clientset.CoreClient to be replaced by clientset.Core().RestClient() iirc

krmayankk · 2017-02-23T06:59:59Z

examples/informer/README.md

+## Use Cases
+
+* Capturing resource events for logging to external systems (eg. monitor non-Normal Events and publish metrics to a time series database)
+* Creating lifecycle controllers for ThirdPartyResources (eg. coordinate create/update/delete of an external datastore represented via a ThirdPartyResource type)


ah i have been looking for such examples. Thanks a ton for initiating this. Couple questions, mostly naive :-)

i am now seeing shared_informer stuff and changes related to that. Is this example already old now ? It might be helpful to at least put a comment in the doc saying how this differs from it

I have seen simple controller examples which dont use the framework. A word or too about that would be helpful as well or how this is better example

krmayankk · 2017-02-23T07:08:16Z

examples/informer/main.go

+		})
+
+	// store can be used to List and Get
+	// NEVER modify objects from the store. It's a read-only, local cache.


can you put some context of this wrt reflector, and cache. My understanding is that reflector uses the source to populate the store and then uses fifo queue for notifications, which are acted by the above add/delete/update functions , is that right ?

krmayankk · 2017-02-23T07:10:35Z

@wfarr @caesarxuchao are you guys still on it ? I will try this example to see if it works. Would love if you can revive this and get this in and some more examples like this on usage of controller framework cc @smarterclayton

wfarr · 2017-02-27T18:36:49Z

I don't really have the time to keep polishing this example up. If someone else wants to pick this up and push it across the finish line, feel free to do so.

Internally at Shopify, we're still making use of more or less the same constructs with good success using v2.0.0 of the client.

caesarxuchao · 2017-03-02T21:51:28Z

@mbohlool would you have time to polishing this example? It's almost there. Thanks.

mbohlool · 2017-03-02T21:57:33Z

@caesarxuchao Sure, assigned it to myself to follow up on it. Will send another PR with polished example.

lavalamp · 2017-03-02T22:11:28Z

I think the canonical location for these examples is currently the main repo, so that they will be continuously tested correctly. If you have time to send another PR (which is VERY much appreciated, btw), that would be the best place to send it. (in the staging/src/k8s.io/client-go/examples directory.)

…

On Thu, Mar 2, 2017 at 1:57 PM, Mehdy Bohlool ***@***.***> wrote: @caesarxuchao <https://github.com/caesarxuchao> Sure, assigned it to myself to follow up on it. Will send another PR with polished example. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglmYXa5pa1qeQrahL6Fue9LUst8anks5rhztWgaJpZM4KqAUT> .

timothysc · 2017-03-03T19:38:30Z

I'll be able to publish mine early next week, and I'll link back and once it's available.

mbohlool · 2017-03-06T22:25:38Z

@timothysc can you clarify your comment? Are you going to send a PR for this example?

timothysc · 2017-03-07T00:32:24Z

@mbohlool I can link to a repo once I open it up. Still doing some vetting atm.

mbohlool · 2017-03-07T18:28:01Z

@timothysc the PR should be send to main kubernetes repo. I am still confused about your comments but assuming you are writing re-writing this example to address comments.

RenaudWasTaken · 2017-03-08T23:48:14Z

examples/informer/main.go

+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/pkg/api"
+	"k8s.io/client-go/pkg/api/v1"
+	"k8s.io/client-go/pkg/fields"


Shouldn't it be k8s.io/apimachinery ?

Fixed in kubernetes/kubernetes@e3f6112

wallrj · 2017-04-13T13:33:21Z

In #30 (comment) @ideahitme wrote:

example is no longer correct as per the latest code. clientset.CoreClient to be replaced by clientset.Core().RestClient() iirc

Fixed in kubernetes/kubernetes@c89391b

wallrj · 2017-04-13T14:34:45Z

In #30 (comment) @lavalamp wrote:

I think the canonical location for these examples is currently the main repo...

I've opened a new PR kubernetes/kubernetes#44446 and attempted to address the remaining code review comments.

…ess remaining code review comments.

mbohlool · 2017-04-13T20:53:01Z

closing this in favor of kubernetes/kubernetes#44446

…ess remaining code review comments.

caesarxuchao reviewed Nov 16, 2016

View reviewed changes

Add Informer examples

8f36315

Tested against kubernetes 1.4.5 on GKE.

caesarxuchao reviewed Dec 3, 2016

View reviewed changes

Remove inaccurate bits from README, cleanup code

f5446e2

wfarr force-pushed the informer-example branch from 284b234 to f5446e2 Compare December 3, 2016 20:22

Add a note about store

ba0f238

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2016

wfarr added 4 commits December 3, 2016 15:31

fix stopchan name

0010196

Move away from 1.5 paths

d950115

remove unnecessary loop

c236ce6

syntax

577d190

caesarxuchao reviewed Dec 15, 2016

View reviewed changes

mkobetic reviewed Dec 15, 2016

View reviewed changes

caesarxuchao mentioned this pull request Feb 22, 2017

Improve the coverage of examples #128

Closed

11 tasks

krmayankk reviewed Feb 23, 2017

View reviewed changes

mbohlool self-assigned this Mar 2, 2017

RenaudWasTaken reviewed Mar 8, 2017

View reviewed changes

wallrj mentioned this pull request Apr 13, 2017

Add an Informer example in client-go kubernetes/kubernetes#44446

Closed

wallrj added a commit to wallrj/kubernetes that referenced this pull request Apr 13, 2017

Copy informer example from @wfarr 's kubernetes/client-go#30 and addr…

32faf96

…ess remaining code review comments.

mbohlool closed this Apr 13, 2017

wallrj added a commit to wallrj/kubernetes that referenced this pull request Apr 18, 2017

Copy informer example from @wfarr 's kubernetes/client-go#30 and addr…

3a53ff8

…ess remaining code review comments.


		## Use Cases

		* Capturing resource events for logging to external systems (eg. monitor non-Normal Events and publish metrics to a time series database)


		> If you write a controller that for a ThirdPartyResource that manages the lifecycle of Database objects, you might use `{{.Metadata.Name}}-{{.Metadata.UID}}` as the name of the remote database instance.

		Some resources may require more than UID to guarantee uniqueness (for example `v1.Event`).


		Example:

		> If you write a controller that for a ThirdPartyResource that manages the lifecycle of Database objects, you might use `{{.Metadata.Name}}-{{.Metadata.UID}}` as the name of the remote database instance.

Add Informer examples #30

Add Informer examples #30

Conversation

wfarr commented Nov 4, 2016 • edited by thockin Loading

caesarxuchao 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfarr commented Dec 3, 2016

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

caesarxuchao commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ideahitme commented Feb 15, 2017 • edited Loading

ideahitme commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmayankk commented Feb 23, 2017

wfarr commented Feb 27, 2017

caesarxuchao commented Mar 2, 2017

mbohlool commented Mar 2, 2017

lavalamp commented Mar 2, 2017 via email

timothysc commented Mar 3, 2017

mbohlool commented Mar 6, 2017

timothysc commented Mar 7, 2017

mbohlool commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallrj commented Apr 13, 2017

wallrj commented Apr 13, 2017

mbohlool commented Apr 13, 2017

wfarr commented Nov 4, 2016 •

edited by thockin

Loading

ideahitme commented Feb 15, 2017 •

edited

Loading

ideahitme commented Feb 16, 2017 •

edited

Loading