Fix hyperkube flag parsing #25512

colhom · 2016-05-11T22:34:21Z

Hyperkube flag parsing was not playing nicely with kubectl command and sub-commands. This PR addresses that problem, and adds some tests which exercise hyperkube dispatching to nested cobra commands.

\cc @aaronlevy @kbrwn @mumoshu

fixes #24088

googlebot · 2016-05-11T22:34:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

k8s-bot · 2016-05-11T22:35:10Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-11T22:36:10Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-11T22:36:29Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

colhom · 2016-05-11T23:46:06Z

@k8s-bot I signed it!

thockin · 2016-05-12T04:26:38Z

I'm assigning to to Mike semi-randomly

bgrant0607 · 2016-05-12T06:13:21Z

ok to test

bgrant0607 · 2016-05-12T06:14:59Z

See also #22737

cc @ingvagabund

ingvagabund · 2016-05-12T06:58:31Z

@colhom I am definitely +1 for the hyperkube_test.go.

ingvagabund · 2016-05-12T07:12:04Z

cmd/hyperkube/hyperkube.go

 	if serverName == hk.Name {
-		args = args[1:]


As long as you run hyperkube kubectl, this should not matter. Why movin 2 lines higher?

This looks to perhaps break things when you softlink kubectl to the hyperkube binary. Have you tested that?

Ironically enough, this was actually a fix to make softlinking work better :) I've tested it.

There's no reason to pass the command arg to any of the Server Run blocks . In particular, since kubectl is now a Cobra command, it requires that if args[0] is not a flag, it match the command name exactly. In the case of a softlink, it's possible thats args[0] will be for example /usr/bin/kubectl, and the Cobra command will complain. This removes that possibility.

Ah! Looking at the documentation for pflags.FlagSet.Parse it looks like the args should not contain the command name. That would imply that we were doing it wrong before when the command name was being included. interspersed defaulting to true must have covered our sins.

Yep, that's very true!

jbeda · 2016-05-12T18:24:12Z

cmd/hyperkube/kubectl.go

-			}
-			os.Exit(0)
-			return nil
+			cmd.SetArgs(args)


if we don't muck with args above, this should be args[1:]

Depending on whether or not it's a softlink (os.Args[0] == "hyperkube"), it's either os.Args[1:] or os.Args[2:]. It's kind of nice to let the hyperkube server deal with that logic and sub-servers can just assume that their args list starts with the first relevant flag. This would easily support, for example, nesting servers in servers.

jbeda · 2016-05-12T18:31:11Z

I'm in favor of integrating this with the cobra command structure (I looked at it originally but found it a forced fit) but we need to make sure we don't change behavior of existing commands. Also, need to make sure we don't get divergent implementations between the hyperkube version and the stand along version of kubectl.

jbeda · 2016-05-12T22:23:20Z

Looks good to me.

@ingvagabund, what are your thoughts?

Honestly, I lean toward merging both changes. The cobra stuff does enough fancy flag parsing that it makes sense to let it do it. It is a decent service for the rest of the servers.

Also, we need to figure out what is going on with the CLA for @colhom. I think you need to respond with I signed it! without any other text. Apparently the googlebot is picky.

colhom · 2016-05-12T23:38:16Z

I signed it!

k8s-github-robot · 2016-05-21T01:01:23Z

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

k8s-bot · 2016-05-21T01:41:25Z

GCE e2e build/test passed for commit fd1771e.

jbeda · 2016-05-22T17:00:35Z

I believe this is not merged because it isn't marked for 1.3. @bgrant0607 -- what is the criteria/process for that? I don't see it linked off of the 1.3 release wiki page or obvious from kubernetes-dev chatter.

luxas · 2016-05-22T17:24:24Z

@jbeda I added it to the v1.3 milestone.
This will be merged although it doesn't have the milestone attached to it, but having a milestone adds a little more priority to it
It's just waiting to be on the top of the queue

k8s-github-robot · 2016-05-22T17:33:11Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-22T18:19:43Z

GCE e2e build/test passed for commit fd1771e.

k8s-github-robot · 2016-05-22T19:00:27Z

Automatic merge from submit-queue

jbeda · 2016-05-22T21:28:20Z

Thanks @luxas!

colhom · 2016-05-26T23:51:17Z

@mikedanese what do you think about a cherrypick-candidate label?

colhom · 2016-05-26T23:52:37Z

\cc @jbeda

roberthbailey · 2016-06-02T07:26:30Z

@colhom I assume you are asking about cherry picking into 1.2 (and that the 1.3 milestone was added to get through the submit queue). Is that correct?

colhom · 2016-06-02T08:01:56Z

@roberthbailey that is correct.

roberthbailey · 2016-06-02T20:40:56Z

@colhom I've adjusted the milestone label to indicate where you want it cherry picked to. Please send a cherry pick PR for the 1.2 release branch.

colhom · 2016-06-02T23:53:51Z

@roberthbailey cherry pick PR is up: #26754

…-upstream-release-1.2 Automated cherry pick of #25512

roberthbailey · 2016-06-18T15:33:24Z

This was cherrypicked into 1.2 in #26754. Manually removing the cherrypick candidate label.

…-of-#25512-upstream-release-1.2 Automated cherry pick of kubernetes#25512

googlebot added the cla: no label May 11, 2016

colhom mentioned this pull request May 11, 2016

hyperkube + kubectl - unknown flag #24088

Closed

k8s-github-robot assigned thockin May 11, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 11, 2016

thockin assigned mikedanese and unassigned thockin May 12, 2016

bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 12, 2016

bgrant0607 assigned jbeda and unassigned mikedanese May 12, 2016

ingvagabund reviewed May 12, 2016
View reviewed changes

colhom mentioned this pull request May 12, 2016

Hyperkube: don't parse args for all servers #22737

Closed

jbeda reviewed May 12, 2016
View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 2016

luxas added this to the v1.3 milestone May 22, 2016

k8s-github-robot merged commit 8e81025 into kubernetes:master May 22, 2016

This was referenced May 26, 2016

kube-aws: Drain nodes before shutting them down coreos/coreos-kubernetes#465

Closed

Use kubectl instead of curl to load kube-system manifests coreos/coreos-kubernetes#462

Closed

xiang90 unassigned jbeda May 31, 2016

xiang90 added the cherrypick-candidate label May 31, 2016

roberthbailey modified the milestones: v1.2, v1.3 Jun 2, 2016

roberthbailey added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 2, 2016

colhom mentioned this pull request Jun 3, 2016

Automated cherry pick of #25512 #26754

Merged

brendandburns mentioned this pull request Jun 10, 2016

Fix arg parsing for hyperkube kubectl ... #27213

Closed

roberthbailey added a commit that referenced this pull request Jun 17, 2016

Merge pull request #26754 from colhom/automated-cherry-pick-of-#25512…

6ea7fa7

…-upstream-release-1.2 Automated cherry pick of #25512

roberthbailey removed the cherrypick-candidate label Jun 18, 2016

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#26754 from colhom/automated-cherry-pick…

a4f471e

…-of-#25512-upstream-release-1.2 Automated cherry pick of kubernetes#25512

shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017

Merge pull request kubernetes#26754 from colhom/automated-cherry-pick…

a0e497c

…-of-#25512-upstream-release-1.2 Automated cherry pick of kubernetes#25512

Fix hyperkube flag parsing #25512

Fix hyperkube flag parsing #25512

Conversation

colhom commented May 11, 2016

googlebot commented May 11, 2016

k8s-bot commented May 11, 2016

k8s-bot commented May 11, 2016

k8s-bot commented May 11, 2016

colhom commented May 11, 2016

thockin commented May 12, 2016

bgrant0607 commented May 12, 2016

bgrant0607 commented May 12, 2016

ingvagabund commented May 12, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbeda commented May 12, 2016

jbeda commented May 12, 2016

colhom commented May 12, 2016

k8s-github-robot commented May 21, 2016

k8s-bot commented May 21, 2016

jbeda commented May 22, 2016

luxas commented May 22, 2016

k8s-github-robot commented May 22, 2016

k8s-bot commented May 22, 2016

k8s-github-robot commented May 22, 2016

jbeda commented May 22, 2016

colhom commented May 26, 2016

colhom commented May 26, 2016

roberthbailey commented Jun 2, 2016

colhom commented Jun 2, 2016

roberthbailey commented Jun 2, 2016

colhom commented Jun 2, 2016

roberthbailey commented Jun 18, 2016