Skip to content

Commit 0f46012

Browse files
vinceprivincepri
committed
⚠️ Propagate context on Manager.Start(...)
This change reshuffles how the manager accepts and operates on a context. With this change, the user experience is greatly improved, users can now use `ctrl.SetupSignalHandler()` to create a context, enrich it if they want to, and pass it to `manager.Start`. In addition, this PR changes how the context and stop channel are handled internally to ensure proper cancellation. Signed-off-by: Vince Prignano <vincepri@vmware.com>
1 parent af2f5b1 commit 0f46012

22 files changed

+224
-220
lines changed

‎designs/move-cluster-specific-code-out-of-manager.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ func NewSecretMirrorReconciler(mgr manager.Manager, mirrorCluster cluster.Cluste
203203

204204
func main(){
205205

206-
mgr, err := manager.New(context.Background(), cfg1, manager.Options{})
206+
mgr, err := manager.New( cfg1, manager.Options{})
207207
if err != nil {
208208
panic(err)
209209
}

‎example_test.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import (
3838
func Example() {
3939
var log = controllers.Log.WithName("builder-examples")
4040

41-
manager, err := controllers.NewManager(context.Background(), controllers.GetConfigOrDie(), controllers.Options{})
41+
manager, err := controllers.NewManager(controllers.GetConfigOrDie(), controllers.Options{})
4242
if err != nil {
4343
log.Error(err, "could not create manager")
4444
os.Exit(1)
@@ -79,7 +79,6 @@ func Example_updateLeaderElectionDurations() {
7979
renewDeadline := 80 * time.Second
8080
retryPeriod := 20 * time.Second
8181
manager, err := controllers.NewManager(
82-
context.Background(),
8382
controllers.GetConfigOrDie(),
8483
controllers.Options{
8584
LeaseDuration: &leaseDuration,

‎examples/builtins/main.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package main
1818

1919
import (
20-
"context"
2120
"os"
2221

2322
appsv1 "k8s.io/api/apps/v1"
@@ -43,7 +42,7 @@ func main() {
4342

4443
// Setup a Manager
4544
entryLog.Info("setting up manager")
46-
mgr, err := manager.New(context.Background(), config.GetConfigOrDie(), manager.Options{})
45+
mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{})
4746
if err != nil {
4847
entryLog.Error(err, "unable to set up overall controller manager")
4948
os.Exit(1)

‎examples/crd/main.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
104104
func main() {
105105
ctrl.SetLogger(zap.New())
106106

107-
mgr, err := ctrl.NewManager(context.Background(), ctrl.GetConfigOrDie(), ctrl.Options{})
107+
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{})
108108
if err != nil {
109109
setupLog.Error(err, "unable to start manager")
110110
os.Exit(1)

‎examples/scratch-env/main.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@ package main
33
import (
44
goflag "flag"
55
"fmt"
6-
flag "github.com/spf13/pflag"
76
"io"
87
"io/ioutil"
98
"os"
109

10+
flag "github.com/spf13/pflag"
11+
1112
"k8s.io/client-go/tools/clientcmd"
1213
kcapi "k8s.io/client-go/tools/clientcmd/api"
1314
ctrl "sigs.k8s.io/controller-runtime"
@@ -102,7 +103,8 @@ func runMain() int {
102103
log.Info("Wrote kubeconfig")
103104
}
104105

105-
<-ctrl.SetupSignalHandler()
106+
ctx := ctrl.SetupSignalHandler()
107+
<-ctx.Done()
106108

107109
log.Info("Shutting down apiserver & etcd")
108110
err = env.Stop()

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,6 @@ require (
2424
k8s.io/apiextensions-apiserver v0.19.2
2525
k8s.io/apimachinery v0.19.2
2626
k8s.io/client-go v0.19.2
27-
k8s.io/utils v0.0.0-20200729134348-d5654de09c73
27+
k8s.io/utils v0.0.0-20200912215256-4140de9c8800
2828
sigs.k8s.io/yaml v1.2.0
2929
)

‎go.sum

+2
Original file line numberDiff line numberDiff line change
@@ -632,6 +632,8 @@ k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6 h1:+WnxoVtG8TMiudHBSEtrVL
632632
k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6/go.mod h1:UuqjUnNftUyPE5H64/qeyjQoUZhGpeFDVdxjTeEVN2o=
633633
k8s.io/utils v0.0.0-20200729134348-d5654de09c73 h1:uJmqzgNWG7XyClnU/mLPBWwfKKF1K8Hf8whTseBgJcg=
634634
k8s.io/utils v0.0.0-20200729134348-d5654de09c73/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
635+
k8s.io/utils v0.0.0-20200912215256-4140de9c8800 h1:9ZNvfPvVIEsp/T1ez4GQuzCcCTEQWhovSofhqR73A6g=
636+
k8s.io/utils v0.0.0-20200912215256-4140de9c8800/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
635637
rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
636638
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.9/go.mod h1:dzAXnQbTRyDlZPJX2SUPEqvnB+j7AJjtlox7PEwigU0=
637639
sigs.k8s.io/structured-merge-diff/v4 v4.0.1 h1:YXTMot5Qz/X1iBRJhAt+vI+HVttY0WkSqqhKxQ0xVbA=

‎pkg/builder/controller_test.go

+26-25
Original file line numberDiff line numberDiff line change
@@ -61,25 +61,18 @@ func (l *testLogger) WithValues(_ ...interface{}) logr.Logger {
6161
}
6262

6363
var _ = Describe("application", func() {
64-
var stop chan struct{}
65-
6664
BeforeEach(func() {
67-
stop = make(chan struct{})
6865
newController = controller.New
6966
})
7067

71-
AfterEach(func() {
72-
close(stop)
73-
})
74-
7568
noop := reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
7669
return reconcile.Result{}, nil
7770
})
7871

7972
Describe("New", func() {
8073
It("should return success if given valid objects", func() {
8174
By("creating a controller manager")
82-
m, err := manager.New(context.Background(), cfg, manager.Options{})
75+
m, err := manager.New(cfg, manager.Options{})
8376
Expect(err).NotTo(HaveOccurred())
8477

8578
instance, err := ControllerManagedBy(m).
@@ -92,7 +85,7 @@ var _ = Describe("application", func() {
9285

9386
It("should return error if given two apiType objects in For function", func() {
9487
By("creating a controller manager")
95-
m, err := manager.New(context.Background(), cfg, manager.Options{})
88+
m, err := manager.New(cfg, manager.Options{})
9689
Expect(err).NotTo(HaveOccurred())
9790

9891
instance, err := ControllerManagedBy(m).
@@ -106,7 +99,7 @@ var _ = Describe("application", func() {
10699

107100
It("should return an error if For function is not called", func() {
108101
By("creating a controller manager")
109-
m, err := manager.New(context.Background(), cfg, manager.Options{})
102+
m, err := manager.New(cfg, manager.Options{})
110103
Expect(err).NotTo(HaveOccurred())
111104

112105
instance, err := ControllerManagedBy(m).
@@ -118,7 +111,7 @@ var _ = Describe("application", func() {
118111

119112
It("should return an error if there is no GVK for an object, and thus we can't default the controller name", func() {
120113
By("creating a controller manager")
121-
m, err := manager.New(context.Background(), cfg, manager.Options{})
114+
m, err := manager.New(cfg, manager.Options{})
122115
Expect(err).NotTo(HaveOccurred())
123116

124117
By("creating a controller with a bad For type")
@@ -141,7 +134,7 @@ var _ = Describe("application", func() {
141134
}
142135

143136
By("creating a controller manager")
144-
m, err := manager.New(context.Background(), cfg, manager.Options{})
137+
m, err := manager.New(cfg, manager.Options{})
145138
Expect(err).NotTo(HaveOccurred())
146139

147140
instance, err := ControllerManagedBy(m).
@@ -164,7 +157,7 @@ var _ = Describe("application", func() {
164157
}
165158

166159
By("creating a controller manager")
167-
m, err := manager.New(context.Background(), cfg, manager.Options{})
160+
m, err := manager.New(cfg, manager.Options{})
168161
Expect(err).NotTo(HaveOccurred())
169162

170163
instance, err := ControllerManagedBy(m).
@@ -186,7 +179,7 @@ var _ = Describe("application", func() {
186179
}
187180

188181
By("creating a controller manager")
189-
m, err := manager.New(context.Background(), cfg, manager.Options{})
182+
m, err := manager.New(cfg, manager.Options{})
190183
Expect(err).NotTo(HaveOccurred())
191184

192185
instance, err := ControllerManagedBy(m).
@@ -209,7 +202,7 @@ var _ = Describe("application", func() {
209202
}
210203

211204
By("creating a controller manager")
212-
m, err := manager.New(context.Background(), cfg, manager.Options{})
205+
m, err := manager.New(cfg, manager.Options{})
213206
Expect(err).NotTo(HaveOccurred())
214207

215208
instance, err := ControllerManagedBy(m).
@@ -230,7 +223,7 @@ var _ = Describe("application", func() {
230223
}
231224

232225
By("creating a controller manager")
233-
m, err := manager.New(context.Background(), cfg, manager.Options{})
226+
m, err := manager.New(cfg, manager.Options{})
234227
Expect(err).NotTo(HaveOccurred())
235228

236229
instance, err := ControllerManagedBy(m).
@@ -244,7 +237,7 @@ var _ = Describe("application", func() {
244237

245238
It("should allow multiple controllers for the same kind", func() {
246239
By("creating a controller manager")
247-
m, err := manager.New(context.Background(), cfg, manager.Options{})
240+
m, err := manager.New(cfg, manager.Options{})
248241
Expect(err).NotTo(HaveOccurred())
249242

250243
By("registering the type in the Scheme")
@@ -273,33 +266,39 @@ var _ = Describe("application", func() {
273266

274267
Describe("Start with ControllerManagedBy", func() {
275268
It("should Reconcile Owns objects", func(done Done) {
276-
m, err := manager.New(context.Background(), cfg, manager.Options{})
269+
m, err := manager.New(cfg, manager.Options{})
277270
Expect(err).NotTo(HaveOccurred())
278271

279272
bldr := ControllerManagedBy(m).
280273
For(&appsv1.Deployment{}).
281274
Owns(&appsv1.ReplicaSet{})
282-
doReconcileTest("3", stop, bldr, m, false)
275+
276+
ctx, cancel := context.WithCancel(context.Background())
277+
defer cancel()
278+
doReconcileTest(ctx, "3", bldr, m, false)
283279
close(done)
284280
}, 10)
285281

286282
It("should Reconcile Watches objects", func(done Done) {
287-
m, err := manager.New(context.Background(), cfg, manager.Options{})
283+
m, err := manager.New(cfg, manager.Options{})
288284
Expect(err).NotTo(HaveOccurred())
289285

290286
bldr := ControllerManagedBy(m).
291287
For(&appsv1.Deployment{}).
292288
Watches( // Equivalent of Owns
293289
&source.Kind{Type: &appsv1.ReplicaSet{}},
294290
&handler.EnqueueRequestForOwner{OwnerType: &appsv1.Deployment{}, IsController: true})
295-
doReconcileTest("4", stop, bldr, m, true)
291+
292+
ctx, cancel := context.WithCancel(context.Background())
293+
defer cancel()
294+
doReconcileTest(ctx, "4", bldr, m, true)
296295
close(done)
297296
}, 10)
298297
})
299298

300299
Describe("Set custom predicates", func() {
301300
It("should execute registered predicates only for assigned kind", func(done Done) {
302-
m, err := manager.New(context.Background(), cfg, manager.Options{})
301+
m, err := manager.New(cfg, manager.Options{})
303302
Expect(err).NotTo(HaveOccurred())
304303

305304
var (
@@ -347,7 +346,9 @@ var _ = Describe("application", func() {
347346
Owns(&appsv1.ReplicaSet{}, WithPredicates(replicaSetPrct)).
348347
WithEventFilter(allPrct)
349348

350-
doReconcileTest("5", stop, bldr, m, true)
349+
ctx, cancel := context.WithCancel(context.Background())
350+
defer cancel()
351+
doReconcileTest(ctx, "5", bldr, m, true)
351352

352353
Expect(deployPrctExecuted).To(BeTrue(), "Deploy predicated should be called at least once")
353354
Expect(replicaSetPrctExecuted).To(BeTrue(), "ReplicaSet predicated should be called at least once")
@@ -359,7 +360,7 @@ var _ = Describe("application", func() {
359360

360361
})
361362

362-
func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr manager.Manager, complete bool) {
363+
func doReconcileTest(ctx context.Context, nameSuffix string, blder *Builder, mgr manager.Manager, complete bool) {
363364
deployName := "deploy-name-" + nameSuffix
364365
rsName := "rs-name-" + nameSuffix
365366

@@ -389,7 +390,7 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr
389390
By("Starting the application")
390391
go func() {
391392
defer GinkgoRecover()
392-
Expect(mgr.Start(stop)).NotTo(HaveOccurred())
393+
Expect(mgr.Start(ctx)).NotTo(HaveOccurred())
393394
By("Stopping the application")
394395
}()
395396

‎pkg/builder/example_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func ExampleBuilder() {
4545

4646
var log = logf.Log.WithName("builder-examples")
4747

48-
mgr, err := manager.New(context.Background(), config.GetConfigOrDie(), manager.Options{})
48+
mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{})
4949
if err != nil {
5050
log.Error(err, "could not create manager")
5151
os.Exit(1)

‎pkg/builder/example_webhook_test.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package builder_test
1818

1919
import (
20-
"context"
2120
"os"
2221

2322
"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -40,7 +39,7 @@ var _ admission.Validator = &examplegroup.ChaosPod{}
4039
func ExampleWebhookBuilder() {
4140
var log = logf.Log.WithName("webhookbuilder-example")
4241

43-
mgr, err := manager.New(context.Background(), config.GetConfigOrDie(), manager.Options{})
42+
mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{})
4443
if err != nil {
4544
log.Error(err, "could not create manager")
4645
os.Exit(1)

‎pkg/builder/webhook_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ var _ = Describe("webhook", func() {
5151
Describe("New", func() {
5252
It("should scaffold a defaulting webhook if the type implements the Defaulter interface", func() {
5353
By("creating a controller manager")
54-
m, err := manager.New(context.Background(), cfg, manager.Options{})
54+
m, err := manager.New(cfg, manager.Options{})
5555
Expect(err).NotTo(HaveOccurred())
5656

5757
By("registering the type in the Scheme")
@@ -123,7 +123,7 @@ var _ = Describe("webhook", func() {
123123

124124
It("should scaffold a validating webhook if the type implements the Validator interface", func() {
125125
By("creating a controller manager")
126-
m, err := manager.New(context.Background(), cfg, manager.Options{})
126+
m, err := manager.New(cfg, manager.Options{})
127127
Expect(err).NotTo(HaveOccurred())
128128

129129
By("registering the type in the Scheme")
@@ -196,7 +196,7 @@ var _ = Describe("webhook", func() {
196196

197197
It("should scaffold defaulting and validating webhooks if the type implements both Defaulter and Validator interfaces", func() {
198198
By("creating a controller manager")
199-
m, err := manager.New(context.Background(), cfg, manager.Options{})
199+
m, err := manager.New(cfg, manager.Options{})
200200
Expect(err).NotTo(HaveOccurred())
201201

202202
By("registering the type in the Scheme")
@@ -273,7 +273,7 @@ var _ = Describe("webhook", func() {
273273
By("creating a controller manager")
274274
ctx, cancel := context.WithCancel(context.Background())
275275

276-
m, err := manager.New(ctx, cfg, manager.Options{})
276+
m, err := manager.New(cfg, manager.Options{})
277277
Expect(err).NotTo(HaveOccurred())
278278

279279
By("registering the type in the Scheme")

‎pkg/controller/controller_integration_test.go

+4-8
Original file line numberDiff line numberDiff line change
@@ -38,25 +38,19 @@ import (
3838

3939
var _ = Describe("controller", func() {
4040
var reconciled chan reconcile.Request
41-
var stop chan struct{}
4241
ctx := context.Background()
4342

4443
BeforeEach(func() {
45-
stop = make(chan struct{})
4644
reconciled = make(chan reconcile.Request)
4745
Expect(cfg).NotTo(BeNil())
4846
})
4947

50-
AfterEach(func() {
51-
close(stop)
52-
})
53-
5448
Describe("controller", func() {
5549
// TODO(directxman12): write a whole suite of controller-client interaction tests
5650

5751
It("should reconcile", func(done Done) {
5852
By("Creating the Manager")
59-
cm, err := manager.New(ctx, cfg, manager.Options{})
53+
cm, err := manager.New(cfg, manager.Options{})
6054
Expect(err).NotTo(HaveOccurred())
6155

6256
By("Creating the Controller")
@@ -84,9 +78,11 @@ var _ = Describe("controller", func() {
8478
Expect(err).To(Equal(&cache.ErrCacheNotStarted{}))
8579

8680
By("Starting the Manager")
81+
ctx, cancel := context.WithCancel(context.Background())
82+
defer cancel()
8783
go func() {
8884
defer GinkgoRecover()
89-
Expect(cm.Start(stop)).NotTo(HaveOccurred())
85+
Expect(cm.Start(ctx)).NotTo(HaveOccurred())
9086
}()
9187

9288
deployment := &appsv1.Deployment{

0 commit comments

Comments
 (0)