Skip to content

Commit 9dc7370

Browse files
authored
Merge pull request #1164 from vincepri/prepare-0511
[0.5] 🐛 Controller.Watch() should not store watches if already started
2 parents d7b2d62 + bd89cd4 commit 9dc7370

File tree

2 files changed

+20
-11
lines changed

2 files changed

+20
-11
lines changed

pkg/internal/controller/controller.go

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ type Controller struct {
8484

8585
// TODO(community): Consider initializing a logger with the Controller Name as the tag
8686

87-
// watches maintains a list of sources, handlers, and predicates to start when the controller is started.
88-
watches []watchDescription
87+
// startWatches maintains a list of sources, handlers, and predicates to start when the controller is started.
88+
startWatches []watchDescription
8989

9090
// Log is used to log messages to users during reconciliation, or for example when a watch is started.
9191
Log logr.Logger
@@ -121,13 +121,16 @@ func (c *Controller) Watch(src source.Source, evthdler handler.EventHandler, prc
121121
}
122122
}
123123

124-
c.watches = append(c.watches, watchDescription{src: src, handler: evthdler, predicates: prct})
125-
if c.Started {
126-
c.Log.Info("Starting EventSource", "source", src)
127-
return src.Start(evthdler, c.Queue, prct...)
124+
// Controller hasn't started yet, store the watches locally and return.
125+
//
126+
// These watches are going to be held on the controller struct until the manager or user calls Start(...).
127+
if !c.Started {
128+
c.startWatches = append(c.startWatches, watchDescription{src: src, handler: evthdler, predicates: prct})
129+
return nil
128130
}
129131

130-
return nil
132+
c.Log.Info("Starting EventSource", "source", src)
133+
return src.Start(evthdler, c.Queue, prct...)
131134
}
132135

133136
// Start implements controller.Controller
@@ -148,7 +151,7 @@ func (c *Controller) Start(stop <-chan struct{}) error {
148151
// NB(directxman12): launch the sources *before* trying to wait for the
149152
// caches to sync so that they have a chance to register their intendeded
150153
// caches.
151-
for _, watch := range c.watches {
154+
for _, watch := range c.startWatches {
152155
c.Log.Info("Starting EventSource", "source", watch.src)
153156
if err := watch.src.Start(watch.handler, c.Queue, watch.predicates...); err != nil {
154157
return err
@@ -158,7 +161,7 @@ func (c *Controller) Start(stop <-chan struct{}) error {
158161
// Start the SharedIndexInformer factories to begin populating the SharedIndexInformer caches
159162
c.Log.Info("Starting Controller")
160163

161-
for _, watch := range c.watches {
164+
for _, watch := range c.startWatches {
162165
syncingSource, ok := watch.src.(source.SyncingSource)
163166
if !ok {
164167
continue
@@ -172,6 +175,12 @@ func (c *Controller) Start(stop <-chan struct{}) error {
172175
}
173176
}
174177

178+
// All the watches have been started, we can reset the local slice.
179+
//
180+
// We should never hold watches more than necessary, each watch source can hold a backing cache,
181+
// which won't be garbage collected if we hold a reference to it.
182+
c.startWatches = nil
183+
175184
if c.JitterPeriod == 0 {
176185
c.JitterPeriod = 1 * time.Second
177186
}

pkg/internal/controller/controller_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ var _ = Describe("controller", func() {
9090
Describe("Start", func() {
9191
It("should return an error if there is an error waiting for the informers", func(done Done) {
9292
f := false
93-
ctrl.watches = []watchDescription{{
93+
ctrl.startWatches = []watchDescription{{
9494
src: source.NewKindWithCache(&corev1.Pod{}, &informertest.FakeInformers{Synced: &f}),
9595
}}
9696
ctrl.Name = "foo"
@@ -114,7 +114,7 @@ var _ = Describe("controller", func() {
114114
Expect(err).NotTo(HaveOccurred())
115115
_, err = c.GetInformer(&appsv1.ReplicaSet{})
116116
Expect(err).NotTo(HaveOccurred())
117-
ctrl.watches = []watchDescription{{
117+
ctrl.startWatches = []watchDescription{{
118118
src: source.NewKindWithCache(&appsv1.Deployment{}, &informertest.FakeInformers{}),
119119
}}
120120

0 commit comments

Comments
 (0)