Only listen to configured k8s namespaces.

[timoreimann]

Watch configured namespaces only.

We previously watched all namespaces, which was causing issues for users that want to apply restrictive RBAC rules. Hence, we change the logic to hook up one informer per namespace and kind we observe.

Note that this moves the namespace filtering into the client/informers (where it properly belongs). A nice side effect of this is that we stop receiving tons of events from unrelated namespaces (most notably kube-system), which has been a source of confusion for users in the past. We also don't need the extra event notifications as "retry signals" in case of ephemeral API failures anymore since we can now safely rely on a fairly low sync period for recovery purposes. This is preferable to getting hammered with event updates at sub-second intervals, only to have Traefik's back-off sleep of 2 seconds kicking in very often.

A few additional improvements that made it in:

• Wait for the store synchronization to happen during WatchAll instead of when we process events. The wait needs to happen exactly once.
• Do not close the event channel until all controllers have stopped processing. This was a pending race condition that could yield to crash.
• Remove one superfluous event/watch channel (name changes depending on which method you're looking at).
• Remove the namespace-related unit tests. (Must be replaced by to-be-written integration tests.)
• Group methods logically.
• Improve GoDocs.

Finally, we stop logging complete events as we receive them to reduce noise and stop leaking sensitive secrets data into the logs.

Fixes #1626.

[timoreimann]

This is still WIP, mostly because a proper integration test is yet missing (#1889). Nevertheless, I'd be happy to hear some early feedback (from everyone in general and in particular from @containous/kubernetes).

[timoreimann]

It looks like getting Kubernetes integration tests up and running will still require a bit of effort. Nevertheless, merging the PR is advisable IMO as Traefik can be operated in a much more secure way on Kubernetes with namespace-specific watches. Plus, we stop spamming the logs with tons of unrelated events.

I'll remove the WIP label for now and suggest we test this manually like we did with the previous changes.

[timoreimann]

Note to myself: The PR currently sets the default namespace by default. Depending on how #1961 resolves, I may need to change that to all namespaces.

[emilevauge]

Wow @timoreimann!
Design LGTM
As you completely refactored this provider, I would strongly recommend to make some benchmarks and profiling to ensure no memory leaks has been introduced :)

[errm]

@timoreimann this is on my radar and I will have a through review done by early next week..

RE the all namespaces thing, my setup currently depends on on this so we should certainly keep it as the default IMO

[timoreimann]

@errm I have pushed a commit that makes NamespaceAll the default (which should be in line with the now-merged #1961).

[errm]

Looks very nice and clean to me...

Subject to @emilevauge 's concerns about benchmarking / soak testing LGTM

[timoreimann]

Yeah I'll definitely try to work out some kind of benchmark.

Probably not going to happen before the 1.4 freeze, but not critical either.

[dtomcej]

This looks super clean and boss.

LGTM

[aledbf]

@timoreimann this is too low, it should be like each 10 minutes. Just in case with this you are forcing a full sync (from api server) each 10 seconds.

[timoreimann]

AFAIU, there will be syncs only for the restricted items each informer is watching. Or am I missing something?

I'm okay with increasing the period in general. Syncing every 10 minutes though sounds like we'd not be catching up on missed Ingresses, Services, and Endpoints for a fairly long time. I wonder if that would still be acceptable?

[aledbf]

full resync != delta updates (like Ingresses, Services, and Endpoints )

Edit: please check the gce ingress controller
https://github.com/kubernetes/kubernetes/blob/master/cluster/saltbase/salt/l7-gcp/glbc.manifest#L47

[timoreimann]

AFAIK, the glbc watches all namespaces. With this PR, Traefik will be watching selective namespaces only. What's being pulled in by a full sync should be subject to the list part of the ListWatch interface. So to my understanding, the sync volume should be much more restricted.

Please let me know if my assumptions are wrong somehow.

[aledbf]

So to my understanding, the sync volume should be much more restricted.

Yes, that's correct but you are assuming the namespace only contains a few objects (ingress,services,secrets, etc). What happens if the user have hundred of objects in the namespace/s?

[timoreimann]

I think it's a trade-off between not overloading the cluster and being able to compensate for ephemeral errors in time. Maybe there's no one size fits all, and it'd be best to make the parameter configurable.

I agree though that we can probably bump the interval somewhat for now. Will go with a minute or so.

[timoreimann]

@aledbf you might be interested in this thread I started on the kubernetes-sig-api-machinery mailing list. Some take aways for me:

• The resync period only causes the local cache content to get replayed; in particular, it does not hit the API server (that's what an explicit relist is for).
• First and foremost, the resync period is a means to compensate for controller (not watch) bugs, though re-queuing techniques available in more recent informer versions take away a lot of that rationale. That said, resyncs may also be used for other cases. (Daniel describes how you can use them as a clock to initiate polls for external APIs periodically.)
• It's a viable option to disable resyncs completely in order to surface controller bugs more quickly instead of risking to hide them. One example component that does this is the Kubernetes scheduler.

Hope this helps.

[timoreimann]

I finally came around running benchmarks. This is what I did:

1. Submitted 30 deployments into 3 namespaces with 10 deployments per namespace.
2. Added Ingresses and Services for each Deployment.
3. Started Traefik with --debug.
4. Fired off a script that continuously rescaled all Deployments to a replica count between 1 and 3.
5. Observe the memory consumption and goroutine count via expvarmon.

Here's what I observed:

• The number of goroutines varied in a low range between ~110 and ~120. This is higher than the pre-PR count where I could measure ~45. However, this makes total sense given that we now run 4 informers per namespace versus 4 informers overall. The fact that watching a single namespace with the new implementation consumes about ~45 goroutines as well matches up nicely with the expected math.
• The memory consumption varied between 5 and 10 MB. It would rise and fall even during my load tests, which shows that GC manages to reclaim memory successfully.
• After stopping the rescaling script, memory consumption dropped as well after a short while. This shows we're not leaking memory.

I also verified that the number of backend servers matches the pod count.

Overall, this looks good to me. We surely want to gain more real world experience to solidify the new implementation. I do think though that the tests demonstrate basic correctness.

Happy to run more tests if desired.

[timoreimann]

@dtomcej @errm since you first reviewed, I pushed a few more commits (mostly just refactorings). Are you still okay with the status quo of this PR?

Asking bot not to merge until I get another approval confirmation.

[timoreimann]

Shortly going back to WIP as I realized RBAC documentation updates are yet missing...

[timoreimann]

...aaaaand it's done.

[timoreimann]

To all involved reviewers: Please indicate until Monday midnight CEST (i.e., in a good 22 hours from now) if your existing review should not hold anymore -- I'm happy to address any further comments.

Otherwise, I'll proceed with merging the PR.

[emilevauge]

@timoreimann I think this is a good example that WIP can be confusing some times :) Do we need to dismiss all reviews done before you removed the WIP label?

[timoreimann]

@emilevauge strictly speaking, I guess every new commit should invalidate all existing LGTMs.

Practically speaking, I'm not sure if we want to be that strict, especially since it'd take another three approvals.

For the PR at hand, the changes that happened after the LGTMs were received are rather simple / insignificant. So personally I feel confident moving forward even without another round of eye balls.

Either way, we may want to consider tuning the process to add clarity.

[emilevauge]

@timoreimann

strictly speaking, I guess every new commit should invalidate all existing LGTMs.

Yes, but here I'm talking about the fact that this PR was already reviewed by 3 maintainers, and still in WIP. Having commits following a review is normal, and easy to follow in Github (especially if the contributor produces well named commits). In this case, I'm not able to say if it's safe to merge or not.

Just saying we should really be cautious with WIP PRs in the future ;)