Introduce k8s informer factory #2867

yue9944882 · 2018-02-16T08:20:15Z

What does this PR do?

Use k8s.io/cilent-go/informers to manage informers and its indexers / store.

Motivation

Currently we are managing the stores of informers explicitly which may be unnecessary after bumping up client-go. Informer factory will help sort out and clearify implementation of kubernetes client.

This PR will simplify kubernetes client a lot.

More

Added/updated tests
Added/updated documentation

Additional Notes

ldez · 2018-02-16T08:27:02Z

@yue9944882 like the last time, could you organize your commit to facilitate the review (please respect the order).

one commit for Gopkg.*
one commit for Træfik code.
one commit for the vendor.

Could you use make dep-ensure.

Thanks.

ldez · 2018-02-16T09:02:54Z

Github order by commit date: as you have alter your commit (rebase -i), the commits dates have not changed.

yue9944882 · 2018-02-16T10:06:20Z

provider/kubernetes/client.go

 	return result
 }

 // GetService returns the named service from the given namespace.
 func (c *clientImpl) GetService(namespace, name string) (*corev1.Service, bool, error) {
 	var service *corev1.Service
-	item, exists, err := c.svcStores[c.lookupNamespace(namespace)].GetByKey(namespace + "/" + name)
+	item, exists, err := c.factories[namespace].Core().V1().Services().Informer().GetStore().GetByKey(namespace + "/" + name)


Ideally this line could be svc, err := c.factories[namespace].Core().V1().Services().Lister().Services(namespace).Get(name) which use Lister to Get resources from indexer inside the informer. I tries but this will need to change the return types of GetService(namespace, name string) (*corev1.Service, error) in the Client interface and lots of changes in test facilities will change too. Maybe it can be done in another PR later to make this one clearer.

Note that they are actually the same. But eliminating namespace + "/" + name would be helpful for maintaince.

yue9944882 · 2018-02-16T10:16:13Z

@ldez @timoreimann PTAL thx 😉

ldez · 2018-02-16T11:47:46Z

@yue9944882 please stop ask PTAL, we already have notifications of commits, comments, ...

yue9944882 · 2018-02-16T12:22:00Z

@ldez Oops. Sorry for PTAL🤭I just mean that it is ready for a review.

timoreimann

The code panics if you don't specify a namespace to watch on:

./dist/traefik --defaultentrypoints=http --entryPoints='Name:http Address::8000' --web --web.readonly --web.address=":8080" --logLevel=INFO --checknewversion=false --kubernetes --kubernetes.endpoint=http://127.0.0.1:8001 --kubernetes.watch=true                                
WARN[2018-02-16T23:36:17+01:00] web provider configuration is deprecated, you should use these options : api, rest provider, ping and metrics 
INFO[2018-02-16T23:36:17+01:00] Traefik version dev built on I don't remember exactly 
INFO[2018-02-16T23:36:17+01:00] 
Stats collection is disabled.
Help us improve Traefik by turning this feature on :)
More details on: https://docs.traefik.io/basics/#collected-data
 
INFO[2018-02-16T23:36:17+01:00] Preparing server http &{Address::8000 TLS:<nil> Redirect:<nil> Auth:<nil> WhitelistSourceRange:[] Compress:false ProxyProtocol:<nil> ForwardedHeaders:0xc4205f1680} with readTimeout=0s writeTimeout=0s idleTimeout=3m0s 
INFO[2018-02-16T23:36:17+01:00] Preparing server traefik &{Address::8080 TLS:<nil> Redirect:<nil> Auth:<nil> WhitelistSourceRange:[] Compress:false ProxyProtocol:<nil> ForwardedHeaders:0xc4205f1880} with readTimeout=0s writeTimeout=0s idleTimeout=3m0s 
INFO[2018-02-16T23:36:17+01:00] Starting server on :8000                     
INFO[2018-02-16T23:36:17+01:00] Starting provider *kubernetes.Provider {"Watch":true,"Filename":"","Constraints":[],"Trace":false,"DebugLogGeneratedTemplate":false,"Endpoint":"http://127.0.0.1:8001","Token":"","CertAuthFilePath":"","DisablePassHostHeaders":false,"EnablePassTLSCert":false,"Namespaces":null,"LabelSelector":"","IngressClass":""} 
INFO[2018-02-16T23:36:17+01:00] Starting server on :8080                     
INFO[2018-02-16T23:36:17+01:00] Creating cluster-external Provider client with endpoint http://127.0.0.1:8001 
ERRO[2018-02-16T23:36:17+01:00] Error in Go routine: runtime error: invalid memory address or nil pointer dereference 
goroutine 66 [running]:
runtime/debug.Stack(0x1363b1f, 0xc42008e2d0, 0x2d5c1b3)
	/usr/local/Cellar/go/1.9.4/libexec/src/runtime/debug/stack.go:24 +0xa7
runtime/debug.PrintStack()
	/usr/local/Cellar/go/1.9.4/libexec/src/runtime/debug/stack.go:16 +0x22
github.com/containous/traefik/safe.defaultRecoverGoroutine(0x29ca8e0, 0x3fa68f0)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:148 +0x7d
github.com/containous/traefik/safe.OperationWithRecover.func1.1(0xc4207c9df8)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:156 +0x56
panic(0x29ca8e0, 0x3fa68f0)
	/usr/local/Cellar/go/1.9.4/libexec/src/runtime/panic.go:491 +0x283
github.com/containous/traefik/provider/kubernetes.(*clientImpl).GetService(0xc42051f400, 0xc4206cd3f0, 0xa, 0xc420308e00, 0x1f, 0x50, 0x0, 0x0, 0xc420681c18)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/provider/kubernetes/client.go:183 +0x5f
github.com/containous/traefik/provider/kubernetes.(*Provider).loadIngresses(0xc42039dad0, 0x3d73ea0, 0xc42051f400, 0x1, 0x1, 0x0)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/provider/kubernetes/kubernetes.go:235 +0x102f
github.com/containous/traefik/provider/kubernetes.(*Provider).Provide.func1.1(0x0, 0x0)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/provider/kubernetes/kubernetes.go:115 +0x50c
github.com/containous/traefik/safe.OperationWithRecover.func1(0x0, 0x0)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:160 +0x6c
github.com/containous/traefik/vendor/github.com/cenk/backoff.RetryNotify(0xc42051f410, 0x3d55860, 0xc42051f420, 0x2df4fa0, 0xc420066080, 0xc4201556c0)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/vendor/github.com/cenk/backoff/retry.go:37 +0x88
github.com/containous/traefik/provider/kubernetes.(*Provider).Provide.func1(0xc420292150)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/provider/kubernetes/kubernetes.go:136 +0x162
github.com/containous/traefik/safe.(*Pool).Go.func1()
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:78 +0x3a
github.com/containous/traefik/safe.GoWithRecover.func1(0x2df5140, 0xc420559230)
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:142 +0x4d
created by github.com/containous/traefik/safe.GoWithRecover
	/Users/treimann/Development/traefik/src/github.com/containous/traefik/safe/routine.go:136 +0x49
ERRO[2018-02-16T23:36:17+01:00] Provider connection error: Panic in operation: %!s(<nil>); retrying in 726.896979ms

It fails on client.go:183. I suppose it's due to the removal of the lookupNamespace method, causing the lookup at c.factories[namespace] to yield nil.

(Sorry, we are still missing proper integration tests for the Kubernetes provider.)

yue9944882 · 2018-02-17T10:38:34Z

@timoreimann Good catch, no doubt this would be a horrible BUG 😨

timoreimann · 2018-02-17T20:45:05Z

provider/kubernetes/client.go

+
+		for t, ok := range factory.WaitForCacheSync(stopCh) {
+			if !ok {
+				return nil, fmt.Errorf("timed out waiting for controller caches to sync %s in namespace %s", t.String(), ns)


What's an example output of t.String()?

@timoreimann it will be *v1.Secret which is returned by reflect.TypeOf(&corev1.Secret):
https://github.com/kubernetes/kubernetes/blob/aa02c0f5191af40cbf7a18a8beec24c56ed7b860/staging/src/k8s.io/client-go/informers/core/v1/secret.go#L84

timoreimann · 2018-02-17T20:45:33Z

provider/kubernetes/client.go

+
+		for t, ok := range factory.WaitForCacheSync(stopCh) {
+			if !ok {
+				return nil, fmt.Errorf("timed out waiting for controller caches to sync %s in namespace %s", t.String(), ns)


Let's use %q for printing ns as it may be the empty string when no namespaces are specified.

timoreimann · 2018-02-17T20:46:22Z

provider/kubernetes/client.go

-		// users having granted RBAC permissions for this object.
-		// https://github.com/containous/traefik/issues/1784 should improve the
-		// situation here in the future.
-		informManager.extend(c.WatchObjects(ns, kindSecrets, &corev1.Secret{}, c.secStores, eventCh), false)


We pass false as the last parameter so that we don't wait for the Secrets store to become synchronized for the described reason.

Am I missing something, or did we not carry over that behavior?

Was this addressed?

Yes and we skip the synchronization of secret by delaying the invocation of AddEventHandler.

timoreimann · 2018-02-17T20:48:28Z

provider/kubernetes/client.go

+		factory.Core().V1().Endpoints().Informer().AddEventHandler(eventHandler)
+		factory.Core().V1().Secrets().Informer().AddEventHandler(eventHandler)
+
+		for t, ok := range factory.WaitForCacheSync(stopCh) {


The previous implementation first started all informers and then waited for the caches to sync. The new implementation does it the other way around.

Are we certain this doesn't change the observable behavior?

timoreimann · 2018-02-17T20:51:52Z

Gopkg.lock

+    "listers/settings/v1alpha1",
+    "listers/storage/v1",
+    "listers/storage/v1alpha1",
+    "listers/storage/v1beta1",


Possibly more a question for @ldez: Is it normal that dep references packages which are not used? I thought dep plus our (custom) pruning causes us to only pull in what is really needed.

These are all supposed to be direct dependencies when compiling informerFactory.
https://github.com/kubernetes/kubernetes/blob/aa02c0f5191af40cbf7a18a8beec24c56ed7b860/staging/src/k8s.io/client-go/informers/factory.go#L42
https://github.com/kubernetes/kubernetes/blob/aa02c0f5191af40cbf7a18a8beec24c56ed7b860/staging/src/k8s.io/client-go/informers/core/v1/pod.go#L30

timoreimann · 2018-02-25T23:10:14Z

provider/kubernetes/client.go

 		safe.Go(func() {
 			wg.Add(1)
-			informer.Run(stopCh)
+			c.factories[ns].Start(stopCh)


We need to insert ns := ns after line 141 in order to capture the variable. Otherwise, we'd always end up with the last namespace being referenced in the anonymous goroutine (see also https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables).

I just realise that informerFactory.Start is an asynchronous call itself and we don't have to invoke it via safe.Go(...)

timoreimann · 2018-02-25T23:27:46Z

provider/kubernetes/client.go

+			// users having granted RBAC permissions for this object.
+			// https://github.com/containous/traefik/issues/1784 should improve the
+			// situation here in the future.
+			if t == reflect.TypeOf(&corev1.Secret{}) && !ok {


Nit-pick: Please swap the expression terms for better comprehensibility (and, to a neglectable degree, performance).

timoreimann · 2018-02-25T23:28:09Z

provider/kubernetes/client.go

+	for ns, factory := range c.factories {
+		ings, err := factory.Extensions().V1beta1().Ingresses().Lister().List(labels.Everything())
+		if err != nil {
+			log.Errorf("fail to list ingresses in namespace %s: %s", ns, err)


s/fail/Failed/

timoreimann · 2018-02-25T23:29:21Z

provider/kubernetes/client.go

+			// users having granted RBAC permissions for this object.
+			// https://github.com/containous/traefik/issues/1784 should improve the
+			// situation here in the future.
+			if t == reflect.TypeOf(&corev1.Secret{}) && !ok {


In the previous implementation, we did not wait for the Secrets' cache to sync at all. The new implementation seems to wait for it but does not consider an error to be critical.

Assuming I got it right: Does this possibly mean that users without permitted RBAC access now see Secrets RBAC errors in their logs? I guess I'm okay with a single log occurrence, but multiple ones would be irritating IMHO.

timoreimann · 2018-02-25T23:30:29Z

Gopkg.lock

+    "listers/settings/v1alpha1",
+    "listers/storage/v1",
+    "listers/storage/v1alpha1",
+    "listers/storage/v1beta1",


timoreimann · 2018-03-06T23:32:13Z

Apologies for the laggy response; I will try hard to get back to this PR and do some final testings within the next 2 days.

Sorry!

yue9944882 · 2018-03-07T05:28:49Z

@timoreimann Never mind. The refactoring in this PR might be a big change so we ought to spend more time to prove its correctness. I'd also like to help enhance test for this PR :-)

timoreimann

I finally got around testing the PR against against minikube. Everything seems to be working as intended. 🎉

Regarding the tests: IMHO, unit tests aren't very helpful for the changed files in question. What we need is a proper integration test. Fortunately, I have started work in this regard some time ago. It's (still) not done yet but I'm committed to moving it to completion. :-)

Thanks again for the PR!

mmatur

LGTM

juliens

LGTM

yue9944882 · 2018-03-23T03:47:09Z

@mmatur What does it mean by need-human-merge? Am I missing sth? (facepalm

mmatur · 2018-03-23T13:46:19Z

@yue9944882, you have missed nothing. We have bots that help us to manage PR.

Explanation available here

yue9944882 requested review from a team as code owners February 16, 2018 08:20

traefiker added area/provider/k8s/ingress size/L status/0-needs-triage labels Feb 16, 2018

ldez added status/1-needs-design-review kind/enhancement a new or improved feature. and removed status/0-needs-triage labels Feb 16, 2018

ldez changed the title ~~Refactor kubernetes client: introduce informer factory~~ Introduce k8s informer factory Feb 16, 2018

yue9944882 force-pushed the refactor-kubernetes-client-informer-mgmt branch from ba86d19 to 1c602b4 Compare February 16, 2018 08:41

ldez added status/2-needs-review and removed status/1-needs-design-review labels Feb 16, 2018

yue9944882 force-pushed the refactor-kubernetes-client-informer-mgmt branch from 1c602b4 to 8be009e Compare February 16, 2018 09:09

yue9944882 commented Feb 16, 2018

View reviewed changes

timoreimann suggested changes Feb 16, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Feb 16, 2018

yue9944882 force-pushed the refactor-kubernetes-client-informer-mgmt branch from 2ba5468 to 25dc466 Compare February 17, 2018 10:35

ldez removed the contributor/waiting-for-corrections label Feb 17, 2018

timoreimann suggested changes Feb 17, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Feb 17, 2018

ldez removed the contributor/waiting-for-corrections label Feb 23, 2018

yue9944882 force-pushed the refactor-kubernetes-client-informer-mgmt branch from 393a98c to 5f71ddd Compare February 23, 2018 19:40

timoreimann suggested changes Feb 25, 2018

View reviewed changes

ldez removed the contributor/waiting-for-corrections label Mar 5, 2018

timoreimann approved these changes Mar 13, 2018

View reviewed changes

ldez added this to the 1.6 milestone Mar 13, 2018

mmatur approved these changes Mar 21, 2018

View reviewed changes

juliens approved these changes Mar 21, 2018

View reviewed changes

juliens added status/3-needs-merge and removed status/2-needs-review labels Mar 21, 2018

traefiker added status/4-merge-in-progress bot/need-human-merge labels Mar 21, 2018

mmatur added the bot/no-merge label Mar 21, 2018

mmatur self-assigned this Mar 21, 2018

mmatur removed the status/4-merge-in-progress label Mar 21, 2018

yue9944882 and others added 8 commits March 22, 2018 09:32

generated: Gopkg.lock updates after make dep-ensure

16577cf

refactor k8s client: introduce informer factory for informer mgmt

117a003

make dep-ensure

438f248

remove unused consts

6b12cec

fix all namespace panic: revert lookupNamespace removal

47bed32

skip secret synchronization & sync informers after starting'em

62b9cdf

fix: dep lock.

1b098a0

do not wait for secret sync

aceff7b

mmatur force-pushed the refactor-kubernetes-client-informer-mgmt branch from fba5f22 to aceff7b Compare March 22, 2018 08:58

mmatur removed bot/need-human-merge bot/no-merge labels Mar 22, 2018

traefiker merged commit f94fa78 into traefik:master Mar 22, 2018

traefiker removed the status/3-needs-merge label Mar 22, 2018

timoreimann mentioned this pull request Apr 5, 2018

Traefik cannot find kubernetes secrets #3128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce k8s informer factory #2867

Introduce k8s informer factory #2867

yue9944882 commented Feb 16, 2018

ldez commented Feb 16, 2018 •

edited

ldez commented Feb 16, 2018

yue9944882 Feb 16, 2018 •

edited

yue9944882 Feb 16, 2018 •

edited

yue9944882 commented Feb 16, 2018

ldez commented Feb 16, 2018 •

edited

yue9944882 commented Feb 16, 2018 •

edited

timoreimann left a comment

yue9944882 commented Feb 17, 2018 •

edited

timoreimann Feb 17, 2018

yue9944882 Feb 20, 2018

timoreimann Feb 17, 2018

timoreimann Feb 17, 2018

timoreimann Mar 1, 2018

yue9944882 Mar 2, 2018

timoreimann Feb 17, 2018

timoreimann Feb 17, 2018 •

edited

yue9944882 Feb 20, 2018

timoreimann Feb 25, 2018

timoreimann Feb 25, 2018

yue9944882 Feb 26, 2018

timoreimann Feb 25, 2018

timoreimann Feb 25, 2018

timoreimann Feb 25, 2018

timoreimann Feb 25, 2018

timoreimann commented Mar 6, 2018

yue9944882 commented Mar 7, 2018

timoreimann left a comment

mmatur left a comment

juliens left a comment

yue9944882 commented Mar 23, 2018

mmatur commented Mar 23, 2018

Introduce k8s informer factory #2867

Introduce k8s informer factory #2867

Conversation

yue9944882 commented Feb 16, 2018

What does this PR do?

Motivation

More

Additional Notes

ldez commented Feb 16, 2018 • edited

ldez commented Feb 16, 2018

yue9944882 Feb 16, 2018 • edited

Choose a reason for hiding this comment

yue9944882 Feb 16, 2018 • edited

Choose a reason for hiding this comment

yue9944882 commented Feb 16, 2018

ldez commented Feb 16, 2018 • edited

yue9944882 commented Feb 16, 2018 • edited

timoreimann left a comment

Choose a reason for hiding this comment

yue9944882 commented Feb 17, 2018 • edited

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

timoreimann Feb 17, 2018 • edited

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

timoreimann commented Mar 6, 2018

yue9944882 commented Mar 7, 2018

timoreimann left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

yue9944882 commented Mar 23, 2018

mmatur commented Mar 23, 2018

ldez commented Feb 16, 2018 •

edited

yue9944882 Feb 16, 2018 •

edited

yue9944882 Feb 16, 2018 •

edited

ldez commented Feb 16, 2018 •

edited

yue9944882 commented Feb 16, 2018 •

edited

yue9944882 commented Feb 17, 2018 •

edited

timoreimann Feb 17, 2018 •

edited