🐛 Controller.Watch() should not store watches if already started

[vincepri]

The controller internal struct holds a list of watches
(as []watchDescription) when someone calls .Watch() to then start the
watches and informers once we're ready to call Start().

This behavior caused a memory leak in the case Watch was called after
a controller has already been started and if the source.Kind's cache was
either stopped or not available any longer. The leak was caused by the
watches internal slice holding on to all references to each watch ever
issued (and their respective caches).

Signed-off-by: Vince Prignano vincepri@vmware.com

/assign @alvaroaleman @DirectXMan12 @pwittrock

I'll backport the fix to release-0.5 and release-0.6 if we all agree that this is the fix we want. For consistency, I also nil'd the c.watches slice after Start() has been called and the controller has started.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

• OWNERS [vincepri]

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

[vincepri]

/cc @ncdc @fabriziopandini

[ncdc]

LGTM

[alvaroaleman]

@vincepri are you sure this properly solves the problem? Even when the Controller doesn't hold the Source anymore, the underlying informer will have an eventhandler attached if was ever in use, preventing it from being garbage collected - or am I missing something?

/lgtm
/hold

[ncdc]

@alvaroaleman we are dynamically creating a Cache and closing its stop channel when we're done with it, which GC's the informers.

[vincepri]

What Andy said, if the stop channel is called, the informer properly stops, although the controller holds a reference to the KindSource which has a reference to the underlying cache, which will never be garbage collected

[alvaroaleman]

okay, thanks for clarifying
/hold cancel