Fix device-plugin-test flaky test#138289
Conversation
|
Please note that we're already in Code Freeze for the upcoming v1.36.0 release. Adding the milestone to this PR is strictly prohibited without proper approval. If this PR needs to be included in the v1.36.0 release:
We're also in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Apr 9 11:32:12 UTC 2026. |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: esotsal The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test pull-kubernetes-node-kubelet-serial-device-manager |
ffromani
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'm not sure I got the intent correctly though, question inline.
test/e2e_node/device_plugin_test.go
Outdated
| assignedStr := strings.Join(assigned.UnsortedList(), ",") | ||
| framework.Logf("%s: devices expected %q assigned %q", ident, expectedStr, assignedStr) | ||
| if !assigned.Equal(expected) { | ||
| if assignedStr != expectedStr { |
There was a problem hiding this comment.
the strings are built from unsorted list (arguably a UX mistake) so they can be syntactically different while expressing the same set. OTOH, Equal() previously called was supposed to handle compare two sets semantically.
I'm not sure this is the right fix.
There was a problem hiding this comment.
Good point, checking the golang library used, the calls will used the following
I don't get it why the assignedStr and expectedStr printed is empty but the Equal fails? Will continue digging into this. I see in other device-plugin tests that a clean up is done before the tests perhaps this might be worth investigating.
There was a problem hiding this comment.
A common gotcha we encountered is when one object is nil and the other is empty but non-nil. The string representation is the same ("") but the Equality check fails, depending on the implementation. I didn't check if this is the case here.
It it is, the fix is to make sure we always compare non-nil objects (but possibly empty objects)
There was a problem hiding this comment.
Thanks for explaining.
9102ae9 to
7be1f6d
Compare
|
/test pull-kubernetes-node-kubelet-serial-device-manager |
test/e2e_node/device_plugin_test.go
Outdated
| assignedStr := strings.Join(assigned.UnsortedList(), ",") | ||
| framework.Logf("%s: devices expected %q assigned %q", ident, expectedStr, assignedStr) | ||
| if !assigned.Equal(expected) { | ||
| if !expected.Equal(assigned) { |
There was a problem hiding this comment.
If a == b, then certainly b == a? What is the type that is being used here?
7be1f6d to
6c23520
Compare
|
@esotsal: The following tests failed, say
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. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
If assignedStr and expectedStr is empty this test fails. This commit tries to fix this
6c23520 to
f96b34d
Compare
What type of PR is this?
/kind bug
/kind failing-test
What this PR does / why we need it:
If assignedStr and expectedStr is empty this test fails.
This commit tries to fix this using assignedStr and expectedStr instead of assigned and expected.
Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
A failed test demonstrating the problem can be found here
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: