Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 Inject manager's logger instead of internal one #1289

Merged
merged 1 commit into from
Dec 8, 2020
Merged

🐛 Inject manager's logger instead of internal one #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions pkg/manager/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/healthz"
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
"sigs.k8s.io/controller-runtime/pkg/metrics"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
Expand All @@ -58,7 +57,6 @@ const (
)

var _ Runnable = &controllerManager{}
var log = logf.RuntimeLog.WithName("manager")

type controllerManager struct {
// config is the rest.config used to talk to the apiserver. Required.
Expand Down Expand Up @@ -251,7 +249,7 @@ func (cm *controllerManager) SetFields(i interface{}) error {
if _, err := inject.MapperInto(cm.mapper, i); err != nil {
return err
}
if _, err := inject.LoggerInto(log, i); err != nil {
if _, err := inject.LoggerInto(cm.logger, i); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a mess this is, so the log variable is:

  • logf.RuntimeLog.WithName("manager") where
    -> logf.RuntimeLog is log.Log.WithName("controller-runtime") where
    -> log.Log is NewDelegatingLogger(NullLogger{})

And the managers logger is defaulted to logf.Log

So I think we should have the manager default options.Logger to what is currently the log variable instead, right?

ALso, can you check if the log variable is used by anything else and if not, remove it and move the construction of its logger into the managers log defaulting?

/ok-to-test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, valid point.
Changed the defaulting of the manager's logger to logf.RuntimeLog.WithName("manager") and switched the remaining usages of the log variable now.
Hope I got you right, PTAL

return err
}
return nil
Expand All @@ -272,7 +270,7 @@ func (cm *controllerManager) AddMetricsExtraHandler(path string, handler http.Ha
}

cm.metricsExtraHandlers[path] = handler
log.V(2).Info("Registering metrics http server extra handler", "path", path)
cm.logger.V(2).Info("Registering metrics http server extra handler", "path", path)
return nil
}

Expand Down Expand Up @@ -405,7 +403,7 @@ func (cm *controllerManager) serveMetrics() {
}
// Run the server
cm.startRunnable(RunnableFunc(func(_ context.Context) error {
log.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
}
Expand Down Expand Up @@ -546,7 +544,7 @@ func (cm *controllerManager) engageStopProcedure(stopComplete <-chan struct{}) e
select {
case err, ok := <-cm.errChan:
if ok {
log.Error(err, "error received after stop sequence was engaged")
cm.logger.Error(err, "error received after stop sequence was engaged")
}
case <-stopComplete:
return
Expand Down
8 changes: 4 additions & 4 deletions pkg/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ import (
"sigs.k8s.io/controller-runtime/pkg/config"
"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/healthz"
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
"sigs.k8s.io/controller-runtime/pkg/leaderelection"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/metrics"
"sigs.k8s.io/controller-runtime/pkg/recorder"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
Expand Down Expand Up @@ -314,7 +314,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
// Create the mapper provider
mapper, err := options.MapperProvider(config)
if err != nil {
log.Error(err, "Failed to get API Group-Resources")
options.Logger.Error(err, "Failed to get API Group-Resources")
return nil, err
}

Expand Down Expand Up @@ -345,7 +345,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
// Create the recorder provider to inject event recorders for the components.
// TODO(directxman12): the log for the event provider should have a context (name, tags, etc) specific
// to the particular controller that it's being injected into, rather than a generic one like is here.
recorderProvider, err := options.newRecorderProvider(config, options.Scheme, log.WithName("events"), options.makeBroadcaster)
recorderProvider, err := options.newRecorderProvider(config, options.Scheme, options.Logger.WithName("events"), options.makeBroadcaster)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -599,7 +599,7 @@ func setOptionsDefaults(options Options) Options {
}

if options.Logger == nil {
options.Logger = logf.Log
options.Logger = logf.RuntimeLog.WithName("manager")
}

return options
Expand Down
6 changes: 3 additions & 3 deletions pkg/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ import (
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/leaderelection/resourcelock"
configv1alpha1 "k8s.io/component-base/config/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"

"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
"sigs.k8s.io/controller-runtime/pkg/leaderelection"
fakeleaderelection "sigs.k8s.io/controller-runtime/pkg/leaderelection/fake"
Expand Down Expand Up @@ -1359,7 +1359,7 @@ var _ = Describe("manger.Manager", func() {
},
log: func(logger logr.Logger) error {
defer GinkgoRecover()
Expect(logger).To(Equal(log))
Expect(logger).To(Equal(logf.RuntimeLog.WithName("manager")))
return nil
},
})
Expand Down