New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make the kubelet cobra command complete #58405

Merged
merged 1 commit into from Jan 18, 2018

Conversation

@deads2k
Contributor

deads2k commented Jan 17, 2018

This pull attempts a move from the cmd/kubelet to the cobra command where it can re-used.

/assign @mtaufen
/assign @liggitt
@ncdc fyi

xref: #34732

NONE
}
// initialize logging and defer flush
utilflag.InitFlags()

This comment has been minimized.

@ncdc

ncdc Jan 17, 2018

Member

Don't think you can do this - see

// TODO: once we switch everything over to Cobra commands, we can go back to calling
// 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.

This comment has been minimized.

@deads2k

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

It seems to work ok for the scheduler: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-scheduler/scheduler.go#L32-L34

I see the bug. Registering global flags is naughty. Fixed.

This comment has been minimized.

@ncdc

ncdc Jan 17, 2018

Member

As we discussed on slack, the scheduler is registering its flags globally into pflag.CommandLine. The kube-proxy registers its flags into the cobra command's .Flags(), which is why utilflag.InitFlags() doesn't work for kube-proxy but does for the scheduler. We need to fix the scheduler, finish converting all the executables to use cobra (#34732), then we can use InitFlags.

@@ -97,8 +98,9 @@ const (
// NewKubeletCommand creates a *cobra.Command object with default parameters
func NewKubeletCommand() *cobra.Command {
// ignore the error, as this is just for generating docs and the like

This comment has been minimized.

@ncdc

ncdc Jan 17, 2018

Member

This comment isn't true any more

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

This comment isn't true any more

That is a code error. Updated to panic

// TODO(mtaufen): won't need this this once dynamic config is GA
// set feature gates so we can check if dynamic config is enabled
if err := utilfeature.DefaultFeatureGate.SetFromMap(kubeletServer.KubeletConfiguration.FeatureGates); err != nil {
glog.Fatal(err)

This comment has been minimized.

@ncdc

ncdc Jan 17, 2018

Member

I know this was a move, but consider cmdutil.CheckErr() here and below?

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

I know this was a move, but consider cmdutil.CheckErr() here and below?

That will tie us to pkg/kubectl. I'd rather not end up with that linkage.

This comment has been minimized.

@ncdc

ncdc Jan 17, 2018

Member

Ok, maybe we should consider taking that out of e.g. kube-proxy and (if it uses it) kube-scheduler in follow-ups.

This comment has been minimized.

@smarterclayton

smarterclayton Jan 17, 2018

Contributor

Probably the base checkErr stuff should move out of kubectl.

// set the normalize func, similar to k8s.io/apiserver/pkg/util/flag/flags.go:InitFlags
fs.SetNormalizeFunc(flag.WordSepNormalizeFunc)
// explicitly add flags from libs that register global flags
options.AddGlobalFlags(fs)

This comment has been minimized.

@liggitt

liggitt Jan 17, 2018

Member

does the switch to cobra cmd expose us to picking up all the globally registered flags again?

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

does the switch to cobra cmd expose us to picking up all the globally registered flags again?

Nothing new that I know. It makes some of the previously registered ones more obvious via help since the help is accurate now, but it doesn't mess with the parsing.

This comment has been minimized.

@liggitt

liggitt Jan 17, 2018

Member

this was selectively adding specific global flags, not all global flags, and marked most deprecated. does cobra just pick all the global ones up again?

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

this was selectively adding specific global flags, not all global flags, and marked most deprecated. does cobra just pick all the global ones up again?

It is consistent with the other commands. I'd much prefer to build a common whitelist if its a thing we want. After this, we have #58408 and then we can start dealing with all the command uniformally and fix it everywhere.

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

I don't think we should take a step backwards just to be "consistent" with other commands - which are probably doing it wrong by leaking flags anyway.

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

Ok, I can update this tomorrow morning

@mtaufen

I think you mostly got things in the right place, but we should try to accomplish this without regressing on the Kubelet's existing behavior.

// set the normalize func, similar to k8s.io/apiserver/pkg/util/flag/flags.go:InitFlags
fs.SetNormalizeFunc(flag.WordSepNormalizeFunc)
// explicitly add flags from libs that register global flags
options.AddGlobalFlags(fs)

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

I don't think we should take a step backwards just to be "consistent" with other commands - which are probably doing it wrong by leaking flags anyway.

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

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

Please don't leak the global command line into the Kubelet here. We already did a lot of work to fix this for the Kubelet in #57613. This will be a regression.

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

Why we can't just construct the private flagset in Run (above) and apply the passed in args []string to it, just like we do in main today. Effectively, why can't you just copy the body of today's main to Run, and substitute Run's args for today's os.Args? Does cobra do something special that gets in the way?

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

That's non-cobra-y and doesn't reflect the patterns we've found to work well in kubectl and openshift. I think you'll be happier take a different tack that separates your data and transformation phases more cleanly. This doesn't preclude such a follow-up or make it harder to do, but you should carefully consider the data flow.

@@ -121,8 +124,52 @@ is checked every 20 seconds (also configurable with a flag).
HTTP server: The kubelet can also listen for HTTP and respond to a simple API
(underspec'd currently) to submit a new manifest.`,
Run: func(cmd *cobra.Command, args []string) {

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

We will need to make sure it's still possible for the Kubelet to re-parse its command line multiple times during its bootstrap, see #56995 and #56171. I'd suppose this can be done with the args []string passed in here but need to make sure.

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

Reparsing is possible. I'll link you to some OpenShift code that does it. In general, be careful about doing it.

}
// construct a KubeletServer from kubeletFlags and kubeletConfig
kubeletServer.KubeletConfiguration = *kubeletConfig

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

I'd prefer we construct the kubeletFlags and kubeletConfig independently, and construct the kubeletServer just before we pass it to Run. The code reads more cleanly if it consists of a sequence of constructions rather than a sequence of mutations to a pre-constructed object.

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

I'd like that too, but I think the order should be 1) split the flag struct out, 2) write code to go from structul to runtime, 3) update command

// ignore the error, as this is just for generating docs and the like
s, _ := options.NewKubeletServer()
s.AddFlags(pflag.CommandLine)
kubeletServer, err := options.NewKubeletServer()

This comment has been minimized.

@mtaufen

mtaufen Jan 17, 2018

Contributor

Why can't we construct this inside Run, just like it's constructed in main() today?

This comment has been minimized.

@deads2k

deads2k Jan 17, 2018

Contributor

You need to use it to later add flags.

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Jan 17, 2018

/approve
/hold
(hold for global flags changes, as discussed on slack)
ping me on slack when it's ready, thanks for understanding

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented Jan 17, 2018

/approve
/hold

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 18, 2018

Global flags suppressed. Made it slightly more forward building too.

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 18, 2018

/approve no-issue

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 18, 2018

/approve
/hold
(hold for global flags changes, as discussed on slack)
ping me on slack when it's ready, thanks for understanding

global flags update and managed to move the kubeletServer instantiation down, though now there are two sources for flags (kubeletconfiguration and kubeletflags)

/hold cancel

if err := app.Run(kubeletServer, kubeletDeps); err != nil {
die(err)
if err := command.Execute(); err != nil {
os.Exit(1)

This comment has been minimized.

@mfojtik

mfojtik Jan 18, 2018

Contributor

where I can see the error?

@mfojtik

This comment has been minimized.

Contributor

mfojtik commented Jan 18, 2018

/lgtm

@deads2k deads2k added lgtm and removed lgtm labels Jan 18, 2018

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 18, 2018

/retest

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Jan 18, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, mfojtik, mtaufen

Associated issue: #34732

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Jan 18, 2018

not sure what's going on with that GPU test... maybe the nvidia driver install flaked on that one node?
/retest

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 18, 2018

Automatic merge from submit-queue (batch tested with PRs 58209, 57561, 58405). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 7f6dae7 into kubernetes:master Jan 18, 2018

12 of 13 checks passed

pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation deads2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@deads2k deads2k referenced this pull request Jan 18, 2018

Closed

Use cobra #58472

@deads2k deads2k deleted the deads2k:kubelet-01-start branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment