kubectl/pkg/drain: Include namespace in evictPods return error

[wking]

And also in the terminating-namespace log output. This makes it easier to track down drain-blocking pods, without having to hunt around in earlier logs for evicting pod ... messages. Before this change, caller logs might look like:

evicting pod {namespace}/{name}
...
error when waiting for pod "{name}" terminating: global timeout reached: 20s

With this change, they will look like:

evicting pod {namespace}/{name}
...
error when waiting for pod "{name}" in namespace "{namespace}" to terminate: global timeout reached: 20s

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Makes it easier to identify and dig into drain-blocking pods.

Does this PR introduce a user-facing change?

NONE

[k8s-ci-robot]

Hi @wking. 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.

[pacoxu]

/ok-to-test

[pacoxu]

I am not good at this, and only want to make it brief.

Suggested change

	returnCh <- fmt.Errorf("error when waiting for pod %q in namespace %q to terminate: %v", pod.Name, pod.Namespace, err)
	returnCh <- fmt.Errorf("error when waiting for pod %q/%q terminating: %v", pod.Namespace, pod.Name, err)

[wking]

We expect the {namespace}/{name} semantics to be implicitly clear without surrounding words? There are other */{name} conventions around, such as:

$ kubectl get --help | grep /NAME
  kubectl get [(-o|--output=)j...] (TYPE[.VERSION][.GROUP] [NAME | -l label] | TYPE[.VERSION][.GROUP]/NAME ...) [flags] [options]

and while most error-message consumers will be able to distinguish {namespace}/{name} from {type}/{name}, the fact that folks will see both makes me personally prefer using a separate word in the error message to make it clear which string is the namespace.

[pacoxu]

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/clusterinfo/clusterinfo_dump.go

Lines 268 to 270 in fca9f37

	writer.Write([]byte(fmt.Sprintf("==== START logs for container %s of pod %s/%s ====\n", container.Name, pod.Namespace, pod.Name)))
	defer writer.Write([]byte(fmt.Sprintf("==== END logs for container %s of pod %s/%s ====\n", container.Name, pod.Namespace, pod.Name)))

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/drain/drain.go

Lines 173 to 174 in fca9f37

	fmt.Fprintf(o.Out, "pod %s/%s %s\n", pod.Namespace, pod.Name, verbStr)
	} else {

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/run/run.go

Lines 412 to 413 in fca9f37

	Err: fmt.Errorf("pod %s/%s terminated (%s)\n%s", pod.Namespace, pod.Name, pod.Status.ContainerStatuses[0].State.Terminated.Reason, pod.Status.ContainerStatuses[0].State.Terminated.Message),
	Code: int(rc),

This style is widely used in the current code base.

And pod <ns>/<name> is a warning message, not a structured output.

In kubectl get <x>/<y>: this is not a log but a command, for the input part, the x has to be type.

[pacoxu]

If this concerns you, I would suggest doing a cleanup on the current code base(before it, I think this should be discussed in sig-cli.)

[pacoxu]

Suggested change

	fmt.Fprintf(d.ErrOut, "error when evicting pod %q from terminating namespace %q (will retry after 5s): %v\n", activePod.Name, activePod.Namespace, err)
	fmt.Fprintf(d.ErrOut, ""error when evicting pod %q/%q (will retry after 5s): %v\n", activePod.Namespace, activePod.Name, err)

[wking]

We don't think that the fact that the namespace is terminating is interesting? If someone is investigating the logs for "why is ${POD} failing to drain?", it seemed like ...terminating namespace "${NAMESPACE}"... would help them focus on why that namespace termination was slow, instead of wasting their time poking at the pod specifics.

[pacoxu]

This makes sense to me.

[soltysh]

/triage accepted
/priority backlog
/approve
/assign @pacoxu
for final tag after his comments are addressed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• staging/src/k8s.io/kubectl/OWNERS [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pacoxu]

/lgtm
Adding namespace to the log makes sense and the log style may have no explicit requirements on ns/pod. So I would lgtm to this PR and we may update the code base if <namespace>/<podName> is not the best practice (we should re-think about it?).

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: e610cc4f8eedbb8797e94180346f3927ffca5059