Skip to content

Commit 1ae868d

Browse files
theunrepentantgeektheunrepentantgeek
authored
Upgrade controller logging (#4222)
* Import zapr and zap packages * Create logging package to create loggers * Change ParseFlags() to InitFlags() * Refactor main * Switch other use to textlogger * Switch other uses * Fix lint issues * Refactor based on PR feedback * Address PR feedback
1 parent 618f71a commit 1ae868d

File tree

13 files changed

+200
-85
lines changed

13 files changed

+200
-85
lines changed

‎v2/cmd/controller/app/flags.go‎

Lines changed: 13 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,6 @@ package app
88
import (
99
"flag"
1010
"fmt"
11-
12-
"k8s.io/klog/v2"
13-
14-
"github.com/Azure/azure-service-operator/v2/internal/version"
1511
)
1612

1713
type Flags struct {
@@ -40,45 +36,21 @@ func (f Flags) String() string {
4036
f.CRDPatterns)
4137
}
4238

43-
func ParseFlags(args []string) (Flags, error) {
44-
exeName := args[0] + " " + version.BuildVersion
45-
flagSet := flag.NewFlagSet(exeName, flag.ExitOnError)
46-
klog.InitFlags(flagSet)
47-
48-
var metricsAddr string
49-
var profilingMetrics bool
50-
var secureMetrics bool
51-
var healthAddr string
52-
var webhookPort int
53-
var webhookCertDir string
54-
var enableLeaderElection bool
55-
var crdManagementMode string
56-
var crdPatterns string
39+
func InitFlags(flagSet *flag.FlagSet) *Flags {
40+
result := &Flags{}
5741

5842
// default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted.
59-
flagSet.StringVar(&metricsAddr, "metrics-addr", "0", "The address the metric endpoint binds to.")
60-
flagSet.BoolVar(&secureMetrics, "secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS")
61-
flagSet.BoolVar(&profilingMetrics, "profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints")
62-
63-
flagSet.StringVar(&healthAddr, "health-addr", "", "The address the healthz endpoint binds to.")
64-
flagSet.IntVar(&webhookPort, "webhook-port", 9443, "The port the webhook endpoint binds to.")
65-
flagSet.StringVar(&webhookCertDir, "webhook-cert-dir", "", "The directory the webhook server's certs are stored.")
66-
flagSet.BoolVar(&enableLeaderElection, "enable-leader-election", false,
67-
"Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.")
68-
flagSet.StringVar(&crdManagementMode, "crd-management", "auto",
43+
flagSet.StringVar(&result.MetricsAddr, "metrics-addr", "0", "The address the metric endpoint binds to.")
44+
flagSet.BoolVar(&result.SecureMetrics, "secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS")
45+
flagSet.BoolVar(&result.ProfilingMetrics, "profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints")
46+
flagSet.StringVar(&result.HealthAddr, "health-addr", "", "The address the healthz endpoint binds to.")
47+
flagSet.IntVar(&result.WebhookPort, "webhook-port", 9443, "The port the webhook endpoint binds to.")
48+
flagSet.StringVar(&result.WebhookCertDir, "webhook-cert-dir", "", "The directory the webhook server's certs are stored.")
49+
flagSet.BoolVar(&result.EnableLeaderElection, "enable-leader-election", false, "Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.")
50+
51+
flagSet.StringVar(&result.CRDManagementMode, "crd-management", "auto",
6952
"Instructs the operator on how it should manage the Custom Resource Definitions. One of 'auto', 'none'")
70-
flagSet.StringVar(&crdPatterns, "crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.")
71-
72-
flagSet.Parse(args[1:]) //nolint:errcheck
53+
flagSet.StringVar(&result.CRDPatterns, "crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.")
7354

74-
return Flags{
75-
MetricsAddr: metricsAddr,
76-
SecureMetrics: secureMetrics,
77-
HealthAddr: healthAddr,
78-
WebhookPort: webhookPort,
79-
WebhookCertDir: webhookCertDir,
80-
EnableLeaderElection: enableLeaderElection,
81-
CRDManagementMode: crdManagementMode,
82-
CRDPatterns: crdPatterns,
83-
}, nil
55+
return result
8456
}

‎v2/cmd/controller/app/setup.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ import (
5252
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/conditions"
5353
)
5454

55-
func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flags) manager.Manager {
55+
func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs *Flags) manager.Manager {
5656
scheme := controllers.CreateScheme()
5757
_ = apiextensions.AddToScheme(scheme) // Used for managing CRDs
5858

@@ -211,7 +211,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag
211211
return mgr
212212
}
213213

214-
func getMetricsOpts(flags Flags) server.Options {
214+
func getMetricsOpts(flags *Flags) server.Options {
215215
var metricsOptions server.Options
216216

217217
if flags.SecureMetrics {

‎v2/cmd/controller/logging/logging.go‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
Copyright (c) Microsoft Corporation.
3+
Licensed under the MIT license.
4+
*/
5+
6+
package logging
7+
8+
import (
9+
"flag"
10+
11+
"github.com/go-logr/logr"
12+
"github.com/go-logr/zapr"
13+
"go.uber.org/zap"
14+
"go.uber.org/zap/zapcore"
15+
"k8s.io/klog/v2/textlogger"
16+
17+
"github.com/Azure/azure-service-operator/v2/internal/logging"
18+
)
19+
20+
type Config struct {
21+
// verbosity indicates the level of logging.
22+
// Higher values indicate more logging.
23+
verbosity int
24+
25+
// useJSON indicates whether we should output logs in JSON format.
26+
// Default is no
27+
useJSON bool
28+
}
29+
30+
// Create returns a new logger, ready for use.
31+
// This can be called multiple times if required.
32+
func Create(cfg *Config) logr.Logger {
33+
if cfg != nil && cfg.useJSON {
34+
log, err := createJSONLogger(cfg)
35+
if err != nil {
36+
log = createTextLogger(cfg)
37+
log.Error(err, "failed to create JSON logger, falling back to text")
38+
}
39+
40+
return log
41+
}
42+
43+
return createTextLogger(cfg)
44+
}
45+
46+
func createTextLogger(cfg *Config) logr.Logger {
47+
opts := []textlogger.ConfigOption{}
48+
if cfg != nil {
49+
opts = append(opts, textlogger.Verbosity(cfg.verbosity))
50+
}
51+
52+
c := textlogger.NewConfig(opts...)
53+
return textlogger.NewLogger(c)
54+
}
55+
56+
func createJSONLogger(cfg *Config) (logr.Logger, error) {
57+
level := zap.InfoLevel
58+
if cfg != nil {
59+
switch cfg.verbosity {
60+
case 0:
61+
level = zap.ErrorLevel
62+
case 1:
63+
level = zap.WarnLevel
64+
case 2:
65+
level = zap.InfoLevel
66+
default: // 3 or above
67+
level = zap.DebugLevel
68+
}
69+
}
70+
71+
encoder := zap.NewProductionEncoderConfig()
72+
encoder.EncodeTime = zapcore.ISO8601TimeEncoder
73+
74+
c := zap.Config{
75+
Level: zap.NewAtomicLevelAt(level),
76+
Development: false,
77+
Encoding: "json",
78+
EncoderConfig: encoder,
79+
OutputPaths: []string{"stderr"},
80+
ErrorOutputPaths: []string{"stderr"},
81+
}
82+
83+
logger, err := c.Build()
84+
if err != nil {
85+
return logr.Logger{}, err
86+
}
87+
88+
return zapr.NewLogger(logger), nil
89+
}
90+
91+
// InitFlags initializes the flags for the logging package
92+
func InitFlags(fs *flag.FlagSet) *Config {
93+
result := &Config{}
94+
95+
fs.IntVar(&result.verbosity, "verbose", logging.Verbose, "Enable verbose logging")
96+
fs.IntVar(&result.verbosity, "v", logging.Verbose, "Enable verbose logging")
97+
98+
fs.BoolVar(&result.useJSON, "json-logging", false, "Enable JSON logging")
99+
100+
return result
101+
}

‎v2/cmd/controller/main.go‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,31 +6,45 @@ Licensed under the MIT license.
66
package main
77

88
import (
9+
"flag"
910
"os"
1011

1112
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
12-
"k8s.io/klog/v2/klogr"
1313
ctrl "sigs.k8s.io/controller-runtime"
1414

1515
"github.com/Azure/azure-service-operator/v2/cmd/controller/app"
16+
"github.com/Azure/azure-service-operator/v2/cmd/controller/logging"
17+
"github.com/Azure/azure-service-operator/v2/internal/version"
1618
)
1719

1820
func main() {
19-
setupLog := ctrl.Log.WithName("setup")
20-
ctrl.SetLogger(klogr.New()) //nolint: staticcheck
21+
// Set up to parse command line flags
22+
exeName := os.Args[0] + " " + version.BuildVersion
23+
flagSet := flag.NewFlagSet(exeName, flag.ExitOnError)
24+
25+
// Create a temporary logger for while we get set up
26+
log := logging.Create(&logging.Config{})
27+
2128
ctx := ctrl.SetupSignalHandler()
2229

23-
flgs, err := app.ParseFlags(os.Args)
30+
// Add application and logging flags
31+
appFlags := app.InitFlags(flagSet)
32+
logFlags := logging.InitFlags(flagSet)
33+
err := flagSet.Parse(os.Args[1:])
2434
if err != nil {
25-
setupLog.Error(err, "failed to parse cmdline flags")
35+
log.Error(err, "failed to parse cmdline flags")
2636
os.Exit(1)
2737
}
2838

29-
setupLog.Info("Launching with flags", "flags", flgs.String())
30-
mgr := app.SetupControllerManager(ctx, setupLog, flgs)
31-
setupLog.Info("starting manager")
39+
// Replace the logger with a configured one
40+
log = logging.Create(logFlags)
41+
ctrl.SetLogger(log)
42+
log.Info("Launching with flags", "flags", appFlags.String())
43+
44+
mgr := app.SetupControllerManager(ctx, log, appFlags)
45+
log.Info("starting manager")
3246
if err = mgr.Start(ctx); err != nil {
33-
setupLog.Error(err, "failed to start manager")
47+
log.Error(err, "failed to start manager")
3448
os.Exit(1)
3549
}
3650
}

‎v2/go.mod‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ require (
2727
github.com/benbjohnson/clock v1.3.5
2828
github.com/dnaeon/go-vcr v1.2.0
2929
github.com/go-logr/logr v1.4.2
30+
github.com/go-logr/zapr v1.3.0
3031
github.com/go-sql-driver/mysql v1.8.1
3132
github.com/google/go-cmp v0.6.0
3233
github.com/google/uuid v1.6.0
@@ -40,6 +41,7 @@ require (
4041
github.com/pkg/errors v0.9.1
4142
github.com/prometheus/client_golang v1.20.2
4243
github.com/spf13/cobra v1.8.1
44+
go.uber.org/zap v1.27.0
4345
golang.org/x/crypto v0.26.0
4446
golang.org/x/exp v0.0.0-20240823005443-9b4947da3948
4547
golang.org/x/sync v0.8.0
@@ -115,6 +117,7 @@ require (
115117
go.opentelemetry.io/otel/sdk v1.29.0 // indirect
116118
go.opentelemetry.io/otel/trace v1.29.0 // indirect
117119
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
120+
go.uber.org/multierr v1.11.0 // indirect
118121
golang.org/x/net v0.28.0 // indirect
119122
golang.org/x/oauth2 v0.22.0 // indirect
120123
golang.org/x/sys v0.24.0 // indirect

‎v2/go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,8 +469,8 @@ go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9i
469469
go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
470470
go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
471471
go.uber.org/zap v1.17.0/go.mod h1:MXVU+bhUf/A7Xi2HNOnopQOrmycQ5Ih87HtOu4q5SSo=
472-
go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo=
473-
go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so=
472+
go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8=
473+
go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E=
474474
golang.org/x/crypto v0.0.0-20181029021203-45a5f77698d3/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
475475
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
476476
golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=

‎v2/internal/controllers/suite_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,11 @@ func setup() error {
4242

4343
// If you need to debug envtest setup/teardown,
4444
// set a global logger for controller-runtime:
45+
//
4546
// import (ctrl "sigs.k8s.io/controller-runtime")
46-
// ctrl.SetLogger(klogr.New())
47+
// cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests
48+
// log := textlogger.NewLogger(cfg)
49+
// ctrl.SetLogger(log)
4750

4851
nameConfig := testcommon.NewResourceNameConfig(
4952
testcommon.ResourcePrefix,

‎v2/internal/genericarmclient/suite_test.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,11 @@ import (
1313

1414
"github.com/onsi/gomega"
1515
"github.com/onsi/gomega/format"
16-
"k8s.io/klog/v2/klogr"
16+
"k8s.io/klog/v2/textlogger"
17+
1718
ctrl "sigs.k8s.io/controller-runtime"
1819

20+
. "github.com/Azure/azure-service-operator/v2/internal/logging"
1921
"github.com/Azure/azure-service-operator/v2/internal/testcommon"
2022
)
2123

@@ -26,14 +28,16 @@ var testContext testcommon.TestContext
2628
func setup() error {
2729
recordReplay := os.Getenv("RECORD_REPLAY") != "0"
2830

29-
log.Println("Running test setup")
30-
3131
gomega.SetDefaultEventuallyTimeout(DefaultEventuallyTimeout)
3232
gomega.SetDefaultEventuallyPollingInterval(5 * time.Second)
3333
format.TruncateThreshold = 4000 // Force a longer truncate threshold
3434

3535
// setup global logger for controller-runtime:
36-
ctrl.SetLogger(klogr.New()) //nolint: staticcheck
36+
cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests
37+
log := textlogger.NewLogger(cfg)
38+
ctrl.SetLogger(log)
39+
40+
log.Info("Running test setup")
3741

3842
nameConfig := testcommon.NewResourceNameConfig(
3943
testcommon.ResourcePrefix,
@@ -44,7 +48,7 @@ func setup() error {
4448
// set global test context
4549
testContext = testcommon.NewTestContext(testcommon.DefaultTestRegion, recordReplay, nameConfig)
4650

47-
log.Println("Done with test setup")
51+
log.Info("Done with test setup")
4852

4953
return nil
5054
}

‎v2/internal/testcommon/kube_test_context_envtest.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030
"sigs.k8s.io/controller-runtime/pkg/client"
3131
"sigs.k8s.io/controller-runtime/pkg/controller"
3232
"sigs.k8s.io/controller-runtime/pkg/envtest"
33-
ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
3433
"sigs.k8s.io/controller-runtime/pkg/metrics/server"
3534
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3635
"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -89,10 +88,13 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources)
8988
Scheme: scheme,
9089
}
9190

92-
// TODO: Switch to klogr.New() below the below if we want controller-runtime logs in the tests.
91+
// Switch logger below if we want controller-runtime logs in the tests.
9392
// By default we've disabled controller runtime logs because they're very verbose and usually not useful.
94-
// ctrl.SetLogger(klogr.New())
95-
ctrl.SetLogger(logr.New(ctrllog.NullLogSink{}))
93+
// import (ctrl "sigs.k8s.io/controller-runtime")
94+
// cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests
95+
// log := textlogger.NewLogger(cfg)
96+
// ctrl.SetLogger(log)
97+
ctrl.SetLogger(logr.Discard())
9698

9799
log.Println("Starting envtest")
98100
kubeConfig, err := environment.Start()
@@ -160,8 +162,12 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources)
160162

161163
// TODO: Uncomment the below if we want controller-runtime logs in the tests.
162164
// By default we've disabled controller runtime logs because they're very verbose and usually not useful.
163-
// ctrl.SetLogger(klogr.New())
164-
ctrl.SetLogger(logr.New(ctrllog.NullLogSink{}))
165+
//
166+
// import (ctrl "sigs.k8s.io/controller-runtime")
167+
// cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests
168+
// log := textlogger.NewLogger(cfg)
169+
// ctrl.SetLogger(log)
170+
ctrl.SetLogger(logr.Discard())
165171

166172
loggerFactory := func(obj metav1.Object) logr.Logger {
167173
result := namespaceResources.Lookup(obj.GetNamespace())

‎v2/pkg/genruntime/test/suite_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,11 @@ func setup() error {
4141

4242
// If you need to debug envtest setup/teardown,
4343
// set a global logger for controller-runtime:
44+
//
4445
// import (ctrl "sigs.k8s.io/controller-runtime")
45-
// ctrl.SetLogger(klogr.New())
46+
// cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests
47+
// log := textlogger.NewLogger(cfg)
48+
// ctrl.SetLogger(log)
4649

4750
nameConfig := testcommon.NewResourceNameConfig(
4851
testcommon.ResourcePrefix,

0 commit comments

Comments
 (0)