Merge "Use force=True for os-brick disconnect during delete" into stable/xena

10 files changed, 161 insertions, 13 deletions

diff --git a/nova/tests/unit/cmd/test_status.py b/nova/tests/unit/cmd/test_status.py
index d0343212f2..371f0852e1 100644
--- a/nova/tests/unit/cmd/test_status.py
+++ b/nova/tests/unit/cmd/test_status.py
@@ -502,3 +502,19 @@ class TestCheckMachineTypeUnset(test.NoDBTestCase):
             upgradecheck.Code.SUCCESS,
             result.code
         )
+
+
+class TestUpgradeCheckServiceUserToken(test.NoDBTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.cmd = status.UpgradeCommands()
+
+    def test_service_user_token_not_configured(self):
+        result = self.cmd._check_service_user_token()
+        self.assertEqual(upgradecheck.Code.FAILURE, result.code)
+
+    def test_service_user_token_configured(self):
+        self.flags(send_service_user_token=True, group='service_user')
+        result = self.cmd._check_service_user_token()
+        self.assertEqual(upgradecheck.Code.SUCCESS, result.code)
diff --git a/nova/tests/unit/virt/hyperv/test_vmops.py b/nova/tests/unit/virt/hyperv/test_vmops.py
index 1a71045ea2..0110b595c7 100644
--- a/nova/tests/unit/virt/hyperv/test_vmops.py
+++ b/nova/tests/unit/virt/hyperv/test_vmops.py
@@ -1129,7 +1129,7 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
         mock_unplug_vifs.assert_called_once_with(
             mock_instance, mock.sentinel.fake_network_info)
         mock_disconnect_volumes.assert_called_once_with(
-            mock.sentinel.FAKE_BD_INFO)
+            mock.sentinel.FAKE_BD_INFO, force=True)
         mock_delete_disk_files.assert_called_once_with(
             mock_instance.name)
 
diff --git a/nova/tests/unit/virt/hyperv/test_volumeops.py b/nova/tests/unit/virt/hyperv/test_volumeops.py
index da7262085d..4a088b6030 100644
--- a/nova/tests/unit/virt/hyperv/test_volumeops.py
+++ b/nova/tests/unit/virt/hyperv/test_volumeops.py
@@ -140,7 +140,13 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
 
         self._volumeops.disconnect_volumes(block_device_info)
         fake_volume_driver.disconnect_volume.assert_called_once_with(
-            block_device_mapping[0]['connection_info'])
+            block_device_mapping[0]['connection_info'], force=False)
+
+        # Verify force=True
+        fake_volume_driver.disconnect_volume.reset_mock()
+        self._volumeops.disconnect_volumes(block_device_info, force=True)
+        fake_volume_driver.disconnect_volume.assert_called_once_with(
+            block_device_mapping[0]['connection_info'], force=True)
 
     @mock.patch('time.sleep')
     @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
@@ -180,7 +186,7 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
 
         if attach_failed:
             fake_volume_driver.disconnect_volume.assert_called_once_with(
-                fake_conn_info)
+                fake_conn_info, force=False)
             mock_sleep.assert_has_calls(
                 [mock.call(CONF.hyperv.volume_attach_retry_interval)] *
                     CONF.hyperv.volume_attach_retry_count)
@@ -202,7 +208,13 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
         mock_get_volume_driver.assert_called_once_with(
             mock.sentinel.conn_info)
         fake_volume_driver.disconnect_volume.assert_called_once_with(
-            mock.sentinel.conn_info)
+            mock.sentinel.conn_info, force=False)
+
+        # Verify force=True
+        fake_volume_driver.disconnect_volume.reset_mock()
+        self._volumeops.disconnect_volume(mock.sentinel.conn_info, force=True)
+        fake_volume_driver.disconnect_volume.assert_called_once_with(
+            mock.sentinel.conn_info, force=True)
 
     @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
     def test_detach_volume(self, mock_get_volume_driver):
@@ -346,7 +358,13 @@ class BaseVolumeDriverTestCase(test_base.HyperVBaseTestCase):
         self._base_vol_driver.disconnect_volume(conn_info)
 
         self._conn.disconnect_volume.assert_called_once_with(
-            conn_info['data'])
+            conn_info['data'], force=False)
+
+        # Verify force=True
+        self._conn.disconnect_volume.reset_mock()
+        self._base_vol_driver.disconnect_volume(conn_info, force=True)
+        self._conn.disconnect_volume.assert_called_once_with(
+            conn_info['data'], force=True)
 
     @mock.patch.object(volumeops.BaseVolumeDriver, '_get_disk_res_path')
     def _test_get_disk_resource_path_by_conn_info(self,
diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index 224b36f5c7..f32b5cf1a2 100644
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/nova/tests/unit/virt/libvirt/test_driver.py
@@ -9152,7 +9152,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         drvr._disconnect_volume(
             self.context, fake_connection_info, fake_instance_1)
         mock_volume_driver.disconnect_volume.assert_called_once_with(
-            fake_connection_info, fake_instance_1)
+            fake_connection_info, fake_instance_1, force=False)
 
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_detach_encryptor')
     @mock.patch('nova.objects.InstanceList.get_uuids_by_host')
@@ -9526,7 +9526,12 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                 device_name='vdc',
             ),
             mock.call.detach_encryptor(**encryption),
-            mock.call.disconnect_volume(connection_info, instance)])
+            mock.call.disconnect_volume(
+                connection_info,
+                instance,
+                force=False,
+            )
+        ])
         get_device_conf_func = mock_detach_with_retry.mock_calls[0][1][2]
         self.assertEqual(mock_guest.get_disk, get_device_conf_func.func)
         self.assertEqual(('vdc',), get_device_conf_func.args)
@@ -19605,16 +19610,64 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                 self.context,
                 mock.sentinel.connection_info,
                 instance,
-                destroy_secrets=False
+                destroy_secrets=False,
+                force=True
             ),
             mock.call(
                 self.context,
                 mock.sentinel.connection_info,
                 instance,
-                destroy_secrets=True
+                destroy_secrets=True,
+                force=True
             )
         ])
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_driver')
+    @mock.patch(
+        'nova.virt.libvirt.driver.LibvirtDriver._should_disconnect_target',
+        new=mock.Mock(return_value=True))
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._detach_encryptor',
+                new=mock.Mock())
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain',
+                new=mock.Mock())
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_vpmems',
+                new=mock.Mock(return_value=None))
+    def test_cleanup_disconnect_volume(self, mock_vol_driver):
+        """Verify that we call disconnect_volume() with force=True
+
+        cleanup() is called by destroy() when an instance is being deleted and
+        force=True should be passed down to os-brick's disconnect_volume()
+        call, which will ensure removal of devices regardless of errors.
+
+        We need to ensure that devices are removed when an instance is being
+        deleted to avoid leaving leftover devices that could later be
+        erroneously connected by external entities (example: multipathd) to
+        instances that should not have access to the volumes.
+
+        See https://bugs.launchpad.net/nova/+bug/2004555 for details.
+        """
+        connection_info = mock.MagicMock()
+        block_device_info = {
+            'block_device_mapping': [
+                {
+                    'connection_info': connection_info
+                }
+            ]
+        }
+        instance = objects.Instance(self.context, **self.test_instance)
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+
+        drvr.cleanup(
+            self.context,
+            instance,
+            network_info={},
+            block_device_info=block_device_info,
+            destroy_vifs=False,
+            destroy_disks=False,
+        )
+        mock_vol_driver.return_value.disconnect_volume.assert_called_once_with(
+            connection_info, instance, force=True)
+
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_allow_native_luksv1')
     def test_swap_volume_native_luks_blocked(self, mock_allow_native_luksv1,
diff --git a/nova/tests/unit/virt/libvirt/volume/test_fibrechannel.py b/nova/tests/unit/virt/libvirt/volume/test_fibrechannel.py
index 89a59f2f1a..f0d403e300 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_fibrechannel.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_fibrechannel.py
@@ -81,3 +81,23 @@ class LibvirtFibreChannelVolumeDriverTestCase(
         self.assertEqual(requested_size, new_size)
         libvirt_driver.connector.extend_volume.assert_called_once_with(
            connection_info['data'])
+
+    def test_disconnect_volume(self):
+        device_path = '/dev/fake-dev'
+        connection_info = {'data': {'device_path': device_path}}
+
+        libvirt_driver = fibrechannel.LibvirtFibreChannelVolumeDriver(
+                                                                self.fake_host)
+        libvirt_driver.connector.disconnect_volume = mock.MagicMock()
+        libvirt_driver.disconnect_volume(
+            connection_info, mock.sentinel.instance)
+
+        libvirt_driver.connector.disconnect_volume.assert_called_once_with(
+            connection_info['data'], connection_info['data'], force=False)
+
+        # Verify force=True
+        libvirt_driver.connector.disconnect_volume.reset_mock()
+        libvirt_driver.disconnect_volume(
+            connection_info, mock.sentinel.instance, force=True)
+        libvirt_driver.connector.disconnect_volume.assert_called_once_with(
+            connection_info['data'], connection_info['data'], force=True)
diff --git a/nova/tests/unit/virt/libvirt/volume/test_iscsi.py b/nova/tests/unit/virt/libvirt/volume/test_iscsi.py
index f8a64abea5..540c9c822d 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_iscsi.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_iscsi.py
@@ -57,10 +57,19 @@ class LibvirtISCSIVolumeDriverTestCase(
                 device=device_path))
         libvirt_driver.disconnect_volume(connection_info,
                                          mock.sentinel.instance)
+        libvirt_driver.connector.disconnect_volume.assert_called_once_with(
+            connection_info['data'], None, force=False)
 
         msg = mock_LOG_warning.call_args_list[0]
         self.assertIn('Ignoring VolumeDeviceNotFound', msg[0][0])
 
+        # Verify force=True
+        libvirt_driver.connector.disconnect_volume.reset_mock()
+        libvirt_driver.disconnect_volume(
+            connection_info, mock.sentinel.instance, force=True)
+        libvirt_driver.connector.disconnect_volume.assert_called_once_with(
+            connection_info['data'], None, force=True)
+
     def test_extend_volume(self):
         device_path = '/dev/fake-dev'
         connection_info = {'data': {'device_path': device_path}}
diff --git a/nova/tests/unit/virt/libvirt/volume/test_nvme.py b/nova/tests/unit/virt/libvirt/volume/test_nvme.py
index 7b66f961eb..672e2c3421 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_nvme.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_nvme.py
@@ -63,7 +63,13 @@ class LibvirtNVMEVolumeDriverTestCase(test_volume.LibvirtVolumeBaseTestCase):
         connection_info = {'data': disk_info}
         nvme_driver.disconnect_volume(connection_info, None)
         nvme_driver.connector.disconnect_volume.assert_called_once_with(
-            disk_info, None)
+            disk_info, None, force=False)
+
+        # Verify force=True
+        nvme_driver.connector.disconnect_volume.reset_mock()
+        nvme_driver.disconnect_volume(connection_info, None, force=True)
+        nvme_driver.connector.disconnect_volume.assert_called_once_with(
+            disk_info, None, force=True)
 
     @mock.patch('os_brick.initiator.connector.InitiatorConnector.factory',
         new=mock.Mock(return_value=mock.Mock()))
diff --git a/nova/tests/unit/virt/libvirt/volume/test_scaleio.py b/nova/tests/unit/virt/libvirt/volume/test_scaleio.py
index 6d9247cd2d..ed5ab08a6e 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_scaleio.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_scaleio.py
@@ -49,7 +49,13 @@ class LibvirtScaleIOVolumeDriverTestCase(
         conn = {'data': mock.sentinel.conn_data}
         sio.disconnect_volume(conn, mock.sentinel.instance)
         sio.connector.disconnect_volume.assert_called_once_with(
-            mock.sentinel.conn_data, None)
+            mock.sentinel.conn_data, None, force=False)
+
+        # Verify force=True
+        sio.connector.disconnect_volume.reset_mock()
+        sio.disconnect_volume(conn, mock.sentinel.instance, force=True)
+        sio.connector.disconnect_volume.assert_called_once_with(
+            mock.sentinel.conn_data, None, force=True)
 
     @mock.patch('os_brick.initiator.connector.InitiatorConnector.factory',
         new=mock.Mock(return_value=mock.Mock()))
diff --git a/nova/tests/unit/virt/libvirt/volume/test_storpool.py b/nova/tests/unit/virt/libvirt/volume/test_storpool.py
index e14954f148..9ceac07260 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_storpool.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_storpool.py
@@ -53,9 +53,11 @@ class MockStorPoolConnector(object):
         }
         return {'type': 'block', 'path': test_attached[v]['path']}
 
-    def disconnect_volume(self, connection_info, device_info):
+    def disconnect_volume(self, connection_info, device_info, **kwargs):
         self.inst.assertIn('client_id', connection_info)
         self.inst.assertIn('volume', connection_info)
+        self.inst.assertIn('force', kwargs)
+        self.inst.assertEqual(self.inst.force, kwargs.get('force'))
 
         v = connection_info['volume']
         if v not in test_attached:
@@ -86,6 +88,11 @@ class MockStorPoolInitiator(object):
 class LibvirtStorPoolVolumeDriverTestCase(
         test_volume.LibvirtVolumeBaseTestCase):
 
+    def setUp(self):
+        super().setUp()
+        # This is for testing the force flag of disconnect_volume()
+        self.force = False
+
     def mock_storpool(f):
         def _config_inner_inner1(inst, *args, **kwargs):
             @mock.patch(
@@ -175,3 +182,10 @@ class LibvirtStorPoolVolumeDriverTestCase(
 
         libvirt_driver.disconnect_volume(ci_2, mock.sentinel.instance)
         self.assertDictEqual({}, test_attached)
+
+        # Connect the volume again so we can detach it again
+        libvirt_driver.connect_volume(ci_2, mock.sentinel.instance)
+        # Verify force=True
+        self.force = True
+        libvirt_driver.disconnect_volume(
+            ci_2, mock.sentinel.instance, force=True)
diff --git a/nova/tests/unit/virt/libvirt/volume/test_vzstorage.py b/nova/tests/unit/virt/libvirt/volume/test_vzstorage.py
index 883cebb55a..032ceb4fe5 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_vzstorage.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_vzstorage.py
@@ -95,7 +95,13 @@ class LibvirtVZStorageTestCase(test_volume.LibvirtVolumeBaseTestCase):
         conn = {'data': mock.sentinel.conn_data}
         drv.disconnect_volume(conn, mock.sentinel.instance)
         drv.connector.disconnect_volume.assert_called_once_with(
-            mock.sentinel.conn_data, None)
+            mock.sentinel.conn_data, None, force=False)
+
+        # Verify force=True
+        drv.connector.disconnect_volume.reset_mock()
+        drv.disconnect_volume(conn, mock.sentinel.instance, force=True)
+        drv.connector.disconnect_volume.assert_called_once_with(
+            mock.sentinel.conn_data, None, force=True)
 
     def test_libvirt_vzstorage_driver_get_config(self):
         libvirt_driver = vzstorage.LibvirtVZStorageVolumeDriver(self.fake_host)