Skip to content
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

SR-IOV: match on PCI address, don't do runtime config #31

Merged
merged 1 commit into from Dec 17, 2020
Merged

Conversation

fnordahl
Copy link
Contributor

Populate the sriov-netplan-shim interfaces.yaml config with PCI
address of interfaces. This is necessary as the VFs may be
configured at a time in the boot sequence where interface naming
has not settled yet.

Stop doing runtime reconfiguration of SR-IOV, in addition to being
detrimental to any virtual machine instance consuming the VF this
will break NIC firmware in some configurations.

Merge after:
juju/charm-helpers#549

Closes-Bug: #1908351

@fnordahl fnordahl force-pushed the bug/1908351 branch 3 times, most recently from 8b3f34e to 2ca8bc6 Compare December 17, 2020 07:58
# configuration of Virtual Functions (VFs) in the system.
#
# The charm does not do run-time configuration of VFs as this
# would be detrimental to any instances consuming the VFs. In some
# confiugrations it would also break NIC firmware LP: #1908351.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/confiugrations/configurations/

config.yaml Show resolved Hide resolved
@@ -61,7 +61,7 @@ def test_dpdk_device(self):
self.assertEquals(self.target.dpdk_device.driver, 'fakedriver')

def test_sriov_device(self):
self.assertDictEqual(self.target.sriov_device,
self.assertDictEqual(self.target.sriov_device(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing the legacy behavior. I guess we could have one more such tiny test covering the new get_map()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, testing the innards of the SRIOV context belongs to the unit tests in charm helpers. If anything we should probably remove this test here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I thought about that, on the other hand this kind of tests is good to catch non-backward-compatible changes done to charmhelpers. You could imagine someone changing the behavior of the called function and changing the corresponding test in charmhelpers, so that it's passing there in charmhelpers and lands. It would then turn red here and make obvious that something has to be done in this layer.

I don't mind too much though, as you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test mocks the context in its entirety, so all it tests is that when you access the property you get a object that comes from a given location. The return value is defined in the test itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah true 🤐

Populate the sriov-netplan-shim interfaces.yaml config with PCI
address of interfaces. This is necessary as the VFs may be
configured at a time in the boot sequence where interface naming
has not settled yet.

Stop doing runtime reconfiguration of SR-IOV, in addition to being
detrimental to any virtual machine instance consuming the VF this
will break NIC firmware in some configurations.

Merge after:
juju/charm-helpers#549

Closes-Bug: #1908351
@lourot lourot merged commit 3d10cec into master Dec 17, 2020
coreycb pushed a commit to coreycb/charm-layer-ovn that referenced this pull request Jan 28, 2021
Populate the sriov-netplan-shim interfaces.yaml config with PCI
address of interfaces. This is necessary as the VFs may be
configured at a time in the boot sequence where interface naming
has not settled yet.

Stop doing runtime reconfiguration of SR-IOV, in addition to being
detrimental to any virtual machine instance consuming the VF this
will break NIC firmware in some configurations.

Merge after:
juju/charm-helpers#549
openstack-charmers#31

Change-Id: I397a2a82501c565724d6fce6b49f7b68d57d47fd
Closes-Bug: #1908351
@fnordahl fnordahl deleted the bug/1908351 branch April 7, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants