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

imjching · 2018-10-24T03:21:37Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Some binaries (e.g. kube-apiserver, kube-controller-manager, cloud-controller-manager) tend to output flags in logical sections. This feature was introduced in #64517 and #67362. It seems like it does not consider glog-related flags. This PR includes these glog flags in the named section "misc".

The glog related flags are initialized in the init function (when we include the glog package).

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

Lines 399 to 404 in 0d17976

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

Which issue(s) this PR fixes:
See #70145.

Special notes for your reviewer: I am not quite sure if we're planning to switch everything to logical sections. If yes, we might be able to abstract this so that we don't need to manually add glog commands in the misc section.

Does this PR introduce a user-facing change?:

Fix missing glog flags in kube-apiserver --help and *-controller-manager --help.

/sig cli
cc @sttts @stewart-yu

stewart-yu

I see that bug before, thanks for your PR 👍

have you try in your local Env?
more please refer to ``kube-scheduler kube-scheduler: output flags in logical sections #68054

BTW, I recommend split this PR into three part

stewart-yu · 2018-10-24T03:58:48Z

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

+		Authorization:        kubeoptions.NewBuiltInAuthorizationOptions(),
+		CloudProvider:        kubeoptions.NewCloudProviderOptions(),
+		StorageSerialization: kubeoptions.NewStorageSerializationOptions(),
+		APIEnablement:        genericoptions.NewAPIEnablementOptions(),



why change in here?

Just indentation issues: aligning those variables.

stewart-yu · 2018-10-24T04:01:26Z

may be some bazel failed, but go head
/ok-to-test

imjching · 2018-10-24T04:19:54Z

@stewart-yu Yes, I have tried it in my local environment. It shows up right now.

e.g. ./_output/bin/cloud-controller-manager --help

That works too. Let's wait for the tests. If all is good, I will split the PR into 3 parts tomorrow.

imjching · 2018-10-24T04:34:14Z

@stewart-yu I made a comment in your other PR.

It seems like with your other patch, we can just do the following

options.AddGlobalFlags(namedFlagSets.FlagSet("global"))

Ref: https://github.com/kubernetes/kubernetes/pull/68054/files#diff-f072139971e3329bef6da077a5a03aceR121

I guess we can merge this PR first (without splitting), wait until your other patch lands, and finally use the AddGlobalFlags function that you have just added? What are your thoughts on this?

Signed-off-by: Jay Lim <jay@imjching.com>

imjching · 2018-10-24T04:49:17Z

Okay, looks like there were issues with formatting. I just updated the commit. Will need to retest.

stewart-yu · 2018-10-24T08:10:38Z

Golog flag seems not belong to Misc part

imjching · 2018-10-25T03:56:39Z

/hold
I have broken this PR down into three individual PRs and placed glog flags in the "global" section. (See: #70204, #70205, and #70216)
cc @stewart-yu

sttts · 2018-10-25T08:05:41Z

/lgtm
/approve

k8s-ci-robot · 2018-10-25T08:05:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjching, sttts
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp

If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.

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:

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

jennybuckley · 2018-10-25T20:18:21Z

cc @cheftako @logicalhan

cheftako · 2018-10-26T17:28:51Z

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

@@ -235,5 +236,7 @@ func (s *ServerRunOptions) Flags() (fss apiserverflag.NamedFlagSets) {
 	fs.StringVar(&s.ServiceAccountSigningKeyFile, "service-account-signing-key-file", s.ServiceAccountSigningKeyFile, ""+
 		"Path to the file that contains the current private key of the service account token issuer. The issuer will sign issued ID tokens with this private key. (Requires the 'TokenRequest' feature gate.)")

+	fs.AddGoFlagSet(goflag.CommandLine)


This seems a bit redundant as we are already doing it https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/apiserver.go#L47 for the stand-alone KAS binary.

In #64517, we introduced logical sections, and with that, we no longer read from pflag.CommandLine for the list of flags. #68054 introduced some changes in apiserver/pkg/util/flag/. I'm waiting for that to be merged. See #70204. Sorry! I should probably close this PR since I have broken it down into three individual PRs.

k8s-ci-robot requested review from CaoShuFeng and mml October 24, 2018 03:21

imjching mentioned this pull request Oct 24, 2018

Some commands do not show glog flags #70145

Closed

stewart-yu reviewed Oct 24, 2018

View reviewed changes

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

Show glog flags for some commands which output flags in logical sections

ec6832e

Signed-off-by: Jay Lim <jay@imjching.com>

imjching force-pushed the 70145-fix-glog-flags branch from 04316b7 to ec6832e Compare October 24, 2018 04:47

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 24, 2018

This was referenced Oct 24, 2018

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

Merged

cloud-controller-manager: fix missing global flags for --help #70205

Closed

kube-controller-manager: fix missing global flags for --help #70216

Closed

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 25, 2018

k8s-ci-robot assigned sttts Oct 25, 2018

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

cheftako reviewed Oct 26, 2018

View reviewed changes

imjching closed this Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

imjching commented Oct 24, 2018 •

edited

Loading

stewart-yu left a comment •

edited

Loading

stewart-yu Oct 24, 2018

imjching Oct 24, 2018

stewart-yu commented Oct 24, 2018

imjching commented Oct 24, 2018 •

edited

Loading

imjching commented Oct 24, 2018

imjching commented Oct 24, 2018

stewart-yu commented Oct 24, 2018

imjching commented Oct 25, 2018

sttts commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

jennybuckley commented Oct 25, 2018

cheftako Oct 26, 2018

imjching Oct 26, 2018

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

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

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

Conversation

imjching commented Oct 24, 2018 • edited Loading

stewart-yu left a comment • edited Loading

Choose a reason for hiding this comment

stewart-yu Oct 24, 2018

Choose a reason for hiding this comment

imjching Oct 24, 2018

Choose a reason for hiding this comment

stewart-yu commented Oct 24, 2018

imjching commented Oct 24, 2018 • edited Loading

imjching commented Oct 24, 2018

imjching commented Oct 24, 2018

stewart-yu commented Oct 24, 2018

imjching commented Oct 25, 2018

sttts commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

jennybuckley commented Oct 25, 2018

cheftako Oct 26, 2018

Choose a reason for hiding this comment

imjching Oct 26, 2018

Choose a reason for hiding this comment

imjching commented Oct 24, 2018 •

edited

Loading

stewart-yu left a comment •

edited

Loading

imjching commented Oct 24, 2018 •

edited

Loading