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

@@ -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 {

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.

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

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