Update InstanceNUMACell version in more cases

The data migration of InstanceNUMACell 1.4 to 1.5 only moved the data to
the new pcpuset field but does not update the ovo version string of the
object in the DB. The previous patch added the missing version update
logic. However it only fixes the issue if the data is not already "half"
migrated to the new structure. So this patch adds logic to also do the
right thing if the wrong data migration already happened.

At the end the solution needs to consider multiple scenarios:
* data is never migrated to the new schema so the new code needs
  to migrate it and update the version string to match the new schema.
  (done by the previous patch)
* data is half migrated by the buggy code and the new code need to
  finish the migration by stamping the version in the DB.
* data is half migrated and then further modified to use the new 1.6
  feature cpu_policy mixed.
* data version is older in the DB than we can meaningfully upgrade

Closes-Bug: #2097359
Change-Id: I10ecfa7841b15637dea3e4736e90faa5f33ddff3
(cherry picked from commit 9507b7b)
(cherry picked from commit c008126)
(cherry picked from commit 882c5eb)
(cherry picked from commit e3db83c)

Author: gibizer

nova/objects/instance_numa.py

@@ -196,26 +196,78 @@ def _migrate_legacy_dedicated_instance_cpuset(cls, obj):
         # come from instance_extra or request_spec too.
         update_db = False
         for cell in obj.cells:
-            if len(cell.cpuset) == 0:
+            version = versionutils.convert_version_to_tuple(cell.VERSION)
+
+            if version < (1, 4):
+                LOG.warning(
+                    "InstanceNUMACell %s with version %s for instance %s has "
+                    "too old version in the DB, don't know how to update, "
+                    "ignoring.", cell, cell.VERSION, obj.instance_uuid)
+                continue
+
+            if (version >= (1, 5) and
+                cell.cpu_policy == obj_fields.CPUAllocationPolicy.DEDICATED and
+                (cell.cpuset or not cell.pcpuset)
+            ):
+                LOG.warning(
+                    "InstanceNUMACell %s with version %s is inconsistent as "
+                    "the version is 1.5 or greater, cpu_policy is dedicated, "
+                    "but cpuset is not empty or pcpuset is empty.",
+                    cell, cell.VERSION)
                 continue
-            # NOTE(gibi): This data migration populates the pcpuset field that
-            # is new in version 1.5. However below we bump the object version
-            # to 1.6 directly. This is intentional. The version 1.6 introduced
-            # a new possible value 'mixed' for the cpu_policy field. As that
-            # is a forward compatible change we don't have a specific data
-            # migration for it. But we also don't have an automated way to bump
-            # old object versions from 1.5 to 1.6. So we do it here just to
-            # avoid inconsistency between data and version in the DB.
-            if cell.cpu_policy == obj_fields.CPUAllocationPolicy.DEDICATED:
-                cell.pcpuset = cell.cpuset
-                cell.cpuset = set()
-                cell.VERSION = '1.6'
-                update_db = True
-            else:
-                if 'pcpuset' not in cell:
+
+            # NOTE(gibi): The data migration between 1.4. and 1.5 populates the
+            # pcpuset field that is new in version 1.5. However below we update
+            # the object version to 1.6 directly. This is intentional. The
+            # version 1.6 introduced a new possible value 'mixed' for the
+            # cpu_policy field. As that is a forward compatible change we don't
+            # have a specific data migration for it. But we also don't have an
+            # automated way to update old object versions from 1.5 to 1.6. So
+            # we do it here just to avoid inconsistency between data and
+            # version in the DB.
+            if version < (1, 6):
+                if cell.cpu_policy == obj_fields.CPUAllocationPolicy.DEDICATED:
+                    if "pcpuset" not in cell or not cell.pcpuset:
+                        # this cell was never migrated to 1.6, migrate it.
+                        cell.pcpuset = cell.cpuset
+                        cell.cpuset = set()
+                        cell.VERSION = '1.6'
+                        update_db = True
+                    else:
+                        # This data was already migrated to 1.6 format but the
+                        # version string wasn't updated to 1.6. This happened
+                        # before the fix
+                        # https://bugs.launchpad.net/nova/+bug/2097360
+                        # Only update the version string.
+                        cell.VERSION = '1.6'
+                        update_db = True
+                elif cell.cpu_policy in (
+                        None, obj_fields.CPUAllocationPolicy.SHARED):
+                    # no data migration needed just add the new field and
+                    # stamp the new version in the DB
                     cell.pcpuset = set()
                     cell.VERSION = '1.6'
                     update_db = True
+                else:  # obj_fields.CPUAllocationPolicy.MIXED
+                    # This means the cell data already got updated to the 1.6
+                    # content as MIXED only supported with 1.6 but the version
+                    # was not updated to 1.6.
+                    # We should not do the data migration as that would trample
+                    # the pcpuset field. Just stamp the 1.6 version in the DB
+                    # and hope for the best.
+                    LOG.warning(
+                        "InstanceNUMACell %s with version %s for instance %s "
+                        "has older than 1.6 version in the DB but using the "
+                        "1.6 feature CPUAllocationPolicy.MIXED. So nova "
+                        "assumes that the data is in 1.6 format and only the "
+                        "version string is old. Correcting the version string "
+                        "in the DB.", cell, cell.VERSION, obj.instance_uuid)
+                    cell.VERSION = '1.6'
+                    update_db = True
+
+            # When the next ovo version 1.7 is added it needs to be handed
+            # here to do any migration if needed and to ensure the version in
+            # the DB is stamped to 1.7
 
         return update_db
 

nova/tests/unit/objects/test_instance_numa.py

@@ -363,6 +363,7 @@ def test_obj_from_db_obj(self):
         fake_topo_obj = copy.deepcopy(fake_topo_obj_w_cell_v1_4)
         for cell in fake_topo_obj.cells:
             cell.cpu_policy = objects.fields.CPUAllocationPolicy.DEDICATED
+            cell.VERSION = '1.4'
 
         numa_topology = objects.InstanceNUMATopology.obj_from_db_obj(
             self.context, fake_instance_uuid, fake_topo_obj._to_json())
@@ -402,6 +403,7 @@ def test_obj_from_db_obj_no_pinning(self):
         fake_topo_obj = copy.deepcopy(fake_topo_obj_w_cell_v1_4)
         for cell in fake_topo_obj.cells:
             cell.cpu_policy = objects.fields.CPUAllocationPolicy.SHARED
+            cell.VERSION = '1.4'
 
         numa_topology = objects.InstanceNUMATopology.obj_from_db_obj(
             self.context, fake_instance_uuid, fake_topo_obj._to_json())
@@ -412,7 +414,7 @@ def test_obj_from_db_obj_no_pinning(self):
             self.assertEqual(topo_cell.cpuset, obj_cell.cpuset)
             self.assertEqual(set(), obj_cell.pcpuset)
 
-    def test__migrate_legacy_dedicated_instance_cpuset(self):
+    def test__migrate_legacy_dedicated_instance_cpuset_dedicate_1_4(self):
         # Create a topology with a cell on latest version. Would be nice
         # to create one the old 1.4 cell version directly but that is only
         # possible indirectly as done below.
@@ -446,7 +448,118 @@ def test__migrate_legacy_dedicated_instance_cpuset(self):
         # pcpuset
         self.assertEqual(set(), topo_loaded.cells[0].cpuset)
         self.assertEqual({0, 1}, topo_loaded.cells[0].pcpuset)
-        # and the version is bumped to 1.6
+        # and the version is updated to 1.6
+        self.assertEqual('1.6', topo_loaded.cells[0].VERSION)
+
+    def test__migrate_legacy_dedicated_instance_cpuset_shared_1_4(self):
+        # Create a topology with a cell on latest version. Would be nice
+        # to create one the old 1.4 cell version directly but that is only
+        # possible indirectly as done below.
+        topo = objects.InstanceNUMATopology(
+            instance_uuid=fake_instance_uuid,
+            cells=[
+                objects.InstanceNUMACell(id=0, cpuset={0, 1}, pcpuset=set()),
+            ])
+        topo.cells[0].cpu_policy = objects.fields.CPUAllocationPolicy.SHARED
+
+        # Use the builtin backlevelling logic to pull it back to old cell
+        # version
+        topo_with_cell_1_4 = topo.obj_to_primitive(
+            target_version='1.3', version_manifest={'InstanceNUMACell': '1.4'})
+
+        # Just check that the backlevelling works, and we have a cell with
+        # version and data on 1.4 level
+        cell_1_4_primitive = topo_with_cell_1_4['nova_object.data']['cells'][0]
+        self.assertEqual('1.4', cell_1_4_primitive['nova_object.version'])
+        self.assertEqual(
+            (0, 1), cell_1_4_primitive['nova_object.data']['cpuset'])
+        self.assertNotIn('pcpuset', cell_1_4_primitive['nova_object.data'])
+
+        # Now simulate that such old data is loaded from the DB and migrated
+        # from 1.4 to 1.6 by the data migration
+        topo_loaded = objects.InstanceNUMATopology.obj_from_db_obj(
+            self.context, fake_instance_uuid,
+            jsonutils.dumps(topo_with_cell_1_4))
+
+        # In place data migration did not move the data as the cpu_policy is
+        # shared
+        self.assertEqual({0, 1}, topo_loaded.cells[0].cpuset)
+        self.assertEqual(set(), topo_loaded.cells[0].pcpuset)
+        # but the version is updated to 1.6
+        self.assertEqual('1.6', topo_loaded.cells[0].VERSION)
+
+    def test__migrate_legacy_dedicated_instance_cpuset_dedicated_half_migrated(
+        self
+    ):
+        # Before the fix for https://bugs.launchpad.net/nova/+bug/2097360
+        # landed Nova only half migrated the InstanceNUMACell from 1.4 to 1.6
+        # by doing the data move but not updating the version string in the DB.
+        # This test case ensures that if such migration happened before the fix
+        # was deployed then Nova fixed the DB content on the next load as well.
+
+        # Create a topology with a cell on latest version. Would be nice
+        # to create one the old 1.4 cell version directly but that is only
+        # possible indirectly as done below.
+        topo = objects.InstanceNUMATopology(
+            instance_uuid=fake_instance_uuid,
+            cells=[
+                objects.InstanceNUMACell(id=0, cpuset=set(), pcpuset={0, 1}),
+            ])
+        topo.cells[0].cpu_policy = objects.fields.CPUAllocationPolicy.DEDICATED
+
+        # simulate the half done migration by pulling back the version string
+        # in the primitive form to 1.4 while keeping the data on 1.6 format.
+        topo_primitive = topo.obj_to_primitive()
+        cell_primitive = topo_primitive['nova_object.data']['cells'][0]
+        self.assertEqual('1.6', cell_primitive['nova_object.version'])
+        cell_primitive['nova_object.version'] = '1.4'
+
+        topo_loaded = objects.InstanceNUMATopology.obj_from_db_obj(
+            self.context, fake_instance_uuid, jsonutils.dumps(topo_primitive))
+
+        # the data did not change
+        self.assertEqual(set(), topo_loaded.cells[0].cpuset)
+        self.assertEqual({0, 1}, topo_loaded.cells[0].pcpuset)
+        # but the version is updated to 1.6
+        self.assertEqual('1.6', topo_loaded.cells[0].VERSION)
+
+    def test__migrate_legacy_dedicated_instance_cpuset_mixed_1_4(self):
+        # Before the fix for https://bugs.launchpad.net/nova/+bug/2097360
+        # landed Nova only half migrated the InstanceNUMACell from 1.4 to 1.6
+        # by doing the data move but not updating the version string in the DB.
+        # After this the instance is resized to flavor with mixed cpu_policy
+        # then there is a good chance that the DB has version string 1.4 but
+        # the data is on 1.6 format with the cpu_policy set to mixed.
+        # This test case ensures that if such situation happened before the fix
+        # was deployed then Nova fixed the DB content on the next load as well.
+
+        # Create a topology with a cell on latest version. Would be nice
+        # to create one the old 1.4 cell version directly but that is only
+        # possible indirectly as done below.
+        topo = objects.InstanceNUMATopology(
+            instance_uuid=fake_instance_uuid,
+            cells=[
+                objects.InstanceNUMACell(id=0, cpuset={0, 1}, pcpuset={2, 3}),
+            ])
+        topo.cells[0].cpu_policy = objects.fields.CPUAllocationPolicy.MIXED
+
+        # simulate the half done migration by pulling back the version string
+        # in the primitive form to 1.4 while keeping the data on 1.6 format.
+        topo_primitive = topo.obj_to_primitive()
+        cell_primitive = topo_primitive['nova_object.data']['cells'][0]
+        self.assertEqual('1.6', cell_primitive['nova_object.version'])
+        cell_primitive['nova_object.version'] = '1.4'
+
+        topo_loaded = objects.InstanceNUMATopology.obj_from_db_obj(
+            self.context, fake_instance_uuid, jsonutils.dumps(topo_primitive))
+
+        # the data did not change
+        self.assertEqual({0, 1}, topo_loaded.cells[0].cpuset)
+        self.assertEqual({2, 3}, topo_loaded.cells[0].pcpuset)
+        self.assertEqual(
+            objects.fields.CPUAllocationPolicy.MIXED,
+            topo_loaded.cells[0].cpu_policy)
+        # but the version is updated to 1.6
         self.assertEqual('1.6', topo_loaded.cells[0].VERSION)
 
 

nova/tests/unit/objects/test_request_spec.py

@@ -722,6 +722,9 @@ def test_get_by_instance_uuid_numa_topology_migration(
                     id=1, cpuset={3, 4}, memory=512, cpu_policy="dedicated"),
             ]
         )
+        for cell in numa_topology.cells:
+            cell.VERSION = '1.4'
+
         spec_obj = fake_request_spec.fake_spec_obj()
         spec_obj.numa_topology = numa_topology
         fake_spec = fake_request_spec.fake_db_spec(spec_obj)