Merge "Migrate RequestSpec.numa_topology to use pcpuset" into stable/wallaby

Author: Zuul

nova/objects/instance_numa.py

@@ -166,8 +166,10 @@ def obj_from_db_obj(cls, context, instance_uuid, db_obj):
 
         if 'nova_object.name' in primitive:
             obj = cls.obj_from_primitive(primitive)
-            cls._migrate_legacy_dedicated_instance_cpuset(
-                context, instance_uuid, obj)
+            updated = cls._migrate_legacy_dedicated_instance_cpuset(obj)
+            if updated:
+                cls._save_migrated_cpuset_to_instance_extra(
+                    context, obj, instance_uuid)
         else:
             obj = cls._migrate_legacy_object(context, instance_uuid, primitive)
 
@@ -176,13 +178,14 @@ def obj_from_db_obj(cls, context, instance_uuid, db_obj):
     # TODO(huaqiang): Remove after Wallaby once we are sure these objects have
     # been loaded at least once.
     @classmethod
-    def _migrate_legacy_dedicated_instance_cpuset(cls, context, instance_uuid,
-                                                  obj):
+    def _migrate_legacy_dedicated_instance_cpuset(cls, obj):
         # NOTE(huaqiang): We may meet some topology object with the old version
         # 'InstanceNUMACell' cells, in that case, the 'dedicated' CPU is kept
         # in 'InstanceNUMACell.cpuset' field, but it should be kept in
         # 'InstanceNUMACell.pcpuset' field since Victoria. Making an upgrade
-        # and persisting to database.
+        # here but letting the caller persist the result if needed as we
+        # don't know which table the InstanceNUMACell is coming from. It can
+        # come from instance_extra or request_spec too.
         update_db = False
         for cell in obj.cells:
             if len(cell.cpuset) == 0:
@@ -194,14 +197,20 @@ def _migrate_legacy_dedicated_instance_cpuset(cls, context, instance_uuid,
             cell.pcpuset = cell.cpuset
             cell.cpuset = set()
             update_db = True
+        return update_db
 
-        if update_db:
-            db_obj = jsonutils.dumps(obj.obj_to_primitive())
-            values = {
-                'numa_topology': db_obj,
-            }
-            db.instance_extra_update_by_uuid(context, instance_uuid,
-                                             values)
+    # TODO(huaqiang): Remove after Yoga once we are sure these objects have
+    # been loaded at least once.
+    @classmethod
+    def _save_migrated_cpuset_to_instance_extra(
+        cls, context, obj, instance_uuid
+    ):
+        db_obj = jsonutils.dumps(obj.obj_to_primitive())
+        values = {
+            'numa_topology': db_obj,
+        }
+        db.instance_extra_update_by_uuid(
+            context, instance_uuid, values)
 
     # TODO(stephenfin): Remove in X or later, once this has bedded in
     @classmethod

nova/objects/request_spec.py

@@ -585,6 +585,8 @@ def ensure_network_information(self, instance):
     @staticmethod
     def _from_db_object(context, spec, db_spec):
         spec_obj = spec.obj_from_primitive(jsonutils.loads(db_spec['spec']))
+        data_migrated = False
+
         for key in spec.fields:
             # Load these from the db model not the serialized object within,
             # though they should match.
@@ -605,6 +607,13 @@ def _from_db_object(context, spec, db_spec):
                 # fields. If they are not set, set None.
                 if not spec.obj_attr_is_set(key):
                     setattr(spec, key, None)
+            elif key == "numa_topology":
+                if key in spec_obj:
+                    spec.numa_topology = spec_obj.numa_topology
+                    if spec.numa_topology:
+                        data_migrated = objects.InstanceNUMATopology.\
+                            _migrate_legacy_dedicated_instance_cpuset(
+                                spec.numa_topology)
             elif key in spec_obj:
                 setattr(spec, key, getattr(spec_obj, key))
         spec._context = context
@@ -626,6 +635,9 @@ def _from_db_object(context, spec, db_spec):
                 # NOTE(danms): Instance group may have been deleted
                 spec.instance_group = None
 
+        if data_migrated:
+            spec.save()
+
         spec.obj_reset_changes()
         return spec
 

nova/tests/unit/objects/test_request_spec.py

@@ -611,29 +611,12 @@ def test_get_by_instance_uuid(self, mock_get_ig, get_by_uuid):
         self.assertIsInstance(req_obj.instance_group, objects.InstanceGroup)
         self.assertEqual('fresh', req_obj.instance_group.name)
 
-    # FIXME(gibi): This is bug 1952941. When the cpuset -> pcpuset data
-    # migration was added to InstanceNUMATopology it was missed that such
-    # object is not only hydrated via
-    # InstanceNUMATopology.get_by_instance_uuid() but also hydrated by
-    # RequestSpec.get_by_instance_uuid() indirectly. However the
-    # latter code patch does not call InstanceNUMATopology.obj_from_db_obj()
-    # that triggers the data migration via
-    # InstanceNUMATopology._migrate_legacy_dedicated_instance_cpuset.
-    # This causes that when the new nova code loads an old RequestSpec object
-    # from the DB (e.g. during migration of an instance) the
-    # InstanceNUMATopology in the RequestSpec will not be migrated to the new
-    # object version and it will lead to errors when the pcpuset field is read
-    # during scheduling.
-    @mock.patch(
-        'nova.objects.instance_numa.InstanceNUMATopology.'
-        '_migrate_legacy_dedicated_instance_cpuset',
-        new=mock.NonCallableMock()
-    )
+    @mock.patch('nova.objects.request_spec.RequestSpec.save')
     @mock.patch.object(
         request_spec.RequestSpec, '_get_by_instance_uuid_from_db')
     @mock.patch('nova.objects.InstanceGroup.get_by_uuid')
     def test_get_by_instance_uuid_numa_topology_migration(
-        self, mock_get_ig, get_by_uuid
+        self, mock_get_ig, get_by_uuid, mock_save
     ):
         # Simulate a pre-Victoria RequestSpec where the pcpuset field is not
         # defined for the embedded InstanceNUMACell objects but the cpu_policy
@@ -661,18 +644,10 @@ def test_get_by_instance_uuid_numa_topology_migration(
             self.context, fake_spec['instance_uuid'])
 
         self.assertEqual(2, len(req_obj.numa_topology.cells))
+        self.assertEqual({1, 2}, req_obj.numa_topology.cells[0].pcpuset)
+        self.assertEqual({3, 4}, req_obj.numa_topology.cells[1].pcpuset)
 
-        # This is bug 1952941 as the pcpuset is not defined in object as the
-        # object is not migrated
-        ex = self.assertRaises(
-            NotImplementedError,
-            lambda: req_obj.numa_topology.cells[0].pcpuset
-        )
-        self.assertIn("Cannot load 'pcpuset' in the base class", str(ex))
-
-        # This is the expected behavior
-        # self.assertEqual({1, 2}, req_obj.numa_topology.cells[0].pcpuset)
-        # self.assertEqual({3, 4}, req_obj.numa_topology.cells[1].pcpuset)
+        mock_save.assert_called_once()
 
     def _check_update_primitive(self, req_obj, changes):
         self.assertEqual(req_obj.instance_uuid, changes['instance_uuid'])

releasenotes/notes/bug-1952941-request-spec-numa-topology-migration-c97dbd51b3c6c116.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    The `bug 1952941`_  is fixed where a pre-Victoria server with pinned
+    CPUs cannot be migrated or evacuated after the cloud is upgraded to
+    Victoria or newer as the scheduling fails with
+    ``NotImplementedError: Cannot load 'pcpuset'`` error.
+
+    .. _bug 1952941: https://bugs.launchpad.net/nova/+bug/1952941