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

cli: give up and suppress klog output by default #6288

Merged
merged 1 commit into from
Dec 15, 2023
Merged

cli: give up and suppress klog output by default #6288

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 29 additions & 35 deletions internal/cli/klog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,13 @@ import (
"io"
"log"
"os"
"regexp"

"github.com/tilt-dev/tilt/internal/filteredwriter"

"k8s.io/klog/v2"
)

// 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.
Expand All @@ -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",
Expand All @@ -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)
}
}
4 changes: 2 additions & 2 deletions internal/cli/klog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions internal/cli/up.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down