From 0ceb2f97ad3f08753660fa8ce099ed66eebd37b2 Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Fri, 15 Dec 2023 12:13:54 -0500 Subject: [PATCH] cli: give up and suppress klog output by default (#6288) i don't think these logs have ever caught a real problem, and are often confusing / inscrutable. they're really meant for people writing k8s controllers, and i don't think they make sense to show to tilt users. Signed-off-by: Nick Santos --- internal/cli/klog.go | 64 ++++++++++++++++++--------------------- internal/cli/klog_test.go | 4 +-- internal/cli/up.go | 3 +- 3 files changed, 32 insertions(+), 39 deletions(-) diff --git a/internal/cli/klog.go b/internal/cli/klog.go index 82429ebc46..2c176e7864 100644 --- a/internal/cli/klog.go +++ b/internal/cli/klog.go @@ -6,9 +6,6 @@ import ( "io" "log" "os" - "regexp" - - "github.com/tilt-dev/tilt/internal/filteredwriter" "k8s.io/klog/v2" ) @@ -16,22 +13,6 @@ import ( // https://kubernetes.io/docs/reference/kubectl/cheatsheet/#kubectl-output-verbosity-and-debugging var klogLevel = 0 -// everything on google indicates this warning is useless and should be ignored -// https://github.com/kubernetes/kubernetes/issues/22024 -var isResourceVersionTooOldRegexp = regexp.MustCompile("reflector.go.*watch of.*ended with: too old resource version") - -// isGroupVersionEmptyRegexp matches errors that are logged deep within K8s when -// populating the cache of resource types for a group-version if the group-version -// has no resource types registered. This is an uncommon but valid scenario that -// most commonly occurs with prometheus-adapter and the `external.metrics.k8s.io/v1beta1` -// group if no external metrics are actually registered (but the group will still -// exist). The K8s code returns an error instead of treating it as empty, which gets -// logged at an error level so will show up in Tilt logs and leads to confusion, -// particularly on `tilt down` where they show up mixed in the CLI output. -// https://github.com/kubernetes/kubernetes/blob/a4e5239bdc3d85f1f90c7811b03dc153d5121ffe/staging/src/k8s.io/client-go/discovery/cached/memory/memcache.go#L212-L221 -// https://github.com/kubernetes/kubernetes/issues/92480 -var isGroupVersionEmptyRegexp = regexp.MustCompile("couldn't get resource list for.*Got empty response") - // HACK(nick): The Kubernetes libs we use sometimes use klog to log things to // os.Stderr. There are no API hooks to configure this. And printing to Stderr // scrambles the HUD tty. @@ -41,13 +22,7 @@ var isGroupVersionEmptyRegexp = regexp.MustCompile("couldn't get resource list f func initKlog(w io.Writer) { var tmpFlagSet = flag.NewFlagSet(os.Args[0], flag.ExitOnError) klog.InitFlags(tmpFlagSet) - if klogLevel == 0 { - w = filteredwriter.New(w, filterMux( - isResourceVersionTooOldRegexp.MatchString, - isGroupVersionEmptyRegexp.MatchString, - )) - } - klog.SetOutput(w) + maybeSetKlogOutput(w) flags := []string{ "--stderrthreshold=FATAL", @@ -64,14 +39,33 @@ func initKlog(w io.Writer) { } } -// filterMux combines multiple filter functions, returning true if any match. -func filterMux(filterFuncs ...func(s string) bool) func(s string) bool { - return func(s string) bool { - for _, fn := range filterFuncs { - if fn(s) { - return true - } - } - return false +// We've historically had a lot of problems with bad klog output. For example: +// +// everything on google indicates this warning is useless and should be ignored +// https://github.com/kubernetes/kubernetes/issues/22024 +// +// errors that are logged deep within K8s when populating the cache of resource +// types for a group-version if the group-version has no resource types +// registered. This is an uncommon but valid scenario that most commonly occurs +// with prometheus-adapter and the `external.metrics.k8s.io/v1beta1` group if no +// external metrics are actually registered (but the group will still +// exist). The K8s code returns an error instead of treating it as empty, which +// gets logged at an error level so will show up in Tilt logs and leads to +// confusion, particularly on `tilt down` where they show up mixed in the CLI +// output. +// https://github.com/kubernetes/kubernetes/blob/a4e5239bdc3d85f1f90c7811b03dc153d5121ffe/staging/src/k8s.io/client-go/discovery/cached/memory/memcache.go#L212-L221 +// https://github.com/kubernetes/kubernetes/issues/92480 +// +// informer errors when CRDs are removed +// https://github.com/kubernetes/kubernetes/issues/79610 +// +// We're not convinced that controller-runtime logs EVER make sense to show to +// tilt users, so let's filter them out by default. Users can turn them on +// with --klog if they want. +func maybeSetKlogOutput(w io.Writer) { + if klogLevel == 0 { + klog.SetOutput(io.Discard) + } else { + klog.SetOutput(w) } } diff --git a/internal/cli/klog_test.go b/internal/cli/klog_test.go index 55a3876ad8..7a447d3fa6 100644 --- a/internal/cli/klog_test.go +++ b/internal/cli/klog_test.go @@ -18,8 +18,8 @@ func TestResourceVersionTooOldWarningsSilenced(t *testing.T) { PrintWatchEndedWarning() klog.Flush() - assert.Contains(t, out.String(), "klog_test.go") - assert.Contains(t, out.String(), "watch ended") + assert.Contains(t, out.String(), "") + assert.Contains(t, out.String(), "") } func TestResourceVersionTooOldWarningsPrinted(t *testing.T) { diff --git a/internal/cli/up.go b/internal/cli/up.go index 07036146fb..4bc636aec0 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -13,7 +13,6 @@ import ( "github.com/mattn/go-isatty" "github.com/spf13/cobra" - "k8s.io/klog/v2" "github.com/tilt-dev/tilt/internal/analytics" engineanalytics "github.com/tilt-dev/tilt/internal/engine/analytics" @@ -177,7 +176,7 @@ func (c *upCmd) run(ctx context.Context, args []string) error { func redirectLogs(ctx context.Context, l logger.Logger) context.Context { ctx = logger.WithLogger(ctx, l) log.SetOutput(l.Writer(logger.InfoLvl)) - klog.SetOutput(l.Writer(logger.InfoLvl)) + maybeSetKlogOutput(l.Writer(logger.InfoLvl)) return ctx }