Skip to content
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

Remove unused flags from kubectl run #110668

Merged
merged 1 commit into from Jun 27, 2022

Conversation

brianpursley
Copy link
Member

@brianpursley brianpursley commented Jun 20, 2022

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

The following flags, which do not apply to kubectl run, have been removed:

  • --cascade
  • --filename
  • --force
  • --grace-period
  • --kustomize
  • --recursive
  • --timeout
  • --wait

These flags were being added to the run command to support pod deletion after attach, but they are not used if set, so they effectively do nothing.

This PR also displays an error message if the pod fails to be deleted (when the --rm flag is used). Previously any error during deletion would be suppressed and the pod would remain.

This PR also adds some unit tests for run and attach with and without the --rm flag. As such, some minor refactoring of the run command has been done to support mocking dependencies.

Which issue(s) this PR fixes:

Fixes #108630

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Removed unused flags from kubectl run command

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 20, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2022
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 20, 2022
@brianpursley
Copy link
Member Author

/retest

@pacoxu pacoxu added this to Needs Approver in SIG CLI Jun 21, 2022
Copy link
Member

@ardaguclu ardaguclu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good to me but I'd expect removal of this line https://github.com/kubernetes/kubernetes/blob/202d1e6fc5993b93ef6100f0e3f516a249386f4a/staging/src/k8s.io/kubectl/pkg/cmd/run/run.go#L167. This PR still exposes delete flags?

staging/src/k8s.io/kubectl/pkg/cmd/run/run.go Show resolved Hide resolved
@brianpursley
Copy link
Member Author

That looks good to me but I'd expect removal of this line

https://github.com/kubernetes/kubernetes/blob/202d1e6fc5993b93ef6100f0e3f516a249386f4a/staging/src/k8s.io/kubectl/pkg/cmd/run/run.go#L167

. This PR still exposes delete flags?

Yeah. DeleteFlags.AddFlags() has some logic that excludes any flag that does not have a default set.

I was approaching this PR with a mindset of trying to keep things as similar to how they are currently as possible.

But after doing this, I wonder if it might be better to simply remove the DeleteFlags altogether. I will circle back and take a look at what they would look like.

@brianpursley
Copy link
Member Author

brianpursley commented Jun 21, 2022

Here is the help output, with flags removed:

~ $ kubectl run --help
Create and run a particular image in a pod.

Examples:
  # Start a nginx pod
  kubectl run nginx --image=nginx
  
  # Start a hazelcast pod and let the container expose port 5701
  kubectl run hazelcast --image=hazelcast/hazelcast --port=5701
  
  # Start a hazelcast pod and set environment variables "DNS_DOMAIN=cluster" and "POD_NAMESPACE=default" in the
container
  kubectl run hazelcast --image=hazelcast/hazelcast --env="DNS_DOMAIN=cluster" --env="POD_NAMESPACE=default"
  
  # Start a hazelcast pod and set labels "app=hazelcast" and "env=prod" in the container
  kubectl run hazelcast --image=hazelcast/hazelcast --labels="app=hazelcast,env=prod"
  
  # Dry run; print the corresponding API objects without creating them
  kubectl run nginx --image=nginx --dry-run=client
  
  # Start a nginx pod, but overload the spec with a partial set of values parsed from JSON
  kubectl run nginx --image=nginx --overrides='{ "apiVersion": "v1", "spec": { ... } }'
  
  # Start a busybox pod and keep it in the foreground, don't restart it if it exits
  kubectl run -i -t busybox --image=busybox --restart=Never
  
  # Start the nginx pod using the default command, but use custom arguments (arg1 .. argN) for that command
  kubectl run nginx --image=nginx -- <arg1> <arg2> ... <argN>
  
  # Start the nginx pod using a different command and custom arguments
  kubectl run nginx --image=nginx --command -- <cmd> <arg1> ... <argN>

Options:
    --allow-missing-template-keys=true:
	If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to
	golang and jsonpath output formats.

    --annotations=[]:
	Annotations to apply to the pod.

    --attach=false:
	If true, wait for the Pod to start running, and then attach to the Pod as if 'kubectl attach ...' were called.
	Default false, unless '-i/--stdin' is set, in which case the default is true. With '--restart=Never' the exit
	code of the container process is returned.

    --command=false:
	If true and extra arguments are present, use them as the 'command' field in the container, rather than the
	'args' field which is the default.

    --dry-run='none':
	Must be "none", "server", or "client". If client strategy, only print the object that would be sent, without
	sending it. If server strategy, submit server-side request without persisting the resource.

    --env=[]:
	Environment variables to set in the container.

    --expose=false:
	If true, create a ClusterIP service associated with the pod.  Requires `--port`.

    --field-manager='kubectl-run':
	Name of the manager used to track field ownership.

    --image='':
	The image for the container to run.

    --image-pull-policy='':
	The image pull policy for the container.  If left empty, this value will not be specified by the client and
	defaulted by the server.

    -l, --labels='':
	Comma separated labels to apply to the pod. Will override previous values.

    --leave-stdin-open=false:
	If the pod is started in interactive mode or with stdin, leave stdin open after the first attach completes. By
	default, stdin will be closed after the first attach completes.

    -o, --output='':
	Output format. One of: (json, yaml, name, go-template, go-template-file, template, templatefile, jsonpath,
	jsonpath-as-json, jsonpath-file).

    --override-type='merge':
	The method used to override the generated object: json, merge, or strategic.

    --overrides='':
	An inline JSON override for the generated object. If this is non-empty, it is used to override the generated
	object. Requires that the object supply a valid apiVersion field.

    --pod-running-timeout=1m0s:
	The length of time (like 5s, 2m, or 3h, higher than zero) to wait until at least one pod is running

    --port='':
	The port that this container exposes.

    --privileged=false:
	If true, run the container in privileged mode.

    -q, --quiet=false:
	If true, suppress prompt messages.

    --restart='Always':
	The restart policy for this Pod.  Legal values [Always, OnFailure, Never].

    --rm=false:
	If true, delete the pod after it exits.  Only valid when attaching to the container, e.g. with '--attach' or
	with '-i/--stdin'.

    --save-config=false:
	If true, the configuration of current object will be saved in its annotation. Otherwise, the annotation will
	be unchanged. This flag is useful when you want to perform kubectl apply on this object in the future.

    --show-managed-fields=false:
	If true, keep the managedFields when printing objects in JSON or YAML format.

    -i, --stdin=false:
	Keep stdin open on the container in the pod, even if nothing is attached.

    --template='':
	Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format
	is golang templates [http://golang.org/pkg/text/template/#pkg-overview].

    -t, --tty=false:
	Allocate a TTY for the container in the pod.

Usage:
  kubectl run NAME --image=image [--env="key=value"] [--port=port] [--dry-run=server|client] [--overrides=inline-json]
[--command] -- [COMMAND] [args...] [options]

Use "kubectl options" for a list of global command-line options (applies to all commands).

@brianpursley
Copy link
Member Author

@ardaguclu I removed DeleteFlags altogether. I think it simplifies things even more. Thanks for the suggestion. PTAL.

@ardaguclu
Copy link
Member

@ardaguclu I removed DeleteFlags altogether. I think it simplifies things even more. Thanks for the suggestion. PTAL.

That looks good to me. I'll label it later for the other reviewers have a chance to look at.

The following flags, which do not apply to kubectl run,
have been removed:
--cascade
--filename
--force
--grace-period
--kustomize
--recursive
--timeout
--wait

These flags were being added to the run command to support
pod deletion after attach, but they are not used if set, so
they effectively do nothing.

This PR also displays an error message if the pod fails to be
deleted (when the --rm flag is used).  Previously any error
during deletion would be suppressed and the pod would remain.

This PR also adds some unit tests for run and attach with and
without the --rm flag.  As such, some minor refactoring of the
run command has been done to support mocking dependencies.
@brianpursley
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 24, 2022
@ardaguclu
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit d9e7f25 into kubernetes:master Jun 27, 2022
SIG CLI automation moved this from Needs Approver to Done Jun 27, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 27, 2022
@saschagrunert
Copy link
Member

Hey folks, we come across this during our k8s upgrade to v1.25 😊 I think having the information that the flags are unused and which have being removed would be great to capture in the release notes.

@brianpursley
Copy link
Member Author

Hey folks, we come across this during our k8s upgrade to v1.25 blush I think having the information that the flags are unused and which have being removed would be great to capture in the release notes.

Actually, I think it was a mistake to completely remove the flags at this time, and this PR needs to be reverted.

While the flags are completely useless, they do exist, and if someone happened to be setting them, their kubectl run command would still work. However starting in 1.25, the command will now fail with an unknown flag error.

Even though the change is a good one to make, it hasn't gone through the required deprecation period, and could break backward compatibility if someone happened to be using those flags.

@ardaguclu
Copy link
Member

Hey folks, we come across this during our k8s upgrade to v1.25 blush I think having the information that the flags are unused and which have being removed would be great to capture in the release notes.

Actually, I think it was a mistake to completely remove the flags at this time, and this PR needs to be reverted.

While the flags are completely useless, they do exist, and if someone happened to be setting them, their kubectl run command would still work. However starting in 1.25, the command will now fail with an unknown flag error.

Even though the change is a good one to make, it hasn't gone through the required deprecation period, and could break backward compatibility if someone happened to be using those flags.

Wouldn't be better adding manual deprecation for flags in run command instead directly reverting this useful fix?. These commands are redundant and we just don't want to break any script. In any case, there will be a backport to 1.25.

@brianpursley
Copy link
Member Author

Hey folks, we come across this during our k8s upgrade to v1.25 blush I think having the information that the flags are unused and which have being removed would be great to capture in the release notes.

Actually, I think it was a mistake to completely remove the flags at this time, and this PR needs to be reverted.
While the flags are completely useless, they do exist, and if someone happened to be setting them, their kubectl run command would still work. However starting in 1.25, the command will now fail with an unknown flag error.
Even though the change is a good one to make, it hasn't gone through the required deprecation period, and could break backward compatibility if someone happened to be using those flags.

Wouldn't be better adding manual deprecation for flags in run command instead directly reverting this useful fix?. These commands are redundant and we just don't want to break any script. In any case, there will be a backport to 1.25.

Ideally, but I think process-wise, it makes sense to revert this and then do the deprecation in a new PR.

This will allow time for the re-do PR to be reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging this pull request may close these issues.

kubectl run accidentally exposes --kustomize flag
4 participants