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

Conflicts:
    nova/tests/functional/libvirt/test_numa_servers.py
NOTE(sean-k-mooney): conflict was due to the use of
NUMAHostInfo instead of HostInfo.

Change-Id: I8975e524cd5a9c7dfb065bb2dc8ceb03f1b89e7b
(cherry picked from commit f6060ab6b54261ff50b8068732f6e509619d713e)
(cherry picked from commit 48bb9a9663374936221144bb6a24688128a51146)

6 files changed, 41 insertions, 36 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 77b2ef85ce..07ff85b0e3 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -3505,8 +3505,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
         """
 
@@ -3521,25 +3519,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 2796303117..b4a300ff25 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -1938,7 +1938,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 5e3d6eb593..d318a7aa7f 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 40baec8111..ce1c7fc49e 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
@@ -262,7 +261,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)
@@ -387,7 +386,7 @@ class NUMAServersRebuildTests(NUMAServersTestBase):
     def setUp(self):
         super(NUMAServersRebuildTests, self).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']
 
@@ -451,9 +450,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.NUMAHostInfo(
             cpu_nodes=1, cpu_sockets=1, cpu_cores=2, kB_mem=15740000)
         fake_connection = self._get_connection(host_info=host_info)
@@ -484,7 +483,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()
@@ -492,11 +491,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 9942595610..090aacbca6 100644
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -6540,7 +6540,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(
@@ -6554,7 +6554,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(
@@ -6572,7 +6572,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.
         """
@@ -6580,6 +6580,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
@@ -6594,7 +6595,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.
         """
@@ -6602,6 +6603,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
@@ -6624,7 +6626,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.
         """
@@ -6636,6 +6638,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 646bb69d04..2fde588538 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