Changed code to improve output for files under test/e2e/node #106038
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @NikhilSharmaWe. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-priority
|
@pohly I think the code for output is fine in this case, please confirm. https://github.com/kubernetes/kubernetes/blob/master/test/e2e/node/runtimeclass.go#L62 |
| @@ -360,6 +360,8 @@ func getNpdPodStat(f *framework.Framework, nodeName string) (cpuUsage, rss, work | |||
| hasNpdPod = true | |||
| break | |||
| } | |||
| framework.ExpectEqual(hasNpdPod, true) | |||
| if !hasNpdPod { | |||
| framework.Failf("No node-problem-detector pod is present in %v", summary.Pods) | |||
There was a problem hiding this comment.
how the output will look like on failure, curious how %v will format this?
There was a problem hiding this comment.
When I was not using any formatting directive in the Failf function an error was shown that Failf needs format directive to run. So, I added %v in this the output statement. I think %v will print list of pods present and no npd pod will be present in that list.
There was a problem hiding this comment.
When I was not using any formatting directive in the Failf function an error was shown that Failf needs format directive to run. So, I added %v in this the output statement. I think %v will print list of pods present and no npd pod will be present in that list.
There was a problem hiding this comment.
why not %+q? Also the structure will be huge, this is why I wonder how it will look like and whether it will be readable at all.
There was a problem hiding this comment.
Actually I am unable to figure out how to see the output which function throws if condition follows, can you help me to know how it can be done.
There was a problem hiding this comment.
Here's a short test program:
package main
import (
"fmt"
"k8s.io/kubelet/pkg/apis/stats/v1alpha1"
)
func main() {
summary := v1alpha1.Summary{}
for i := 0; i < 10; i++ {
summary.Pods = append(summary.Pods, v1alpha1.PodStats{
PodRef: v1alpha1.PodReference{
Name: fmt.Sprintf("pod-%d", i),
},
})
}
fmt.Printf("%v", summary.Pods)
}
I can't be run in the Go playground (downloading the imports times out), otherwise I would link to that.
It prints:
[{{pod-0 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-1 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-2 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-3 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-4 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-5 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-6 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-7 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-8 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>} {{pod-9 } 0001-01-01 00:00:00 +0000 UTC [] <nil> <nil> <nil> [] <nil> <nil>}]
It's a bit better with %+v:
[{PodRef:{Name:pod-0 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-1 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-2 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-3 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-4 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-5 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-6 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-7 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-8 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>} {PodRef:{Name:pod-9 Namespace: UID:} StartTime:0001-01-01 00:00:00 +0000 UTC Containers:[] CPU:<nil> Memory:<nil> Network:<nil> VolumeStats:[] EphemeralStorage:<nil> ProcessStats:<nil>}]
Not particularly readable, but IMHO still better than nothing. I don't think a helper function that extracts just the names is justified.
There was a problem hiding this comment.
@NikhilSharmaWe can you please change the formatter to %+v. @pohly thank you for providing an easy way to validate this change!
SergeyKanzhelev
moved this from Triage
to PRs Waiting on Author
in SIG Node CI/Test Board
|
/test |
|
@SergeyKanzhelev: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
hm, I don't think this test may run on PR, it only runs here: https://testgrid.k8s.io/sig-node-node-problem-detector#ci-npd-e2e-kubernetes-gce-gci so the easiest way to test it is to run this test locally. The idea of the PR is right. My only concern is the size and formatting of the proposed output. Perhaps just keeping pod names would be sufficient. |
Could you please explain what changes the PR needs exactly. |
NikhilSharmaWe
force-pushed
the
betterOutputNode
branch
from
e87cf7f
to
0316542
Compare
|
Looks good to me, but I'll let @SergeyKanzhelev decide whether he wants this as failure output. |
|
@SergeyKanzhelev did that (changed %v to %+v) already. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: NikhilSharmaWe, SergeyKanzhelev 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 |
k8s-ci-robot
added
the
approved
|
/kind cleanup |
k8s-ci-robot
added
kind/cleanup
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@SergeyKanzhelev could you please sponsor my application to become a member of Kubernetes org. |
Looked at a few of your merged PRs, looks good, I can support. Please keep up the good work and expand the scope of contributions. |
What type of PR is this?
Enhancement
What this PR does / why we need it:
For better (more informative) output for developers when test fails. Changed files are under test/e2e/node for this PR.
Which issue(s) this PR fixes:
Part of #105678
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: