From 5b1d487d7fc49580a606b68e1271cae5f2f2df87 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 8 Apr 2022 11:38:54 +0100 Subject: [PATCH 1/4] db: Resolve additional SAWarning warnings Resolving the following SAWarning warnings: Coercing Subquery object into a select() for use in IN(); please pass a select() construct explicitly SELECT statement has a cartesian product between FROM element(s) "foo" and FROM element "bar". Apply join condition(s) between each element to resolve. While the first of these was a trivial fix, the second one is a little more involved. It was caused by attempting to build a query across tables that had no relationship as part of our archive logic. For example, consider the following queries, generated early in '_get_fk_stmts': SELECT instances.uuid FROM instances, security_group_instance_association WHERE security_group_instance_association.instance_uuid = instances.uuid AND instances.id IN (__[POSTCOMPILE_id_1]) SELECT security_groups.id FROM security_groups, security_group_instance_association, instances WHERE security_group_instance_association.security_group_id = security_groups.id AND instances.id IN (__[POSTCOMPILE_id_1]) While the first of these is fine, the second is clearly wrong: why are we filtering on a field that is of no relevance to our join? These were generated because we were attempting to archive one or more instances (in this case, the instance with id=1) and needed to find related tables to archive at the same time. A related table is any table that references our "source" table - 'instances' here - by way of a foreign key. For each of *these* tables, we then lookup each foreign key and join back to the source table, filtering by matching entries in the source table. The issue here is that we're looking up every foreign key. What we actually want to do is lookup only the foreign keys that point back to our source table. This flaw is why we were generating the second SELECT above: the 'security_group_instance_association' has two foreign keys, one pointing to our 'instances' table but also another pointing to the 'security_groups' table. We want the first but not the second. Resolve this by checking if the table that each foreign key points to is actually the source table and simply skip if not. With this issue resolved, we can enable errors on SAWarning warnings in general without any filters. Conflicts: nova/tests/fixtures/nova.py NOTE(melwitt): The conflict is because change I63f57980e01f472a25821790610f0836f1882a7f (tests: Restore - don't reset - warning filters) is not in Xena. Change-Id: I63208c7bd5f9f4c3d5e4a40bd0f6253d0f042a37 Signed-off-by: Stephen Finucane (cherry picked from commit 8142b9dc47b7096ab9d8180f0b5b1e52d513e2dc) (cherry picked from commit ce2cc54bfe236554badb9f6bf53a958417e5525d) --- nova/db/main/api.py | 7 +++++++ nova/objects/cell_mapping.py | 12 ++++++++---- nova/tests/fixtures/nova.py | 10 ++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/nova/db/main/api.py b/nova/db/main/api.py index 0ae9a453b8d..32a2e2c8e0f 100644 --- a/nova/db/main/api.py +++ b/nova/db/main/api.py @@ -4332,6 +4332,12 @@ def _get_fk_stmts(metadata, conn, table, column, records): fk_column = fk_table.c.id for fk in fk_table.foreign_keys: + if table != fk.column.table: + # if the foreign key doesn't actually point to the table we're + # archiving entries from then it's not relevant; trying to + # resolve this would result in a cartesian product + continue + # We need to find the records in the referring (child) table that # correspond to the records in our (parent) table so we can archive # them. @@ -4378,6 +4384,7 @@ def _get_fk_stmts(metadata, conn, table, column, records): # deque. fk_delete = fk_table.delete().where(fk_column.in_(fk_records)) deletes.appendleft(fk_delete) + # Repeat for any possible nested child tables. i, d = _get_fk_stmts(metadata, conn, fk_table, fk_column, fk_records) inserts.extendleft(i) diff --git a/nova/objects/cell_mapping.py b/nova/objects/cell_mapping.py index 595ec43e480..13551824205 100644 --- a/nova/objects/cell_mapping.py +++ b/nova/objects/cell_mapping.py @@ -279,11 +279,15 @@ def _get_by_project_id_from_db(context, project_id): # SELECT DISTINCT cell_id FROM instance_mappings \ # WHERE project_id = $project_id; cell_ids = context.session.query( - api_db_models.InstanceMapping.cell_id).filter_by( - project_id=project_id).distinct().subquery() + api_db_models.InstanceMapping.cell_id + ).filter_by( + project_id=project_id + ).distinct() # SELECT cell_mappings WHERE cell_id IN ($cell_ids); - return context.session.query(api_db_models.CellMapping).filter( - api_db_models.CellMapping.id.in_(cell_ids)).all() + return context.session.query( + api_db_models.CellMapping).filter( + api_db_models.CellMapping.id.in_(cell_ids) + ).all() @classmethod def get_by_project_id(cls, context, project_id): diff --git a/nova/tests/fixtures/nova.py b/nova/tests/fixtures/nova.py index f8f4cc89f96..5bca8f115ad 100644 --- a/nova/tests/fixtures/nova.py +++ b/nova/tests/fixtures/nova.py @@ -833,6 +833,16 @@ def setUp(self): self.addCleanup(warnings.resetwarnings) + # Enable general SQLAlchemy warnings also to ensure we're not doing + # silly stuff. It's possible that we'll need to filter things out here + # with future SQLAlchemy versions, but that's a good thing + + warnings.filterwarnings( + 'error', + module='nova', + category=sqla_exc.SAWarning, + ) + class ConfPatcher(fixtures.Fixture): """Fixture to patch and restore global CONF. From 30c7180e1e655176952609e80f1f21cf8b6d13a0 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 11 Nov 2021 19:06:56 +0100 Subject: [PATCH 2/4] Add debug log for scheduler weight calculation We have all the weighers enabled by default and each can have its own multiplier making the final compute node order calculation pretty complex. This patch adds some debug logging that helps understanding how the final ordering was reached. Change-Id: I7606d6eb3e08548c1df9dc245ab39cced7de1fb5 (cherry picked from commit 154ab7b2f9ad80fe432d2c036d5e8c4ee171897b) --- nova/weights.py | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/nova/weights.py b/nova/weights.py index a093df03455..b88caaa1862 100644 --- a/nova/weights.py +++ b/nova/weights.py @@ -19,9 +19,14 @@ import abc +from oslo_log import log as logging + from nova import loadables +LOG = logging.getLogger(__name__) + + def normalize(weight_list, minval=None, maxval=None): """Normalize the values in a list between 0 and 1.0. @@ -127,13 +132,40 @@ def get_weighed_objects(self, weighers, obj_list, weighing_properties): for weigher in weighers: weights = weigher.weigh_objects(weighed_objs, weighing_properties) + LOG.debug( + "%s: raw weights %s", + weigher.__class__.__name__, + {(obj.obj.host, obj.obj.nodename): weight + for obj, weight in zip(weighed_objs, weights)} + ) + # Normalize the weights - weights = normalize(weights, - minval=weigher.minval, - maxval=weigher.maxval) + weights = list( + normalize( + weights, minval=weigher.minval, maxval=weigher.maxval)) + + LOG.debug( + "%s: normalized weights %s", + weigher.__class__.__name__, + {(obj.obj.host, obj.obj.nodename): weight + for obj, weight in zip(weighed_objs, weights)} + ) + + log_data = {} for i, weight in enumerate(weights): obj = weighed_objs[i] - obj.weight += weigher.weight_multiplier(obj.obj) * weight + multiplier = weigher.weight_multiplier(obj.obj) + weigher_score = multiplier * weight + obj.weight += weigher_score + + log_data[(obj.obj.host, obj.obj.nodename)] = ( + f"{multiplier} * {weight}") + + LOG.debug( + "%s: score (multiplier * weight) %s", + weigher.__class__.__name__, + {name: log for name, log in log_data.items()} + ) return sorted(weighed_objs, key=lambda x: x.weight, reverse=True) From fdd4e7dc51ad34d3bc4741e7e40a10e8f3a7d138 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Thu, 21 Jul 2022 18:21:51 +0200 Subject: [PATCH 3/4] Reproducer for bug 1951656 Due to a new mdev naming, we can't parse it. Change-Id: I0f785178b132dfef668829558dea9f7e674abadb Related-Bug: #1951656 (cherry picked from commit 185201974775bab966f4e5ca3bbdc31b8269fa4c) (cherry picked from commit 857df72d3166a8f7e8a8cdfeabb62ad6ead46565) (cherry picked from commit 71aa17a487136be5e938192857721d9119222811) --- .../regressions/test_bug_1951656.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 nova/tests/functional/regressions/test_bug_1951656.py diff --git a/nova/tests/functional/regressions/test_bug_1951656.py b/nova/tests/functional/regressions/test_bug_1951656.py new file mode 100644 index 00000000000..9aad191072c --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1951656.py @@ -0,0 +1,83 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_utils import uuidutils + + +from nova.tests.fixtures import libvirt as fakelibvirt +from nova.tests.functional.libvirt import test_vgpu +from nova.virt.libvirt import utils as libvirt_utils + + +class VGPUTestsLibvirt7_7(test_vgpu.VGPUTestBase): + + def _create_mdev(self, physical_device, mdev_type, uuid=None): + # We need to fake the newly created sysfs object by adding a new + # FakeMdevDevice in the existing persisted Connection object so + # when asking to get the existing mdevs, we would see it. + if not uuid: + uuid = uuidutils.generate_uuid() + mdev_name = libvirt_utils.mdev_uuid2name(uuid) + libvirt_parent = self.pci2libvirt_address(physical_device) + + # Libvirt 7.7 now creates mdevs with a parent_addr suffix. + new_mdev_name = '_'.join([mdev_name, libvirt_parent]) + + # Here, we get the right compute thanks by the self.current_host that + # was modified just before + connection = self.computes[ + self._current_host].driver._host.get_connection() + connection.mdev_info.devices.update( + {mdev_name: fakelibvirt.FakeMdevDevice(dev_name=new_mdev_name, + type_id=mdev_type, + parent=libvirt_parent)}) + return uuid + + def setUp(self): + super(VGPUTestsLibvirt7_7, self).setUp() + extra_spec = {"resources:VGPU": "1"} + self.flavor = self._create_flavor(extra_spec=extra_spec) + + # Start compute1 supporting only nvidia-11 + self.flags( + enabled_mdev_types=fakelibvirt.NVIDIA_11_VGPU_TYPE, + group='devices') + + self.compute1 = self.start_compute_with_vgpu('host1') + + def test_create_servers_with_vgpu(self): + + # Create a single instance against a specific compute node. + self._create_server( + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + flavor_id=self.flavor, host=self.compute1.host, + networks='auto', expected_state='ACTIVE') + + # TODO(sbauza): Modify this once bug #1851656 is fixed. + # mdev_name2uuid() raises a badly formed hexadecimal UUID string error + self.assertRaises(ValueError, + self.assert_mdev_usage, + self.compute1, expected_amount=1) + + # Now, the problem is that we can't create new instances with VGPUs + # from this host. + server = self._create_server( + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + flavor_id=self.flavor, host=self.compute1.host, + networks='auto', expected_state='ERROR') + # The error is due to a bad mdev name parsing + self.assertIn('fault', server) + # since we only have one host, we have a RescheduledException as this + # service was creating an exception and we can't use another one. + self.assertIn('Exceeded maximum number of retries', + server['fault']['message']) From c3800f7863116412e90029381085cd01ebeae220 Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Thu, 21 Apr 2022 19:42:27 -0700 Subject: [PATCH 4/4] Handle mdev devices in libvirt 7.7+ Libvirt 7.7 changed the mdev device naming to include the parent PCI device when listing node devices. The domain, however, will still only see the UUID and not see the parent PCI device. Changing the parsing to simply drop the PCI identifier is not enough as the device cannot be found when attempting to lookup the new ID. Modify the Libvirt Driver's _get_mediated_device_information to tolerate different formats of the mdev name. This first uses the legacy behavior by trying to lookup the device name that is passed in (typically mdev_ format) and if that is not found, iterates the list of mdev node devices until the right UUID is found and selects that one. Note that the lookup of the mdev device by UUID are needed in order to keep the ability to recreate assigned mediated devices on a reboot of the compute node. Additionally, the libvirt utils parsing method mdev_name2uuid, has been updated to tolerate both mdev_ and mdev__ formats. Closes-Bug: 1951656 Change-Id: Ifed0fa16053228990a6a8df8d4c666521db7e329 (cherry picked from commit a28b907c4f0dbba6e141a8fbea807e6cb0438977) (cherry picked from commit 98d8c9eaa3c415cc234193e6a9115db887751363) (cherry picked from commit 28053917200e3e242148672efda0e1a2b043dc48) --- .../regressions/test_bug_1951656.py | 22 +++------- nova/tests/unit/virt/libvirt/test_config.py | 26 +++++++++++ nova/virt/libvirt/config.py | 3 ++ nova/virt/libvirt/driver.py | 43 +++++++++++++++++-- nova/virt/libvirt/host.py | 2 +- nova/virt/libvirt/utils.py | 28 +++++++++--- 6 files changed, 97 insertions(+), 27 deletions(-) diff --git a/nova/tests/functional/regressions/test_bug_1951656.py b/nova/tests/functional/regressions/test_bug_1951656.py index 9aad191072c..d705ff6fe31 100644 --- a/nova/tests/functional/regressions/test_bug_1951656.py +++ b/nova/tests/functional/regressions/test_bug_1951656.py @@ -63,21 +63,11 @@ def test_create_servers_with_vgpu(self): flavor_id=self.flavor, host=self.compute1.host, networks='auto', expected_state='ACTIVE') - # TODO(sbauza): Modify this once bug #1851656 is fixed. - # mdev_name2uuid() raises a badly formed hexadecimal UUID string error - self.assertRaises(ValueError, - self.assert_mdev_usage, - self.compute1, expected_amount=1) - - # Now, the problem is that we can't create new instances with VGPUs - # from this host. - server = self._create_server( + self.assert_mdev_usage(self.compute1, expected_amount=1) + + self._create_server( image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', flavor_id=self.flavor, host=self.compute1.host, - networks='auto', expected_state='ERROR') - # The error is due to a bad mdev name parsing - self.assertIn('fault', server) - # since we only have one host, we have a RescheduledException as this - # service was creating an exception and we can't use another one. - self.assertIn('Exceeded maximum number of retries', - server['fault']['message']) + networks='auto', expected_state='ACTIVE') + + self.assert_mdev_usage(self.compute1, expected_amount=2) diff --git a/nova/tests/unit/virt/libvirt/test_config.py b/nova/tests/unit/virt/libvirt/test_config.py index 2d690e5dfc8..96f0d5f3e55 100644 --- a/nova/tests/unit/virt/libvirt/test_config.py +++ b/nova/tests/unit/virt/libvirt/test_config.py @@ -3135,6 +3135,32 @@ def test_config_mdev_device(self): config.LibvirtConfigNodeDeviceMdevInformation) self.assertEqual("nvidia-11", obj.mdev_information.type) self.assertEqual(12, obj.mdev_information.iommu_group) + self.assertIsNone(obj.mdev_information.uuid) + + def test_config_mdev_device_uuid(self): + xmlin = """ + + mdev_b2107403_110c_45b0_af87_32cc91597b8a_0000_41_00_0 + /sys/devices/pci0000:40/0000:40:03.1/0000:41:00.0/b2107403-110c-45b0-af87-32cc91597b8a + pci_0000_41_00_0 + + vfio_mdev + + + + b2107403-110c-45b0-af87-32cc91597b8a + + + """ + + obj = config.LibvirtConfigNodeDevice() + obj.parse_str(xmlin) + self.assertIsInstance(obj.mdev_information, + config.LibvirtConfigNodeDeviceMdevInformation) + self.assertEqual("nvidia-442", obj.mdev_information.type) + self.assertEqual(57, obj.mdev_information.iommu_group) + self.assertEqual("b2107403-110c-45b0-af87-32cc91597b8a", + obj.mdev_information.uuid) def test_config_vdpa_device(self): xmlin = """ diff --git a/nova/virt/libvirt/config.py b/nova/virt/libvirt/config.py index de352477702..09b82b01caf 100644 --- a/nova/virt/libvirt/config.py +++ b/nova/virt/libvirt/config.py @@ -3289,6 +3289,7 @@ def __init__(self, **kwargs): root_name="capability", **kwargs) self.type = None self.iommu_group = None + self.uuid = None def parse_dom(self, xmldoc): super(LibvirtConfigNodeDeviceMdevInformation, @@ -3298,6 +3299,8 @@ def parse_dom(self, xmldoc): self.type = c.get('id') if c.tag == "iommuGroup": self.iommu_group = int(c.get('number')) + if c.tag == "uuid": + self.uuid = c.text class LibvirtConfigGuestRng(LibvirtConfigGuestDevice): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index c9b40dcda59..5248fb852c5 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -7790,15 +7790,52 @@ def _get_mdev_capable_devices(self, types=None): def _get_mediated_device_information(self, devname): """Returns a dict of a mediated device.""" - virtdev = self._host.device_lookup_by_name(devname) + # LP #1951656 - In Libvirt 7.7, the mdev name now includes the PCI + # address of the parent device (e.g. mdev__) due to + # the mdevctl allowing for multiple mediated devs having the same UUID + # defined (only one can be active at a time). Since the guest + # information doesn't have the parent ID, try to lookup which + # mediated device is available that matches the UUID. If multiple + # devices are found that match the UUID, then this is an error + # condition. + try: + virtdev = self._host.device_lookup_by_name(devname) + except libvirt.libvirtError as ex: + if ex.get_error_code() != libvirt.VIR_ERR_NO_NODE_DEVICE: + raise + mdevs = [dev for dev in self._host.list_mediated_devices() + if dev.startswith(devname)] + # If no matching devices are found, simply raise the original + # exception indicating that no devices are found. + if not mdevs: + raise + elif len(mdevs) > 1: + msg = ("The mediated device name %(devname)s refers to a UUID " + "that is present in multiple libvirt mediated devices. " + "Matching libvirt mediated devices are %(devices)s. " + "Mediated device UUIDs must be unique for Nova." % + {'devname': devname, + 'devices': ', '.join(mdevs)}) + raise exception.InvalidLibvirtMdevConfig(reason=msg) + + LOG.debug('Found requested device %s as %s. Using that.', + devname, mdevs[0]) + virtdev = self._host.device_lookup_by_name(mdevs[0]) xmlstr = virtdev.XMLDesc(0) cfgdev = vconfig.LibvirtConfigNodeDevice() cfgdev.parse_str(xmlstr) + # Starting with Libvirt 7.3, the uuid information is available in the + # node device information. If its there, use that. Otherwise, + # fall back to the previous behavior of parsing the uuid from the + # devname. + if cfgdev.mdev_information.uuid: + mdev_uuid = cfgdev.mdev_information.uuid + else: + mdev_uuid = libvirt_utils.mdev_name2uuid(cfgdev.name) device = { "dev_id": cfgdev.name, - # name is like mdev_00ead764_fdc0_46b6_8db9_2963f5c815b4 - "uuid": libvirt_utils.mdev_name2uuid(cfgdev.name), + "uuid": mdev_uuid, # the physical GPU PCI device "parent": cfgdev.parent, "type": cfgdev.mdev_information.type, diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 604e25c49cb..6a10ae723f5 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1416,7 +1416,7 @@ def list_mdev_capable_devices(self, flags=0): def list_mediated_devices(self, flags=0): """Lookup mediated devices. - :returns: a list of virNodeDevice instance + :returns: a list of strings with the name of the instance """ return self._list_devices("mdev", flags=flags) diff --git a/nova/virt/libvirt/utils.py b/nova/virt/libvirt/utils.py index da2a6e8b8a1..8716ca1736d 100644 --- a/nova/virt/libvirt/utils.py +++ b/nova/virt/libvirt/utils.py @@ -577,17 +577,31 @@ def get_default_machine_type(arch: str) -> ty.Optional[str]: def mdev_name2uuid(mdev_name: str) -> str: - """Convert an mdev name (of the form mdev_) to a - uuid (of the form 8-4-4-4-12). + """Convert an mdev name (of the form mdev_ or + mdev__) to a uuid + (of the form 8-4-4-4-12). + + :param mdev_name: the name of the mdev to parse the UUID from + :returns: string containing the uuid """ - return str(uuid.UUID(mdev_name[5:].replace('_', '-'))) + mdev_uuid = mdev_name[5:].replace('_', '-') + # Unconditionnally remove the PCI address from the name + mdev_uuid = mdev_uuid[:36] + return str(uuid.UUID(mdev_uuid)) + +def mdev_uuid2name(mdev_uuid: str, parent: str = None) -> str: + """Convert an mdev uuid (of the form 8-4-4-4-12) and optionally its parent + device to a name (of the form mdev_[_]). -def mdev_uuid2name(mdev_uuid: str) -> str: - """Convert an mdev uuid (of the form 8-4-4-4-12) to a name (of the form - mdev_). + :param mdev_uuid: the uuid of the mediated device + :param parent: the parent device id for the mediated device + :returns: name of the mdev to reference in libvirt """ - return "mdev_" + mdev_uuid.replace('-', '_') + name = "mdev_" + mdev_uuid.replace('-', '_') + if parent and parent.startswith('pci_'): + name = name + parent[4:] + return name def get_flags_by_flavor_specs(flavor: 'objects.Flavor') -> ty.Set[str]: