diff --git a/nova/objects/pci_device.py b/nova/objects/pci_device.py index fca59b5199f8..fd9a56adbd12 100644 --- a/nova/objects/pci_device.py +++ b/nova/objects/pci_device.py @@ -177,11 +177,22 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): # - "parent_ifname": the netdev name of the parent (PF) # device of a VF # - "mac_address": the MAC address of the PF + # - "managed": "true"/"false" if the device is managed by + # hypervisor extra_info = self.extra_info data = v if isinstance(v, str) else jsonutils.dumps(v) extra_info.update({k: data}) self.extra_info = extra_info + # Remove the "managed" key if it was set previously + # As with the previous case, we must explicitly assign to + # self.extra_info so that obj_what_changed detects the modification + # and triggers a save later. + if "managed" not in dev_dict and "managed" in self.extra_info: + extra_info = self.extra_info + del extra_info["managed"] + self.extra_info = extra_info + def __init__(self, *args, **kwargs): super(PciDevice, self).__init__(*args, **kwargs) diff --git a/nova/pci/devspec.py b/nova/pci/devspec.py index 386005c8eb0b..ebe667e2a7ad 100644 --- a/nova/pci/devspec.py +++ b/nova/pci/devspec.py @@ -317,6 +317,15 @@ class PciDeviceSpec(PciAddressSpec): address_obj = self._address_obj() self._remote_managed = strutils.bool_from_string( self.tags.get(PCI_REMOTE_MANAGED_TAG)) + + if self.tags.get("managed", None) is not None: + try: + self.tags["managed"] = ( + "true" if strutils.bool_from_string( + self.tags.get("managed"), strict=True) else "false" + ) + except ValueError as e: + raise exception.PciConfigInvalidSpec(reason=str(e)) if self._remote_managed: if address_obj is None: # Note that this will happen if a netdev was specified in the @@ -399,3 +408,8 @@ class PciDeviceSpec(PciAddressSpec): def get_tags(self) -> ty.Dict[str, str]: return self.tags + + def enhanced_pci_device_with_spec_tags(self, dev: ty.Dict[str, ty.Any]): + managed = self.tags.get("managed") + if managed is not None: + dev.update({'managed': managed}) diff --git a/nova/pci/manager.py b/nova/pci/manager.py index 6168bb7c79ef..3f3093bb8bce 100644 --- a/nova/pci/manager.py +++ b/nova/pci/manager.py @@ -131,7 +131,12 @@ class PciDevTracker(object): devices = [] for dev in jsonutils.loads(devices_json): try: - if self.dev_filter.device_assignable(dev): + pci_dev_spec = self.dev_filter.device_assignable(dev) + if pci_dev_spec is not None: + # Since some configuration parameters cannot be + # discovered by the driver, we need to add them from + # the device specification provided by the operator. + pci_dev_spec.enhanced_pci_device_with_spec_tags(dev) devices.append(dev) except exception.PciConfigInvalidSpec as e: # The raised exception is misleading as the problem is not with diff --git a/nova/pci/whitelist.py b/nova/pci/whitelist.py index 152cc29ca65c..5847640ba7f3 100644 --- a/nova/pci/whitelist.py +++ b/nova/pci/whitelist.py @@ -82,15 +82,20 @@ class Whitelist(object): return specs - def device_assignable(self, dev: ty.Dict[str, ty.Any]) -> bool: - """Check if a device can be assigned to a guest. + def device_assignable( + self, dev: ty.Dict[str, ty.Any] + ) -> ty.Optional[devspec.PciDeviceSpec]: + """Check if a device is part of pci device_spec (whitelist) and so + can be assigned to a guest. + If yes return the spec, else return None :param dev: A dictionary describing the device properties + :return: A devspec.PciDeviceSpec or None """ for spec in self.specs: if spec.match(dev): - return True - return False + return spec + return None def get_devspec( self, pci_dev: 'objects.PciDevice', diff --git a/nova/tests/unit/objects/test_pci_device.py b/nova/tests/unit/objects/test_pci_device.py index e0570b69a8ef..741e8a67af54 100644 --- a/nova/tests/unit/objects/test_pci_device.py +++ b/nova/tests/unit/objects/test_pci_device.py @@ -190,6 +190,39 @@ class _TestPciDeviceObject(object): self.assertEqual(self.pci_device.obj_what_changed(), set(['vendor_id', 'product_id', 'parent_addr'])) + def test_update_and_remove_extra_info_key(self): + self.dev_dict = copy.copy(dev_dict) + self.dev_dict['managed'] = "true" + self.pci_device = pci_device.PciDevice.create(None, self.dev_dict) + + self.pci_device.obj_reset_changes() + changes = {'managed': 'no'} + self.pci_device.update_device(changes) + self.assertEqual( + self.pci_device.obj_what_changed(), + set( + [ + "parent_addr", + "extra_info" + ] + ), + ) + self.assertEqual(self.pci_device.extra_info, {"managed": "no"}) + + self.pci_device.obj_reset_changes() + changes = {} + self.pci_device.update_device(changes) + self.assertEqual( + self.pci_device.obj_what_changed(), + set( + [ + "parent_addr", + "extra_info" + ] + ), + ) + self.assertNotIn("managed", self.pci_device.extra_info) + def test_update_device_same_value(self): self.pci_device = pci_device.PciDevice.create(None, dev_dict) self.pci_device.obj_reset_changes() diff --git a/nova/tests/unit/pci/test_manager.py b/nova/tests/unit/pci/test_manager.py index 2cc9de253eea..2061b43a4fa8 100644 --- a/nova/tests/unit/pci/test_manager.py +++ b/nova/tests/unit/pci/test_manager.py @@ -56,6 +56,18 @@ fake_pci_5 = dict(fake_pci, address='0000:00:02.2', dev_type=fields.PciDeviceType.SRIOV_VF, parent_addr='0000:00:01.1', vendor_id='v2', product_id='p2', numa_node=None) +fake_pci_6 = dict(fake_pci, address='0000:00:02.2', + dev_type=fields.PciDeviceType.SRIOV_VF, + parent_addr='0000:00:00.1', + vendor_id='10c9', product_id='8086', numa_node=None) +fake_pci_7 = dict(fake_pci, address='0000:00:02.3', + dev_type=fields.PciDeviceType.SRIOV_VF, + parent_addr='0000:00:00.1', + vendor_id='10c9', product_id='8086', numa_node=None) +fake_pci_8 = dict(fake_pci, address='0000:00:02.4', + dev_type=fields.PciDeviceType.SRIOV_VF, + parent_addr='0000:00:00.1', + vendor_id='10c9', product_id='8086', numa_node=None) fake_pci_devs_tree = [fake_pci_3, fake_pci_4, fake_pci_5] fake_db_dev = { @@ -220,16 +232,92 @@ class PciDevTrackerTestCase(test.NoDBTestCase): self.assertIsNone(vf.parent_device) self.assertEqual([], vf.child_devices) - @mock.patch('nova.pci.whitelist.Whitelist.device_assignable', - return_value=True) - def test_update_devices_from_hypervisor_resources(self, _mock_dev_assign): - fake_pci_devs = [copy.deepcopy(fake_pci_4), copy.deepcopy(fake_pci_5)] + # Mocking as a vf to avoid + # os.path.isdir on /sys/bus/pci/devices/0000:00:02.2 + # which is not appropriate for tests + @mock.patch("nova.pci.utils.is_physical_function", return_value=False) + def test_update_devices_from_hypervisor_resources(self, mock_vf): + self.flags( + group="pci", + device_spec=[ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3"}', + ], + ) + # fake_pci_4 and fake_pci_5 vendor id 'v2' is not valid so use + # fake_pci_6 and fake_pci_7 instead + fake_pci_devs = [copy.deepcopy(fake_pci_6), copy.deepcopy(fake_pci_7)] fake_pci_devs_json = jsonutils.dumps(fake_pci_devs) tracker = manager.PciDevTracker( self.fake_context, objects.ComputeNode(id=1, numa_topology=None)) tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) self.assertEqual(5, len(tracker.pci_devs)) + @mock.patch("nova.pci.utils.is_physical_function", return_value=False) + def test_update_devices_from_hypervisor_resources_with_managed( + self, _mock_vf + ): + self.flags( + group="pci", + device_spec=[ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2", "managed":"no"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3", "managed":"yes"}', + ], + ) + fake_pci_devs = [copy.deepcopy(fake_pci_6), copy.deepcopy(fake_pci_7)] + fake_pci_devs_json = jsonutils.dumps(fake_pci_devs) + tracker = manager.PciDevTracker( + self.fake_context, objects.ComputeNode(id=1, numa_topology=None) + ) + tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) + self.assertEqual(5, len(tracker.pci_devs)) + + pci_addr_extra_info = { + dev.address: dev.extra_info for dev in tracker.pci_devs + } + + self.assertEqual( + pci_addr_extra_info["0000:00:02.2"]["managed"], "false" + ) + self.assertEqual( + pci_addr_extra_info["0000:00:02.3"]["managed"], "true" + ) + + @mock.patch("nova.pci.manager.LOG.debug") + @mock.patch("nova.pci.utils.is_physical_function", return_value=False) + def test_update_devices_from_hypervisor_resources_with_managed_invalid( + self, _mock_vf, mock_debug + ): + self.flags( + group="pci", + device_spec=[ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2", "managed":"no"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3", "managed":"yes"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.4", "managed":"invalid"}', + ], + ) + + exc = self.assertRaises( + exception.PciConfigInvalidSpec, + manager.PciDevTracker, + self.fake_context, + objects.ComputeNode(id=1, numa_topology=None), + ) + + self.assertEqual( + "Invalid [pci]device_spec config: Unrecognized value 'invalid', " + "acceptable values are: '0', '1', 'f', 'false', 'n', 'no', 'off', " + "'on', 't', 'true', 'y', 'yes'", + str(exc) + ) + @mock.patch("nova.pci.manager.LOG.debug") def test_update_devices_from_hypervisor_resources_32bit_domain( self, mock_debug): diff --git a/nova/tests/unit/pci/test_whitelist.py b/nova/tests/unit/pci/test_whitelist.py index 2f6c185ec5e2..3c85c37ce2a2 100644 --- a/nova/tests/unit/pci/test_whitelist.py +++ b/nova/tests/unit/pci/test_whitelist.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +from nova import exception from nova.pci import whitelist from nova import test @@ -81,3 +82,59 @@ class WhitelistTestCase(test.NoDBTestCase): white_list = '{"product_id":"0001", "vendor_id":"8086"}' parsed = whitelist.Whitelist([white_list]) self.assertTrue(parsed.device_assignable(dev_dict)) + + def test_device_managed(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086", "managed": "yes"}' + ) + parsed = whitelist.Whitelist([white_list]) + self.assertEqual(1, len(parsed.specs)) + self.assertTrue(parsed.specs[0].tags["managed"], "true") + + def test_device_managed_true(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086", "managed": "true"}' + ) + parsed = whitelist.Whitelist([white_list]) + self.assertEqual(1, len(parsed.specs)) + self.assertTrue(parsed.specs[0].tags["managed"], "true") + + def test_device_managed_int(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086", "managed": 1}' + ) + parsed = whitelist.Whitelist([white_list]) + self.assertEqual(1, len(parsed.specs)) + self.assertTrue(parsed.specs[0].tags["managed"], "true") + + def test_device_not_managed(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086", "managed": "no"}' + ) + parsed = whitelist.Whitelist([white_list]) + self.assertEqual(1, len(parsed.specs)) + self.assertTrue(parsed.specs[0].tags["managed"], "false") + + def test_device_managed_not_set(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086"}' + ) + parsed = whitelist.Whitelist([white_list]) + self.assertEqual(1, len(parsed.specs)) + self.assertNotIn("managed", parsed.specs[0].tags) + + def test_device_managed_invalid_value(self): + white_list = ( + '{"product_id":"0001", "vendor_id":"8086", "managed": "invalid"}' + ) + + exc = self.assertRaises( + exception.PciConfigInvalidSpec, whitelist.Whitelist, [white_list] + ) + + self.assertEqual( + "Invalid [pci]device_spec config: Unrecognized value 'invalid', " + "acceptable values are: '0', '1', 'f', 'false', 'n', 'no', 'off', " + "'on', 't', 'true', 'y', 'yes'", + str(exc) + )