Merge "Set instance host and drop migration under lock" into stable/victoria

Author: Zuul

nova/compute/manager.py

@@ -3472,19 +3472,11 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                 self._notify_instance_rebuild_error(context, instance, e, bdms)
                 raise
             else:
-                instance.apply_migration_context()
-                # NOTE (ndipanov): This save will now update the host and node
-                # attributes making sure that next RT pass is consistent since
-                # it will be based on the instance and not the migration DB
-                # entry.
-                instance.host = self.host
-                instance.node = scheduled_node
-                instance.save()
-                instance.drop_migration_context()
-
-                # NOTE (ndipanov): Mark the migration as done only after we
-                # mark the instance as belonging to this host.
-                self._set_migration_status(migration, 'done')
+                # NOTE(gibi): Let the resource tracker set the instance
+                # host and drop the migration context as we need to hold the
+                # COMPUTE_RESOURCE_SEMAPHORE to avoid the race with
+                # _update_available_resources. See bug 1896463.
+                self.rt.finish_evacuation(instance, scheduled_node, migration)
 
     def _do_rebuild_instance_with_claim(
             self, context, instance, orig_image_ref, image_meta,

nova/compute/resource_tracker.py

@@ -1970,3 +1970,21 @@ def free_pci_device_claims_for_instance(self, context, instance):
         """
         self.pci_tracker.free_instance_claims(context, instance)
         self.pci_tracker.save(context)
+
+    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE, fair=True)
+    def finish_evacuation(self, instance, node, migration):
+        instance.apply_migration_context()
+        # NOTE (ndipanov): This save will now update the host and node
+        # attributes making sure that next RT pass is consistent since
+        # it will be based on the instance and not the migration DB
+        # entry.
+        instance.host = self.host
+        instance.node = node
+        instance.save()
+        instance.drop_migration_context()
+
+        # NOTE (ndipanov): Mark the migration as done only after we
+        # mark the instance as belonging to this host.
+        if migration:
+            migration.status = 'done'
+            migration.save()

nova/tests/functional/regressions/test_bug_1896463.py

@@ -219,13 +219,4 @@ def slow_get_mig(*args, **kwargs):
             server, {'OS-EXT-SRV-ATTR:host': 'host2', 'status': 'ACTIVE'})
 
         self._assert_pci_device_allocated(server['id'], self.compute1_id)
-
-        # This is bug #1896463 as the PCI allocation was deleted by the racing
-        # _update_available_resource periodic task.
-        self._assert_pci_device_allocated(
-            server['id'], self.compute2_id, num=0)
-
-        # FIXME(gibi): When this bug is fixed (or if you remove the sleeps
-        # above to avoid the race condition) then we expect that the PCI
-        # allocation exists on the destination host too.
-        # self._assert_pci_device_allocated(server['id'], self.compute2_id)
+        self._assert_pci_device_allocated(server['id'], self.compute2_id)

nova/tests/unit/compute/test_compute_mgr.py

@@ -5340,19 +5340,21 @@ def test_rebuild_node_not_updated_if_not_recreate(self):
         node = uuidutils.generate_uuid()  # ironic node uuid
         instance = fake_instance.fake_instance_obj(self.context, node=node)
         instance.migration_context = None
+        migration = objects.Migration(status='accepted')
         with test.nested(
             mock.patch.object(self.compute, '_get_compute_info'),
             mock.patch.object(self.compute, '_do_rebuild_instance_with_claim'),
             mock.patch.object(objects.Instance, 'save'),
-            mock.patch.object(self.compute, '_set_migration_status'),
-        ) as (mock_get, mock_rebuild, mock_save, mock_set):
+            mock.patch.object(migration, 'save'),
+        ) as (mock_get, mock_rebuild, mock_save, mock_migration_save):
             self.compute.rebuild_instance(
                 self.context, instance, None, None,
                 None, None, None, None, False,
-                False, False, None, None, {}, None, [])
+                False, False, migration, None, {}, None, [])
             self.assertFalse(mock_get.called)
             self.assertEqual(node, instance.node)
-            mock_set.assert_called_once_with(None, 'done')
+            self.assertEqual('done', migration.status)
+            mock_migration_save.assert_called_once_with()
 
     def test_rebuild_node_updated_if_recreate(self):
         dead_node = uuidutils.generate_uuid()
@@ -5365,16 +5367,15 @@ def test_rebuild_node_updated_if_recreate(self):
         with test.nested(
             mock.patch.object(self.compute, '_get_compute_info'),
             mock.patch.object(self.compute, '_do_rebuild_instance'),
-            mock.patch.object(objects.Instance, 'save'),
-            mock.patch.object(self.compute, '_set_migration_status'),
-        ) as (mock_get, mock_rebuild, mock_save, mock_set):
+        ) as (mock_get, mock_rebuild):
             mock_get.return_value.hypervisor_hostname = 'new-node'
             self.compute.rebuild_instance(
                 self.context, instance, None, None, None, None, None,
-                None, True, False, False, None, None, {}, None, [])
+                None, True, False, False, mock.sentinel.migration, None, {},
+                None, [])
             mock_get.assert_called_once_with(mock.ANY, self.compute.host)
-            self.assertEqual('new-node', instance.node)
-            mock_set.assert_called_once_with(None, 'done')
+            mock_rt.finish_evacuation.assert_called_once_with(
+                instance, 'new-node', mock.sentinel.migration)
             # Make sure the rebuild_claim was called with the proper image_meta
             # from the instance.
             mock_rt.rebuild_claim.assert_called_once()