From d8a2a739d6b359a325ee6bb1dcbee13ecfee8dcc Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Thu, 6 Jun 2019 18:17:41 -0700 Subject: [PATCH] Eliminate controller registration in favor of passing a list to the shared main. (#447) --- injection/README.md | 25 +++++++---------- injection/controllers.go | 43 ---------------------------- injection/controllers_test.go | 53 ----------------------------------- injection/doc.go | 27 ++++++++++-------- injection/interface.go | 18 ++++-------- injection/sharedmain/main.go | 25 +++++++---------- 6 files changed, 41 insertions(+), 150 deletions(-) delete mode 100644 injection/controllers.go delete mode 100644 injection/controllers_test.go diff --git a/injection/README.md b/injection/README.md index 4e954111a..c6f956be1 100644 --- a/injection/README.md +++ b/injection/README.md @@ -3,7 +3,7 @@ This library supports the production of controller processes with minimal boilerplate outside of the reconciler implementation. -## Registering Controllers +## Building Controllers To adopt this model of controller construction, implementations should start with the following controller constructor: @@ -14,7 +14,6 @@ import ( "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" - "github.com/knative/pkg/injection" "github.com/knative/pkg/logging" ) @@ -32,11 +31,6 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl return impl } - -// Register our controller process. -func init() { - injection.Default.RegisterController(NewController) -} ``` ## Consuming Informers @@ -152,26 +146,27 @@ func TestFoo(t *testing.T) { ## Starting controllers -By registering our controller with `injection.Default` via `init()` above we -enable our shared main method to bootstrap the entire container process. All we -do is link the controller packages containing the `init()` registering them and -this transitively links in all of the things it needs. Then our shared main -method sets it all up and runs our controllers. +All we do is import the controller packages and pass their constructors along +with a component name to our shared main. Then our shared main method sets it +all up and runs our controllers. ```go package main import ( // The set of controllers this process will run. - _ "github.com/knative/foo/pkg/reconciler/bar" - _ "github.com/knative/baz/pkg/reconciler/blah" + "github.com/knative/foo/pkg/reconciler/bar" + "github.com/knative/baz/pkg/reconciler/blah" // This defines the shared main for injected controllers. "github.com/knative/pkg/injection/sharedmain" ) func main() { - sharedmain.Main() + sharedmain.Main("component-name", + bar.NewController, + blah.NewController, + ) } ``` diff --git a/injection/controllers.go b/injection/controllers.go deleted file mode 100644 index 2b8bd62d7..000000000 --- a/injection/controllers.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -Copyright 2019 The Knative Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package injection - -import ( - "context" - - "github.com/knative/pkg/configmap" - "github.com/knative/pkg/controller" -) - -// ControllerInjector holds the type of a callback that attaches a particular -// controller type to a context. -type ControllerInjector func(context.Context, configmap.Watcher) *controller.Impl - -func (i *impl) RegisterController(ii ControllerInjector) { - i.m.Lock() - defer i.m.Unlock() - - i.controllers = append(i.controllers, ii) -} - -func (i *impl) GetControllers() []ControllerInjector { - i.m.RLock() - defer i.m.RUnlock() - - // Copy the slice before returning. - return append(i.controllers[:0:0], i.controllers...) -} diff --git a/injection/controllers_test.go b/injection/controllers_test.go deleted file mode 100644 index f7e252ea2..000000000 --- a/injection/controllers_test.go +++ /dev/null @@ -1,53 +0,0 @@ -/* -Copyright 2019 The Knative Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package injection - -import ( - "context" - "testing" - - "github.com/knative/pkg/configmap" - "github.com/knative/pkg/controller" -) - -func injectFooController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { - return nil -} - -func injectBarController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { - return nil -} - -func TestRegisterController(t *testing.T) { - i := &impl{} - - if want, got := 0, len(i.GetControllers()); got != want { - t.Errorf("GetControllers() = %d, wanted %d", want, got) - } - - i.RegisterController(injectFooController) - - if want, got := 1, len(i.GetControllers()); got != want { - t.Errorf("GetControllers() = %d, wanted %d", want, got) - } - - i.RegisterController(injectBarController) - - if want, got := 2, len(i.GetControllers()); got != want { - t.Errorf("GetControllers() = %d, wanted %d", want, got) - } -} diff --git a/injection/doc.go b/injection/doc.go index ca4e8ed22..857e3d728 100644 --- a/injection/doc.go +++ b/injection/doc.go @@ -25,7 +25,7 @@ limitations under the License. // import ( // // Simply linking this triggers the injection of the informer, which links // // the factory triggering its injection, and which links the client, -// // triggering its injection. +// // triggering its injection. All you need to know is that it works :) // deployinformer "github.com/knative/pkg/injection/informers/kubeinformers/appsv1/deployment" // "github.com/knative/pkg/injection" // ) @@ -38,28 +38,31 @@ limitations under the License. // ... // } // -// func init() { -// injection.Default.RegisterController(NewController) -// } -// // Then in `package main` the entire controller process can be set up via: // // package main // // import ( // // The set of controllers this controller process runs. -// // Linking these will register the controllers and their transitive -// // dependencies, after which the shared main can set up the rest. -// _ "github.com/knative/foo/pkg/reconciler/matt" -// _ "github.com/knative/foo/pkg/reconciler/scott" -// _ "github.com/knative/foo/pkg/reconciler/ville" -// _ "github.com/knative/foo/pkg/reconciler/dave" +// // Linking these will register their transitive dependencies, after +// // which the shared main can set up the rest. +// "github.com/knative/foo/pkg/reconciler/matt" +// "github.com/knative/foo/pkg/reconciler/scott" +// "github.com/knative/foo/pkg/reconciler/ville" +// "github.com/knative/foo/pkg/reconciler/dave" // // // This defines the shared main for injected controllers. // "github.com/knative/pkg/injection/sharedmain" // ) // // func main() { -// sharedmain.Main() +// sharedmain.Main("my-component", +// // We pass in the list of controllers to construct, and that's it! +// // If we forget to add this, go will complain about the unused import. +// matt.NewController, +// scott.NewController, +// ville.NewController, +// dave.NewController, +// ) // } package injection diff --git a/injection/interface.go b/injection/interface.go index 6a0df357e..6413b0619 100644 --- a/injection/interface.go +++ b/injection/interface.go @@ -22,6 +22,7 @@ import ( "k8s.io/client-go/rest" + "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" ) @@ -54,17 +55,11 @@ type Interface interface { // along with a list of the .Informer() for each of the injected informers, // which is suitable for passing to controller.StartInformers(). // This does not setup or start any controllers. - // TODO(mattmoor): Consider setting up and starting controllers? SetupInformers(context.Context, *rest.Config) (context.Context, []controller.Informer) - - // RegisterController registers a new injector callback for associating - // a new controller with a context. - RegisterController(ControllerInjector) - - // GetControllers fetches all of the registered controller injectors. - GetControllers() []ControllerInjector } +type ControllerConstructor func(context.Context, configmap.Watcher) *controller.Impl + var ( // Check that impl implements Interface _ Interface = (*impl)(nil) @@ -83,8 +78,7 @@ var ( type impl struct { m sync.RWMutex - clients []ClientInjector - factories []InformerFactoryInjector - informers []InformerInjector - controllers []ControllerInjector + clients []ClientInjector + factories []InformerFactoryInjector + informers []InformerInjector } diff --git a/injection/sharedmain/main.go b/injection/sharedmain/main.go index 8463a5b6d..5b30a8d2e 100644 --- a/injection/sharedmain/main.go +++ b/injection/sharedmain/main.go @@ -38,17 +38,12 @@ import ( "go.uber.org/zap" ) -func Main() { - // The default component name is "controller" - MainWithComponent("controller") -} - -func MainWithComponent(component string) { +func Main(component string, ctors ...injection.ControllerConstructor) { // Set up signals so we handle the first shutdown signal gracefully. - MainWithContext(signals.NewContext(), component) + MainWithContext(signals.NewContext(), component, ctors...) } -func MainWithContext(ctx context.Context, component string) { +func MainWithContext(ctx context.Context, component string, ctors ...injection.ControllerConstructor) { var ( masterURL = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.") kubeconfig = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.") @@ -59,10 +54,10 @@ func MainWithContext(ctx context.Context, component string) { if err != nil { log.Fatal("Error building kubeconfig", err) } - MainWithConfig(ctx, component, cfg) + MainWithConfig(ctx, component, cfg, ctors...) } -func MainWithConfig(ctx context.Context, component string, cfg *rest.Config) { +func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) { // Set up our logger. loggingConfigMap, err := configmap.Load("/etc/config-logging") if err != nil { @@ -79,11 +74,11 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config) { logger.Infof("Registering %d clients", len(injection.Default.GetClients())) logger.Infof("Registering %d informer factories", len(injection.Default.GetInformerFactories())) logger.Infof("Registering %d informers", len(injection.Default.GetInformers())) - logger.Infof("Registering %d controllers", len(injection.Default.GetControllers())) + logger.Infof("Registering %d controllers", len(ctors)) // Adjust our client's rate limits based on the number of controller's we are running. - cfg.QPS = float32(len(injection.Default.GetControllers())) * rest.DefaultQPS - cfg.Burst = len(injection.Default.GetControllers()) * rest.DefaultBurst + cfg.QPS = float32(len(ctors)) * rest.DefaultQPS + cfg.Burst = len(ctors) * rest.DefaultBurst ctx, informers := injection.Default.SetupInformers(ctx, cfg) @@ -91,8 +86,8 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config) { cmw := configmap.NewInformedWatcher(kubeclient.Get(ctx), system.Namespace()) // Based on the reconcilers we have linked, build up the set of controllers to run. - controllers := make([]*controller.Impl, 0, len(injection.Default.GetControllers())) - for _, cf := range injection.Default.GetControllers() { + controllers := make([]*controller.Impl, 0, len(ctors)) + for _, cf := range ctors { controllers = append(controllers, cf(ctx, cmw)) }