Add alpha command to kubectl #46151

verb · 2017-05-20T00:01:08Z

Also allow new commands to disable themselves by returning a nil value. This can be used to disable commands based on feature gates.

What this PR does / why we need it: Method of enabling alpha functionality in kubectl

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): ref #45922

Special notes for your reviewer: Part of a discussion in #45922 with @pwittrock

Release note:

NONE

verb · 2017-05-20T00:02:04Z

/assign @pwittrock
/unassign @derekwaynecarr
/unassign @sttts

verb · 2017-05-20T01:55:35Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

xiangpengzhao · 2017-05-20T18:39:15Z

pkg/kubectl/cmd/cmd.go

@@ -252,6 +255,12 @@ var (

 // NewKubectlCommand creates the `kubectl` command and its nested children.
 func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cobra.Command {
+	if f := os.Getenv("KUBECTL_FEATURE_GATES"); f != "" {


Document how to set this env?

or give some help info via kubectl --help

how about set feature gates via a subcommand or flag instead of setting env vars?

Thanks for asking this, it made me realize that I hadn't actually Submitted my comment explaining my reasoning and it was just hanging out in a text box in another tab waiting for me to close my browser. Here it is: #45922 (comment)

There's a bit of a catch-22 with setting feature gates on kubectl itself. If we set them with a flag then the gates won't be available prior to flag parsing and so can't gate flags or the command tree.

We definitely should document this, but first let's reach consensus on how the feature gates will be implemented.

xiangpengzhao · 2017-05-20T18:41:30Z

pkg/kubectl/cmd/cmd.go

@@ -252,6 +255,12 @@ var (

 // NewKubectlCommand creates the `kubectl` command and its nested children.
 func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cobra.Command {
+	if f := os.Getenv("KUBECTL_FEATURE_GATES"); f != "" {
+		if e := utilfeature.DefaultFeatureGate.Set(f); e != nil {
+			fmt.Fprintf(err, "Invalid KUBECTL_FEATURE_GATES %q. Available features:\n%s\n", f, strings.Join(utilfeature.DefaultFeatureGate.KnownFeatures(), "\n"))


provide some way to get KnownFeatures and whether they are enabled or not?

Good idea. I'll include this in kubectl --help as you suggested.

xiangpengzhao · 2017-05-20T18:46:01Z

pkg/kubectl/cmd/cmd.go

@@ -348,6 +357,7 @@ func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cob
 			},
 		},
 	}
+	groups.PruneCommands()


What if users run a disabled command?

When a command disables itself by returning nil it's as if the command doesn't exist, which is intentional. My goal is that alpha features don't influence the behavior of kubectl in any way unless they're enabled.

@verb Thanks for the explanation!

sttts · 2017-05-22T08:23:01Z

We have had the discussion a number of times whether clients should depend on apiserver features. IMO if we want feature gates in kubectl at all, we should make them independent from the apiserver ones.

/cc @smarterclayton

verb · 2017-05-22T15:45:04Z

@sttts Sure, it would be easy to use the feature gate framework with an independent configuration. I rather like the feature gate framework, but I'm not tied to it. If we do use feature gates do you like the environment-based approach?

I think the alternative would have to be a kubectl alpha subcommand would could segregate the alpha commands but not hide them entirely.

alexandercampbell

Code looks fine to me. I'll defer to the existing discussion on whether this is the correct path to take.

alexandercampbell · 2017-05-23T20:48:38Z

pkg/kubectl/cmd/cmd.go

@@ -252,6 +255,12 @@ var (

 // NewKubectlCommand creates the `kubectl` command and its nested children.
 func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cobra.Command {
+	if f := os.Getenv("KUBECTL_FEATURE_GATES"); f != "" {
+		if e := utilfeature.DefaultFeatureGate.Set(f); e != nil {


If this is an error it should be named err, no?

Normally it would, but I want to print to the io.Writer with the same name from the enclosing scope.

Gotcha, thanks!

verb · 2017-06-01T01:08:10Z

@sttts @alexandercampbell

I tried a different approach and created a kubectl alpha subcommand based on your feedback. I think I may like this approach better. The command is hidden if there are no alpha commands available. The command for my alpha feature would be come kubectl alpha debug.

Right now alpha commands are visible if they exist, but it would be easy to hide them further.

xiangpengzhao · 2017-06-01T02:36:01Z

@verb pull-kubernetes-verify failed due to:

I0531 19:07:33.642] FAILED hack/make-rules/../../hack/verify-generated-docs.sh 38s

sttts · 2017-06-01T07:09:25Z

I like the alpha command more than the env var 👍

sttts · 2017-06-01T07:10:07Z

/cc @fabianofranz you are nearer to kubectl. What's your opinion?

alexandercampbell · 2017-06-01T17:42:52Z

pkg/kubectl/cmd/alpha.go

+		Long:  templates.LongDesc(i18n.T("These commands correspond to alpha features that are not enabled in Kubernetes clusters by default.")),
+	}
+
+	// Alpha commands should be added to alphaGroups, which is arranged similar to the main


Where is alphaGroups defined?

Whoops, that was left over from a previous implementation. Updated the comment to reflect the current implementation.

alexandercampbell · 2017-06-01T17:47:23Z

pkg/kubectl/cmd/cmd.go

@@ -354,6 +354,10 @@ func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cob
 		"options",
 		Deprecated("kubectl", "delete", cmds, NewCmdStop(f, out)),
 	}
+	alpha := NewCmdAlpha(f, in, out, err)
+	if !alpha.HasSubCommands() {
+		filters = append(filters, alpha.Name())


Comment might be nice here. Something like

// If there are no alpha commands in this build, hide the "alpha" subcommand.

Good call, done.

fejta · 2017-06-01T21:04:17Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref kubernetes/test-infra#2931

alexandercampbell · 2017-06-13T17:54:32Z

pkg/kubectl/cmd/alpha.go

+// NewCmdAlpha creates a command that acts as an alternate root command for features in alpha
+func NewCmdAlpha(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "alpha",


Maybe this should be:

Use: "alpha SUBCOMMAND",

To be consistent with other commands in the repo:

cmd/certificates.go:35: Use: "certificate SUBCOMMAND", cmd/set/set.go:37: Use: "set SUBCOMMAND", cmd/rollout/rollout.go:51: Use: "rollout SUBCOMMAND", cmd/config/config.go:39: Use: "config SUBCOMMAND",

I'd be happy to do that, but alpha is a little bit different from set/rollout/config as its more of a container for commands rather than a command with subcommands. For example, config get and config set are related subcommands that operate on config, but alpha debug and alpha foo are unrelated other than both being in an alpha state.

Also, the commands in alpha could themselves have subcommands, which would make the "Usage: kubectl alpha SUBCOMMAND [options]" not quite correct.

As its coded now, without a run function, Usage will be omitted from help and look like this:

These commands correspond to alpha features that are not enabled in Kubernetes clusters by default. Available Commands: debug Run a debug container in a pod Use "kubectl <command> --help" for more information about a given command.

If we were to give it a default run function and set Use to "alpha COMMAND", then the help would be:

These commands correspond to alpha features that are not enabled in Kubernetes clusters by default. Available Commands: debug Run a debug container in a pod Usage: kubectl alpha COMMAND [options] Use "kubectl <command> --help" for more information about a given command. Use "kubectl options" for a list of global command-line options (applies to all commands).

alexandercampbell · 2017-06-13T17:54:53Z

pkg/kubectl/cmd/alpha.go

+	// NewKubeletCommand() will hide the alpha command if it has no subcommands. Overriding
+	// the help function ensures a reasonable message if someone types the hidden command anyway.
+	if !cmd.HasSubCommands() {
+		cmd.SetHelpFunc(func(_ *cobra.Command, _ []string) {


You don't need the underscores here.

whoops, fixed.

pwittrock · 2017-06-15T22:22:51Z

/lgtm

pwittrock · 2017-06-15T22:23:12Z

@verb would you squash before I approve?

pwittrock · 2017-06-15T22:45:25Z

Looks like the merge bot was over eager about the approve label

Alpha commands can be added under `kubectl alpha` and are always accessible (regardless of feature gates). If no alpha commands have been defined then `alpha` is not displayed in `help`.

verb · 2017-06-16T07:13:29Z

squashed!

verb · 2017-06-16T09:16:59Z

/retest

pwittrock · 2017-06-16T19:31:30Z

/lgtm

k8s-github-robot · 2017-06-16T19:31:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pwittrock, verb

Associated issue: 45922

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

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

~~docs/OWNERS~~ [pwittrock]
~~pkg/kubectl/OWNERS~~ [pwittrock]

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

k8s-github-robot · 2017-06-23T12:00:35Z

Automatic merge from submit-queue

k8s-github-robot assigned derekwaynecarr and sttts May 20, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 20, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 20, 2017

k8s-ci-robot assigned pwittrock and unassigned sttts and derekwaynecarr May 20, 2017

verb force-pushed the kubectl-featuregate branch 2 times, most recently from 60c9cfc to eaa84d6 Compare May 20, 2017 00:12

xiangpengzhao reviewed May 20, 2017

View reviewed changes

verb mentioned this pull request May 20, 2017

FR: New kubectl command kubectl debug #45922

Closed

k8s-ci-robot requested a review from smarterclayton May 22, 2017 08:23

verb mentioned this pull request May 22, 2017

WIP: Create a new kubectl command to attach debug container to a running pod #46256

Closed

2 tasks

pwittrock assigned alexandercampbell May 23, 2017

alexandercampbell reviewed May 23, 2017

View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 1, 2017

alexandercampbell reviewed Jun 1, 2017

View reviewed changes

verb changed the title ~~Set feature gates in kubectl based on environment~~ Add alpha commands to kubectl Jun 1, 2017

verb force-pushed the kubectl-featuregate branch from 3072965 to d85bdc5 Compare June 1, 2017 19:52

verb changed the title ~~Add alpha commands to kubectl~~ Add alpha command to kubectl Jun 1, 2017

alexandercampbell suggested changes Jun 13, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2017

pwittrock removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2017

Create a kubectl alpha subcommand

01c7d12

Alpha commands can be added under `kubectl alpha` and are always accessible (regardless of feature gates). If no alpha commands have been defined then `alpha` is not displayed in `help`.

verb force-pushed the kubectl-featuregate branch from a02a614 to 01c7d12 Compare June 16, 2017 07:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2017

k8s-github-robot merged commit 1864a24 into kubernetes:master Jun 23, 2017

Add alpha command to kubectl #46151

Add alpha command to kubectl #46151

Conversation

verb commented May 20, 2017

verb commented May 20, 2017

verb commented May 20, 2017

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

Choose a reason for hiding this comment

sttts commented May 22, 2017

verb commented May 22, 2017

alexandercampbell left a 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

verb commented Jun 1, 2017

xiangpengzhao commented Jun 1, 2017

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

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

fejta commented Jun 1, 2017

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

pwittrock commented Jun 15, 2017

pwittrock commented Jun 15, 2017

pwittrock commented Jun 15, 2017

verb commented Jun 16, 2017

verb commented Jun 16, 2017

pwittrock commented Jun 16, 2017

k8s-github-robot commented Jun 16, 2017

k8s-github-robot commented Jun 23, 2017