From 1957fb6508c4dbf629f13c5bbf5e36b6bffa6a61 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 29 Sep 2021 08:45:43 +0200 Subject: [PATCH] command lines: harmonize command line parse error handling The recommendation from #sig-cli was to print usage, then the error. Extra care is taken to only print the usage instruction when the error really was about flag parsing. Taking kube-scheduler as example: $ _output/bin/kube-scheduler I0929 09:42:42.289039 149029 serving.go:348] Generated self-signed cert in-memory ... W0929 09:42:42.489255 149029 client_config.go:620] error creating inClusterConfig, falling back to default config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined E0929 09:42:42.489366 149029 run.go:98] "command failed" err="invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable" $ _output/bin/kube-scheduler --xxx Usage: kube-scheduler [flags] ... --vmodule moduleSpec comma-separated list of pattern=N settings for file-filtered logging Error: unknown flag: --xxx The kubectl behavior doesn't change: $ _output/bin/kubectl get nodes Unable to connect to the server: dial tcp: lookup xxxx: No address associated with hostname $ _output/bin/kubectl --xxx Error: unknown flag: --xxx See 'kubectl --help' for usage. --- cmd/kube-scheduler/app/server.go | 2 - .../cloud-provider/app/testing/testserver.go | 1 + staging/src/k8s.io/component-base/cli/run.go | 62 ++++++++++++++++--- test/cmd/get.sh | 2 +- 4 files changed, 56 insertions(+), 11 deletions(-) diff --git a/cmd/kube-scheduler/app/server.go b/cmd/kube-scheduler/app/server.go index 1bda9d0e579..004d0f20e9c 100644 --- a/cmd/kube-scheduler/app/server.go +++ b/cmd/kube-scheduler/app/server.go @@ -88,8 +88,6 @@ for more information about scheduling and the kube-scheduler component.`, } return nil }, - SilenceErrors: true, - SilenceUsage: true, Args: func(cmd *cobra.Command, args []string) error { for _, arg := range args { if len(arg) > 0 { diff --git a/staging/src/k8s.io/cloud-provider/app/testing/testserver.go b/staging/src/k8s.io/cloud-provider/app/testing/testserver.go index 16181464ddb..bd7a7b2c8ee 100644 --- a/staging/src/k8s.io/cloud-provider/app/testing/testserver.go +++ b/staging/src/k8s.io/cloud-provider/app/testing/testserver.go @@ -32,6 +32,7 @@ import ( "k8s.io/cloud-provider/app" "k8s.io/cloud-provider/app/config" "k8s.io/cloud-provider/options" + cliflag "k8s.io/component-base/cli/flag" ) // TearDownFunc is to be called to tear down a test server. diff --git a/staging/src/k8s.io/component-base/cli/run.go b/staging/src/k8s.io/component-base/cli/run.go index 8456b178a68..cc2c977e386 100644 --- a/staging/src/k8s.io/component-base/cli/run.go +++ b/staging/src/k8s.io/component-base/cli/run.go @@ -24,8 +24,9 @@ import ( "github.com/spf13/cobra" - "k8s.io/component-base/logs" cliflag "k8s.io/component-base/cli/flag" + "k8s.io/component-base/logs" + "k8s.io/klog/v2" ) // Run provides the common boilerplate code around executing a cobra command. @@ -39,6 +40,44 @@ func Run(cmd *cobra.Command) int { cmd.SetGlobalNormalizationFunc(cliflag.WordSepNormalizeFunc) + // When error printing is enabled for the Cobra command, a flag parse + // error gets printed first, then optionally the often long usage + // text. This is very unreadable in a console because the last few + // lines that will be visible on screen don't include the error. + // + // The recommendation from #sig-cli was to print the usage text, then + // the error. We implement this consistently for all commands here. + // However, we don't want to print the usage text when command + // execution fails for other reasons than parsing. We detect this via + // the FlagParseError callback. + // + // A variable is used instead of wrapping the error with a special + // error type because the variable is simpler and less fragile: the + // original FlagErrorFunc might replace the wrapped error. + parsingFailed := false + if cmd.SilenceUsage { + // Some commands, like kubectl, already deal with this themselves. + // We don't change the behavior for those and just track whether + // parsing failed for the error output below. + flagErrorFunc := cmd.FlagErrorFunc() + cmd.SetFlagErrorFunc(func(c *cobra.Command, err error) error { + parsingFailed = true + return flagErrorFunc(c, err) + }) + } else { + cmd.SilenceUsage = true + cmd.SetFlagErrorFunc(func(c *cobra.Command, err error) error { + parsingFailed = true + + // Re-enable usage printing. + c.SilenceUsage = false + return err + }) + } + + // In all cases error printing is done below. + cmd.SilenceErrors = true + // This is idempotent. logs.AddFlags(cmd.PersistentFlags()) @@ -64,15 +103,22 @@ func Run(cmd *cobra.Command) int { } if err := cmd.Execute(); err != nil { - // The error may be about the command line ("unknown shorthand - // flag: 'e' in -elp" in kube-scheduler). Printing that without - // any decoration like the one that klog would add ("E0923 + // If the error is about flag parsing, then printing that error + // with the decoration that klog would add ("E0923 // 23:02:03.219216 4168816 run.go:61] unknown shorthand flag") - // is more readable. + // is less readable. Using klog.Fatal is even worse because it + // dumps a stack trace that isn't about the error. // - // We also don't know in which state logging (configuration may - // have failed) is whereas os.Stderr always should be usable. - fmt.Fprintf(os.Stderr, "%v\n", err) + // But if it is some other error encountered at runtime, then + // we want to log it as error. + // + // We can distinguish these two cases depending on whether + // our FlagErrorFunc above was called. + if parsingFailed { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + } else { + klog.ErrorS(err, "command failed") + } return 1 } return 0 diff --git a/test/cmd/get.sh b/test/cmd/get.sh index d9b9107c80f..7f7ceb0c611 100755 --- a/test/cmd/get.sh +++ b/test/cmd/get.sh @@ -421,7 +421,7 @@ run_deprecated_api_tests() { kube::test::if_has_string "${output_message}" 'PodSecurityPolicy is deprecated' output_message=$(! kubectl get podsecuritypolicies.v1beta1.policy --warnings-as-errors 2>&1 "${kube_flags[@]}") kube::test::if_has_string "${output_message}" 'PodSecurityPolicy is deprecated' - kube::test::if_has_string "${output_message}" 'Error: 1 warning received' + kube::test::if_has_string "${output_message}" 'err="1 warning received"' set +o nounset set +o errexit