-
Notifications
You must be signed in to change notification settings - Fork 32
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
Adjust SRIOV MTU test #2527
Adjust SRIOV MTU test #2527
Conversation
from change #2527: |
from change #2527: |
Tested with a workload using SRIOV. Even though the job passed, the test is still failing: https://www.distributed-ci.io/jobs/48058ea7-5a50-4971-a4d3-72cb37287b16/tests/414d16ad-982b-4819-8349-40aee6e2ce95?testcase=networking-network-attachment-definition-sriov-mtu
Some logs from certsuite execution:
|
Also, some more info that may be interesting for you @sebrandon1 . I think that, starting in OCP 4.14, you can see some details in the pod network annotations regarding SRIOV config, because here, in the
Maybe the code you need here would be simpler, something like:
What do you think? |
8b680c9
to
f836639
Compare
from change #2527: |
The result is now correct in a setup that is not using SRIOV, let me check in a cluster with a workload consuming from SRIOV resources. |
from change #2527: |
Our example-cnf workload is still failing the MTU test:
The error is the same, it looks like it's not able to read the JSON input:
|
I'll adjust to look at the |
Cool, feel free to reach me when you want to test it. I think the most suitable way of executing the test would be 1) check if the network is SRIOV type, as you're doing right now, and 2) if it's not SRIOV type, then this is skipped, but if it's SRIOV type, then check the network annotation of the pod, find that network, and see if mtu is defined there. It should work in that way. |
f836639
to
96b5d2c
Compare
from change #2527: |
96b5d2c
to
d77e646
Compare
from change #2527: |
@ramperher Okay I pushed a new change. The logic goes something like:
I think I covered those scenarios in the unit tests by spoofing the client and using fake objects. |
d77e646
to
1226483
Compare
from change #2527: |
1226483
to
b82e4c3
Compare
from change #2527: |
/dci-rerun |
from change #2527: |
2e4a7be
to
81f15d5
Compare
from change #2527: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from my comment regarding the mtu check, I left some suggestions to remove the nolint:gocritc comments.
61ba728
to
9894033
Compare
from change #2527: |
9894033
to
925187c
Compare
from change #2527: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've re-validated the new refactor and it's working as expected, LGTM
14b6927
to
2fb3602
Compare
from change #2527: |
Just checked and confirmed that the test is still working fine and it is |
9a10327
to
1d78382
Compare
from change #2527: |
6f95071
into
redhat-best-practices-for-k8s:main
from change #2527: |
Thanks to @ramperher who pointed out that the
mtu
value we were looking for in the prior iteration of the #2514 SRIOV MTU test was looking in the wrong place for the MTU value.There is a
network-status
annotation that (if matching the NAD) can display the mtu value. So if the existing check fails, we search through thenetwork-status
annotation to see if that JSON has the mtu value.