-
Notifications
You must be signed in to change notification settings - Fork 205
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
sriov: accept setting the eswitch mode without VFs (LP: #2020409) #454
sriov: accept setting the eswitch mode without VFs (LP: #2020409) #454
Conversation
a784da7
to
ef39285
Compare
ef39285
to
936af5c
Compare
The embedded switch mode can be set even if the interface doesn't have any virtual functions. This is a requisite to use the PF with only scalable functions for example. With this change, the presence of the 'embedded-switch-mode' will be enough to identify the interface as a PF and emit the systemd services required to apply the configuration. The changes required by the CLI will be done is the next commit.
Refactor get_vf_count_and_functions() in 3 different ones. It was doing too many things and was hard to read. Three of its 5 parameters were being used as output and one of them was being changed by _get_target_interface() in the same call chain. The new _get_physical_functions() will also return interfaces that only have the property embedded_switch_mode. This is a requirement to enable netplan to change the eswitch mode even if it doesn't have VFs. See LP: #2020409 Adapt the unit tests to the new functions and implement new tests.
936af5c
to
bdce7ff
Compare
d8dcab7
to
cf31ec0
Compare
The call to pcidev.devlink_set was still inside a check for the existence of VFs. Move it to outside of it. Add a test to cover the case where the unbind_vfs operation fails.
cf31ec0
to
391b7e5
Compare
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.
Thank you for the excellent testing story!
Also, I like the refactoring & breakdown into smaller methods a lot!
LGTM. I left some minor inline comments, it's up to you if you want to address those.
for interface in interfaces: | ||
if not _interface_matches(netdef, interface): | ||
continue | ||
# we have a matching PF | ||
# error out if we matched more than one | ||
if len(matches) > 1: | ||
raise ConfigurationError('matched more than one interface for a PF device: %s' % netdef.id) | ||
matches.add(interface) | ||
if matches: | ||
return list(matches)[0] |
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.
suggestion (non-blocking): I think some of this could be replaced by the utils.find_matching_iface()
method. Error handling cannot be clearly translated, though.
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.
this method returns None if it either found more than one match or no matches at all. Based on the current logic I need to know if there's multiple matches. But yeah I understand there some code duplication going on here... maybe it's a task for a future refactoring :|
# we walk through all the system interfaces to determine if there is | ||
# more than one matched interface | ||
for interface in interfaces: | ||
if not _interface_matches(netdef, interface): |
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.
question (non-blocking): Why not just call netdef._match_interface()
directly here?
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 don't have a strong opinion here, but I don't know it looks cleaner with netdef._match_interface
wrapped in this method...
The embedded switch mode can be set even if the interface doesn't have any virtual functions. This is a requisite to use the PF with only scalable functions for example.
With this change, the presence of the 'embedded-switch-mode' will be enough to identify the interface as a PF and emit the systemd services required to apply the configuration.
The changes required by the CLI will be done is the next commit.
A PPA for Noble is available here https://launchpad.net/~danilogondolfo/+archive/ubuntu/netplan-sriov
Description
I tested some scenarios on real hardware to make sure the changes work as intended and the existing features are not broken.
Test 1: basic configuration - PFs without VFs (the use case for scalable functions)
It tests if setting the embedded switch mode will now work when the PFs don’t have VFs (when SR-IOV is disabled basically).
Initial state:
The eswitch mode legacy is set to both interfaces
Applying the configuration below will change the eswitch mode to switchdev. Note that these PFs don’t have VFs.
After calling netplan applythe eswitch mode will be changed
Test 2: change the eswitch mode but with VFs
Initial state
Both PFs are on legacy mode with zero VFs.
The configuration below is applied, setting them to switchdev and at the same time creating two VFs:
Result:
Test 3: VF LAG activation still works after these changes
VF LAG requires that the setup happens in a very specific order.
Before creating the LAG interface containing both PFs, the eswitch mode must be set and all the VFs must be unbound. Then after waiting for networkd to setup the bond, netplan rebind needs to be called to bind the VFs to the driver.
To test if it’s working I use the configuration below and reboot the system:
All the settings are applied and the LAG state is active
Checklist
make check
successfully.make check-coverage
).