From f9c5f50376181ccc5c5216bdfcb988ead3573e6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Ribaud?= Date: Fri, 14 Feb 2025 18:17:24 +0100 Subject: [PATCH] Add live_migratable flag to PCI device specification The target goal of these series of patch is to enable VFIO devices migration with kernel variant drivers. Partially-Implements: blueprint migrate-vfio-devices-using-kernel-variant-drivers Change-Id: I23af0d36448e9b659f6383d602d9dfa0e2798e60 --- nova/objects/pci_device.py | 12 ++- nova/pci/devspec.py | 30 +++++--- nova/tests/unit/objects/test_pci_device.py | 14 ++-- nova/tests/unit/pci/test_manager.py | 90 +++++++++++++++++----- nova/tests/unit/pci/test_whitelist.py | 72 ++++++++--------- 5 files changed, 143 insertions(+), 75 deletions(-) diff --git a/nova/objects/pci_device.py b/nova/objects/pci_device.py index fd9a56adbd..77a28fb184 100644 --- a/nova/objects/pci_device.py +++ b/nova/objects/pci_device.py @@ -179,6 +179,8 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): # - "mac_address": the MAC address of the PF # - "managed": "true"/"false" if the device is managed by # hypervisor + # - "live_migratable": true/false if the device can be live + # migratable extra_info = self.extra_info data = v if isinstance(v, str) else jsonutils.dumps(v) extra_info.update({k: data}) @@ -188,10 +190,12 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): # 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 + spec_tags = ["managed", "live_migratable"] + for tag in spec_tags: + if tag not in dev_dict and tag in self.extra_info: + extra_info = self.extra_info + del extra_info[tag] + 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 ebe667e2a7..669a223416 100644 --- a/nova/pci/devspec.py +++ b/nova/pci/devspec.py @@ -318,14 +318,9 @@ class PciDeviceSpec(PciAddressSpec): 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)) + self._normalize_device_spec_tag("managed") + self._normalize_device_spec_tag("live_migratable") + if self._remote_managed: if address_obj is None: # Note that this will happen if a netdev was specified in the @@ -409,7 +404,20 @@ class PciDeviceSpec(PciAddressSpec): def get_tags(self) -> ty.Dict[str, str]: return self.tags + def _normalize_device_spec_tag(self, tag): + if self.tags.get(tag, None) is not None: + try: + self.tags[tag] = ( + "true" if strutils.bool_from_string( + self.tags.get(tag), strict=True) else "false") + except ValueError as e: + raise exception.PciConfigInvalidSpec( + reason=f"Cannot parse tag '{tag}': " + str(e) + ) + 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}) + spec_tags = ["managed", "live_migratable"] + for tag in spec_tags: + tag_value = self.tags.get(tag) + if tag_value is not None: + dev.update({tag: tag_value}) diff --git a/nova/tests/unit/objects/test_pci_device.py b/nova/tests/unit/objects/test_pci_device.py index 741e8a67af..53434abd65 100644 --- a/nova/tests/unit/objects/test_pci_device.py +++ b/nova/tests/unit/objects/test_pci_device.py @@ -14,6 +14,7 @@ # under the License. import copy +import ddt from unittest import mock from oslo_serialization import jsonutils @@ -113,6 +114,7 @@ fake_db_dev_old = { } +@ddt.ddt class _TestPciDeviceObject(object): def _create_fake_instance(self): self.inst = instance.Instance() @@ -190,13 +192,15 @@ 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): + @ddt.data({"tag": "managed"}, + {"tag": "live_migratable"}) + def test_update_and_remove_extra_info_key(self, data): self.dev_dict = copy.copy(dev_dict) - self.dev_dict['managed'] = "true" + self.dev_dict[data['tag']] = "true" self.pci_device = pci_device.PciDevice.create(None, self.dev_dict) self.pci_device.obj_reset_changes() - changes = {'managed': 'no'} + changes = {data['tag']: 'no'} self.pci_device.update_device(changes) self.assertEqual( self.pci_device.obj_what_changed(), @@ -207,7 +211,7 @@ class _TestPciDeviceObject(object): ] ), ) - self.assertEqual(self.pci_device.extra_info, {"managed": "no"}) + self.assertEqual(self.pci_device.extra_info, {data['tag']: "no"}) self.pci_device.obj_reset_changes() changes = {} @@ -221,7 +225,7 @@ class _TestPciDeviceObject(object): ] ), ) - self.assertNotIn("managed", self.pci_device.extra_info) + self.assertNotIn(data['tag'], self.pci_device.extra_info) def test_update_device_same_value(self): self.pci_device = pci_device.PciDevice.create(None, dev_dict) diff --git a/nova/tests/unit/pci/test_manager.py b/nova/tests/unit/pci/test_manager.py index 2061b43a4f..0707d49f2a 100644 --- a/nova/tests/unit/pci/test_manager.py +++ b/nova/tests/unit/pci/test_manager.py @@ -14,6 +14,7 @@ # under the License. import copy +import ddt from unittest import mock from oslo_serialization import jsonutils @@ -124,6 +125,7 @@ fake_pci_requests = [ 'spec': [{'vendor_id': 'v1'}]}] +@ddt.ddt class PciDevTrackerTestCase(test.NoDBTestCase): def _create_fake_instance(self): self.inst = objects.Instance() @@ -256,17 +258,54 @@ class PciDevTrackerTestCase(test.NoDBTestCase): 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 + @ddt.data({"tag": "managed"}, + {"tag": "live_migratable"}) + def test_update_devices_from_hypervisor_resources_with_tag( + self, data, _mock_vf ): + spec = [ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2", "' + data["tag"] + '":"no"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3", "' + data["tag"] + '":"yes"}', + ] 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"}', - ], + device_spec=spec, + ) + 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"][data["tag"]], "false") + self.assertEqual( + pci_addr_extra_info["0000:00:02.3"][data["tag"]], "true" + ) + + @mock.patch("nova.pci.utils.is_physical_function", return_value=False) + def test_update_devices_from_hypervisor_resources_with_multiple_tags( + self, _mock_vf + ): + spec = [ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2", "managed":"no", ' + '"live_migratable":"no"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3", "managed":"no", ' + '"live_migratable":"yes"}', + ] + self.flags( + group="pci", + device_spec=spec, ) fake_pci_devs = [copy.deepcopy(fake_pci_6), copy.deepcopy(fake_pci_7)] fake_pci_devs_json = jsonutils.dumps(fake_pci_devs) @@ -284,24 +323,34 @@ class PciDevTrackerTestCase(test.NoDBTestCase): pci_addr_extra_info["0000:00:02.2"]["managed"], "false" ) self.assertEqual( - pci_addr_extra_info["0000:00:02.3"]["managed"], "true" + pci_addr_extra_info["0000:00:02.2"]["live_migratable"], "false" + ) + self.assertEqual( + pci_addr_extra_info["0000:00:02.3"]["managed"], "false" + ) + self.assertEqual( + pci_addr_extra_info["0000:00:02.3"]["live_migratable"], "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 + @ddt.data({"tag": "managed"}, + {"tag": "live_migratable"}) + def test_update_devices_from_hypervisor_resources_with_invalid_tag( + self, data, _mock_vf, mock_debug ): + spec = [ + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.2", "' + data["tag"] + '":"no"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.3", "' + data["tag"] + '":"yes"}', + '{"product_id":"8086", "vendor_id":"10c9", ' + '"address":"0000:00:02.4", "' + data["tag"] + '":"invalid"}', + ] + 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"}', - ], + device_spec=spec, ) exc = self.assertRaises( @@ -312,7 +361,8 @@ class PciDevTrackerTestCase(test.NoDBTestCase): ) self.assertEqual( - "Invalid [pci]device_spec config: Unrecognized value 'invalid', " + "Invalid [pci]device_spec config: Cannot parse tag " + f"'{data['tag']}': Unrecognized value 'invalid', " "acceptable values are: '0', '1', 'f', 'false', 'n', 'no', 'off', " "'on', 't', 'true', 'y', 'yes'", str(exc) diff --git a/nova/tests/unit/pci/test_whitelist.py b/nova/tests/unit/pci/test_whitelist.py index 3c85c37ce2..aec2bd5018 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. +import ddt from nova import exception from nova.pci import whitelist from nova import test @@ -28,6 +29,7 @@ dev_dict = { } +@ddt.ddt class WhitelistTestCase(test.NoDBTestCase): def test_whitelist(self): white_list = '{"product_id":"0001", "vendor_id":"8086"}' @@ -83,57 +85,57 @@ class WhitelistTestCase(test.NoDBTestCase): 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"}' + @ddt.data( + {"tag": "managed", "value": "yes", "expected": "true"}, + {"tag": "managed", "value": "true", "expected": "true"}, + {"tag": "managed", "value": "1", "expected": "true"}, + {"tag": "managed", "value": "no", "expected": "false"}, + {"tag": "managed", "value": "false", "expected": "false"}, + {"tag": "managed", "value": "0", "expected": "false"}, + {"tag": "live_migratable", "value": "yes", "expected": "true"}, + {"tag": "live_migratable", "value": "true", "expected": "true"}, + {"tag": "live_migratable", "value": "1", "expected": "true"}, + {"tag": "live_migratable", "value": "no", "expected": "false"}, + {"tag": "live_migratable", "value": "false", "expected": "false"}, + {"tag": "live_migratable", "value": "0", "expected": "false"}, + ) + def test_device_tags(self, data): + wl = ( + '{"product_id":"0001", "vendor_id":"8086", "' + + data["tag"] + + '":"' + + data["value"] + + '"}' ) + white_list = wl parsed = whitelist.Whitelist([white_list]) self.assertEqual(1, len(parsed.specs)) - self.assertTrue(parsed.specs[0].tags["managed"], "true") + self.assertEqual(parsed.specs[0].tags[data["tag"]], data["expected"]) - 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): + @ddt.data({"tag": "managed"}, + {"tag": "live_migratable"}) + def test_device_managed_not_set(self, data): 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) + self.assertNotIn(data["tag"], parsed.specs[0].tags) - def test_device_managed_invalid_value(self): - white_list = ( - '{"product_id":"0001", "vendor_id":"8086", "managed": "invalid"}' - ) + @ddt.data({"tag": "managed"}, + {"tag": "live_migratable"}) + def test_device_managed_invalid_value(self, data): + wl = '{"product_id":"0001", "vendor_id":"8086", "' + \ + data["tag"] + '":"' + 'invalid"}' + white_list = (wl) exc = self.assertRaises( exception.PciConfigInvalidSpec, whitelist.Whitelist, [white_list] ) self.assertEqual( - "Invalid [pci]device_spec config: Unrecognized value 'invalid', " + "Invalid [pci]device_spec config: Cannot parse tag " + f"'{data['tag']}': Unrecognized value 'invalid', " "acceptable values are: '0', '1', 'f', 'false', 'n', 'no', 'off', " "'on', 't', 'true', 'y', 'yes'", str(exc)