kube-apiserver: fix missing global flags for --help #70204

imjching · 2018-10-24T20:51:17Z

What type of PR is this?
/kind bug

What this PR does / why we need it: In #64517, we introduced logical sections for the --help output in the kube-apiserver binary. It seems like it does not consider global flags that were registered through the flag package. This PR fixes that issue. We will output glog related and version flags in the "global" section and everything else in the "generic" section.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
See #70145.

Special notes for your reviewer: The original PR is in #70164. The plan to is to break it down into individual PRs.

Here's the simplified result of kube-apiserver --help. It now shows the appropriate global flags and generic flags that were missing.

Generic flags:

      --advertise-address ip
                The IP address on which to advertise the apiserver to members of the cluster. This address must be reachable by the rest of the cluster. If blank, the --bind-address will be used. If
                --bind-address is unspecified, the host's default interface will be used.
      --cloud-provider-gce-lb-src-cidrs cidrs
                CIDRs opened in GCE firewall for LB traffic proxy & health checks (default 130.211.0.0/22,209.85.152.0/22,209.85.204.0/22,35.191.0.0/16)
      --cors-allowed-origins strings
                List of allowed origins for CORS, comma separated.  An allowed origin can be a regular expression to support subdomain matching. If this list is empty CORS will not be enabled.
      --default-not-ready-toleration-seconds int
                Indicates the tolerationSeconds of the toleration for notReady:NoExecute that is added by default to every pod that does not already have such a toleration. (default 300)
      --default-unreachable-toleration-seconds int
                Indicates the tolerationSeconds of the toleration for unreachable:NoExecute that is added by default to every pod that does not already have such a toleration. (default 300)
      ...

...

Global flags:

      --alsologtostderr
                log to standard error as well as files
  -h, --help
                help for kube-apiserver
      --log-backtrace-at traceLocation
                when logging hits line file:N, emit a stack trace (default :0)
      --log-dir string
                If non-empty, write log files in this directory
      --log-file string
                If non-empty, use this log file
      --log-flush-frequency duration
                Maximum number of seconds between log flushes (default 5s)
      --logtostderr
                log to standard error instead of files (default true)
      --skip-headers
                If true, avoid header prefixes in the log messages
      --stderrthreshold severity
                logs at or above this threshold go to stderr (default 2)
  -v, --v Level
                log level for V logs
      --version version[=true]
                Print version information and quit
      --vmodule moduleSpec
                comma-separated list of pattern=N settings for file-filtered logging

Does this PR introduce a user-facing change?:

Fix missing flags in kube-apiserver --help.

/sig cli
/cc @stewart-yu

imjching · 2018-10-24T20:53:34Z

/assign @nikhiljindal
/cc @dims @sttts

dims · 2018-10-24T21:08:40Z

/ok-to-test

imjching · 2018-10-24T21:20:50Z

Thank you! Hmm, looks like there are some dependency issues. Will look into them.

imjching · 2018-10-24T21:50:00Z

/retest. This should work now.

yue9944882 · 2018-10-25T10:11:28Z

cmd/kube-apiserver/app/options/globalflags.go

+
+// normalize replaces underscores with hyphens. We should always use hyphens
+// instead of underscores when registering kube-apiserver flags.
+func normalize(s string) string {


it should belong to another pull i suppose

yue9944882 · 2018-10-25T10:12:46Z

cmd/kube-apiserver/app/options/globalflags.go

+	register(global, local, "stderrthreshold")
+	register(global, local, "vmodule")
+	register(global, local, "log_backtrace_at")
+	register(global, local, "log_dir")


do we have to register these by hardcoding their name here?

I couldn't think of a better way since the flags are initialized the moment the glog package is used:

kubernetes/vendor/github.com/golang/glog/glog.go

Lines 399 to 404 in 5336866

flag.BoolVar(&logging.toStderr, "logtostderr", false, "log to standard error instead of files")

flag.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files")

flag.Var(&logging.verbosity, "v", "log level for V logs")

flag.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")

flag.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")

flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")

We're doing the same thing in #68054 too. Previously without the named sections, we added everything from the global flag set:

kubernetes/cmd/kube-apiserver/apiserver.go

Line 47 in 5336866

pflag.CommandLine.AddGoFlagSet(goflag.CommandLine)

With the named logical sections, we will need to extract out relevant flags so that we could group them.

yue9944882 · 2018-10-25T10:13:27Z

cmd/kube-apiserver/app/options/globalflags.go

+		pflagFlag.Name = normalize(pflagFlag.Name)
+		local.AddFlag(pflagFlag)
+	} else {
+		panic(fmt.Sprintf("failed to find flag in global flagset (flag): %s", globalName))


dont panic, return an error instead

yue9944882

scattering multiple local functions makes it hard to read. lumping them into one is fine. also, plz keep a minimal change and put unrelated changes into another pull if u want.

jennybuckley · 2018-10-25T20:20:38Z

cc @logicalhan

imjching · 2018-10-26T03:09:47Z

/hold

#68054 is trying to add in globalflags.go into apiserver/pkg/util/flag/. Will wait until that is merged in so that we could rebase and use methods from there.

dims · 2018-11-11T01:04:38Z

/milestone v1.13

AishSundar · 2018-11-19T05:52:02Z

@dims can you (or add anyone else who can) please add the approval label for this, #70205 and #70216

sttts · 2018-11-19T10:35:18Z

cmd/kube-apiserver/app/options/customflags.go

+// 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 {
+		local.AddGoFlag(f)


why is the logic different from a77652e#diff-2fd8f885c4e1d904b7d678e5991aade4R68 ?

@sttts @imjching @stewart-yu we need both to match.

@sttts @dims Originally I removed the normalize part because we don't need it. It was there to ensure that all globally registered flags that use underscores will be normalized to use hyphens instead.

klog registers flags using underscores:
https://github.com/dims/klog/blob/master/klog.go#L411-L419

Those cloud provider and admission packages register flags using hyphens instead. I have updated the code and added the normalize part back. Do we actually need both files to match? The commit that was provided above was for klog flags and therefore we need to normalize those flags.

Also, from documentation, the two snippets of code below are equivalent:

pflagFlag := pflag.PFlagFromGoFlag(f) local.AddFlag(pflagFlag)

local.AddGoFlag(f)

@imjching thanks!
@sttts your call :)

cc @stewart-yu

seems below enough in here @dims

f.Name = normalize(f.Name) local.AddFlag(f)

but add

pflagFlag := pflag.PFlagFromGoFlag(f)

i'm not obey also, but seems no need for component flag now, may be need for future

sttts · 2018-11-19T10:39:03Z

cmd/kube-apiserver/app/options/customflags.go

+	global := flag.CommandLine
+	local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+	register(global, local, "default-not-ready-toleration-seconds")


how do we know we did not forget flags? We need a test for that, or if that is tricky due to globals in tests, we should at least have a check with a glog.Error here.

I feel that doing a panic here rather than returning an error would be better since it would alert us that we failed to allow libraries to globally register their flags. What do you think? @yue9944882

@imjching iiuc, assuming any flag required by register here got deprecated in the vendored dependency, then we would be surprised w/ a panic, amiright? if u notice, we seldom do panic thru the codebase, which imho is neither user-friendly nor developer-friendly.

while i see we've been already doing that in controller, so...

@sttts @yue9944882 Updated the patch by adding in tests. (It's similar to the other one that was merged earlier.) Are those sufficient?

stewart-yu · 2018-11-20T02:21:40Z

cmd/kube-apiserver/app/options/customflags.go

+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)


Why add Line72? no need

Do refer to the discussion above with @sttts and @dims. I have provided my reasoning to remove it, but it seems like we need both files to match in terms of their logic, so I'm not too sure what are their thoughts.

@imjching i am ok with "remove it" with a tracking issue (or PR) for the other piece of code for 1.14

@dims Sorry, I'm not quite sure if I understand you. If we were to remove Line 72 temporarily, i.e. pflagFlag := pflag.PFlagFromGoFlag(f), what should the issue track and what does the "other piece of code" refer to?

stewart-yu · 2018-11-20T02:38:42Z

add test look much better now
Just as summary：

if we no need Line72 logic, copy pflagRegister() in cmd/kubelet to
staging/src/k8s.io/apiserver/pkg/util/globalflag/globalflags.go ; if need, make register in 'staging/src/k8s.io/apiserver/pkg/util/globalflag/globalflags.go' public, so we can reuse it in KCM, CCM and kube-apiserver

2、combine addCloudProviderFlags() and addCloudProviderFlags into AddGenericFlags(), like that:

func AddGenericFlags(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)
       
       //adds flags from k8s.io/kubernetes/pkg/cloudprovider/providers.
 	globalflag.FlagRegister(global, local, "cloud-provider-gce-lb-src-cidrs")

       //adds flags from k8s.io/apiserver/pkg/admission.
        globalflag.FlagRegister(global, local, "default-not-ready-toleration-seconds")
	globalflag.FlagRegister(global, local, "default-unreachable-toleration-seconds")

 	fs.AddFlagSet(local)
}

WDYT? @imjching

imjching · 2018-11-20T13:47:02Z

Thank you for your prompt review, @stewart-yu! Here are my thoughts for your summary:

Unfortunately, I don't think pflagRegister in cmd/kubelet/app/options/globalflags.go will work. In that function, global has type *pflag.FlagSet, but the global that we have (i.e. flag.CommandLine) has type *flag.FlagSet. We need to grab our global from flag.CommandLine and not pflag.CommandLine because we registered our flags under flag, i.e.

kubernetes/plugin/pkg/admission/defaulttolerationseconds/admission.go

Lines 34 to 40 in 50e02fd

    
           defaultNotReadyTolerationSeconds = flag.Int64("default-not-ready-toleration-seconds", 300, 
        
           	"Indicates the tolerationSeconds of the toleration for notReady:NoExecute"+ 
        
           		" that is added by default to every pod that does not already have such a toleration.") 
        
           defaultUnreachableTolerationSeconds = flag.Int64("default-unreachable-toleration-seconds", 300, 
        
           	"Indicates the tolerationSeconds of the toleration for unreachable:NoExecute"+ 
        
           		" that is added by default to every pod that does not already have such a toleration.")

which is very different from

kubernetes/pkg/credentialprovider/azure/azure_credentials.go

Lines 38 to 39 in 50e02fd

    
           var flagConfigFile = pflag.String("azure-container-registry-config", "", 
        
           	"Path to the file containing Azure container registry configuration information.")

Also, regarding the changes to make register public, I made that function public back then in one of the 3 patches that I have, but I was told that it's easier to just copy the function into customflags.go.

Merging addCloudProviderFlags and addAdmissionFlags into AddGenericFlags sounds good to me.

Would like to hear thoughts from everyone else as well 😄

Signed-off-by: Jay Lim <[email protected]>

nikopen · 2018-11-21T18:01:21Z

@imjching I see the other two related PRs got closed but this is still going in, are the other two dropped for a later time (1.14) ?

imjching · 2018-11-21T18:36:21Z

/test pull-kubernetes-integration

@nikopen The other two got combined in #71298 and was already merged. This should still go in.

imjching · 2018-11-21T19:11:01Z

/assign @sttts
Can you take a look at it again? It should be good now.

stewart-yu · 2018-11-22T00:43:45Z

/lgtm

sttts · 2018-11-22T13:10:13Z

/approve

k8s-ci-robot · 2018-11-22T13:12:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imjching, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kube-apiserver/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imjching · 2018-11-22T13:29:42Z

/test pull-kubernetes-verify

imjching · 2018-11-22T15:41:27Z

/test pull-kubernetes-integration

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 24, 2018

k8s-ci-robot requested a review from stewart-yu October 24, 2018 20:51

k8s-ci-robot assigned nikhiljindal Oct 24, 2018

k8s-ci-robot requested review from dims and sttts October 24, 2018 20:53

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 24, 2018

imjching force-pushed the 70145-fix-glog-flags-apiserver branch 3 times, most recently from d6e087d to 83c25d9 Compare October 24, 2018 21:48

imjching mentioned this pull request Oct 25, 2018

Fix missing glog flags for some commands which output flags in logical sections #70164

Closed

yue9944882 reviewed Oct 25, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2018

k8s-ci-robot added this to the v1.13 milestone Nov 11, 2018

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 19, 2018

sttts reviewed Nov 19, 2018

View reviewed changes

imjching force-pushed the 70145-fix-glog-flags-apiserver branch from 1d7d26e to 85b794e Compare November 20, 2018 02:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2018

stewart-yu reviewed Nov 20, 2018

View reviewed changes

stewart-yu mentioned this pull request Nov 21, 2018

add missing flags in *-controller-manager --help #71298

Merged

imjching force-pushed the 70145-fix-glog-flags-apiserver branch from 85b794e to 103066c Compare November 21, 2018 17:27

kube-apiserver: fix missing global flags for --help

7fbdcf8

Signed-off-by: Jay Lim <[email protected]>

imjching force-pushed the 70145-fix-glog-flags-apiserver branch from 103066c to 7fbdcf8 Compare November 21, 2018 17:30

k8s-ci-robot assigned sttts Nov 21, 2018

k8s-ci-robot assigned stewart-yu Nov 22, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2018

k8s-ci-robot merged commit 69f100e into kubernetes:master Nov 22, 2018

imjching deleted the 70145-fix-glog-flags-apiserver branch November 22, 2018 16:50

imjching mentioned this pull request Nov 22, 2018

Some commands do not show glog flags #70145

Closed

	flag.BoolVar(&logging.toStderr, "logtostderr", false, "log to standard error instead of files")
	flag.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files")
	flag.Var(&logging.verbosity, "v", "log level for V logs")
	flag.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")
	flag.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
	flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")

kube-apiserver: fix missing global flags for --help #70204

kube-apiserver: fix missing global flags for --help #70204

Conversation

imjching commented Oct 24, 2018 • edited Loading

imjching commented Oct 24, 2018

dims commented Oct 24, 2018

imjching commented Oct 24, 2018

imjching commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue9944882 left a comment • edited Loading

Choose a reason for hiding this comment

jennybuckley commented Oct 25, 2018

imjching commented Oct 26, 2018

dims commented Nov 11, 2018

AishSundar commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stewart-yu Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stewart-yu Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stewart-yu commented Nov 20, 2018 • edited Loading

imjching commented Nov 20, 2018

nikopen commented Nov 21, 2018

imjching commented Nov 21, 2018

imjching commented Nov 21, 2018

stewart-yu commented Nov 22, 2018

sttts commented Nov 22, 2018

k8s-ci-robot commented Nov 22, 2018

imjching commented Nov 22, 2018

imjching commented Nov 22, 2018

imjching commented Oct 24, 2018 •

edited

Loading

yue9944882 left a comment •

edited

Loading

stewart-yu Nov 20, 2018 •

edited

Loading

stewart-yu Nov 20, 2018 •

edited

Loading

stewart-yu commented Nov 20, 2018 •

edited

Loading