improve log for pod deletion poll loop #49597
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/S
|
/test-all |
|
/test pull-kubernetes-kubemark-e2e-gce |
|
/test pull-kubernetes-e2e-gce-etcd3 |
1 similar comment
|
/test pull-kubernetes-e2e-gce-etcd3 |
|
+1 Logging before |
| @@ -764,28 +764,28 @@ func waitForServiceAccountInNamespace(c clientset.Interface, ns, serviceAccountN | |||
| } | |||
|
|
|||
| func WaitForPodCondition(c clientset.Interface, ns, podName, desc string, timeout time.Duration, condition podCondition) error { | |||
| Logf("Waiting up to %[1]v for pod %[2]s status to be %[3]s", timeout, podName, desc) | |||
| Logf("Waiting up to %v for pod %q in namespace %q to be %q", timeout, podName, ns, desc) | |||
There was a problem hiding this comment.
I think no because leaving "status" in this message is misleading since the insert used is the passed-in desc. The passed in condition func may use the pod's Status to decide when to stop the loop but we don't know that here.
There was a problem hiding this comment.
I guess it's kind of strange because in the terminated case, the desc string == ""
There was a problem hiding this comment.
When the loop ends for the pod-delete calls, Status.Reason is typically "" but sometimes I see other reasons. The desc value is passed in by the caller and meant to be descriptive of the condition AFAICT. I've created a new pr to deal with better detection of pod deletion and in this pr the reason parm will not be checked if it is "".
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@spiffxp Any chance you can take a look and approve? I have the lgtm and reviews and all tests pass. I'd like this in so I can get better logging on some gci-gce-slow flakes. Thanks. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeffvance, msau42, spiffxp Associated issue: 49529 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 49619, 49598, 47267, 49597, 49638) |
Automatic merge from submit-queue (batch tested with PRs 49651, 49707, 49662, 47019, 49747) improve detectability of deleted pods **What this PR does / why we need it**: Adds comment to `waitForPodTerminatedInNamespace` to better explain how it's implemented. ~~It improves pod deletion detection in the e2e framework as follows:~~ ~~1. the `waitForPodTerminatedInNamespace` func looks for pod.Status.Phase == _PodFailed_ or _PodSucceeded_ since both values imply that all containers have terminated.~~ ~~2. the `waitForPodTerminatedInNamespace` func also ignores the pod's Reason if the passed-in `reason` parm is "". Reason is not really relevant to the pod being deleted or not, but if the caller passes a non-blank `reason` then it will be lower-cased, de-blanked and compared to the pod's Reason (also lower-cased and de-blanked). The idea is to make Reason checking more flexible and to prevent a pod from being considered running when all of its containers have terminated just because of a Reason mis-match.~~ Releated to pr [49597](#49597) and issue [49529](#49529). **Release note**: ```release-note NONE ```
What this PR does / why we need it:
It improves some logging related to waiting for a pod to reach a passed-in condition. Specifically, related to issue 49529 where better logging may help to debug the root cause.
Release note: