Fix unplugging VIF when migrate/resize VM

When migrating/resizing VM to destination
host that has VIF type difference from source VIF
type, it fails due to exception in unplugging
VIF on source host after user perform a confirmation
action.

This change unplugs the vifs in resize_instance and
wraps the call to unplug in confirm with an try except
block. the call to unplug_vifs in confirm is not removed
to support rolling upgrades but a todo is added to remove
it after the Wallaby release.

Change-Id: I2c195df5fcf844c0587933b5b5995bdca1a3ebed
Closes-Bug: #1895220
(cherry picked from commit 66c7f00e1d9d7c0eebe46eb4b24b2b21f7413789)

3 files changed, 71 insertions, 5 deletions

diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index 8df027bee3..9707f15d08 100644
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/nova/tests/unit/virt/libvirt/test_driver.py
@@ -21904,6 +21904,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
                           context.get_admin_context(), ins_ref, '10.0.0.2',
                           flavor_obj, None)
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs')
     @mock.patch('nova.virt.libvirt.utils.save_and_migrate_vtpm_dir')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.'
                 '_get_instance_disk_info')
@@ -21918,8 +21919,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
     def _test_migrate_disk_and_power_off(
             self, ctxt, flavor_obj, mock_execute, mock_exists, mock_rename,
             mock_is_shared, mock_get_host_ip, mock_destroy,
-            mock_get_disk_info, mock_vtpm, block_device_info=None,
-            params_for_instance=None):
+            mock_get_disk_info, mock_vtpm, mock_unplug_vifs,
+            block_device_info=None, params_for_instance=None):
         """Test for nova.virt.libvirt.driver.LivirtConnection
         .migrate_disk_and_power_off.
         """
@@ -21937,7 +21938,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         self.assertEqual(out, disk_info_text)
         mock_vtpm.assert_called_with(
             instance.uuid, mock.ANY, mock.ANY, '10.0.0.2', mock.ANY, mock.ANY)
-
+        mock_unplug_vifs.assert_called_once()
+        mock_unplug_vifs.reset_mock()
         # dest is same host case
         out = self.drvr.migrate_disk_and_power_off(
                ctxt, instance, '10.0.0.1', flavor_obj, None,
@@ -21946,6 +21948,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         self.assertEqual(out, disk_info_text)
         mock_vtpm.assert_called_with(
             instance.uuid, mock.ANY, mock.ANY, '10.0.0.1', mock.ANY, mock.ANY)
+        mock_unplug_vifs.assert_called_once()
 
     def test_migrate_disk_and_power_off(self):
         flavor = {'root_gb': 10, 'ephemeral_gb': 20}
@@ -22002,6 +22005,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         disconnect_volume.assert_called_with(self.context,
             mock.sentinel.conn_info_vda, mock.ANY)
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs',
+                new=mock.Mock())
     @mock.patch('os.rename')
     @mock.patch('nova.virt.libvirt.utils.copy_image')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._destroy')
@@ -22050,6 +22055,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         for call in mock_copy_image.mock_calls:
             self.assertFalse(call[0].endswith('.swap'))
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs',
+                new=mock.Mock())
     def _test_migrate_disk_and_power_off_resize_check(self, expected_exc):
         """Test for nova.virt.libvirt.libvirt_driver.LibvirtConnection
         .migrate_disk_and_power_off.
@@ -22063,6 +22070,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
               self.drvr.migrate_disk_and_power_off,
               None, instance, '10.0.0.1', flavor_obj, None)
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs',
+                new=mock.Mock())
     @mock.patch('nova.virt.libvirt.utils.save_and_migrate_vtpm_dir')
     @mock.patch('oslo_concurrency.processutils.execute')
     @mock.patch('os.rename')
@@ -22244,6 +22253,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         flavor_obj = objects.Flavor(**flavor)
         self._test_migrate_disk_and_power_off(self.context, flavor_obj)
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs',
+                new=mock.Mock())
     @mock.patch('os.rename')
     @mock.patch('oslo_concurrency.processutils.execute')
     @mock.patch('nova.virt.libvirt.utils.copy_image')
@@ -22934,6 +22945,44 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             mock_unplug.assert_called_once_with(instance, fake_net)
 
     @mock.patch('time.sleep', new=mock.Mock())
+    def test_cleanup_resize_not_same_host_works_when_unplug_fails(self):
+        CONF.set_override('policy_dirs', [], group='oslo_policy')
+        host = 'not' + CONF.host
+        instance = self._create_instance({'host': host})
+        instance.old_flavor = instance.flavor
+        instance.new_flavor = instance.flavor
+        fake_net = _fake_network_info(self)
+
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+
+        with test.nested(
+                mock.patch('nova.compute.utils.is_volume_backed_instance',
+                           return_value=False),
+                mock.patch.object(os.path, 'exists'),
+                mock.patch.object(libvirt_utils, 'get_instance_path'),
+                mock.patch.object(shutil, 'rmtree'),
+                mock.patch.object(drvr.image_backend, 'by_name',
+                                  new_callable=mock.NonCallableMock),
+                mock.patch.object(drvr, '_undefine_domain'),
+                mock.patch.object(drvr, 'unplug_vifs'),
+                mock.patch('nova.virt.libvirt.driver.LOG.debug')
+        ) as (mock_volume_backed, mock_exists, mock_get_path,
+              mock_rmtree, mock_image_by_name, mock_undef, mock_unplug,
+              mock_log):
+            mock_exists.return_value = True
+            mock_get_path.return_value = '/fake/inst'
+            error = exception.InternalError("fake error")
+            mock_unplug.side_effect = error
+
+            drvr._cleanup_resize(self.context, instance, fake_net)
+
+            mock_get_path.assert_called_once_with(instance)
+            self.assertEqual(5, mock_rmtree.call_count)
+            mock_undef.assert_called_once_with(instance)
+            mock_unplug.assert_called_once_with(instance, fake_net)
+            mock_log.assert_called_once_with(error, instance=instance)
+
+    @mock.patch('time.sleep', new=mock.Mock())
     def test_cleanup_resize_not_same_host_volume_backed(self):
         """Tests cleaning up after a resize is confirmed with a volume-backed
         instance. The key point is that the instance base directory should not
diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 0516a700b1..f30dc89e11 100644
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -1658,7 +1658,12 @@ class LibvirtDriver(driver.ComputeDriver):
 
         if instance.host != CONF.host:
             self._undefine_domain(instance)
-            self.unplug_vifs(instance, network_info)
+            # TODO(sean-k-mooney): remove this call to unplug_vifs after
+            # Wallaby is released. VIFs are now unplugged in resize_instance.
+            try:
+                self.unplug_vifs(instance, network_info)
+            except exception.InternalError as e:
+                LOG.debug(e, instance=instance)
 
     def _get_volume_driver(self, connection_info):
         driver_type = connection_info.get('driver_volume_type')
@@ -10350,7 +10355,7 @@ class LibvirtDriver(driver.ComputeDriver):
                     exception.ResizeError(reason=reason))
 
         self.power_off(instance, timeout, retry_interval)
-
+        self.unplug_vifs(instance, network_info)
         block_device_mapping = driver.block_device_info_get_mapping(
             block_device_info)
         for vol in block_device_mapping:
diff --git a/releasenotes/notes/unplug-vifs-in-resize_instance-fcd98ea44e4b8725.yaml b/releasenotes/notes/unplug-vifs-in-resize_instance-fcd98ea44e4b8725.yaml
new file mode 100644
index 0000000000..fa88e7a927
--- /dev/null
+++ b/releasenotes/notes/unplug-vifs-in-resize_instance-fcd98ea44e4b8725.yaml
@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Support for cold migration and resize between hosts with different network backends
+    was previously incomplete. If the os-vif plugin for all network backends available
+    in the cloud are not installed on all nodes unplugging will fail during confirming
+    the resize. The issue is caused by the VIF unplug that happened during the resize
+    confirm action on the source host when the original backend information of the VIF
+    was not available. The fix moved the unplug to happen during the resize action
+    when such information is still available. See `bug #1895220`_ for more details.
+
+    .. _`bug #1895220`: https://bugs.launchpad.net/nova/+bug/1895220