-
Notifications
You must be signed in to change notification settings - Fork 171
tests/platforms: add test for Azure SR-IOV udev rules #3947
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
base: testing-devel
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request introduces a new test to verify that Azure SR-IOV network interfaces are correctly marked as unmanaged by NetworkManager. The test script is well-structured and the logic is sound. I have one suggestion to improve the efficiency of the script by reducing redundant calls to nmcli within a loop. Overall, this is a valuable addition to the test suite.
8bebbcc to
9e3255c
Compare
travier
left a comment
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.
LGTM. Mostly nits
src/config
Outdated
| @@ -0,0 +1 @@ | |||
| /home/aaradhak/coderepo/fedora-coreos-config No newline at end of file | |||
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.
You probably did not want to include this one
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.
my bad, I ll remove this
|
You also have an override file that you probably don't want there: overrides/rootfs/usr/libexec/ignition-apply |
|
Test itself LGTM. Obviously removing files that we don't want in this PR should happen before merge. @marmijo mind a look as well since you are familiar with the Azure pieces? |
|
Yes, I have removed the unwanted files (it got added here accidentally) and also made the suggested change |
Introduce a new test which verifies that udev rules for Azure SR-IOV network interfaces correctly mark them as unmanaged by NetworkManager. It only runs on Azure and uses Standard_D2s_v3 or larger instance type with Accelerated Networking enabled. The test checks that SR-IOV interfaces (PCI devices with vendor drivers like mlx5_core) have the AZURE_UNMANAGED_SRIOV property set, and that NetworkManager respects this property by leaving them unmanaged.
dustymabe
left a comment
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.
LGTM
marmijo
left a comment
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.
LGTM, with a few comments.
@aaradhak and I tested a slightly different version of this manually together in an Azure VM. I'll try to run cosa kola -p azure with the test locally today.
| if [ -e "$iface/device/driver" ]; then | ||
| driver=$(basename "$(readlink "$iface/device/driver")") | ||
| # SR-IOV interfaces are on PCI bus and not vmbus (hv_netvsc is the synthetic interface) | ||
| if [ "$driver" != "hv_netvsc" ] && [ -e "$iface/device/subsystem" ]; then |
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.
In testing, I think were using mlx5_core here to determine which device to work on. Does this driver also work?
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.
No i dont think we would need the driver hv_netvsc for this test.
| ## platforms: azure | ||
| ## # This test requires an instance type that supports Accelerated Networking (SRIOV) | ||
| ## # Standard_D2s_v3 and larger sizes support Accelerated Networking | ||
| ## instanceType: "Standard_D2s_v3" |
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.
We shouldn't need to declare an instance type since Standard_D2s_v3 is the default for x86_64.
I didn't think about this when we were testing manually, but is SR-IOV networking supported on aarch64?
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.
sounds good, I ll remove this reference.
Introduce a new test which verifies that udev rules for Azure SR-IOV network interfaces correctly mark them as unmanaged by NetworkManager. It only runs on Azure and uses Standard_D2s_v3 or larger instance type with Accelerated Networking enabled.
The test checks that SR-IOV interfaces (PCI devices with vendor drivers like mlx5_core) have the AZURE_UNMANAGED_SRIOV property set, and that NetworkManager respects this property by leaving them unmanaged.