Merge pull request #68054 from stewart-yu/stewart-controller-manager-codeclean#02

kube-scheduler: output flags in logical sections

cmd/kube-scheduler/app/BUILD

@@ -39,6 +39,8 @@ go_library(
         "//staging/src/k8s.io/apiserver/pkg/server/mux:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/server/routes:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/flag:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/globalflag:go_default_library",
         "//staging/src/k8s.io/client-go/informers/storage/v1:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/typed/core/v1:go_default_library",
         "//staging/src/k8s.io/client-go/tools/leaderelection:go_default_library",

cmd/kube-scheduler/app/options/BUILD

@@ -29,6 +29,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/server/options:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/flag:go_default_library",
         "//staging/src/k8s.io/client-go/informers:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/typed/core/v1:go_default_library",

cmd/kube-scheduler/app/options/options.go

@@ -23,15 +23,13 @@ import (
 	"strconv"
 	"time"
 
-	"github.com/spf13/pflag"
-	"k8s.io/klog"
-
 	corev1 "k8s.io/api/core/v1"
 	apimachineryconfig "k8s.io/apimachinery/pkg/apis/config"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	apiserveroptions "k8s.io/apiserver/pkg/server/options"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	apiserverflag "k8s.io/apiserver/pkg/util/flag"
 	"k8s.io/client-go/informers"
 	clientset "k8s.io/client-go/kubernetes"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -41,6 +39,7 @@ import (
 	"k8s.io/client-go/tools/leaderelection"
 	"k8s.io/client-go/tools/leaderelection/resourcelock"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/klog"
 	kubeschedulerconfigv1alpha1 "k8s.io/kube-scheduler/config/v1alpha1"
 	schedulerappconfig "k8s.io/kubernetes/cmd/kube-scheduler/app/config"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
@@ -139,20 +138,23 @@ func newDefaultComponentConfig() (*kubeschedulerconfig.KubeSchedulerConfiguratio
 	return &cfg, nil
 }
 
-// AddFlags adds flags for the scheduler options.
-func (o *Options) AddFlags(fs *pflag.FlagSet) {
+// Flags returns flags for a specific scheduler by section name
+func (o *Options) Flags() (nfs apiserverflag.NamedFlagSets) {
+	fs := nfs.FlagSet("misc")
 	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, "The path to the configuration file. Flags override values in this file.")
 	fs.StringVar(&o.WriteConfigTo, "write-config-to", o.WriteConfigTo, "If set, write the configuration values to this file and exit.")
 	fs.StringVar(&o.Master, "master", o.Master, "The address of the Kubernetes API server (overrides any value in kubeconfig)")
 
-	o.SecureServing.AddFlags(fs)
-	o.CombinedInsecureServing.AddFlags(fs)
-	o.Authentication.AddFlags(fs)
-	o.Authorization.AddFlags(fs)
-	o.Deprecated.AddFlags(fs, &o.ComponentConfig)
+	o.SecureServing.AddFlags(nfs.FlagSet("secure serving"))
+	o.CombinedInsecureServing.AddFlags(nfs.FlagSet("insecure serving"))
+	o.Authentication.AddFlags(nfs.FlagSet("authentication"))
+	o.Authorization.AddFlags(nfs.FlagSet("authorization"))
+	o.Deprecated.AddFlags(nfs.FlagSet("deprecated"), &o.ComponentConfig)
 
-	leaderelectionconfig.BindFlags(&o.ComponentConfig.LeaderElection.LeaderElectionConfiguration, fs)
-	utilfeature.DefaultFeatureGate.AddFlag(fs)
+	leaderelectionconfig.BindFlags(&o.ComponentConfig.LeaderElection.LeaderElectionConfiguration, nfs.FlagSet("leader election"))
+	utilfeature.DefaultFeatureGate.AddFlag(nfs.FlagSet("feature gate"))
+
+	return nfs
 }
 
 // ApplyTo applies the scheduler options to the given scheduler app configuration.

cmd/kube-scheduler/app/server.go

@@ -39,6 +39,8 @@ import (
 	"k8s.io/apiserver/pkg/server/mux"
 	"k8s.io/apiserver/pkg/server/routes"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	apiserverflag "k8s.io/apiserver/pkg/util/flag"
+	"k8s.io/apiserver/pkg/util/globalflag"
 	storageinformers "k8s.io/client-go/informers/storage/v1"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/leaderelection"
@@ -87,8 +89,25 @@ through the API as necessary.`,
 			}
 		},
 	}
+	fs := cmd.Flags()
+	namedFlagSets := opts.Flags()
+	verflag.AddFlags(namedFlagSets.FlagSet("global"))
+	globalflag.AddGlobalFlags(namedFlagSets.FlagSet("global"), cmd.Name())
+	for _, f := range namedFlagSets.FlagSets {
+		fs.AddFlagSet(f)
+	}
 
-	opts.AddFlags(cmd.Flags())
+	usageFmt := "Usage:\n  %s\n"
+	cols, _, _ := apiserverflag.TerminalSize(cmd.OutOrStdout())
+	cmd.SetUsageFunc(func(cmd *cobra.Command) error {
+		fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine())
+		apiserverflag.PrintSections(cmd.OutOrStderr(), namedFlagSets, cols)
+		return nil
+	})
+	cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) {
+		fmt.Fprintf(cmd.OutOrStdout(), "%s\n\n"+usageFmt, cmd.Long, cmd.UseLine())
+		apiserverflag.PrintSections(cmd.OutOrStdout(), namedFlagSets, cols)
+	})
 	cmd.MarkFlagFilename("config", "yaml", "yml", "json")
 
 	return cmd

cmd/kube-scheduler/app/testing/testserver.go

@@ -86,7 +86,10 @@ func StartTestServer(t Logger, customFlags []string) (result TestServer, err err
 	if err != nil {
 		return TestServer{}, err
 	}
-	s.AddFlags(fs)
+	namedFlagSets := s.Flags()
+	for _, f := range namedFlagSets.FlagSets {
+		fs.AddFlagSet(f)
+	}
 
 	fs.Parse(customFlags)
 

cmd/kube-scheduler/scheduler.go

@@ -17,13 +17,13 @@ limitations under the License.
 package main
 
 import (
-	goflag "flag"
 	"fmt"
 	"math/rand"
 	"os"
 	"time"
 
 	"github.com/spf13/pflag"
+
 	utilflag "k8s.io/apiserver/pkg/util/flag"
 	"k8s.io/apiserver/pkg/util/logs"
 	"k8s.io/kubernetes/cmd/kube-scheduler/app"
@@ -40,7 +40,6 @@ func main() {
 	// utilflag.InitFlags() (by removing its pflag.Parse() call). For now, we have to set the
 	// normalize func and add the go flag set by hand.
 	pflag.CommandLine.SetNormalizeFunc(utilflag.WordSepNormalizeFunc)
-	pflag.CommandLine.AddGoFlagSet(goflag.CommandLine)
 	// utilflag.InitFlags()
 	logs.InitLogs()
 	defer logs.FlushLogs()

staging/src/BUILD

@@ -102,6 +102,7 @@ filegroup(
         "//staging/src/k8s.io/apiserver/pkg/util/feature:all-srcs",
         "//staging/src/k8s.io/apiserver/pkg/util/flag:all-srcs",
         "//staging/src/k8s.io/apiserver/pkg/util/flushwriter:all-srcs",
+        "//staging/src/k8s.io/apiserver/pkg/util/globalflag:all-srcs",
         "//staging/src/k8s.io/apiserver/pkg/util/logs:all-srcs",
         "//staging/src/k8s.io/apiserver/pkg/util/openapi:all-srcs",
         "//staging/src/k8s.io/apiserver/pkg/util/proxy:all-srcs",

staging/src/k8s.io/apiserver/pkg/util/globalflag/BUILD

@@ -0,0 +1,37 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["globalflags.go"],
+    importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/globalflag",
+    importpath = "k8s.io/apiserver/pkg/util/globalflag",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//staging/src/k8s.io/apiserver/pkg/util/logs:go_default_library",
+        "//vendor/github.com/spf13/pflag:go_default_library",
+    ],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["globalflags_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//staging/src/k8s.io/apiserver/pkg/util/flag:go_default_library",
+        "//vendor/github.com/spf13/pflag:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)

staging/src/k8s.io/apiserver/pkg/util/globalflag/globalflags.go

@@ -0,0 +1,74 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package globalflag
+
+import (
+	"flag"
+	"fmt"
+	"os"
+	"strings"
+
+	"github.com/spf13/pflag"
+
+	"k8s.io/apiserver/pkg/util/logs"
+)
+
+// AddGlobalFlags explicitly registers flags that libraries (klog, verflag, etc.) register
+// against the global flagsets from "flag" and "github.com/spf13/pflag".
+// We do this in order to prevent unwanted flags from leaking into the component's flagset.
+func AddGlobalFlags(fs *pflag.FlagSet, name string) {
+	addGlogFlags(fs)
+	logs.AddFlags(fs)
+
+	fs.BoolP("help", "h", false, fmt.Sprintf("help for %s", name))
+}
+
+// addGlogFlags explicitly registers flags that klog libraries(k8s.io/klog) register.
+func addGlogFlags(fs *pflag.FlagSet) {
+	// lookup flags in global flag set and re-register the values with our flagset
+	global := flag.CommandLine
+	local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+	register(global, local, "logtostderr")
+	register(global, local, "alsologtostderr")
+	register(global, local, "v")
+	register(global, local, "skip_headers")
+	register(global, local, "stderrthreshold")
+	register(global, local, "vmodule")
+	register(global, local, "log_backtrace_at")
+	register(global, local, "log_dir")
+	register(global, local, "log_file")
+
+	fs.AddFlagSet(local)
+}
+
+// normalize replaces underscores with hyphens
+// we should always use hyphens instead of underscores when registering component flags
+func normalize(s string) string {
+	return strings.Replace(s, "_", "-", -1)
+}
+
+// register adds a flag to local that targets the Value associated with the Flag named globalName in global
+func register(global *flag.FlagSet, local *pflag.FlagSet, globalName string) {
+	if f := global.Lookup(globalName); f != nil {
+		pflagFlag := pflag.PFlagFromGoFlag(f)
+		pflagFlag.Name = normalize(pflagFlag.Name)
+		local.AddFlag(pflagFlag)
+	} else {
+		panic(fmt.Sprintf("failed to find flag in global flagset (flag): %s", globalName))
+	}
+}

staging/src/k8s.io/apiserver/pkg/util/globalflag/globalflags_test.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package globalflag
+
+import (
+	"flag"
+	"reflect"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/spf13/pflag"
+
+	apiserverflag "k8s.io/apiserver/pkg/util/flag"
+)
+
+func TestAddGlobalFlags(t *testing.T) {
+	namedFlagSets := &apiserverflag.NamedFlagSets{}
+	nfs := namedFlagSets.FlagSet("global")
+	AddGlobalFlags(nfs, "test-cmd")
+
+	actualFlag := []string{}
+	nfs.VisitAll(func(flag *pflag.Flag) {
+		actualFlag = append(actualFlag, flag.Name)
+	})
+
+	// Get all flags from flags.CommandLine, except flag `test.*`.
+	wantedFlag := []string{"help"}
+	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+	pflag.VisitAll(func(flag *pflag.Flag) {
+		if !strings.Contains(flag.Name, "test.") {
+			wantedFlag = append(wantedFlag, normalize(flag.Name))
+		}
+	})
+	sort.Strings(wantedFlag)
+
+	if !reflect.DeepEqual(wantedFlag, actualFlag) {
+		t.Errorf("[Default]: expected %+v, got %+v", wantedFlag, actualFlag)
+	}
+
+	tests := []struct {
+		expectedFlag  []string
+		matchExpected bool
+	}{
+		{
+			// Happy case
+			expectedFlag:  []string{"alsologtostderr", "help", "log-backtrace-at", "log-dir", "log-file", "log-flush-frequency", "logtostderr", "skip-headers", "stderrthreshold", "v", "vmodule"},
+			matchExpected: false,
+		},
+		{
+			// Missing flag
+			expectedFlag:  []string{"logtostderr", "log-dir"},
+			matchExpected: true,
+		},
+		{
+			// Empty flag
+			expectedFlag:  []string{},
+			matchExpected: true,
+		},
+		{
+			// Invalid flag
+			expectedFlag:  []string{"foo"},
+			matchExpected: true,
+		},
+	}
+
+	for i, test := range tests {
+		if reflect.DeepEqual(test.expectedFlag, actualFlag) == test.matchExpected {
+			t.Errorf("[%d]: expected %+v, got %+v", i, test.expectedFlag, actualFlag)
+		}
+	}
+}