From 5767de536c68b033c7e6ccc643c6a191b26a6e08 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Wed, 6 Oct 2021 10:54:51 +0200 Subject: [PATCH 1/4] fake-client: extend builder with an option to set a RESTMapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- pkg/client/fake/client.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 1f0efc82bb..f72afe19d9 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -51,6 +51,7 @@ type versionedTracker struct { type fakeClient struct { tracker versionedTracker scheme *runtime.Scheme + restMapper meta.RESTMapper schemeWriteLock sync.Mutex } @@ -87,6 +88,7 @@ func NewClientBuilder() *ClientBuilder { // ClientBuilder builds a fake client. type ClientBuilder struct { scheme *runtime.Scheme + restMapper meta.RESTMapper initObject []client.Object initLists []client.ObjectList initRuntimeObjects []runtime.Object @@ -99,6 +101,15 @@ func (f *ClientBuilder) WithScheme(scheme *runtime.Scheme) *ClientBuilder { return f } +// WithRESTMapper sets this builder's restMapper. +// The restMapper is directly set as mapper in the Client. This can be used for example +// with a meta.DefaultRESTMapper to provide a static rest mapping. +// If not set, defaults to an empty meta.DefaultRESTMapper. +func (f *ClientBuilder) WithRESTMapper(restMapper meta.RESTMapper) *ClientBuilder { + f.restMapper = restMapper + return f +} + // WithObjects can be optionally used to initialize this fake client with client.Object(s). func (f *ClientBuilder) WithObjects(initObjs ...client.Object) *ClientBuilder { f.initObject = append(f.initObject, initObjs...) @@ -122,6 +133,9 @@ func (f *ClientBuilder) Build() client.WithWatch { if f.scheme == nil { f.scheme = scheme.Scheme } + if f.restMapper == nil { + f.restMapper = meta.NewDefaultRESTMapper([]schema.GroupVersion{}) + } tracker := versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme} for _, obj := range f.initObject { @@ -140,8 +154,9 @@ func (f *ClientBuilder) Build() client.WithWatch { } } return &fakeClient{ - tracker: tracker, - scheme: f.scheme, + tracker: tracker, + scheme: f.scheme, + restMapper: f.restMapper, } } @@ -417,8 +432,7 @@ func (c *fakeClient) Scheme() *runtime.Scheme { } func (c *fakeClient) RESTMapper() meta.RESTMapper { - // TODO: Implement a fake RESTMapper. - return nil + return c.restMapper } func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { From 88a57abf07dd0a3eb51caef89e7c459148b59557 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Thu, 7 Oct 2021 19:24:04 +0200 Subject: [PATCH 2/4] Improve startup logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- pkg/builder/webhook.go | 4 ++-- pkg/internal/controller/controller.go | 2 +- pkg/manager/internal.go | 2 +- pkg/metrics/listener.go | 2 +- pkg/source/source.go | 6 +++--- pkg/webhook/server.go | 6 +++--- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index dcfa787bb2..18feb1cd74 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -167,14 +167,14 @@ func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook { func (blder *WebhookBuilder) registerConversionWebhook() error { ok, err := conversion.IsConvertible(blder.mgr.GetScheme(), blder.apiType) if err != nil { - log.Error(err, "conversion check failed", "object", blder.apiType) + log.Error(err, "conversion check failed", "GVK", blder.gvk) return err } if ok { if !blder.isAlreadyHandled("/convert") { blder.mgr.GetWebhookServer().Register("/convert", &conversion.Webhook{}) } - log.Info("conversion webhook enabled", "object", blder.apiType) + log.Info("Conversion webhook enabled", "GVK", blder.gvk) } return nil diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go index 87431a438f..1f4712d8bf 100644 --- a/pkg/internal/controller/controller.go +++ b/pkg/internal/controller/controller.go @@ -175,7 +175,7 @@ func (c *Controller) Start(ctx context.Context) error { // caches to sync so that they have a chance to register their intendeded // caches. for _, watch := range c.startWatches { - c.Log.Info("Starting EventSource", "source", watch.src) + c.Log.Info("Starting EventSource", "source", fmt.Sprintf("%s", watch.src)) if err := watch.src.Start(ctx, watch.handler, c.Queue, watch.predicates...); err != nil { return err diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 7c25bd3c60..878142b0ba 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -380,7 +380,7 @@ func (cm *controllerManager) serveMetrics() { } // Run the server cm.startRunnable(RunnableFunc(func(_ context.Context) error { - cm.logger.Info("starting metrics server", "path", defaultMetricsEndpoint) + cm.logger.Info("Starting metrics server", "path", defaultMetricsEndpoint) if err := server.Serve(cm.metricsListener); err != nil && err != http.ErrServerClosed { return err } diff --git a/pkg/metrics/listener.go b/pkg/metrics/listener.go index d32ae58186..123d8c15f9 100644 --- a/pkg/metrics/listener.go +++ b/pkg/metrics/listener.go @@ -41,7 +41,7 @@ func NewListener(addr string) (net.Listener, error) { return nil, nil } - log.Info("metrics server is starting to listen", "addr", addr) + log.Info("Metrics server is starting to listen", "addr", addr) ln, err := net.Listen("tcp", addr) if err != nil { er := fmt.Errorf("error listening on %s: %w", addr, err) diff --git a/pkg/source/source.go b/pkg/source/source.go index 708c5a5bfc..8f649eaac4 100644 --- a/pkg/source/source.go +++ b/pkg/source/source.go @@ -161,10 +161,10 @@ func (ks *Kind) Start(ctx context.Context, handler handler.EventHandler, queue w } func (ks *Kind) String() string { - if ks.Type != nil && ks.Type.GetObjectKind() != nil { - return fmt.Sprintf("kind source: %v", ks.Type.GetObjectKind().GroupVersionKind().String()) + if ks.Type != nil { + return fmt.Sprintf("kind source: %T", ks.Type) } - return "kind source: unknown GVK" + return "kind source: unknown type" } // WaitForSync implements SyncingSource to allow controllers to wait with starting diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index d2338d0b77..1db38113f7 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -142,7 +142,7 @@ func (s *Server) Register(path string, hook http.Handler) { s.WebhookMux.Handle(path, metrics.InstrumentedHook(path, hook)) regLog := log.WithValues("path", path) - regLog.Info("registering webhook") + regLog.Info("Registering webhook") // we've already been "started", inject dependencies here. // Otherwise, InjectFunc will do this for us later. @@ -210,7 +210,7 @@ func (s *Server) Start(ctx context.Context) error { s.defaultingOnce.Do(s.setDefaults) baseHookLog := log.WithName("webhooks") - baseHookLog.Info("starting webhook server") + baseHookLog.Info("Starting webhook server") certPath := filepath.Join(s.CertDir, s.CertName) keyPath := filepath.Join(s.CertDir, s.KeyName) @@ -259,7 +259,7 @@ func (s *Server) Start(ctx context.Context) error { return err } - log.Info("serving webhook server", "host", s.Host, "port", s.Port) + log.Info("Serving webhook server", "host", s.Host, "port", s.Port) srv := &http.Server{ Handler: s.WebhookMux, From b6a5752ebc63ba48ece2a3f10d26115a01914e66 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Thu, 4 Nov 2021 22:53:49 +0100 Subject: [PATCH 3/4] Start web hooks first --- pkg/manager/internal.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 878142b0ba..aacbc4f09a 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -476,6 +476,11 @@ func (cm *controllerManager) Start(ctx context.Context) (err error) { go cm.serveHealthProbes() } + // Webhooks MUST start before any cache is populated, otherwise there is a race condition + // between conversion webhooks and the cache sync (usually initial list) which causes the webhooks + // to never start because no cache can be populated. + cm.startWebhookRunnables() + go cm.startNonLeaderElectionRunnables() go func() { @@ -573,13 +578,10 @@ func (cm *controllerManager) waitForRunnableToEnd(shutdownCancel context.CancelF return nil } -func (cm *controllerManager) startNonLeaderElectionRunnables() { +func (cm *controllerManager) startWebhookRunnables() { cm.mu.Lock() defer cm.mu.Unlock() - // First start any webhook servers, which includes conversion, validation, and defaulting - // webhooks that are registered. - // // WARNING: Webhooks MUST start before any cache is populated, otherwise there is a race condition // between conversion webhooks and the cache sync (usually initial list) which causes the webhooks // to never start because no cache can be populated. @@ -588,6 +590,11 @@ func (cm *controllerManager) startNonLeaderElectionRunnables() { cm.startRunnable(c) } } +} + +func (cm *controllerManager) startNonLeaderElectionRunnables() { + cm.mu.Lock() + defer cm.mu.Unlock() // Start and wait for caches. cm.waitForCache(cm.internalCtx) From 708d5397c4f300c897aaa0fbacab4804964e1f25 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Fri, 5 Nov 2021 10:09:59 +0100 Subject: [PATCH 4/4] =?UTF-8?q?start=20healhprobes=20sync=20Co-authored-by?= =?UTF-8?q?:=20Stefan=20B=C3=BCringer=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/manager/internal.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index aacbc4f09a..59794d9620 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -425,11 +425,13 @@ func (cm *controllerManager) serveHealthProbes() { cm.healthzStarted = true }() - // Shutdown the server when stop is closed - <-cm.internalProceduresStop - if err := server.Shutdown(cm.shutdownCtx); err != nil { - cm.errChan <- err - } + go func() { + // Shutdown the server when stop is closed + <-cm.internalProceduresStop + if err := server.Shutdown(cm.shutdownCtx); err != nil { + cm.errChan <- err + } + }() } func (cm *controllerManager) Start(ctx context.Context) (err error) { @@ -473,7 +475,7 @@ func (cm *controllerManager) Start(ctx context.Context) (err error) { // Serve health probes if cm.healthProbeListener != nil { - go cm.serveHealthProbes() + cm.serveHealthProbes() } // Webhooks MUST start before any cache is populated, otherwise there is a race condition