Disconnecting volume from the compute host

cmd nova-manage volume_attachment refresh vm-id vol-id connetor

There were cases where the instance said to live in compute#1 but the
connection_info in the BDM record was for compute#2, and when the script
called `remote_volume_connection` then nova would call os-brick on
compute#1 (the wrong node) and try to detach it.

In some case os-brick would mistakenly think that the volume was
attached (because the target and lun matched an existing volume on the
host) and would try to disconnect, resulting in errors on the compute
logs.

- Added HostConflict exception
- Fixes dedent in cmd/manange.py
- Updates nova-mange doc

Closes-Bug: #2012365
Change-Id: I21109752ff1c56d3cefa58fcd36c68bf468e0a73

doc/source/cli/nova-manage.rst

@@ -1572,7 +1572,9 @@ command.
    * - 5
      - Instance state invalid (must be stopped and unlocked)
    * - 6
-     - Instance is not attached to volume
+     - Volume is not attached to the instance
+   * - 7
+     - Connector host is not correct
 
 
 Libvirt Commands

nova/cmd/manage.py

@@ -164,18 +164,14 @@ def locked_instance(cell_mapping, instance, reason):
     initial_state = 'locked' if instance.locked else 'unlocked'
     if not instance.locked:
         with context.target_cell(
-                    context.get_admin_context(),
+                context.get_admin_context(), cell_mapping) as cctxt:
-                    cell_mapping
-                ) as cctxt:
             compute_api.lock(cctxt, instance, reason=reason)
     try:
         yield
     finally:
         if initial_state == 'unlocked':
             with context.target_cell(
-                    context.get_admin_context(),
+                    context.get_admin_context(), cell_mapping) as cctxt:
-                    cell_mapping
-                ) as cctxt:
                 compute_api.unlock(cctxt, instance)
 
 
@@ -3139,8 +3135,15 @@ class VolumeAttachmentCommands(object):
             # TODO(lyarwood): Add delete_attachment as a kwarg to
             # remove_volume_connection as is available in the private
             # method within the manager.
+            if instance.host == connector['host']:
                 compute_rpcapi.remove_volume_connection(
                     cctxt, instance, volume_id, instance.host)
+            else:
+                msg = (
+                    f"The compute host '{connector['host']}' in the "
+                    f"connector does not match the instance host "
+                    f"'{instance.host}'.")
+                raise exception.HostConflict(_(msg))
 
             # Delete the existing volume attachment if present in the bdm.
             # This isn't present when the original attachment was made
@@ -3222,6 +3225,7 @@ class VolumeAttachmentCommands(object):
         * 4: Instance does not exist.
         * 5: Instance state invalid.
         * 6: Volume is not attached to instance.
+        * 7: Connector host is not correct.
         """
         try:
             # TODO(lyarwood): Make this optional and provide a rpcapi capable
@@ -3237,6 +3241,12 @@ class VolumeAttachmentCommands(object):
             # Refresh the volume attachment
             return self._refresh(instance_uuid, volume_id, connector)
 
+        except exception.HostConflict as e:
+            print(
+                f"The command 'nova-manage volume_attachment get_connector' "
+                f"may have been run on the wrong compute host. Or the "
+                f"instance host may be wrong and in need of repair.\n{e}")
+            return 7
         except exception.VolumeBDMNotFound as e:
             print(str(e))
             return 6

nova/exception.py

@@ -2556,3 +2556,7 @@ class EphemeralEncryptionSecretNotFound(Invalid):
 class EphemeralEncryptionCleanupFailed(NovaException):
     msg_fmt = _("Failed to clean up ephemeral encryption secrets: "
                 "%(error)s")
+
+
+class HostConflict(Exception):
+    pass

nova/tests/unit/cmd/test_manage.py

@@ -3642,6 +3642,50 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         mock_action_start.assert_called_once()
         mock_action.finish.assert_called_once()
 
+    @mock.patch('nova.compute.rpcapi.ComputeAPI', autospec=True)
+    @mock.patch('nova.volume.cinder.API', autospec=True)
+    @mock.patch('nova.compute.api.API', autospec=True)
+    @mock.patch.object(objects.BlockDeviceMapping, 'save')
+    @mock.patch.object(
+        objects.BlockDeviceMapping, 'get_by_volume_and_instance')
+    @mock.patch.object(objects.Instance, 'get_by_uuid')
+    @mock.patch.object(objects.InstanceAction, 'action_start')
+    def test_refresh_invalid_connector_host(
+        self, mock_action_start, mock_get_instance,
+        mock_get_bdm, mock_save_bdm, mock_compute_api, mock_volume_api,
+        mock_compute_rpcapi
+    ):
+        """Test refresh with a old host not disconnected properly
+        and connector host info is not correct, a fake-host is
+        passed.
+        """
+
+        fake_volume_api = mock_volume_api.return_value
+        device_name = '/dev/vda'
+
+        mock_get_instance.return_value = objects.Instance(
+            uuid=uuidsentinel.instance,
+            vm_state=obj_fields.InstanceState.STOPPED,
+            host='old-host', locked=False)
+        mock_get_bdm.return_value = objects.BlockDeviceMapping(
+            uuid=uuidsentinel.bdm, volume_id=uuidsentinel.volume,
+            attachment_id=uuidsentinel.instance,
+            device_name=device_name)
+        mock_action = mock.Mock(spec=objects.InstanceAction)
+        mock_action_start.return_value = mock_action
+
+        fake_volume_api.attachment_create.return_value = {
+            'id': uuidsentinel.new_attachment,
+        }
+        # in instance we have host as 'old-host'
+        # but here 'fake-host' is passed in connector info.
+        fake_volume_api.attachment_update.return_value = {
+            'connection_info': self._get_fake_connector_info(),
+        }
+
+        ret = self._test_refresh()
+        self.assertEqual(7, ret)
+
     @mock.patch('nova.compute.rpcapi.ComputeAPI', autospec=True)
     @mock.patch('nova.volume.cinder.API', autospec=True)
     @mock.patch('nova.compute.api.API', autospec=True)
@@ -3664,7 +3708,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         mock_get_instance.return_value = objects.Instance(
             uuid=uuidsentinel.instance,
             vm_state=obj_fields.InstanceState.STOPPED,
-            host='foo', locked=False)
+            host='fake-host', locked=False)
         mock_get_bdm.return_value = objects.BlockDeviceMapping(
             uuid=uuidsentinel.bdm, volume_id=uuidsentinel.volume,
             attachment_id=uuidsentinel.instance,