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.

Sign up for GitHub

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

Jump to bottom

kubectl: exec and attach break scripting and should honor --quiet #99004

Merged
merged 2 commits into from Mar 5, 2021
Merged

kubectl: exec and attach break scripting and should honor --quiet #99004

merged 2 commits into from Mar 5, 2021

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Feb 11, 2021 •
edited

--quiet means no informational output for the human that could be confused with the output of the underlying system. We have been very lax in allowing new debugging messages to be added to exec and attach which breaks scripting (exec and attach are designed to work properly for remote scripting and only error when that error cannot be confused with the remote system). Make exec and attach properly honor --quiet to suppress informational / non-failure output from the local command. We added quiet as an internal option several years back and have been steadily growing less tolerant of flakes (originally exec was a good citizen in almost all cases, now it is less so).

Secondly, attach and exec are inconsistent with how they show the containers the user can exec/attach to. Printing the describe command is a lot of text output (80 characters) and describe itself is pretty bad at telling you container names. The behavior of logs up to this point is more accurate, and since the vast majority of pods have a small number of containers, it would be better for humans to print a single line with all ephemeral and init containers. Update the commands to share similar debugging code (and suppress the defaulting message when --quiet is provided). After this change we no longer need EnableCmdSuggestion:

Example output:

# show init containers inline when defaulting to a container
$ kubectl exec -i --tty -n my-namespace some-etcd-pod -- /bin/sh
Defaulted container "etcdctl" out of: etcdctl, etcd, etcd-metrics, etcd-ensure-env-vars (init), etcd-resources-copy (init)
sh-4.4# 

# suppress output with --quiet
$ kubectl exec -q -i --tty -n my-namespace some-etcd-pod -- /bin/sh
sh-4.4#

/kind bug
/kind cleanup

`kubectl exec` and `kubectl attach` now honor the `--quiet` flag which suppresses output from the local binary that could be confused by a script with the remote command output (all non-failure output is hidden). In addition, print inline with exec and attach the list of alternate containers when we default to the first spec.container.

@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 Feb 11, 2021
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 11, 2021
@smarterclayton smarterclayton force-pushed the simplify_debug branch 2 times, most recently from 62ef450 to 4493ef0 Compare February 11, 2021 18:47
@smarterclayton smarterclayton changed the title kubectl: exec and attach are inconsistent and don't honor --quiet correctly kubectl: exec and attach are inconsistent and should honor --quiet Feb 11, 2021
@smarterclayton smarterclayton changed the title kubectl: exec and attach are inconsistent and should honor --quiet kubectl: exec and attach are hard to script and should honor --quiet Feb 11, 2021
@smarterclayton smarterclayton changed the title kubectl: exec and attach are hard to script and should honor --quiet kubectl: exec and attach break scripting and should honor --quiet Feb 11, 2021
@smarterclayton smarterclayton force-pushed the simplify_debug branch 2 times, most recently from 1924a37 to 46afd41 Compare February 11, 2021 20:04
@smarterclayton
Copy link
Contributor Author

/retest

2 similar comments
@smarterclayton
Copy link
Contributor Author

/retest

@smarterclayton
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021
@soltysh
Copy link
Contributor

soltysh commented Mar 3, 2021

/retest

@smarterclayton smarterclayton force-pushed the simplify_debug branch 2 times, most recently from 018e2b0 to 35097dd Compare March 3, 2021 21:11
@smarterclayton
kubectl: Properly respect --quiet in exec/attach
75700d3 
--quiet means no informational output for the human that could be
confused with the output of the shell / command on the other side.
@smarterclayton
kubectl: Inline the containers for the user in attach/exec
43e8ebb 
The behavior of the container defaulting in attach/exec is inconsistent
and should be unified. As a user, when we default the vast majority of
pods will have a small number of containers and so printing the container
names inline (as kubectl logs did) is more appropriate. The debug message
we printed about using describe was already longer than 99% of all pod
container names, so we were wasting user time.

Unify container selection for exec and attach to be consistent with old
behavior. Properly handle the --quiet flag (should not print in that case)
for both commands. Remove EnableCmdSuggestion and the machinery it needs.

The message now prints:

> Defaulted container "etcdctl" out of: etcdctl, etcd, etcd-metrics, etcd-ensure-env-vars (init), etcd-resources-copy (init)
@soltysh soltysh self-assigned this Mar 5, 2021
soltysh
soltysh approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021
@soltysh
Copy link
Contributor

soltysh commented Mar 5, 2021

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed 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 Mar 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, soltysh

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 merged commit 9cc3665 into kubernetes:master Mar 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@dixudx dixudx Awaiting requested review from dixudx

Assignees

@soltysh soltysh

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. priority/backlog Higher priority than priority/awaiting-more-evidence. 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
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smarterclayton @soltysh @k8s-ci-robot