Handle Cinder 3.27 style attachments in swap_volume

This adds the support for new style volume attachments
to the swap volume operation.

If the existing volume attachment (old_volume) was created
using the new flow, it will have attachment_id set. Based
on that the API will create a new volume attachment for the
volume being swapped to and then pass that attachment to the
compute over RPC. Note that we do not yet create new-style
volume attachments so this is all no-op code for now. And
when we do start creating new-style volume attachments, that
will only be allowed when Cinder is new enough (3.27 microversion)
and all compute services are upgraded, so we don't perform
any nova-compute service version checks here.

Once on the compute, we key off the new_attachment_id being
passed from the API to tell if we should use the old or new
style calls to Cinder. For example, initialize_connection and
terminate_connection are replaced with attachment_update and
attachment_delete.

Co-Authored-By: Matt Riedemann <mriedem.os@gmail.com>
Co-Authored-By: John Griffith <john.griffith8@gmail.com>

Depends-On: I23391300468cc5e2f45a12d5ad8617ab48c21e6a

Partially Implements: blueprint cinder-new-attach-apis

Change-Id: I4b8bd01f1ffe2640fe7313213bf853d2e1bef9dd

doc/notification_samples/service-update.json

@@ -13,7 +13,7 @@
             "disabled_reason": null,
             "report_count": 1,
             "forced_down": false,
-            "version": 20,
+            "version": 21,
             "availability_zone": null,
             "uuid": "fa69c544-906b-4a6a-a9c6-c1f7a8078c73"
         }

nova/api/openstack/compute/volumes.py

@@ -411,7 +411,8 @@ class VolumeAttachmentController(wsgi.Controller):
                                                  new_volume)
                     found = True
                     break
-                except exception.VolumeUnattached:
+                except (exception.VolumeUnattached,
+                        exception.VolumeBDMNotFound):
                     # The volume is not attached.  Treat it as NotFound
                     # by falling through.
                     pass

nova/compute/api.py

@@ -3906,16 +3906,36 @@ class API(base.Base):
         self.volume_api.check_availability_zone(context, new_volume,
                                                 instance=instance)
         self.volume_api.begin_detaching(context, old_volume['id'])
-        self.volume_api.reserve_volume(context, new_volume['id'])
+
+        # Get the BDM for the attached (old) volume so we can tell if it was
+        # attached with the new-style Cinder 3.27 API.
+        bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
+            context, old_volume['id'], instance.uuid)
+        new_attachment_id = None
+        if bdm.attachment_id is None:
+            # This is an old-style attachment so reserve the new volume before
+            # we cast to the compute host.
+            self.volume_api.reserve_volume(context, new_volume['id'])
+        else:
+            # This is a new-style attachment so for the volume that we are
+            # going to swap to, create a new volume attachment.
+            new_attachment_id = self.volume_api.attachment_create(
+                context, new_volume['id'], instance.uuid)['id']
+
         try:
             self.compute_rpcapi.swap_volume(
                     context, instance=instance,
                     old_volume_id=old_volume['id'],
-                    new_volume_id=new_volume['id'])
+                    new_volume_id=new_volume['id'],
+                    new_attachment_id=new_attachment_id)
         except Exception:
             with excutils.save_and_reraise_exception():
                 self.volume_api.roll_detaching(context, old_volume['id'])
-                self.volume_api.unreserve_volume(context, new_volume['id'])
+                if new_attachment_id is None:
+                    self.volume_api.unreserve_volume(context, new_volume['id'])
+                else:
+                    self.volume_api.attachment_delete(
+                        context, new_attachment_id)
 
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED,

nova/compute/manager.py

@@ -481,7 +481,7 @@ class ComputeVirtAPI(virtapi.VirtAPI):
 class ComputeManager(manager.Manager):
     """Manages the running instances from creation to destruction."""
 
-    target = messaging.Target(version='4.16')
+    target = messaging.Target(version='4.17')
 
     # How long to wait in seconds before re-issuing a shutdown
     # signal to an instance during power off.  The overall
@@ -5007,11 +5007,24 @@ class ComputeManager(manager.Manager):
                             attachment_id=attachment_id)
 
     def _init_volume_connection(self, context, new_volume_id,
-                                old_volume_id, connector, instance, bdm):
+                                old_volume_id, connector, bdm,
+                                new_attachment_id):
+
+        if new_attachment_id is None:
+            # We're dealing with an old-style attachment so initialize the
+            # connection so we can get the connection_info.
+            new_cinfo = self.volume_api.initialize_connection(context,
+                                                              new_volume_id,
+                                                              connector)
+        else:
+            # This is a new style attachment and the API created the new
+            # volume attachment and passed the id to the compute over RPC.
+            # At this point we need to update the new volume attachment with
+            # the host connector, which will give us back the new attachment
+            # connection_info.
+            new_cinfo = self.volume_api.attachment_update(
+                context, new_attachment_id, connector).connection_info
 
-        new_cinfo = self.volume_api.initialize_connection(context,
-                                                          new_volume_id,
-                                                          connector)
         old_cinfo = jsonutils.loads(bdm['connection_info'])
         if old_cinfo and 'serial' not in old_cinfo:
             old_cinfo['serial'] = old_volume_id
@@ -5023,17 +5036,15 @@ class ComputeManager(manager.Manager):
         return (old_cinfo, new_cinfo)
 
     def _swap_volume(self, context, instance, bdm, connector,
-                     old_volume_id, new_volume_id, resize_to):
+                     old_volume_id, new_volume_id, resize_to,
+                     new_attachment_id, is_cinder_migration):
         mountpoint = bdm['device_name']
         failed = False
         new_cinfo = None
         try:
-            old_cinfo, new_cinfo = self._init_volume_connection(context,
-                                                                new_volume_id,
-                                                                old_volume_id,
-                                                                connector,
-                                                                instance,
-                                                                bdm)
+            old_cinfo, new_cinfo = self._init_volume_connection(
+                context, new_volume_id, old_volume_id, connector,
+                bdm, new_attachment_id)
             # NOTE(lyarwood): The Libvirt driver, the only virt driver
             # currently implementing swap_volume, will modify the contents of
             # new_cinfo when connect_volume is called. This is then saved to
@@ -5068,39 +5079,85 @@ class ComputeManager(manager.Manager):
                     LOG.exception(msg, {'volume_id': new_volume_id,
                                         'mountpoint': bdm['device_name']},
                                   instance=instance)
+
+                # The API marked the volume as 'detaching' for the old volume
+                # so we need to roll that back so the volume goes back to
+                # 'in-use' state.
                 self.volume_api.roll_detaching(context, old_volume_id)
-                self.volume_api.unreserve_volume(context, new_volume_id)
+
+                if new_attachment_id is None:
+                    # The API reserved the new volume so it would be in
+                    # 'attaching' status, so we need to unreserve it so it
+                    # goes back to 'available' status.
+                    self.volume_api.unreserve_volume(context, new_volume_id)
+                else:
+                    # This is a new style attachment for the new volume, which
+                    # was created in the API. We just need to delete it here
+                    # to put the new volume back into 'available' status.
+                    self.volume_api.attachment_delete(
+                        context, new_attachment_id)
         finally:
+            # TODO(mriedem): This finally block is terribly confusing and is
+            # trying to do too much. We should consider removing the finally
+            # block and move whatever needs to happen on success and failure
+            # into the blocks above for clarity, even if it means a bit of
+            # redundant code.
             conn_volume = new_volume_id if failed else old_volume_id
             if new_cinfo:
-                LOG.debug("swap_volume: calling Cinder terminate_connection "
-                          "for %(volume)s", {'volume': conn_volume},
+                LOG.debug("swap_volume: removing Cinder connection "
+                          "for volume %(volume)s", {'volume': conn_volume},
+                          instance=instance)
+                if bdm.attachment_id is None:
+                    # This is the pre-3.27 flow for new-style volume
+                    # attachments so just terminate the connection.
+                    self.volume_api.terminate_connection(context,
+                                                         conn_volume,
+                                                         connector)
+                else:
+                    # This is a new style volume attachment. If we failed, then
+                    # the new attachment was already deleted above in the
+                    # exception block and we have nothing more to do here. If
+                    # swap_volume was successful in the driver, then we need to
+                    # "detach" the original attachment by deleting it.
+                    if not failed:
+                        self.volume_api.attachment_delete(
+                            context, bdm.attachment_id)
+
+            # Need to make some decisions based on whether this was
+            # a Cinder initiated migration or not. The callback to
+            # migration completion isn't needed in the case of a
+            # nova initiated simple swap of two volume
+            # "volume-update" call so skip that. The new attachment
+            # scenarios will give us a new attachment record and
+            # that's what we want.
+            if bdm.attachment_id and not is_cinder_migration:
+                # we don't callback to cinder
+                comp_ret = {'save_volume_id': new_volume_id}
+            else:
+                # NOTE(lyarwood): The following call to
+                # os-migrate-volume-completion returns a dict containing
+                # save_volume_id, this volume id has two possible values :
+                # 1. old_volume_id if we are migrating (retyping) volumes
+                # 2. new_volume_id if we are swapping between two existing
+                #    volumes
+                # This volume id is later used to update the volume_id and
+                # connection_info['serial'] of the BDM.
+                comp_ret = self.volume_api.migrate_volume_completion(
+                                                          context,
+                                                          old_volume_id,
+                                                          new_volume_id,
+                                                          error=failed)
+                LOG.debug("swap_volume: Cinder migrate_volume_completion "
+                          "returned: %(comp_ret)s", {'comp_ret': comp_ret},
                           instance=instance)
-                self.volume_api.terminate_connection(context,
-                                                     conn_volume,
-                                                     connector)
-            # NOTE(lyarwood): The following call to
-            # os-migrate-volume-completion returns a dict containing
-            # save_volume_id, this volume id has two possible values :
-            # 1. old_volume_id if we are migrating (retyping) volumes
-            # 2. new_volume_id if we are swapping between two existing volumes
-            # This volume id is later used to update the volume_id and
-            # connection_info['serial'] of the BDM.
-            comp_ret = self.volume_api.migrate_volume_completion(
-                                                      context,
-                                                      old_volume_id,
-                                                      new_volume_id,
-                                                      error=failed)
-            LOG.debug("swap_volume: Cinder migrate_volume_completion "
-                      "returned: %(comp_ret)s", {'comp_ret': comp_ret},
-                      instance=instance)
 
         return (comp_ret, new_cinfo)
 
     @wrap_exception()
     @reverts_task_state
     @wrap_instance_fault
-    def swap_volume(self, context, old_volume_id, new_volume_id, instance):
+    def swap_volume(self, context, old_volume_id, new_volume_id, instance,
+                    new_attachment_id=None):
         """Swap volume for an instance."""
         context = context.elevated()
 
@@ -5115,7 +5172,16 @@ class ComputeManager(manager.Manager):
         connector = self.driver.get_volume_connector(instance)
 
         resize_to = 0
-        old_vol_size = self.volume_api.get(context, old_volume_id)['size']
+        old_volume = self.volume_api.get(context, old_volume_id)
+        # Yes this is a tightly-coupled state check of what's going on inside
+        # cinder, but we need this while we still support old (v1/v2) and
+        # new style attachments (v3.27). Once we drop support for old style
+        # attachments we could think about cleaning up the cinder-initiated
+        # swap volume API flows.
+        is_cinder_migration = (
+            True if old_volume['status'] in ('retyping',
+                                             'migrating') else False)
+        old_vol_size = old_volume['size']
         new_vol_size = self.volume_api.get(context, new_volume_id)['size']
         if new_vol_size > old_vol_size:
             resize_to = new_vol_size
@@ -5123,12 +5189,15 @@ class ComputeManager(manager.Manager):
         LOG.info('Swapping volume %(old_volume)s for %(new_volume)s',
                  {'old_volume': old_volume_id, 'new_volume': new_volume_id},
                  instance=instance)
-        comp_ret, new_cinfo = self._swap_volume(context, instance,
-                                                         bdm,
-                                                         connector,
-                                                         old_volume_id,
-                                                         new_volume_id,
-                                                         resize_to)
+        comp_ret, new_cinfo = self._swap_volume(context,
+                                                instance,
+                                                bdm,
+                                                connector,
+                                                old_volume_id,
+                                                new_volume_id,
+                                                resize_to,
+                                                new_attachment_id,
+                                                is_cinder_migration)
 
         # NOTE(lyarwood): Update the BDM with the modified new_cinfo and
         # correct volume_id returned by Cinder.
@@ -5145,6 +5214,11 @@ class ComputeManager(manager.Manager):
         if resize_to:
             values['volume_size'] = resize_to
 
+        if new_attachment_id is not None:
+            # This was a volume swap for a new-style attachment so we
+            # need to update the BDM attachment_id for the new attachment.
+            values['attachment_id'] = new_attachment_id
+
         LOG.debug("swap_volume: Updating volume %(volume_id)s BDM record with "
                   "%(updates)s", {'volume_id': bdm.volume_id,
                                   'updates': values},

nova/compute/rpcapi.py

@@ -326,6 +326,7 @@ class ComputeAPI(object):
                  was bumped to signal the availability of the corrected RPC API
         * 4.15 - Add tag argument to reserve_block_device_name()
         * 4.16 - Add tag argument to attach_interface()
+        * 4.17 - Add new_attachment_id to swap_volume.
     '''
 
     VERSION_ALIASES = {
@@ -914,13 +915,20 @@ class ComputeAPI(object):
                 server=host, version=version)
         return cctxt.call(ctxt, 'set_host_enabled', enabled=enabled)
 
-    def swap_volume(self, ctxt, instance, old_volume_id, new_volume_id):
-        version = '4.0'
-        cctxt = self.router.client(ctxt).prepare(
-                server=_compute_host(None, instance), version=version)
-        cctxt.cast(ctxt, 'swap_volume',
-                   instance=instance, old_volume_id=old_volume_id,
-                   new_volume_id=new_volume_id)
+    def swap_volume(self, ctxt, instance, old_volume_id, new_volume_id,
+                    new_attachment_id=None):
+        version = '4.17'
+        client = self.router.client(ctxt)
+        kwargs = dict(instance=instance,
+                      old_volume_id=old_volume_id,
+                      new_volume_id=new_volume_id,
+                      new_attachment_id=new_attachment_id)
+        if not client.can_send_version(version):
+            version = '4.0'
+            kwargs.pop('new_attachment_id')
+        cctxt = client.prepare(
+            server=_compute_host(None, instance), version=version)
+        cctxt.cast(ctxt, 'swap_volume', **kwargs)
 
     def get_host_uptime(self, ctxt, host):
         version = '4.0'

nova/objects/service.py

@@ -32,7 +32,7 @@ LOG = logging.getLogger(__name__)
 
 
 # NOTE(danms): This is the global service version counter
-SERVICE_VERSION = 20
+SERVICE_VERSION = 21
 
 
 # NOTE(danms): This is our SERVICE_VERSION history. The idea is that any
@@ -108,6 +108,8 @@ SERVICE_VERSION_HISTORY = (
     {'compute_rpc': '4.15'},
     # Version 20: Compute RPC version 4.16
     {'compute_rpc': '4.16'},
+    # Version 21: Compute RPC version 4.17
+    {'compute_rpc': '4.17'},
 )
 
 

nova/tests/unit/api/openstack/compute/test_volumes.py

@@ -688,6 +688,16 @@ class VolumeAttachTestsV21(test.NoDBTestCase):
                           self.attachments,
                           body=body)
 
+    def test_swap_volume_for_bdm_not_found(self):
+
+        def fake_swap_volume_for_bdm_not_found(self, context, instance,
+                                           old_volume, new_volume):
+            raise exception.VolumeBDMNotFound(volume_id=FAKE_UUID_C)
+
+        self.assertRaises(webob.exc.HTTPNotFound, self._test_swap,
+                          self.attachments,
+                          fake_func=fake_swap_volume_for_bdm_not_found)
+
 
 class VolumeAttachTestsV249(test.NoDBTestCase):
     validation_error = exception.ValidationError

nova/tests/unit/compute/test_compute_api.py

@@ -2424,7 +2424,14 @@ class _ComputeAPIUnitTestMixIn(object):
     def test_swap_volume_volume_api_usage(self):
         self._test_swap_volume()
 
-    def _test_swap_volume(self, expected_exception=None):
+    def test_swap_volume_volume_api_usage_new_attach_flow(self):
+        self._test_swap_volume(attachment_id=uuids.attachment_id)
+
+    def test_swap_volume_with_swap_volume_error_new_attach_flow(self):
+        self._test_swap_volume(expected_exception=AttributeError,
+                               attachment_id=uuids.attachment_id)
+
+    def _test_swap_volume(self, expected_exception=None, attachment_id=None):
         volumes = self._get_volumes_for_test_swap_volume()
         instance = self._get_instance_for_test_swap_volume()
 
@@ -2447,19 +2454,45 @@ class _ComputeAPIUnitTestMixIn(object):
             if volumes[volume_id]['status'] == 'attaching':
                 volumes[volume_id]['status'] = 'available'
 
+        def fake_vol_api_attachment_create(context, volume_id, instance_id):
+            self.assertTrue(uuidutils.is_uuid_like(volume_id))
+            self.assertEqual('available', volumes[volume_id]['status'])
+            volumes[volume_id]['status'] = 'reserved'
+            return {'id': uuids.attachment_id}
+
+        def fake_vol_api_attachment_delete(context, attachment_id):
+            self.assertTrue(uuidutils.is_uuid_like(attachment_id))
+            if volumes[uuids.new_volume]['status'] == 'reserved':
+                volumes[uuids.new_volume]['status'] = 'available'
+
         @mock.patch.object(self.compute_api.compute_rpcapi, 'swap_volume',
                            return_value=True)
         @mock.patch.object(self.compute_api.volume_api, 'unreserve_volume',
                            side_effect=fake_vol_api_unreserve)
+        @mock.patch.object(self.compute_api.volume_api, 'attachment_delete',
+                           side_effect=fake_vol_api_attachment_delete)
         @mock.patch.object(self.compute_api.volume_api, 'reserve_volume',
                            side_effect=fake_vol_api_reserve)
+        @mock.patch.object(self.compute_api.volume_api, 'attachment_create',
+                           side_effect=fake_vol_api_attachment_create)
         @mock.patch.object(self.compute_api.volume_api, 'roll_detaching',
                            side_effect=fake_vol_api_roll_detaching)
+        @mock.patch.object(objects.BlockDeviceMapping,
+                           'get_by_volume_and_instance')
         @mock.patch.object(self.compute_api.volume_api, 'begin_detaching',
                            side_effect=fake_vol_api_begin_detaching)
-        def _do_test(mock_begin_detaching, mock_roll_detaching,
-                     mock_reserve_volume, mock_unreserve_volume,
-                     mock_swap_volume):
+        def _do_test(mock_begin_detaching, mock_get_by_volume_and_instance,
+                     mock_roll_detaching, mock_attachment_create,
+                     mock_reserve_volume, mock_attachment_delete,
+                     mock_unreserve_volume, mock_swap_volume):
+            bdm = objects.BlockDeviceMapping(
+                        **fake_block_device.FakeDbBlockDeviceDict(
+                        {'no_device': False, 'volume_id': '1', 'boot_index': 0,
+                         'connection_info': 'inf', 'device_name': '/dev/vda',
+                         'source_type': 'volume', 'destination_type': 'volume',
+                         'tag': None, 'attachment_id': attachment_id},
+                        anon=True))
+            mock_get_by_volume_and_instance.return_value = bdm
             if expected_exception:
                 mock_swap_volume.side_effect = AttributeError()
                 self.assertRaises(expected_exception,
@@ -2469,10 +2502,45 @@ class _ComputeAPIUnitTestMixIn(object):
                 self.assertEqual('in-use', volumes[uuids.old_volume]['status'])
                 self.assertEqual('available',
                                  volumes[uuids.new_volume]['status'])
+                # Make assertions about what was called if there was or was not
+                # a Cinder 3.27 style attachment provided.
+                if attachment_id is None:
+                    # Old style attachment, so unreserve was called and
+                    # attachment_delete was not called.
+                    mock_unreserve_volume.assert_called_once_with(
+                        self.context, uuids.new_volume)
+                    mock_attachment_delete.assert_not_called()
+                else:
+                    # New style attachment, so unreserve was not called and
+                    # attachment_delete was called.
+                    mock_unreserve_volume.assert_not_called()
+                    mock_attachment_delete.assert_called_once_with(
+                        self.context, attachment_id)
             else:
                 self.compute_api.swap_volume(self.context, instance,
                                              volumes[uuids.old_volume],
                                              volumes[uuids.new_volume])
+                # Make assertions about what was called if there was or was not
+                # a Cinder 3.27 style attachment provided.
+                if attachment_id is None:
+                    # Old style attachment, so reserve was called and
+                    # attachment_create was not called.
+                    mock_reserve_volume.assert_called_once_with(
+                        self.context, uuids.new_volume)
+                    mock_attachment_create.assert_not_called()
+                else:
+                    # New style attachment, so reserve was not called and
+                    # attachment_create was called.
+                    mock_reserve_volume.assert_not_called()
+                    mock_attachment_create.assert_called_once_with(
+                        self.context, uuids.new_volume, instance.uuid)
+
+            # Assert the call to the rpcapi.
+            mock_swap_volume.assert_called_once_with(
+                self.context, instance=instance,
+                old_volume_id=uuids.old_volume,
+                new_volume_id=uuids.new_volume,
+                new_attachment_id=attachment_id)
 
         _do_test()
 

nova/tests/unit/compute/test_compute_mgr.py

@@ -1868,7 +1868,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
                    {'device_name': '/dev/vdb', 'source_type': 'volume',
                     'destination_type': 'volume',
                     'instance_uuid': uuids.instance,
-                    'connection_info': '{"foo": "bar"}'})
+                    'connection_info': '{"foo": "bar"}',
+                    'volume_id': uuids.old_volume,
+                    'attachment_id': None})
 
         def fake_vol_api_roll_detaching(context, volume_id):
             self.assertTrue(uuidutils.is_uuid_like(volume_id))
@@ -2012,7 +2014,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
                     'destination_type': 'volume',
                     'instance_uuid': uuids.instance,
                     'delete_on_termination': True,
-                    'connection_info': '{"foo": "bar"}'})
+                    'connection_info': '{"foo": "bar"}',
+                    'attachment_id': None})
         comp_ret = {'save_volume_id': old_volume_id}
         new_info = {"foo": "bar", "serial": old_volume_id}
         swap_volume_mock.return_value = (comp_ret, new_info)
@@ -2032,6 +2035,261 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
         update_bdm_mock.assert_called_once_with(mock.ANY, mock.ANY,
                                                 update_values, legacy=False)
 
+    @mock.patch.object(compute_utils, 'notify_about_volume_swap')
+    @mock.patch.object(objects.BlockDeviceMapping,
+                       'get_by_volume_and_instance')
+    @mock.patch('nova.volume.cinder.API.get')
+    @mock.patch('nova.volume.cinder.API.attachment_update',
+                return_value=mock.Mock(connection_info={}))
+    @mock.patch('nova.volume.cinder.API.attachment_delete')
+    @mock.patch('nova.volume.cinder.API.migrate_volume_completion',
+                return_value={'save_volume_id': uuids.old_volume_id})
+    def test_swap_volume_with_new_attachment_id_cinder_migrate_true(
+            self, migrate_volume_completion, attachment_delete,
+            attachment_update, get_volume, get_bdm, notify_about_volume_swap):
+        """Tests a swap volume operation with a new style volume attachment
+        passed in from the compute API, and the case that Cinder initiated
+        the swap volume because of a volume retype situation. This is a happy
+        path test. Since it is a retype there is no volume size change.
+        """
+        bdm = objects.BlockDeviceMapping(
+            volume_id=uuids.old_volume_id, device_name='/dev/vda',
+            attachment_id=uuids.old_attachment_id,
+            connection_info='{"data": {}}', volume_size=1)
+        old_volume = {
+            'id': uuids.old_volume_id, 'size': 1, 'status': 'retyping'
+        }
+        new_volume = {
+            'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved'
+        }
+        get_bdm.return_value = bdm
+        get_volume.side_effect = (old_volume, new_volume)
+        instance = fake_instance.fake_instance_obj(self.context)
+        with test.nested(
+            mock.patch.object(self.context, 'elevated',
+                              return_value=self.context),
+            mock.patch.object(self.compute.driver, 'get_volume_connector',
+                              return_value=mock.sentinel.connector),
+            mock.patch.object(bdm, 'save')
+        ) as (
+            mock_elevated, mock_get_volume_connector, mock_save
+        ):
+            self.compute.swap_volume(
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                instance, uuids.new_attachment_id)
+            # Assert the expected calls.
+            get_bdm.assert_called_once_with(
+                self.context, uuids.old_volume_id, instance.uuid)
+            # We updated the new attachment with the host connector.
+            attachment_update.assert_called_once_with(
+                self.context, uuids.new_attachment_id, mock.sentinel.connector)
+            # After a successful swap volume, we deleted the old attachment.
+            attachment_delete.assert_called_once_with(
+                self.context, uuids.old_attachment_id)
+            # After a successful swap volume, we tell Cinder so it can complete
+            # the retype operation.
+            migrate_volume_completion.assert_called_once_with(
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                error=False)
+            # The BDM should have been updated. Since it's a retype, the old
+            # volume ID is returned from Cinder so that's what goes into the
+            # BDM but the new attachment ID is saved.
+            mock_save.assert_called_once_with()
+            self.assertEqual(uuids.old_volume_id, bdm.volume_id)
+            self.assertEqual(uuids.new_attachment_id, bdm.attachment_id)
+            self.assertEqual(1, bdm.volume_size)
+            self.assertEqual(uuids.old_volume_id,
+                             jsonutils.loads(bdm.connection_info)['serial'])
+
+    @mock.patch.object(compute_utils, 'notify_about_volume_swap')
+    @mock.patch.object(objects.BlockDeviceMapping,
+                       'get_by_volume_and_instance')
+    @mock.patch('nova.volume.cinder.API.get')
+    @mock.patch('nova.volume.cinder.API.attachment_update',
+                return_value=mock.Mock(connection_info={}))
+    @mock.patch('nova.volume.cinder.API.attachment_delete')
+    @mock.patch('nova.volume.cinder.API.migrate_volume_completion')
+    def test_swap_volume_with_new_attachment_id_cinder_migrate_false(
+            self, migrate_volume_completion, attachment_delete,
+            attachment_update, get_volume, get_bdm, notify_about_volume_swap):
+        """Tests a swap volume operation with a new style volume attachment
+        passed in from the compute API, and the case that Cinder did not
+        initiate the swap volume. This is a happy path test. Since it is not a
+        retype we also change the size.
+        """
+        bdm = objects.BlockDeviceMapping(
+            volume_id=uuids.old_volume_id, device_name='/dev/vda',
+            attachment_id=uuids.old_attachment_id,
+            connection_info='{"data": {}}')
+        old_volume = {
+            'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching'
+        }
+        new_volume = {
+            'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved'
+        }
+        get_bdm.return_value = bdm
+        get_volume.side_effect = (old_volume, new_volume)
+        instance = fake_instance.fake_instance_obj(self.context)
+        with test.nested(
+            mock.patch.object(self.context, 'elevated',
+                              return_value=self.context),
+            mock.patch.object(self.compute.driver, 'get_volume_connector',
+                              return_value=mock.sentinel.connector),
+            mock.patch.object(bdm, 'save')
+        ) as (
+            mock_elevated, mock_get_volume_connector, mock_save
+        ):
+            self.compute.swap_volume(
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                instance, uuids.new_attachment_id)
+            # Assert the expected calls.
+            get_bdm.assert_called_once_with(
+                self.context, uuids.old_volume_id, instance.uuid)
+            # We updated the new attachment with the host connector.
+            attachment_update.assert_called_once_with(
+                self.context, uuids.new_attachment_id, mock.sentinel.connector)
+            # After a successful swap volume, we deleted the old attachment.
+            attachment_delete.assert_called_once_with(
+                self.context, uuids.old_attachment_id)
+            # After a successful swap volume, since it was not a
+            # Cinder-initiated call, we don't call migrate_volume_completion.
+            migrate_volume_completion.assert_not_called()
+            # The BDM should have been updated. Since it's a not a retype, the
+            # volume_id is now the new volume ID.
+            mock_save.assert_called_once_with()
+            self.assertEqual(uuids.new_volume_id, bdm.volume_id)
+            self.assertEqual(uuids.new_attachment_id, bdm.attachment_id)
+            self.assertEqual(2, bdm.volume_size)
+            self.assertEqual(uuids.new_volume_id,
+                             jsonutils.loads(bdm.connection_info)['serial'])
+
+    @mock.patch.object(compute_utils, 'add_instance_fault_from_exc')
+    @mock.patch.object(compute_utils, 'notify_about_volume_swap')
+    @mock.patch.object(objects.BlockDeviceMapping,
+                       'get_by_volume_and_instance')
+    @mock.patch('nova.volume.cinder.API.get')
+    @mock.patch('nova.volume.cinder.API.attachment_update',
+                side_effect=exception.VolumeAttachmentNotFound(
+                    attachment_id=uuids.new_attachment_id))
+    @mock.patch('nova.volume.cinder.API.roll_detaching')
+    @mock.patch('nova.volume.cinder.API.attachment_delete')
+    @mock.patch('nova.volume.cinder.API.migrate_volume_completion')
+    def test_swap_volume_with_new_attachment_id_attachment_update_fails(
+            self, migrate_volume_completion, attachment_delete, roll_detaching,
+            attachment_update, get_volume, get_bdm, notify_about_volume_swap,
+            add_instance_fault_from_exc):
+        """Tests a swap volume operation with a new style volume attachment
+        passed in from the compute API, and the case that Cinder initiated
+        the swap volume because of a volume migrate situation. This is a
+        negative test where attachment_update fails.
+        """
+        bdm = objects.BlockDeviceMapping(
+            volume_id=uuids.old_volume_id, device_name='/dev/vda',
+            attachment_id=uuids.old_attachment_id,
+            connection_info='{"data": {}}')
+        old_volume = {
+            'id': uuids.old_volume_id, 'size': 1, 'status': 'migrating'
+        }
+        new_volume = {
+            'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved'
+        }
+        get_bdm.return_value = bdm
+        get_volume.side_effect = (old_volume, new_volume)
+        instance = fake_instance.fake_instance_obj(self.context)
+        with test.nested(
+            mock.patch.object(self.context, 'elevated',
+                              return_value=self.context),
+            mock.patch.object(self.compute.driver, 'get_volume_connector',
+                              return_value=mock.sentinel.connector)
+        ) as (
+            mock_elevated, mock_get_volume_connector
+        ):
+            self.assertRaises(
+                exception.VolumeAttachmentNotFound, self.compute.swap_volume,
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                instance, uuids.new_attachment_id)
+            # Assert the expected calls.
+            get_bdm.assert_called_once_with(
+                self.context, uuids.old_volume_id, instance.uuid)
+            # We tried to update the new attachment with the host connector.
+            attachment_update.assert_called_once_with(
+                self.context, uuids.new_attachment_id, mock.sentinel.connector)
+            # After a failure, we rollback the detaching status of the old
+            # volume.
+            roll_detaching.assert_called_once_with(
+                self.context, uuids.old_volume_id)
+            # After a failure, we deleted the new attachment.
+            attachment_delete.assert_called_once_with(
+                self.context, uuids.new_attachment_id)
+            # After a failure for a Cinder-initiated swap volume, we called
+            # migrate_volume_completion to let Cinder know things blew up.
+            migrate_volume_completion.assert_called_once_with(
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                error=True)
+
+    @mock.patch.object(compute_utils, 'add_instance_fault_from_exc')
+    @mock.patch.object(compute_utils, 'notify_about_volume_swap')
+    @mock.patch.object(objects.BlockDeviceMapping,
+                       'get_by_volume_and_instance')
+    @mock.patch('nova.volume.cinder.API.get')
+    @mock.patch('nova.volume.cinder.API.attachment_update',
+                return_value=mock.Mock(connection_info={}))
+    @mock.patch('nova.volume.cinder.API.roll_detaching')
+    @mock.patch('nova.volume.cinder.API.attachment_delete')
+    @mock.patch('nova.volume.cinder.API.migrate_volume_completion')
+    def test_swap_volume_with_new_attachment_id_driver_swap_fails(
+            self, migrate_volume_completion, attachment_delete, roll_detaching,
+            attachment_update, get_volume, get_bdm, notify_about_volume_swap,
+            add_instance_fault_from_exc):
+        """Tests a swap volume operation with a new style volume attachment
+        passed in from the compute API, and the case that Cinder did not
+        initiate the swap volume. This is a negative test where the compute
+        driver swap_volume method fails.
+        """
+        bdm = objects.BlockDeviceMapping(
+            volume_id=uuids.old_volume_id, device_name='/dev/vda',
+            attachment_id=uuids.old_attachment_id,
+            connection_info='{"data": {}}')
+        old_volume = {
+            'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching'
+        }
+        new_volume = {
+            'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved'
+        }
+        get_bdm.return_value = bdm
+        get_volume.side_effect = (old_volume, new_volume)
+        instance = fake_instance.fake_instance_obj(self.context)
+        with test.nested(
+            mock.patch.object(self.context, 'elevated',
+                              return_value=self.context),
+            mock.patch.object(self.compute.driver, 'get_volume_connector',
+                              return_value=mock.sentinel.connector),
+            mock.patch.object(self.compute.driver, 'swap_volume',
+                              side_effect=test.TestingException('yikes'))
+        ) as (
+            mock_elevated, mock_get_volume_connector, mock_save
+        ):
+            self.assertRaises(
+                test.TestingException, self.compute.swap_volume,
+                self.context, uuids.old_volume_id, uuids.new_volume_id,
+                instance, uuids.new_attachment_id)
+            # Assert the expected calls.
+            get_bdm.assert_called_once_with(
+                self.context, uuids.old_volume_id, instance.uuid)
+            # We updated the new attachment with the host connector.
+            attachment_update.assert_called_once_with(
+                self.context, uuids.new_attachment_id, mock.sentinel.connector)
+            # After a failure, we rollback the detaching status of the old
+            # volume.
+            roll_detaching.assert_called_once_with(
+                self.context, uuids.old_volume_id)
+            # After a failed swap volume, we deleted the new attachment.
+            attachment_delete.assert_called_once_with(
+                self.context, uuids.new_attachment_id)
+            # After a failed swap volume, since it was not a
+            # Cinder-initiated call, we don't call migrate_volume_completion.
+            migrate_volume_completion.assert_not_called()
+
     @mock.patch.object(fake_driver.FakeDriver,
                        'check_can_live_migrate_source')
     @mock.patch.object(manager.ComputeManager,

nova/tests/unit/compute/test_rpcapi.py

@@ -30,6 +30,7 @@ from nova import test
 from nova.tests.unit import fake_block_device
 from nova.tests.unit import fake_flavor
 from nova.tests.unit import fake_instance
+from nova.tests import uuidsentinel as uuids
 
 CONF = nova.conf.CONF
 
@@ -445,7 +446,29 @@ class ComputeRpcAPITestCase(test.NoDBTestCase):
     def test_swap_volume(self):
         self._test_compute_api('swap_volume', 'cast',
                 instance=self.fake_instance_obj, old_volume_id='oldid',
-                new_volume_id='newid')
+                new_volume_id='newid', new_attachment_id=uuids.attachment_id,
+                version='4.17')
+
+    def test_swap_volume_cannot_send_version_4_17(self):
+        """Tests that if the RPC client cannot send version 4.17 we drop back
+        to version 4.0 and don't send the new_attachment_id kwarg.
+        """
+        rpcapi = compute_rpcapi.ComputeAPI()
+        fake_context = mock.Mock()
+        fake_client = mock.Mock()
+        fake_client.can_send_version.return_value = False
+        fake_client.prepare.return_value = fake_context
+        with mock.patch.object(rpcapi.router, 'client',
+                               return_value=fake_client):
+            rpcapi.swap_volume(self.context, self.fake_instance_obj,
+                               uuids.old_volume_id, uuids.new_volume_id,
+                               uuids.new_attachment_id)
+            fake_client.prepare.assert_called_once_with(
+                server=self.fake_instance_obj.host, version='4.0')
+            fake_context.cast.assert_called_once_with(
+                self.context, 'swap_volume', instance=self.fake_instance_obj,
+                old_volume_id=uuids.old_volume_id,
+                new_volume_id=uuids.new_volume_id)
 
     def test_restore_instance(self):
         self._test_compute_api('restore_instance', 'cast',