Skip to content

Instantly share code, notes, and snippets.

@rainest

rainest/failures.diff Secret

Last active September 16, 2023 00:45
Show Gist options
  • Save rainest/4d82f944416903f448d49c52ac4ff5cb to your computer and use it in GitHub Desktop.
Save rainest/4d82f944416903f448d49c52ac4ff5cb to your computer and use it in GitHub Desktop.
Download ZIP
Raw
failures.diff
diff --git a/internal/dataplane/failures/failures.go b/internal/dataplane/failures/failures.go
index ac5381080..d468ea195 100644
--- a/internal/dataplane/failures/failures.go
+++ b/internal/dataplane/failures/failures.go
@@ -4,8 +4,10 @@ import (
"errors"
"fmt"
- "github.com/sirupsen/logrus"
+ "github.com/go-logr/logr"
"sigs.k8s.io/controller-runtime/pkg/client"
+
+ "github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)
const (
@@ -65,21 +67,18 @@ func (p ResourceFailure) Message() string {
// ResourceFailuresCollector collects resource failures across different stages of resource processing.
type ResourceFailuresCollector struct {
failures []ResourceFailure
- logger logrus.FieldLogger
+ logger logr.Logger
}
-func NewResourceFailuresCollector(logger logrus.FieldLogger) (*ResourceFailuresCollector, error) {
- if logger == nil {
- return nil, errors.New("missing logger")
- }
- return &ResourceFailuresCollector{logger: logger}, nil
+func NewResourceFailuresCollector(logger logr.Logger) *ResourceFailuresCollector {
+ return &ResourceFailuresCollector{logger: logger}
}
// PushResourceFailure adds a resource processing failure to the collector and logs it.
func (c *ResourceFailuresCollector) PushResourceFailure(reason string, causingObjects ...client.Object) {
resourceFailure, err := NewResourceFailure(reason, causingObjects...)
if err != nil {
- c.logger.WithField("resource_failure_reason", reason).Warningf("failed to create resource failure: %s", err)
+ c.logger.V(util.WarnLevel).Info("failed to create resource failure", "resource_failure_reason", reason, "error", err)
return
}
@@ -90,11 +89,11 @@ func (c *ResourceFailuresCollector) PushResourceFailure(reason string, causingOb
// logResourceFailure logs an error with a resource processing failure message for each causing object.
func (c *ResourceFailuresCollector) logResourceFailure(reason string, causingObjects ...client.Object) {
for _, obj := range causingObjects {
- c.logger.WithFields(logrus.Fields{
- "name": obj.GetName(),
- "namespace": obj.GetNamespace(),
- "GVK": obj.GetObjectKind().GroupVersionKind().String(),
- }).Errorf("resource processing failed: %s", reason)
+ c.logger.Error(fmt.Errorf("resource processing failed"),
+ "name", obj.GetName(),
+ "namespace", obj.GetNamespace(),
+ "GVK", obj.GetObjectKind().GroupVersionKind().String(),
+ "reason", reason)
}
}
diff --git a/internal/dataplane/failures/failures_test.go b/internal/dataplane/failures/failures_test.go
index e74b6ee3e..498ad65fa 100644
--- a/internal/dataplane/failures/failures_test.go
+++ b/internal/dataplane/failures/failures_test.go
@@ -3,6 +3,7 @@ package failures
import (
"testing"
+ "github.com/bombsimon/logrusr/v4"
"github.com/sirupsen/logrus"
"github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"
@@ -61,23 +62,19 @@ func TestResourceFailure(t *testing.T) {
}
func TestResourceFailuresCollector(t *testing.T) {
- testLogger, _ := test.NewNullLogger()
-
t.Run("is created when logger valid", func(t *testing.T) {
- collector, err := NewResourceFailuresCollector(testLogger)
- require.NoError(t, err)
- require.NotNil(t, collector)
- })
+ logrusNull, _ := test.NewNullLogger()
+ logger := logrusr.New(logrusNull)
- t.Run("requires non nil logger", func(t *testing.T) {
- _, err := NewResourceFailuresCollector(nil)
- require.Error(t, err)
+ collector := NewResourceFailuresCollector(logger)
+ require.NotNil(t, collector)
})
t.Run("pushes, logs and pops resource failures", func(t *testing.T) {
- logger, loggerHook := test.NewNullLogger()
- collector, err := NewResourceFailuresCollector(logger)
- require.NoError(t, err)
+ logrusNull, loggerHook := test.NewNullLogger()
+ logger := logrusr.New(logrusNull)
+
+ collector := NewResourceFailuresCollector(logger)
collector.PushResourceFailure(someValidResourceFailureReason, someResourceFailureCausingObjects()...)
collector.PushResourceFailure(someValidResourceFailureReason, someResourceFailureCausingObjects()...)
@@ -92,9 +89,10 @@ func TestResourceFailuresCollector(t *testing.T) {
})
t.Run("does not crash but logs warning when no causing objects passed", func(t *testing.T) {
- logger, loggerHook := test.NewNullLogger()
- collector, err := NewResourceFailuresCollector(logger)
- require.NoError(t, err)
+ logrusNull, loggerHook := test.NewNullLogger()
+ logger := logrusr.New(logrusNull)
+
+ collector := NewResourceFailuresCollector(logger)
collector.PushResourceFailure(someValidResourceFailureReason)
Raw
loggers.md

Code path

Initial setup

internal/cmd/rootcmd/run.go:Run() runs internal/manager/setup.go:SetupLoggers(), providing a deprecated logger and logger. Briefly:

// creates a logrus.FieldLogger after validating the requested level and format
deprecated := util.MakeLogger(c.LogLevel, c.LogFormat, io.Writer)

// wraps a logrus.FieldLogger into a go-logr.Logger
logger := logrusr.New(deprecated)

// binds logger to a context. this is apparently equivalent to running
// SetLogger, as we don't call that elsewhere outside of test code:
// https://github.com/kubernetes-sigs/controller-runtime/blob/v0.16.2/pkg/log/log.go#L78-L82
ctx = ctrl.LoggerInto(ctx, logger)

We pass these to RunWithLogger()

We pass logger to StartDiagnosticsServer() and deprecated+ctx to manager.Run().

Manager

manager.Run() further instantiates additional loggers.

setupLog := controller-runtime.LoggerFrom(ctx).WithName("setup"). This and other controller-runtime-derived loggers are go-logr instances built from our bound ctx.

We pass deprecated to:

  • the API clients manager
  • the parser
  • the store
  • the update strategy resolver
  • the configuration change detector
  • the dataplane client and synchronizer

Apparently we haven't been good at honoring the "deprecated" name hint.

We pass logger (via LoggerFrom) to:

  • license-agent
  • the controller-runtime manager
  • the controllers

We pass setupLog to:

  • the readiness checker (which apparently uses it indefinitely)
  • the feature gates thing
  • the health server starter
  • the admin API clients initializer
  • the root getter
  • a bunch of other things that use it for init logging

setupLog recipients do generally apparently discard it after startup. the readiness checker appears to be the only exception, but I didn't check exhaustively.

Output

Since our go-logr instance is built from our logrus instance, the choice of log library does not affect output format. If we choose some other library, we can stuff it into go-logr fine so long as there's a compatibility layer for the LogSink interface.

There are, however, some logs that use klog, e.g. from startup:

W0913 23:40:19.233791       1 client_config.go:618] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0913 23:40:19.276121       1 leaderelection.go:250] attempting to acquire leader lease kong/5b374a9e.konghq.com...
I0913 23:40:19.280382       1 leaderelection.go:260] successfully acquired lease kong/5b374a9e.konghq.com

AFAIK these are all client-go logs, and they are from calls initiated by controller-runtime. We (rather, they) apparently cannot control the log engine used there or haven't bothered. Given that one of the calls calls klog.Info() directly, it's more likely a "can't" situation. AFAIK klog does not accept alternate engines.

klog does apparently use global settings, since functions like https://pkg.go.dev/k8s.io/klog#SetOutput are at the package level. We don't call any, however.

https://pkg.go.dev/k8s.io/klog/v2/klogr is available, active, and Apache-licensed if we wanted to get fullly consistent logs, but I don't think there are so many klog logs that it'd warrant it on the basis of consistency alone.

Input

logrus

logrus uses per-level functions (Infof, Debugf, Warnf, etc. and no-format equivalents) and WithField(s)/WithError functions to build a log line. For example:

log.WithFields(logrus.Fields{ // a map[string]interface{} alias
	"service": *service.Name,
	"fake": 20,
}).Error("multi-service backend lacks parent info, cannot generate tags")

logr

logr uses Info() and Error() functions along with a V() call to control the log level. These functions accept variable arity interface{} arguments for fields which must alternate between string (field names) and any type values. For example:

a.logger.V(util.DebugLevel).Info("removed outdated kong gateway node", 
	"node_id", node.ID, "hostname", node.Hostname)

We have defined our own V-level constants.

Migration

Estimating the number of pure logrus calls is difficult because the log functions match either stdlib or go-logr function names and the calling object does not use a consistent name.

As best I know, there is no major functionality in logrus that is not available in logr. Both offer levels (with the output caveat at the end of this section) and fields. Converting them is a matter of (tedious) work, not figuring out ways to handle missing functionality.

As our go-logr usage is a wrapper around logrus, the main costs of not unifying them are:

  • Not being able to apply high-level settings to parent loggers consistently. While this is a limitation, I can't recall that it's often come up.
  • Continued accidental use of the plain logrus instance despite its name. This makes future conversion more tedious. We have apparently done this several times for newer subsystems and didn't catch it in review.
  • Inability to easily switch to a new logging engine that supports logr interfaces. Doing so now would result in a difference in formats.

Conversion to go-logr calls is likely not so arduous that I would not recommend it up front. Neither is plumbing logger instances.

The V-level/named level discrepancy Jarek raised earlier is a problem. This appears to be an artifact of the logrusr wrapper rather than something inherent to logr. The function to adopt logr's Info() and Error() functions to logrus calls that equivalent-level function only.

We are, however, already affected by this problem where we use the logr wrapper. To address it we should explore whether we can modify logrusr to infer the level from the V-level or review other base logger options (zap or whatever) to see if they handle this better.

Index of package includes

go-logr

internal/adminapi/konnect.go:	"github.com/go-logr/logr"
internal/clients/config_status.go:	"github.com/go-logr/logr"
internal/clients/config_status_test.go:	"github.com/go-logr/logr"
internal/clients/readiness.go:	"github.com/go-logr/logr"
internal/clients/readiness_test.go:	"github.com/go-logr/logr"
internal/cmd/rootcmd/run.go:	"github.com/go-logr/logr"
internal/cmd/rootcmd/servers.go:	"github.com/go-logr/logr"
internal/cmd/rootcmd/signal.go:	"github.com/go-logr/logr"
internal/controllers/configuration/kongadminapi_controller.go:	"github.com/go-logr/logr"
internal/controllers/configuration/secret_controller.go:	"github.com/go-logr/logr"
internal/controllers/configuration/zz_generated_controllers.go:	"github.com/go-logr/logr"
internal/controllers/controller.go:	"github.com/go-logr/logr"
internal/controllers/crds/dynamic_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/gateway_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/gateway_utils.go:	"github.com/go-logr/logr"
internal/controllers/gateway/gatewayclass_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/grpcroute_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/httproute_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/referencegrant_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/tcproute_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/tlsroute_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/udproute_controller.go:	"github.com/go-logr/logr"
internal/controllers/gateway/utils.go:	"github.com/go-logr/logr"
internal/controllers/knative/knative.go:	"github.com/go-logr/logr"
internal/controllers/reference/indexer.go:	"github.com/go-logr/logr"
internal/controllers/reference/indexer_test.go:	"github.com/go-logr/logr"
internal/dataplane/parser/translators/grpcroute_atc.go:	"github.com/go-logr/logr"
internal/dataplane/parser/translators/grpcroute_atc_test.go:	"github.com/go-logr/logr"
internal/dataplane/parser/translators/httproute_atc.go:	"github.com/go-logr/logr"
internal/dataplane/parser/translators/httproute_atc_test.go:	"github.com/go-logr/logr"
internal/dataplane/sendconfig/kong.go:	"github.com/go-logr/logr"
internal/dataplane/synchronizer.go:	"github.com/go-logr/logr"
internal/diagnostics/server.go:	"github.com/go-logr/logr"
internal/konnect/node_agent.go:	"github.com/go-logr/logr"
internal/konnect/node_agent_test.go:	"github.com/go-logr/logr"
internal/license/agent.go:	"github.com/go-logr/logr"
internal/license/agent_test.go:	"github.com/go-logr/logr"
internal/manager/featuregates/feature_gates.go:	"github.com/go-logr/logr"
internal/manager/health_check.go:	"github.com/go-logr/logr"
internal/manager/health_check_test.go:	"github.com/go-logr/logr"
internal/manager/run.go:	"github.com/go-logr/logr"
internal/manager/setup.go:	"github.com/go-logr/logr"
internal/manager/setup_test.go:	"github.com/go-logr/logr"
internal/manager/telemetry/manager_test.go:	"github.com/go-logr/logr"
internal/manager/utils/kongconfig/root.go:	"github.com/go-logr/logr"
internal/util/test/controller_manager.go:	"github.com/go-logr/logr"

logrusr

internal/dataplane/parser/translate_grpcroute.go:	"github.com/bombsimon/logrusr/v4"
internal/dataplane/parser/translate_httproute.go:	"github.com/bombsimon/logrusr/v4"
internal/dataplane/synchronizer.go:	"github.com/bombsimon/logrusr/v4"
internal/manager/featuregates/feature_gates_test.go:	"github.com/bombsimon/logrusr/v4"
internal/manager/setup.go:	"github.com/bombsimon/logrusr/v4"
internal/manager/telemetry/manager.go:	"github.com/bombsimon/logrusr/v4"

logrus

internal/admission/handler.go:	"github.com/sirupsen/logrus"
internal/admission/server.go:	"github.com/sirupsen/logrus"
internal/admission/server_test.go:	"github.com/sirupsen/logrus"
internal/admission/validation/ingress/ingress.go:	"github.com/sirupsen/logrus"
internal/admission/validator.go:	"github.com/sirupsen/logrus"
internal/admission/validator_test.go:	"github.com/sirupsen/logrus"
internal/clients/manager.go:	"github.com/sirupsen/logrus"
internal/clients/manager_test.go:	"github.com/sirupsen/logrus"
internal/cmd/rootcmd/run.go:	"github.com/sirupsen/logrus"
internal/dataplane/configfetcher/config_fetcher.go:	"github.com/sirupsen/logrus"
internal/dataplane/configfetcher/config_fetcher_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/deckgen/generate.go:	"github.com/sirupsen/logrus"
internal/dataplane/deckgen/generate_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/failures/failures.go:	"github.com/sirupsen/logrus"
internal/dataplane/failures/failures_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/failures/failures_test.go:	"github.com/sirupsen/logrus/hooks/test"
internal/dataplane/kong_client.go:	"github.com/sirupsen/logrus"
internal/dataplane/kong_client_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/kongstate.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/kongstate_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/route.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/route_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/service.go:	"github.com/sirupsen/logrus"
internal/dataplane/kongstate/service_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/backendref.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/golden_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/ingressclassparemeters_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/ingressrules.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/ingressrules_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/ingressrules_test.go:	"github.com/sirupsen/logrus/hooks/test"
internal/dataplane/parser/parser.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/parser_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/parser_test.go:	"github.com/sirupsen/logrus/hooks/test"
internal/dataplane/parser/translate_grpcroute_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/translate_httproute_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/translate_tcproute_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/translate_udproute_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/parser/translate_utils.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/backoff_strategy.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/backoff_strategy_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/config_change_detector.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/config_change_detector_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/error_handling_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/inmemory.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/inmemory_error_handling.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/sendconfig.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/strategy.go:	"github.com/sirupsen/logrus"
internal/dataplane/sendconfig/strategy_test.go:	"github.com/sirupsen/logrus"
internal/dataplane/synchronizer.go:	"github.com/sirupsen/logrus"
internal/dataplane/synchronizer_test.go:	"github.com/sirupsen/logrus"
internal/manager/featuregates/feature_gates_test.go:	"github.com/sirupsen/logrus"
internal/manager/run.go:	"github.com/sirupsen/logrus"
internal/manager/setup.go:	"github.com/sirupsen/logrus"
internal/manager/telemetry/manager.go:	"github.com/sirupsen/logrus"
internal/store/fake_store.go:	"github.com/sirupsen/logrus"
internal/store/store.go:	"github.com/sirupsen/logrus"
internal/util/debug_logging.go:	"github.com/sirupsen/logrus"
internal/util/debug_logging_test.go:	"github.com/sirupsen/logrus"
internal/util/logging.go:	"github.com/sirupsen/logrus"
internal/util/test/controller_manager.go:	"github.com/sirupsen/logrus"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment