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

When running `kubectl drain` in dry-run, list warnings and pods that would be deleted. #82660

Merged
merged 1 commit into from Dec 6, 2019

Conversation

@sallyom
Copy link
Contributor

sallyom commented Sep 12, 2019

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This PR will result in listing pods that would be deleted if running kubectl drain <node> along with any warnings or errors that would occur if not in dry-run.

Which issue(s) this PR fixes:

Fixes #
kubernetes/kubectl#719

Does this PR introduce a user-facing change?:

kubectl drain node --dry-run will list pods that would be evicted or deleted

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


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 12, 2019

Hi @sallyom. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Sep 12, 2019

/assign @soltysh

@k8s-ci-robot k8s-ci-robot requested review from deads2k and eparis Sep 12, 2019
@sallyom sallyom mentioned this pull request Sep 12, 2019
6 of 6 tasks complete
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Sep 13, 2019

/ok-to-test

:)

@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch 2 times, most recently from 8935c18 to 2de8782 Sep 13, 2019
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Sep 13, 2019

sample output:

$ kubectl drain ip-10-0-138-243.blah --dry-run
node/ip-10-0-138-243.blah cordoned (dry run)
evicting pod ns1/pod1
evicting pod ns2/pod2
evicting pod ns3/pod3
evicting pod ns1/pod2
node/ip-10-0-138-243.blah drained (dry run)
@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from 2de8782 to 0d06f3a Sep 23, 2019
@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Oct 14, 2019

/priority important-longtemr

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 14, 2019

@soltysh: The label(s) priority/important-longtemr cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/priority important-longtemr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Oct 14, 2019

/priority important-longterm

@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from 0d06f3a to 141917d Oct 14, 2019
@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from 141917d to 99aa82b Oct 14, 2019
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Oct 15, 2019

/retest

@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch 3 times, most recently from e3207af to 84154d0 Oct 22, 2019
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Oct 23, 2019

/retest

@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Oct 23, 2019

/test pull-kubernetes-node-e2e-containerd

1 similar comment
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Oct 24, 2019

/test pull-kubernetes-node-e2e-containerd

if len(list.Pods()) > 0 && o.drainer.DryRun {
fmt.Fprintf(o.Out, "namespace/pod (dry run)\n")
for _, pod := range list.Pods() {
fmt.Fprintf(o.Out, "%s/%s\n", pod.Namespace, pod.Name)

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

From what I see DeleteOrEvicPods is doing this:

fmt.Fprintf(d.Out, "evicting pod %q\n", pod.Name)
so I'd expect something similar for printing here maybe with (dry run) added at the end.

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

Although we should probably update that to print both namespace and name altogether, in which case what you have here would be in sync.

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

Honestly, in both cases, we should probably fix that helper method to be reusable by passing ResourcePrinter but let's leave that as a separate PR 😉

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

I added namespace to kubectl/pkg/drain/drain.go added "evicting pod ... (dry-run)" to dry-run list.

$ kubectl drain ip-10-0-144-193.us-east-2.compute.internal --dry-run --pod-selector="app=hello-node"
node/ip-10-0-144-193.us-east-2.compute.internal cordoned (dry run)
evicting pod test/hello-node-7676b5fb8d-6w57 (dry run)
node/ip-10-0-144-193.us-east-2.compute.internal drained (dry run)

non-dry-run:

$ kubectl drain ip-10-0-144-193.us-east-2.compute.internal --pod-selector="app=hello-node"
node/ip-10-0-144-193.us-east-2.compute.internal cordoned
evicting pod test/hello-node-7676b5fb8d-6w57
pod/hello-node-7676b5fb8d-6w57r evicted
node/ip-10-0-144-193.us-east-2.compute.internal evicted

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

also, I changed '%q' to '%s' for printing, so instead of

evicting pod "test"/"hello-node-7676b5fb8d-6w57r"
pod/hello-node-7676b5fb8d-6w57r evicted

it prints

evicting pod test/hello-node-7676b5fb8d-6w57r
pod/hello-node-7676b5fb8d-6w57r evicted

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

but will change to %q if that's preferred

@@ -321,12 +319,21 @@ func (o *DrainCmdOptions) RunDrain() error {

func (o *DrainCmdOptions) deleteOrEvictPodsSimple(nodeInfo *resource.Info) error {
list, errs := o.drainer.GetPodsForDeletion(nodeInfo.Name)
if len(list.Pods()) > 0 && o.drainer.DryRun {
fmt.Fprintf(o.Out, "namespace/pod (dry run)\n")

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

Remove this line entirely, you'll not gonna get it in regular drain execution path, so it can't be here either.

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

got it, removed

for _, pod := range list.Pods() {
fmt.Fprintf(o.Out, "%s/%s\n", pod.Namespace, pod.Name)
}
}

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

This entire if should be after the below error check, in the normal when you encounter an error we'll return that error, same applies for dry-run.

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

ah right, doh, thanks, updated

if errs != nil {
return utilerrors.NewAggregate(errs)
}
if warnings := list.Warnings(); warnings != "" {
fmt.Fprintf(o.ErrOut, "WARNING: %s\n", warnings)
}
if o.drainer.DryRun {
return nil

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 27, 2019

Contributor

Here should the print happen.

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 2, 2019

Author Contributor

yup got it.

@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from 84154d0 to 993d4a1 Dec 2, 2019
@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Dec 2, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Dec 3, 2019

/test pull-kubernetes-integration

@sallyom

This comment has been minimized.

Copy link
Contributor Author

sallyom commented Dec 3, 2019

/test pull-kubernetes-e2e-gce-100-performance

@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from 993d4a1 to c98a0d9 Dec 5, 2019
Copy link
Contributor

soltysh left a comment

Two small nits and this is ready to go, thanks for keeping up with me 🙃

}
if err == nil || o.drainer.DryRun {
err = o.deleteOrEvictPodsSimple(info)
if err == nil {

This comment has been minimized.

Copy link
@soltysh

soltysh Dec 5, 2019

Contributor

By now you should be aware that people call me short-if 👮 😉 the above 3 lines can be easily squashed into a single:

if err := o.deleteOrEvictPodsSimple(info); err == nil {

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 5, 2019

Author Contributor

of course : ) thanks - squashed

@@ -327,6 +325,14 @@ func (o *DrainCmdOptions) deleteOrEvictPodsSimple(nodeInfo *resource.Info) error
if warnings := list.Warnings(); warnings != "" {
fmt.Fprintf(o.ErrOut, "WARNING: %s\n", warnings)
}
if o.drainer.DryRun {
if len(list.Pods()) > 0 {

This comment has been minimized.

Copy link
@soltysh

soltysh Dec 5, 2019

Contributor

No need for this check, for-range will handle that for you

This comment has been minimized.

Copy link
@sallyom

sallyom Dec 5, 2019

Author Contributor

oh right, ok. dropped

…would be deleted.
@sallyom sallyom force-pushed the sallyom:list-pods-dry-run-drain branch from c98a0d9 to 909300b Dec 5, 2019
@soltysh
soltysh approved these changes Dec 5, 2019
Copy link
Contributor

soltysh left a comment

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 5, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, 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

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Dec 5, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 646afd5 into kubernetes:master Dec 6, 2019
14 of 15 checks passed
14 of 15 checks passed
pull-kubernetes-e2e-gce Job triggered.
Details
cla/linuxfoundation sallyom authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 6, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 6, 2019

@sallyom: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 909300b link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.