From f6060ab6b54261ff50b8068732f6e509619d713e Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Tue, 10 Dec 2019 14:20:33 +0000 Subject: [PATCH] FUP for in-place numa rebuild This patch addresses a number of typos and minor issues raised during review of [1][2]. A summary of the changes are corrections to typos in comments, a correction to the exception message, an update to the release note and the addition of debug logging. [1] I0322d872bdff68936033a6f5a54e8296a6fb3434 [2] I48bccc4b9adcac3c7a3e42769c11fdeb8f6fd132 Related-Bug: #1804502 Related-Bug: #1763766 Change-Id: I8975e524cd5a9c7dfb065bb2dc8ceb03f1b89e7b --- nova/compute/api.py | 19 ++++++++-------- nova/exception.py | 2 +- .../scheduler/filters/numa_topology_filter.py | 2 +- .../functional/libvirt/test_numa_servers.py | 19 ++++++++-------- nova/tests/unit/compute/test_compute_api.py | 13 ++++++----- .../notes/numa-rebuild-b75f9a1966f576ea.yaml | 22 ++++++++++--------- 6 files changed, 41 insertions(+), 36 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 11b782a53935..c85a3d6d092d 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3524,8 +3524,6 @@ class API(base.Base): :param instance: nova.objects.instance.Instance object :param image: the new image the instance will be rebuilt with. :param flavor: the flavor of the current instance. - - :returns: True or raises on failure. :raises: nova.exception.ImageNUMATopologyRebuildConflict """ @@ -3540,25 +3538,28 @@ class API(base.Base): # early out for non NUMA instances if old_constraints is None and new_constraints is None: - # return true for easy unit testing - return True + return - # if only one of the constrains are non-None (or 'set') then the + # if only one of the constraints are non-None (or 'set') then the # constraints changed so raise an exception. if old_constraints is None or new_constraints is None: + action = "removing" if old_constraints else "introducing" + LOG.debug("NUMA rebuild validation failed. The requested image " + "would alter the NUMA constraints by %s a NUMA " + "topology.", action, instance=instance) raise exception.ImageNUMATopologyRebuildConflict() - # otherwise since both the old a new constrains are non none compare + # otherwise since both the old a new constraints are non none compare # them as dictionaries. old = old_constraints.obj_to_primitive() new = new_constraints.obj_to_primitive() if old != new: + LOG.debug("NUMA rebuild validation failed. The requested image " + "conflicts with the existing NUMA constraints.", + instance=instance) raise exception.ImageNUMATopologyRebuildConflict() # TODO(sean-k-mooney): add PCI NUMA affinity policy check. - # return true for easy unit testing - return True - @staticmethod def _check_quota_for_upsize(context, instance, current_flavor, new_flavor): project_id, user_id = quotas_obj.ids_from_instance(context, diff --git a/nova/exception.py b/nova/exception.py index d8ee0de665bb..814f6208a441 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -1857,7 +1857,7 @@ class ImageNUMATopologyForbidden(Forbidden): class ImageNUMATopologyRebuildConflict(Invalid): msg_fmt = _( - "An instance's NUMA typology cannot be changed as part of a rebuild. " + "An instance's NUMA topology cannot be changed as part of a rebuild. " "The image provided is invalid for this instance.") diff --git a/nova/scheduler/filters/numa_topology_filter.py b/nova/scheduler/filters/numa_topology_filter.py index fae8a45600f0..74d6012f82ec 100644 --- a/nova/scheduler/filters/numa_topology_filter.py +++ b/nova/scheduler/filters/numa_topology_filter.py @@ -25,7 +25,7 @@ class NUMATopologyFilter(filters.BaseHostFilter): # NOTE(sean-k-mooney): In change I0322d872bdff68936033a6f5a54e8296a6fb343 # we validate that the NUMA topology does not change in the api. If the - # requested image would alter the NUMA constrains we reject the rebuild + # requested image would alter the NUMA constraints we reject the rebuild # request and therefore do not need to run this filter on rebuild. RUN_ON_REBUILD = False diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index a9dada5fe19c..f95318123b1e 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -15,8 +15,7 @@ import mock import six - -from testtools import skip +import testtools from oslo_config import cfg from oslo_log import log as logging @@ -845,7 +844,7 @@ class NUMAServersWithNetworksTest(NUMAServersTestBase): # would be violated and it should fail at that point not when the # instance is rebuilt. This is a latent bug which will be addressed # in a separate patch. - @skip("bug 1855332") + @testtools.skip("bug 1855332") def test_attach_interface_with_network_affinity_violation(self): extra_spec = {'hw:numa_nodes': '1'} flavor_id = self._create_flavor(extra_spec=extra_spec) @@ -949,7 +948,7 @@ class NUMAServersRebuildTests(NUMAServersTestBase): def setUp(self): super().setUp() images = self.api.get_images() - # save references to first two image for server create and rebuild + # save references to first two images for server create and rebuild self.image_ref_0 = images[0]['id'] self.image_ref_1 = images[1]['id'] @@ -1013,9 +1012,9 @@ class NUMAServersRebuildTests(NUMAServersTestBase): extra_spec = {'hw:cpu_policy': 'dedicated'} flavor_id = self._create_flavor(extra_spec=extra_spec) - # cpu_cores is set to 2 to ensure that - # we have enough space to boot the vm but not enough space to rebuild - # by doubling the resource use during scheduling. + # cpu_cores is set to 2 to ensure that we have enough space + # to boot the vm but not enough space to rebuild + # by doubling the resource use during scheduling. host_info = fakelibvirt.HostInfo( cpu_nodes=1, cpu_sockets=1, cpu_cores=2, kB_mem=15740000) fake_connection = self._get_connection(host_info=host_info) @@ -1046,7 +1045,7 @@ class NUMAServersRebuildTests(NUMAServersTestBase): server = self._create_active_server( server_args={"flavorRef": flavor_id}) - # the original vm had an implicit numa topology of 1 virtual numa nodes + # The original vm had an implicit numa topology of 1 virtual numa node # so we alter the requested numa topology in image_ref_1 to request # 2 virtual numa nodes. ctx = nova_context.get_admin_context() @@ -1054,11 +1053,11 @@ class NUMAServersRebuildTests(NUMAServersTestBase): self.fake_image_service.update(ctx, self.image_ref_1, image_meta) # NOTE(sean-k-mooney): this should fail because rebuild uses noop - # claims therefor it is not allowed for the numa topology or resource + # claims therefore it is not allowed for the NUMA topology or resource # usage to change during a rebuild. ex = self.assertRaises( client.OpenStackApiException, self._rebuild_server, server, self.image_ref_1) self.assertEqual(400, ex.response.status_code) - self.assertIn("An instance's NUMA typology cannot be changed", + self.assertIn("An instance's NUMA topology cannot be changed", six.text_type(ex)) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index bde71415beb5..1c6ba364b5d9 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -6252,7 +6252,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): cells) def test__validate_numa_rebuild_non_numa(self): - """This test asserts that a rebuild of an instance without a NUMA + """Assert that a rebuild of an instance without a NUMA topology passes validation. """ flavor = objects.Flavor( @@ -6266,7 +6266,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): self.compute_api._validate_numa_rebuild(instance, image, flavor) def test__validate_numa_rebuild_no_conflict(self): - """This test asserts that a rebuild of an instance without a change + """Assert that a rebuild of an instance without a change in NUMA topology passes validation. """ flavor = objects.Flavor( @@ -6284,7 +6284,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): self.compute_api._validate_numa_rebuild(instance, image, flavor) def test__validate_numa_rebuild_add_numa_toplogy(self): - """This test asserts that a rebuild of an instance with a new image + """Assert that a rebuild of an instance with a new image that requests a NUMA topology when the original instance did not have a NUMA topology is invalid. """ @@ -6292,6 +6292,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): flavor = objects.Flavor( id=42, vcpus=1, memory_mb=512, root_gb=1, extra_specs={}) + # _create_instance_obj results in the instance.image_meta being None. instance = self._create_instance_obj(flavor=flavor) # we use a dict instead of image metadata object as # _validate_numa_rebuild constructs the object internally @@ -6306,7 +6307,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): self.compute_api._validate_numa_rebuild, instance, image, flavor) def test__validate_numa_rebuild_remove_numa_toplogy(self): - """This test asserts that a rebuild of an instance with a new image + """Assert that a rebuild of an instance with a new image that does not request a NUMA topology when the original image did is invalid if it would alter the instances topology as a result. """ @@ -6314,6 +6315,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): flavor = objects.Flavor( id=42, vcpus=1, memory_mb=512, root_gb=1, extra_specs={}) + # _create_instance_obj results in the instance.image_meta being None. instance = self._create_instance_obj(flavor=flavor) # we use a dict instead of image metadata object as # _validate_numa_rebuild constructs the object internally @@ -6336,7 +6338,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): image, flavor) def test__validate_numa_rebuild_alter_numa_toplogy(self): - """This test asserts that a rebuild of an instance with a new image + """Assert that a rebuild of an instance with a new image that requests a different NUMA topology than the original image is invalid. """ @@ -6348,6 +6350,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): flavor = objects.Flavor( id=42, vcpus=2, memory_mb=512, root_gb=1, extra_specs={}) + # _create_instance_obj results in the instance.image_meta being None. instance = self._create_instance_obj(flavor=flavor) # we use a dict instead of image metadata object as # _validate_numa_rebuild constructs the object internally diff --git a/releasenotes/notes/numa-rebuild-b75f9a1966f576ea.yaml b/releasenotes/notes/numa-rebuild-b75f9a1966f576ea.yaml index 646bb69d04da..2fde588538f7 100644 --- a/releasenotes/notes/numa-rebuild-b75f9a1966f576ea.yaml +++ b/releasenotes/notes/numa-rebuild-b75f9a1966f576ea.yaml @@ -6,22 +6,24 @@ fixes: Previously Nova would have ignored the NUMA topology of the new image continuing to use the NUMA topology of the existing instance until a move operation was performed. As Nova did not explicitly guard against - inadvertent changes in resource request contained in a new image, - it was possible to rebuild with an image that would violate this requirement - `bug #1763766`_. This resulted in an inconsistent state as the instance that - was running did not match the instance that was requested. Nova now explicitly - checks if a rebuild would alter the requested NUMA topology of an instance - and rejects the rebuild. + inadvertent changes to resource requests contained in a new image, + it was possible to rebuild with an image that would violate this + requirement; see `bug #1763766`_ for details. This resulted in an + inconsistent state as the instance that was running did not match the + instance that was requested. Nova now explicitly checks if a rebuild would + alter the requested NUMA topology of an instance and rejects the rebuild + if so. .. _`bug #1763766`: https://bugs.launchpad.net/nova/+bug/1763766 -features: - | With the changes introduced to address `bug #1763766`_, Nova now guards against NUMA constraint changes on rebuild. As a result the ``NUMATopologyFilter`` is no longer required to run on rebuild since - we already know the topology will not change and therefor the existing + we already know the topology will not change and therefore the existing resource claim is still valid. As such it is now possible to do an in-place - rebuild of a instance with a NUMA topology even if the image changes - provided the new image does not alter the topology. + rebuild of an instance with a NUMA topology even if the image changes + provided the new image does not alter the topology which addresses + `bug #1804502`_. + .. _`bug #1804502`: https://bugs.launchpad.net/nova/+bug/1804502