Pass migration to finish_revert_migration()

In order to obtain plug-time events, the libvirt driver needs to pass
a Migration object to the network model. Previously [1], this object
was obtained with a database lookup. This was done to avoid modifying
the virt driver interface, thus making the fix backportable. This
patch makes the compute manager pass the migration object to the virt
driver in order to avoid a needless database query.

[1] https://review.opendev.org/#/c/667177/

Change-Id: I9095c1be65b127d40f5bbe7af5d63fefedb73d33

Author: notartom

nova/compute/manager.py

@@ -1040,8 +1040,11 @@ def _init_instance(self, context, instance):
                 block_dev_info = self._get_instance_block_device_info(context,
                                                                       instance)
 
-                self.driver.finish_revert_migration(context,
-                    instance, net_info, block_dev_info, power_on)
+                migration = objects.Migration.get_by_id_and_instance(
+                    context, instance.migration_context.migration_id,
+                    instance.uuid)
+                self.driver.finish_revert_migration(context, instance,
+                    net_info, migration, block_dev_info, power_on)
 
             except Exception:
                 LOG.exception('Failed to revert crashed migration',
@@ -4316,9 +4319,9 @@ def finish_revert_resize(self, context, instance, migration):
                     context, instance, refresh_conn_info=True, bdms=bdms)
 
             power_on = old_vm_state != vm_states.STOPPED
-            self.driver.finish_revert_migration(context, instance,
-                                       network_info,
-                                       block_device_info, power_on)
+            self.driver.finish_revert_migration(
+                context, instance, network_info, migration, block_device_info,
+                power_on)
 
             instance.drop_migration_context()
             instance.launched_at = timeutils.utcnow()

nova/tests/unit/compute/test_compute_mgr.py

@@ -1140,6 +1140,9 @@ def _test_init_instance_reverts_crashed_migrations(self,
                 system_metadata=sys_meta,
                 host=self.compute.host,
                 expected_attrs=['system_metadata'])
+        instance.migration_context = objects.MigrationContext(migration_id=42)
+        migration = objects.Migration(source_compute='fake-host1',
+                                      dest_compute='fake-host2')
 
         with test.nested(
             mock.patch.object(objects.Instance, 'get_network_info',
@@ -1151,22 +1154,25 @@ def _test_init_instance_reverts_crashed_migrations(self,
             mock.patch.object(self.compute.driver, 'get_info'),
             mock.patch.object(instance, 'save'),
             mock.patch.object(self.compute, '_retry_reboot',
-                              return_value=(False, None))
+                              return_value=(False, None)),
+            mock.patch.object(objects.Migration, 'get_by_id_and_instance',
+                              return_value=migration)
         ) as (mock_get_nw, mock_plug, mock_finish, mock_get_inst,
-              mock_get_info, mock_save, mock_retry):
+              mock_get_info, mock_save, mock_retry, mock_get_mig):
             mock_get_info.side_effect = (
                 hardware.InstanceInfo(state=power_state.SHUTDOWN),
                 hardware.InstanceInfo(state=power_state.SHUTDOWN))
 
             self.compute._init_instance(self.context, instance)
 
+            mock_get_mig.assert_called_with(self.context, 42, instance.uuid)
             mock_retry.assert_called_once_with(self.context, instance,
                 power_state.SHUTDOWN)
             mock_get_nw.assert_called_once_with()
             mock_plug.assert_called_once_with(instance, [])
             mock_get_inst.assert_called_once_with(self.context, instance)
             mock_finish.assert_called_once_with(self.context, instance,
-                                                [], [], power_on)
+                                                [], migration, [], power_on)
             mock_save.assert_called_once_with()
             mock_get_info.assert_has_calls(
                 [mock.call(instance, use_cache=False),
@@ -7270,7 +7276,7 @@ def do_test(notify_about_instance_usage,
                                               migration=self.migration,
                                               instance=self.instance)
             finish_revert_migration.assert_called_with(self.context,
-                self.instance, 'nw_info', mock.ANY, mock.ANY)
+                self.instance, 'nw_info', self.migration, mock.ANY, mock.ANY)
             # Make sure the migration.dest_compute is not still set to the
             # source_compute value.
             self.assertNotEqual(self.migration.dest_compute,

nova/tests/unit/virt/hyperv/test_driver.py

@@ -424,8 +424,8 @@ def test_confirm_migration(self):
     def test_finish_revert_migration(self):
         self.driver.finish_revert_migration(
             mock.sentinel.context, mock.sentinel.instance,
-            mock.sentinel.network_info, mock.sentinel.block_device_info,
-            mock.sentinel.power_on)
+            mock.sentinel.network_info, mock.sentinel.migration,
+            mock.sentinel.block_device_info, mock.sentinel.power_on)
 
         finish_revert_migr = self.driver._migrationops.finish_revert_migration
         finish_revert_migr.assert_called_once_with(

nova/tests/unit/virt/libvirt/test_driver.py

@@ -19645,8 +19645,6 @@ def fake_to_xml(self, context, instance, network_info, disk_info,
         with utils.tempdir() as tmpdir:
             self.flags(instances_path=tmpdir)
             ins_ref = self._create_instance()
-            ins_ref.migration_context = objects.MigrationContext(
-                migration_id=migration.id)
             os.mkdir(os.path.join(tmpdir, ins_ref['name']))
             libvirt_xml_path = os.path.join(tmpdir,
                                             ins_ref['name'],
@@ -19669,13 +19667,9 @@ def fake_to_xml(self, context, instance, network_info, disk_info,
                 self.events_passed_to_fake_create = [
                     ('network-vif-plugged', uuids.normal_vif)]
 
-            with mock.patch.object(objects.Migration, 'get_by_id_and_instance',
-                                   return_value=migration) as mock_get_mig:
-                self.drvr.finish_revert_migration(
-                    context.get_admin_context(), ins_ref,
-                    network_info, None, power_on)
-                mock_get_mig.assert_called_with(mock.ANY, migration.id,
-                                                ins_ref.uuid)
+            self.drvr.finish_revert_migration(
+                context.get_admin_context(), ins_ref, network_info, migration,
+                None, power_on)
 
             self.assertTrue(self.fake_create_domain_called)
 
@@ -19707,8 +19701,6 @@ def _test_finish_revert_migration_after_crash(self, backup_made=True,
         drvr.image_backend.by_name.return_value = drvr.image_backend
         context = 'fake_context'
         ins_ref = self._create_instance()
-        ins_ref.migration_context = objects.MigrationContext(
-            migration_id=42)
         migration = objects.Migration(source_compute='fake-host1',
                                       dest_compute='fake-host2')
 
@@ -19720,17 +19712,15 @@ def _test_finish_revert_migration_after_crash(self, backup_made=True,
                 mock.patch.object(drvr, '_get_guest_xml'),
                 mock.patch.object(shutil, 'rmtree'),
                 mock.patch.object(loopingcall, 'FixedIntervalLoopingCall'),
-                mock.patch.object(objects.Migration, 'get_by_id_and_instance',
-                                  return_value=migration)
         ) as (mock_stat, mock_path, mock_rename, mock_cdn, mock_ggx,
-              mock_rmtree, mock_looping_call, mock_get_mig):
+              mock_rmtree, mock_looping_call):
             mock_path.return_value = '/fake/foo'
             if del_inst_failed:
                 mock_rmtree.side_effect = OSError(errno.ENOENT,
                                                   'test exception')
             drvr.finish_revert_migration(context, ins_ref,
-                                         network_model.NetworkInfo())
-            mock_get_mig.assert_called_with(mock.ANY, 42, ins_ref.uuid)
+                                         network_model.NetworkInfo(),
+                                         migration)
             if backup_made:
                 mock_rename.assert_called_once_with('/fake/foo_resize',
                                                     '/fake/foo')
@@ -19759,8 +19749,6 @@ def fake_get_guest_xml(context, instance, network_info, disk_info,
         image_meta = {"disk_format": "raw",
                       "properties": {"hw_disk_bus": "ide"}}
         instance = self._create_instance()
-        instance.migration_context = objects.MigrationContext(
-            migration_id=42)
         migration = objects.Migration(source_compute='fake-host1',
                                       dest_compute='fake-host2')
 
@@ -19773,36 +19761,28 @@ def fake_get_guest_xml(context, instance, network_info, disk_info,
                                   return_value=image_meta),
                 mock.patch.object(drvr, '_get_guest_xml',
                                   side_effect=fake_get_guest_xml),
-                mock.patch.object(objects.Migration, 'get_by_id_and_instance',
-                                  return_value=migration)
-        ) as (mock_img_bkend, mock_cdan, mock_gifsm, mock_ggxml, mock_get_mig):
+        ) as (mock_img_bkend, mock_cdan, mock_gifsm, mock_ggxml):
             drvr.finish_revert_migration('', instance,
                                          network_model.NetworkInfo(),
-                                         power_on=False)
-            mock_get_mig.assert_called_with(mock.ANY, 42, instance.uuid)
+                                         migration, power_on=False)
 
     def test_finish_revert_migration_snap_backend(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
         drvr.image_backend = mock.Mock()
         drvr.image_backend.by_name.return_value = drvr.image_backend
         ins_ref = self._create_instance()
-        ins_ref.migration_context = objects.MigrationContext(
-            migration_id=42)
         migration = objects.Migration(source_compute='fake-host1',
                                       dest_compute='fake-host2')
 
         with test.nested(
                 mock.patch.object(utils, 'get_image_from_system_metadata'),
                 mock.patch.object(drvr, '_create_domain_and_network'),
-                mock.patch.object(drvr, '_get_guest_xml'),
-                mock.patch.object(objects.Migration, 'get_by_id_and_instance',
-                                  return_value=migration)) as (
-                mock_image, mock_cdn, mock_ggx, mock_get_mig):
+                mock.patch.object(drvr, '_get_guest_xml')) as (
+                mock_image, mock_cdn, mock_ggx):
             mock_image.return_value = {'disk_format': 'raw'}
             drvr.finish_revert_migration('', ins_ref,
                                          network_model.NetworkInfo(),
-                                         power_on=False)
-            mock_get_mig.assert_called_with(mock.ANY, 42, ins_ref.uuid)
+                                         migration, power_on=False)
 
             drvr.image_backend.rollback_to_snap.assert_called_once_with(
                     libvirt_utils.RESIZE_SNAPSHOT_NAME)
@@ -19814,6 +19794,8 @@ def test_finish_revert_migration_snap_backend_snapshot_not_found(self):
         drvr.image_backend = mock.Mock()
         drvr.image_backend.by_name.return_value = drvr.image_backend
         ins_ref = self._create_instance()
+        migration = objects.Migration(source_compute='fake-host1',
+                                      dest_compute='fake-host2')
 
         with test.nested(
                 mock.patch.object(utils, 'get_image_from_system_metadata'),
@@ -19825,7 +19807,7 @@ def test_finish_revert_migration_snap_backend_snapshot_not_found(self):
                 exception.SnapshotNotFound(snapshot_id='testing'))
             self.assertRaises(exception.SnapshotNotFound,
                               drvr.finish_revert_migration,
-                              '', ins_ref, None, power_on=False)
+                              '', ins_ref, None, migration, power_on=False)
             drvr.image_backend.remove_snap.assert_not_called()
 
     def test_finish_revert_migration_snap_backend_image_does_not_exist(self):
@@ -19834,26 +19816,21 @@ def test_finish_revert_migration_snap_backend_image_does_not_exist(self):
         drvr.image_backend.by_name.return_value = drvr.image_backend
         drvr.image_backend.exists.return_value = False
         ins_ref = self._create_instance()
-        ins_ref.migration_context = objects.MigrationContext(
-            migration_id=42)
         migration = objects.Migration(source_compute='fake-host1',
                                       dest_compute='fake-host2')
 
         with test.nested(
                 mock.patch.object(rbd_utils, 'RBDDriver'),
                 mock.patch.object(utils, 'get_image_from_system_metadata'),
                 mock.patch.object(drvr, '_create_domain_and_network'),
-                mock.patch.object(drvr, '_get_guest_xml'),
-                mock.patch.object(objects.Migration, 'get_by_id_and_instance',
-                                  return_value=migration)) as (
-                mock_rbd, mock_image, mock_cdn, mock_ggx, mock_get_mig):
+                mock.patch.object(drvr, '_get_guest_xml')) as (
+                mock_rbd, mock_image, mock_cdn, mock_ggx):
             mock_image.return_value = {'disk_format': 'raw'}
             drvr.finish_revert_migration('', ins_ref,
                                          network_model.NetworkInfo(),
-                                         power_on=False)
+                                         migration, power_on=False)
             self.assertFalse(drvr.image_backend.rollback_to_snap.called)
             self.assertFalse(drvr.image_backend.remove_snap.called)
-            mock_get_mig.assert_called_with(mock.ANY, 42, ins_ref.uuid)
 
     @mock.patch.object(shutil, 'rmtree')
     def test_cleanup_failed_migration(self, mock_rmtree):

nova/tests/unit/virt/xenapi/test_xenapi.py

@@ -1451,7 +1451,7 @@ def finish_revert_migration(self, context, instance, block_info,
 
         conn = xenapi_conn.XenAPIDriver(fake.FakeVirtAPI(), False)
         conn._vmops = VMOpsMock()
-        conn.finish_revert_migration(self.context, instance, None)
+        conn.finish_revert_migration(self.context, instance, None, None)
         self.assertTrue(conn._vmops.finish_revert_migration_called)
 
     def test_reboot_hard(self):
@@ -1921,7 +1921,8 @@ def fake_finish_revert_migration(*args, **kwargs):
         self.assertTrue(self.called)
         self.assertEqual(self.fake_vm_start_called, power_on)
 
-        conn.finish_revert_migration(context, instance, network_info)
+        conn.finish_revert_migration(context, instance, network_info,
+                                     self.migration)
         self.assertTrue(self.fake_finish_revert_migration_called)
 
     def test_revert_migrate_power_on(self):

nova/virt/driver.py

@@ -737,12 +737,14 @@ def confirm_migration(self, context, migration, instance, network_info):
         raise NotImplementedError()
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         """Finish reverting a resize/migration.
 
         :param context: the context for the finish_revert_migration
         :param instance: nova.objects.instance.Instance being migrated/resized
         :param network_info: instance network information
+        :param migration: nova.objects.Migration for the migration
         :param block_device_info: instance volume block device info
         :param power_on: True if the instance should be powered on, False
                          otherwise

nova/virt/fake.py

@@ -237,7 +237,8 @@ def migrate_disk_and_power_off(self, context, instance, dest,
         pass
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         self.instances[instance.uuid] = FakeInstance(
             instance.name, power_state.RUNNING, instance.uuid)
 

nova/virt/hyperv/driver.py

@@ -325,7 +325,8 @@ def confirm_migration(self, context, migration, instance, network_info):
                                              instance, network_info)
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         self._migrationops.finish_revert_migration(context, instance,
                                                    network_info,
                                                    block_device_info, power_on)

nova/virt/libvirt/driver.py

@@ -9151,7 +9151,8 @@ def _cleanup_failed_migration(self, inst_base):
                 raise
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         LOG.debug("Starting finish_revert_migration",
                   instance=instance)
 
@@ -9184,11 +9185,6 @@ def finish_revert_migration(self, context, instance, network_info,
         # _finish_revert_resize_network_migrate_finish(), right after updating
         # the port binding. For any ports not covered by those "bind-time"
         # events, we wait for "plug-time" events here.
-        # TODO(artom) This DB lookup is done for backportability. A subsequent
-        # patch will remove it and change the finish_revert_migration() method
-        # signature to pass is the migration object.
-        migration = objects.Migration.get_by_id_and_instance(
-            context, instance.migration_context.migration_id, instance.uuid)
         events = network_info.get_plug_time_events(migration)
         if events:
             LOG.debug('Instance is using plug-time events: %s', events,

nova/virt/vmwareapi/driver.py

@@ -269,7 +269,8 @@ def confirm_migration(self, context, migration, instance, network_info):
         self._vmops.confirm_migration(migration, instance, network_info)
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         """Finish reverting a resize, powering back on the instance."""
         self._vmops.finish_revert_migration(context, instance, network_info,
                                             block_device_info, power_on)

nova/virt/xenapi/driver.py

@@ -254,7 +254,8 @@ def confirm_migration(self, context, migration, instance, network_info):
         self._vmops.confirm_migration(migration, instance, network_info)
 
     def finish_revert_migration(self, context, instance, network_info,
-                                block_device_info=None, power_on=True):
+                                migration, block_device_info=None,
+                                power_on=True):
         """Finish reverting a resize."""
         # NOTE(vish): Xen currently does not use network info.
         self._vmops.finish_revert_migration(context, instance,