Attaching a volume returns HTTP 202

Instead of returning an HTTP 200 and a `volumeAttachment` object,
attaching a volume to an instance returns HTTP 202 starting from API
version 2.101.

To keep the functionality for older API versions, we move the
`_attach_volume()` method from n-api to n-conductor and either do a call
or a cast depending on whether the API needs to return a value.
n-conductor then handles reserving the block_device_mapping's device by
calling n-compute before it starts the previously-already-async volume
attachment.

We have to move `_check_attach_and_reserve_volume` into compute utils,
because it's getting called in both conductor and compute api (for the
shelved offloaded attach).

The new RPC method in the conductor needs a long timeout when used with
API versions less than the new 2.101, because it waits for the call to
`reserve_block_device_name()` in nova-compute which already needs a long
timeout.

Updating the functional tests' `post_server_volume()` and
`_attach_volume()` to not return the attachment anymore is possible, as
no test uses the returned values.

Change-Id: I4d38c2679f0e88cca30055a9c8c45ba1dd6fb5ef
Implements: blueprint async-volume-attachments
Signed-off-by: Johannes Kulik <johannes.kulik@sap.com>

api-ref/source/os-volume-attachments.inc

@@ -60,7 +60,7 @@ Attach a volume to an instance
 
 Attach a volume to an instance.
 
-Normal response codes: 200
+Normal response codes: 200 (microversions 2.0 - 2.100), 202 (microversion 2.101 - )
 
 Error response codes: badRequest(400), unauthorized(401), forbidden(403), itemNotFound(404), conflict(409)
 
@@ -106,6 +106,10 @@ Request
 Response
 --------
 
+Starting with microversion 2.101, there is no body content for the response of
+a successful POST operation. Until microversion 2.100, a response contains the
+following parameters.
+
 .. rest_parameters:: parameters.yaml
 
   - volumeAttachment: volumeAttachment

doc/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json

@@ -0,0 +1,7 @@
+{
+    "volumeAttachment": {
+        "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113",
+        "tag": "foo",
+        "delete_on_termination": true
+    }
+}

doc/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json

@@ -0,0 +1,22 @@
+{
+    "volumeAttachments": [
+        {
+            "attachment_id": "979ce4f8-033a-409d-85e6-6b5c0f6a6302",
+            "delete_on_termination": false,
+            "device": "/dev/sdc",
+            "serverId": "7696780b-3f53-4688-ab25-019bfcbbd806",
+            "tag": null,
+            "volumeId": "227cc671-f30b-4488-96fd-7d0bf13648d8",
+            "bdm_uuid": "c088db45-92b8-49e8-81e2-a1b77a144b3b"
+        },
+        {
+            "attachment_id": "c5684109-0311-4fca-9814-350e46ab7d2a",
+            "delete_on_termination": true,
+            "device": "/dev/sdb",
+            "serverId": "7696780b-3f53-4688-ab25-019bfcbbd806",
+            "tag": "foo",
+            "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113",
+            "bdm_uuid": "1aa24536-6fb5-426c-8894-d627f39aa48b"
+        }
+    ]
+}

doc/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json

@@ -0,0 +1,10 @@
+{
+    "volumeAttachment": {
+        "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113",
+        "id": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113",
+        "serverId": "fddf0901-8caf-42c9-b496-133c570b171b",
+        "device": "/dev/sdb",
+        "tag": "foo",
+        "delete_on_termination": true
+    }
+}

doc/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json

@@ -0,0 +1,11 @@
+{
+    "volumeAttachment": {
+        "attachment_id": "721a5c82-5ebc-4c6a-8339-3d33d8d027ed",
+        "delete_on_termination": true,
+        "device": "/dev/sdb",
+        "serverId": "7ebed2ce-85b3-40b5-84ae-8cc725c37ed2",
+        "tag": "foo",
+        "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113",
+        "bdm_uuid": "c088db45-92b8-49e8-81e2-a1b77a144b3b"
+    }
+}

doc/api_samples/versions/v21-version-get-resp.json

@@ -19,8 +19,8 @@
             }
         ],
         "status": "CURRENT",
-        "version": "2.100",
+        "version": "2.101",
         "min_version": "2.1",
         "updated": "2013-07-23T11:33:21Z"
     }
-}
+}

doc/api_samples/versions/versions-get-resp.json

@@ -22,7 +22,7 @@
                 }
             ],
             "status": "CURRENT",
-            "version": "2.100",
+            "version": "2.101",
             "min_version": "2.1",
             "updated": "2013-07-23T11:33:21Z"
         }

nova/api/openstack/api_version_request.py

@@ -278,6 +278,9 @@ REST_API_VERSION_HISTORY = """REST API Version History:
              ``GET /servers/{server_id}``, ``GET /servers/detail``,
              ``PUT /servers/{server_id}`` and
              ``POST /server/{server_id}/action`` (rebuild) responses.
+    * 2.101 - Attaching a volume via
+              ``POST /servers/{server_id}/os-volume_attachments`` returns HTTP
+              202 Accepted instead of HTTP 200 and a volumeAttachment response.
 """
 
 # The minimum and maximum versions of the API supported
@@ -286,7 +289,7 @@ REST_API_VERSION_HISTORY = """REST API Version History:
 # Note(cyeoh): This only applies for the v2.1 API once microversions
 # support is fully merged. It does not affect the V2 API.
 _MIN_API_VERSION = '2.1'
-_MAX_API_VERSION = '2.100'
+_MAX_API_VERSION = '2.101'
 DEFAULT_API_VERSION = _MIN_API_VERSION
 
 # Almost all proxy APIs which are related to network, images and baremetal

nova/api/openstack/compute/rest_api_version_history.rst

@@ -1300,3 +1300,15 @@ The ``GET /servers/{server_id}``, ``GET /servers/detail``
 ``PUT /servers/{server_id}`` and ``POST /server/{server_id}/action``
 (rebuild) responses now include the scheduler hints provided during
 server creation.
+
+.. _microversion 2.101:
+
+2.101 (Maximum in 2026.1 Gazpacho)
+----------------------------------
+
+Attaching a volume via ``POST /servers/{server_id}/os-volume_attachments``
+returns an HTTP 202 Accepted response instead of the previous HTTP 200
+containing a ``volumeAttachment`` object. The response is now fully aync and
+does not wait for reserving a device name. Like before, callers need to watch
+the volume's state and/or server actions to recognize when the
+volume-attachment process finished.

nova/api/openstack/compute/schemas/volume_attachments.py

@@ -229,6 +229,8 @@ create_response_v279['properties']['volumeAttachment'][
     'required'
 ].append('device')
 
+create_response_v2101 = {'type': 'null'}
+
 update_response = {'type': 'null'}
 
 delete_response = {'type': 'null'}

nova/api/openstack/compute/volume_attachments.py

@@ -16,6 +16,7 @@
 """The volume attachments extension."""
 
 from oslo_utils import strutils
+import webob
 from webob import exc
 
 from nova.api.openstack import api_version_request
@@ -179,14 +180,14 @@ class VolumeAttachmentController(wsgi.Controller):
             )
         }
 
-    # TODO(mriedem): This API should return a 202 instead of a 200 response.
     @wsgi.expected_errors((400, 403, 404, 409))
     @validation.schema(schema.create, '2.0', '2.48')
     @validation.schema(schema.create_v249, '2.49', '2.78')
     @validation.schema(schema.create_v279, '2.79')
     @validation.response_body_schema(schema.create_response, '2.0', '2.69')
     @validation.response_body_schema(schema.create_response_v270, '2.70', '2.78')  # noqa: E501
-    @validation.response_body_schema(schema.create_response_v279, '2.79')
+    @validation.response_body_schema(schema.create_response_v279, '2.79', '2.100')  # noqa: E501
+    @validation.response_body_schema(schema.create_response_v2101, '2.101')
     def create(self, req, server_id, body):
         """Attach a volume to an instance."""
         context = req.environ['nova.context']
@@ -207,12 +208,16 @@ class VolumeAttachmentController(wsgi.Controller):
             _check_request_version(
                 req, '2.20', 'attach_volume', server_id, instance.vm_state)
 
+        needs_device_returned = not api_version_request.is_supported(
+            req, '2.101')
+
         try:
             supports_multiattach = common.supports_multiattach_volume(req)
             device = self.compute_api.attach_volume(
                 context, instance, volume_id, device, tag=tag,
                 supports_multiattach=supports_multiattach,
-                delete_on_termination=delete_on_termination)
+                delete_on_termination=delete_on_termination,
+                needs_device_returned=needs_device_returned)
         except exception.VolumeNotFound as e:
             raise exc.HTTPNotFound(explanation=e.format_message())
         except (exception.InstanceIsLocked, exception.DevicePathInUse) as e:
@@ -231,6 +236,14 @@ class VolumeAttachmentController(wsgi.Controller):
             raise exc.HTTPBadRequest(explanation=e.format_message())
         except exception.TooManyDiskDevices as e:
             raise exc.HTTPForbidden(explanation=e.format_message())
+        except exception.ServiceTooOld:
+            raise exc.HTTPServiceUnavailable(explanation="nova-conductor is "
+                "not on the same version as nova-api and needs to be "
+                "upgraded.")
+
+        if not needs_device_returned:
+            # New return code starting from 2.101
+            return webob.Response(status_int=202)
 
         # The attach is async
         # NOTE(mriedem): It would be nice to use
@@ -246,8 +259,9 @@ class VolumeAttachmentController(wsgi.Controller):
             attachment['tag'] = tag
         if api_version_request.is_supported(req, '2.79'):
             attachment['delete_on_termination'] = delete_on_termination
-        # TODO(stephenfin): We forgot to apply 2.89 here. We should return
+        # NOTE(stephenfin): We forgot to apply 2.89 here, but starting with
-        # 'bdm_uuid' and 'attachment_id' and stop returning 'id'
+        # 2.101 we don't return a body anymore and thus it will stay this way
+        # forever.
         return {'volumeAttachment': attachment}
 
     def _update_volume_swap(self, req, instance, id, body):

nova/compute/api.py

@@ -2039,9 +2039,9 @@ class API:
                     volume = volumes[volume_id]
                     # We do not validate the instance and volume AZ here
                     # because that is done earlier by _provision_instances.
-                    self._check_attach_and_reserve_volume(
+                    compute_utils.check_attach_and_reserve_volume(
-                        context, volume, instance, bdm, supports_multiattach,
+                        context, self.volume_api, volume, instance, bdm,
-                        validate_az=False)
+                        supports_multiattach, validate_az=False)
                     bdm.volume_size = volume.get('size')
                 except (exception.CinderConnectionFailed,
                         exception.InvalidVolume,
@@ -5056,42 +5056,28 @@ class API:
         """Inject network info for the instance."""
         self.compute_rpcapi.inject_network_info(context, instance=instance)
 
-    def _create_volume_bdm(self, context, instance, device, volume,
+    def _create_volume_bdm_locally(self, context, instance, device, volume,
-                           disk_bus, device_type, is_local_creation=False,
+            disk_bus, device_type, tag=None, delete_on_termination=False):
-                           tag=None, delete_on_termination=False):
         volume_id = volume['id']
-        if is_local_creation:
+        # when the creation is done locally we can't specify the device
-            # when the creation is done locally we can't specify the device
+        # name as we do not have a way to check that the name specified is
-            # name as we do not have a way to check that the name specified is
+        # a valid one.
-            # a valid one.
+        # We leave the setting of that value when the actual attach
-            # We leave the setting of that value when the actual attach
+        # happens on the compute manager
-            # happens on the compute manager
+        # NOTE(artom) Local attach (to a shelved-offload instance) cannot
-            # NOTE(artom) Local attach (to a shelved-offload instance) cannot
+        # support device tagging because we have no way to call the compute
-            # support device tagging because we have no way to call the compute
+        # manager to check that it supports device tagging. In fact, we
-            # manager to check that it supports device tagging. In fact, we
+        # don't even know which computer manager the instance will
-            # don't even know which computer manager the instance will
+        # eventually end up on when it's unshelved.
-            # eventually end up on when it's unshelved.
+        volume_bdm = objects.BlockDeviceMapping(
-            volume_bdm = objects.BlockDeviceMapping(
+            context=context,
-                context=context,
+            source_type='volume', destination_type='volume',
-                source_type='volume', destination_type='volume',
+            instance_uuid=instance.uuid, boot_index=None,
-                instance_uuid=instance.uuid, boot_index=None,
+            volume_id=volume_id,
-                volume_id=volume_id,
+            device_name=None, guest_format=None,
-                device_name=None, guest_format=None,
+            disk_bus=disk_bus, device_type=device_type,
-                disk_bus=disk_bus, device_type=device_type,
+            delete_on_termination=delete_on_termination)
-                delete_on_termination=delete_on_termination)
+        volume_bdm.create()
-            volume_bdm.create()
-        else:
-            # NOTE(vish): This is done on the compute host because we want
-            #             to avoid a race where two devices are requested at
-            #             the same time. When db access is removed from
-            #             compute, the bdm will be created here and we will
-            #             have to make sure that they are assigned atomically.
-            volume_bdm = self.compute_rpcapi.reserve_block_device_name(
-                context, instance, device, volume_id, disk_bus=disk_bus,
-                device_type=device_type, tag=tag,
-                multiattach=volume['multiattach'])
-            volume_bdm.delete_on_termination = delete_on_termination
-            volume_bdm.save()
         return volume_bdm
 
     def _check_volume_already_attached(
@@ -5142,86 +5128,6 @@ class API:
             msg = _("volume %s already attached") % volume['id']
             raise exception.InvalidVolume(reason=msg)
 
-    def _check_attach_and_reserve_volume(self, context, volume, instance,
-                                         bdm, supports_multiattach=False,
-                                         validate_az=True):
-        """Perform checks against the instance and volume before attaching.
-
-        If validation succeeds, the bdm is updated with an attachment_id which
-        effectively reserves it during the attach process in cinder.
-
-        :param context: nova auth RequestContext
-        :param volume: volume dict from cinder
-        :param instance: Instance object
-        :param bdm: BlockDeviceMapping object
-        :param supports_multiattach: True if the request supports multiattach
-            volumes, i.e. microversion >= 2.60, False otherwise
-        :param validate_az: True if the instance and volume availability zones
-            should be validated for cross_az_attach, False to not validate AZ
-        """
-        volume_id = volume['id']
-        if validate_az:
-            self.volume_api.check_availability_zone(context, volume,
-                                                    instance=instance)
-        # If volume.multiattach=True and the microversion to
-        # support multiattach is not used, fail the request.
-        if volume['multiattach'] and not supports_multiattach:
-            raise exception.MultiattachNotSupportedOldMicroversion()
-
-        attachment_id = self.volume_api.attachment_create(
-            context, volume_id, instance.uuid)['id']
-        bdm.attachment_id = attachment_id
-        # NOTE(ildikov): In case of boot from volume the BDM at this
-        # point is not yet created in a cell database, so we can't
-        # call save().  When attaching a volume to an existing
-        # instance, the instance is already in a cell and the BDM has
-        # been created in that same cell so updating here in that case
-        # is "ok".
-        if bdm.obj_attr_is_set('id'):
-            bdm.save()
-
-    # TODO(stephenfin): Fold this back in now that cells v1 no longer needs to
-    # override it.
-    def _attach_volume(self, context, instance, volume, device,
-                       disk_bus, device_type, tag=None,
-                       supports_multiattach=False,
-                       delete_on_termination=False):
-        """Attach an existing volume to an existing instance.
-
-        This method is separated to make it possible for cells version
-        to override it.
-        """
-        try:
-            volume_bdm = self._create_volume_bdm(
-                context, instance, device, volume, disk_bus=disk_bus,
-                device_type=device_type, tag=tag,
-                delete_on_termination=delete_on_termination)
-        except oslo_exceptions.MessagingTimeout:
-            # The compute node might have already created the attachment but
-            # we never received the answer. In this case it is safe to delete
-            # the attachment as nobody will ever pick it up again.
-            with excutils.save_and_reraise_exception():
-                try:
-                    objects.BlockDeviceMapping.get_by_volume_and_instance(
-                        context, volume['id'], instance.uuid).destroy()
-                    LOG.debug("Delete BDM after compute did not respond to "
-                              f"attachment request for volume {volume['id']}")
-                except exception.VolumeBDMNotFound:
-                    LOG.debug("BDM not found, ignoring removal. "
-                              f"Error attaching volume {volume['id']}")
-        try:
-            self._check_attach_and_reserve_volume(context, volume, instance,
-                                                  volume_bdm,
-                                                  supports_multiattach)
-            self._record_action_start(
-                context, instance, instance_actions.ATTACH_VOLUME)
-            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                volume_bdm.destroy()
-
-        return volume_bdm.device_name
-
     def _attach_volume_shelved_offloaded(self, context, instance, volume,
                                          device, disk_bus, device_type,
                                          delete_on_termination):
@@ -5252,13 +5158,13 @@ class API:
                                        instance.uuid,
                                        dev)
 
-        volume_bdm = self._create_volume_bdm(
+        volume_bdm = self._create_volume_bdm_locally(
             context, instance, device, volume, disk_bus=disk_bus,
-            device_type=device_type, is_local_creation=True,
+            device_type=device_type,
             delete_on_termination=delete_on_termination)
         try:
-            self._check_attach_and_reserve_volume(context, volume, instance,
+            compute_utils.check_attach_and_reserve_volume(context,
-                                                  volume_bdm)
+                self.volume_api, volume, instance, volume_bdm)
             self._record_action_start(
                 context, instance,
                 instance_actions.ATTACH_VOLUME)
@@ -5278,7 +5184,8 @@ class API:
     def attach_volume(self, context, instance, volume_id, device=None,
                       disk_bus=None, device_type=None, tag=None,
                       supports_multiattach=False,
-                      delete_on_termination=False):
+                      delete_on_termination=False,
+                      needs_device_returned=False):
         """Attach an existing volume to an existing instance."""
         # NOTE(vish): Fail fast if the device is not going to pass. This
         #             will need to be removed along with the test if we
@@ -5289,7 +5196,7 @@ class API:
 
         # Make sure the volume isn't already attached to this instance
         # because we'll use the v3.44 attachment flow in
-        # _check_attach_and_reserve_volume and Cinder will allow multiple
+        # check_attach_and_reserve_volume and Cinder will allow multiple
         # attachments between the same volume and instance but the old flow
         # API semantics don't allow that so we enforce it here.
         # NOTE(lyarwood): Ensure that non multiattach volumes don't already
@@ -5322,10 +5229,11 @@ class API:
                                                          device_type,
                                                          delete_on_termination)
 
-        return self._attach_volume(context, instance, volume, device,
+        return self.compute_task_api.attach_volume(context, instance,
-                                   disk_bus, device_type, tag=tag,
+                volume, device, disk_bus, device_type, tag=tag,
-                                   supports_multiattach=supports_multiattach,
+                supports_multiattach=supports_multiattach,
-                                   delete_on_termination=delete_on_termination)
+                delete_on_termination=delete_on_termination,
+                do_cast=not needs_device_returned)
 
     def _detach_volume_shelved_offloaded(self, context, instance, volume):
         """Detach a volume from an instance in shelved offloaded state.

nova/compute/utils.py

@@ -1633,3 +1633,43 @@ def delete_arqs_if_needed(context, instance, arq_uuids=None):
               {'instance': instance.uuid,
                'uuid': arq_uuids})
         cyclient.delete_arqs_by_uuid(arq_uuids)
+
+
+def check_attach_and_reserve_volume(context, volume_api, volume, instance,
+                                    bdm, supports_multiattach=False,
+                                    validate_az=True):
+    """Perform checks against the instance and volume before attaching.
+
+    If validation succeeds, the bdm is updated with an attachment_id which
+    effectively reserves it during the attach process in cinder.
+
+    :param context: nova auth RequestContext
+    :param volume_api: cinder API object
+    :param volume: volume dict from cinder
+    :param instance: Instance object
+    :param bdm: BlockDeviceMapping object
+    :param supports_multiattach: True if the request supports multiattach
+        volumes, i.e. microversion >= 2.60, False otherwise
+    :param validate_az: True if the instance and volume availability zones
+        should be validated for cross_az_attach, False to not validate AZ
+    """
+    volume_id = volume['id']
+    if validate_az:
+        volume_api.check_availability_zone(context, volume,
+                                                instance=instance)
+    # If volume.multiattach=True and the microversion to
+    # support multiattach is not used, fail the request.
+    if volume['multiattach'] and not supports_multiattach:
+        raise exception.MultiattachNotSupportedOldMicroversion()
+
+    attachment_id = volume_api.attachment_create(
+        context, volume_id, instance.uuid)['id']
+    bdm.attachment_id = attachment_id
+    # NOTE(ildikov): In case of boot from volume the BDM at this
+    # point is not yet created in a cell database, so we can't
+    # call save().  When attaching a volume to an existing
+    # instance, the instance is already in a cell and the BDM has
+    # been created in that same cell so updating here in that case
+    # is "ok".
+    if bdm.obj_attr_is_set('id'):
+        bdm.save()

nova/conductor/api.py

@@ -188,3 +188,11 @@ class ComputeTaskAPI(object):
             self, ctxt, instance, migration):
         self.conductor_compute_rpcapi.revert_snapshot_based_resize(
             ctxt, instance, migration)
+
+    def attach_volume(self, ctxt, instance, volume, device, disk_bus,
+            device_type, tag=None, supports_multiattach=False,
+            delete_on_termination=False, do_cast=False):
+        return self.conductor_compute_rpcapi.attach_volume(ctxt, instance,
+            volume, device, disk_bus, device_type, tag=tag,
+            supports_multiattach=supports_multiattach,
+            delete_on_termination=delete_on_termination, do_cast=do_cast)

nova/conductor/manager.py

@@ -237,7 +237,7 @@ class ComputeTaskManager:
     may involve coordinating activities on multiple compute nodes.
     """
 
-    target = messaging.Target(namespace='compute_task', version='1.25')
+    target = messaging.Target(namespace='compute_task', version='1.26')
 
     def __init__(self):
         self.compute_rpcapi = compute_rpcapi.ComputeAPI()
@@ -2217,3 +2217,62 @@ class ComputeTaskManager:
         task = cross_cell_migrate.RevertResizeTask(
             context, instance, migration, self.notifier, self.compute_rpcapi)
         task.execute()
+
+    def _create_volume_bdm(self, context, instance, device, volume,
+                           disk_bus, device_type, tag=None,
+                           delete_on_termination=False):
+        volume_id = volume['id']
+        # NOTE(vish): This is done on the compute host because we want
+        #             to avoid a race where two devices are requested at
+        #             the same time. When db access is removed from
+        #             compute, the bdm will be created here and we will
+        #             have to make sure that they are assigned atomically.
+        volume_bdm = self.compute_rpcapi.reserve_block_device_name(
+            context, instance, device, volume_id, disk_bus=disk_bus,
+            device_type=device_type, tag=tag,
+            multiattach=volume['multiattach'])
+        volume_bdm.delete_on_termination = delete_on_termination
+        volume_bdm.save()
+        return volume_bdm
+
+    @targets_cell
+    def attach_volume(self, context, instance, volume, device, disk_bus,
+            device_type, tag=None, supports_multiattach=False,
+            delete_on_termination=False):
+        """Attach an existing volume to an existing instance.
+
+        This method lives in conductor so it can do the synchronous call to
+        compute for reserving a block-device mapping instead of having nova-api
+        do it starting with API version 2.101.
+        """
+        try:
+            volume_bdm = self._create_volume_bdm(
+                context, instance, device, volume, disk_bus=disk_bus,
+                device_type=device_type, tag=tag,
+                delete_on_termination=delete_on_termination)
+        except messaging.exceptions.MessagingTimeout:
+            # The compute node might have already created the attachment but
+            # we never received the answer. In this case it is safe to delete
+            # the attachment as nobody will ever pick it up again.
+            with excutils.save_and_reraise_exception():
+                try:
+                    objects.BlockDeviceMapping.get_by_volume_and_instance(
+                        context, volume['id'], instance.uuid).destroy()
+                    LOG.debug("Delete BDM after compute did not respond to "
+                              f"attachment request for volume {volume['id']}")
+                except exception.VolumeBDMNotFound:
+                    LOG.debug("BDM not found, ignoring removal. "
+                              f"Error attaching volume {volume['id']}")
+        try:
+            compute_utils.check_attach_and_reserve_volume(
+                context, self.volume_api, volume, instance, volume_bdm,
+                supports_multiattach)
+            objects.InstanceAction.action_start(
+                context, instance.uuid, instance_actions.ATTACH_VOLUME,
+                want_result=False)
+            self.compute_rpcapi.attach_volume(context, instance, volume_bdm)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                volume_bdm.destroy()
+
+        return volume_bdm.device_name

nova/conductor/rpcapi.py

@@ -288,6 +288,7 @@ class ComputeTaskAPI(object):
     1.23 - Added revert_snapshot_based_resize()
     1.24 - Add reimage_boot_volume parameter to rebuild_instance()
     1.25 - Add target_state parameter to rebuild_instance()
+    1.26 - Added attach_volume()
     """
 
     def __init__(self):
@@ -494,3 +495,23 @@ class ComputeTaskAPI(object):
         kw = {'instance': instance, 'migration': migration}
         cctxt = self.client.prepare(version=version)
         cctxt.cast(ctxt, 'revert_snapshot_based_resize', **kw)
+
+    def attach_volume(self, ctxt, instance, volume, device, disk_bus,
+            device_type, tag=None, supports_multiattach=False,
+            delete_on_termination=False, do_cast=False):
+        version = '1.26'
+        if not self.client.can_send_version(version):
+            raise exception.ServiceTooOld(_('nova-conductor too old'))
+        kw = {'instance': instance, 'volume': volume, 'device': device,
+              'disk_bus': disk_bus, 'device_type': device_type, 'tag': tag,
+              'supports_multiattach': supports_multiattach,
+              'delete_on_termination': delete_on_termination}
+        if do_cast:
+            cctxt = self.client.prepare(version=version)
+            return cctxt.cast(ctxt, 'attach_volume', **kw)
+        # NOTE(jkulik): We call nova-compute's reserve_block_device_name(),
+        # which uses long_rpc_timeout and thus we need a long_rpc_timeout, too.
+        cctxt = self.client.prepare(
+            version=version, call_monitor_timeout=CONF.rpc_response_timeout,
+            timeout=CONF.long_rpc_timeout)
+        return cctxt.call(ctxt, 'attach_volume', **kw)

nova/tests/functional/api/client.py

@@ -384,7 +384,7 @@ class TestOpenStackClient(object):
     def post_server_volume(self, server_id, volume_attachment):
         return self.api_post('/servers/%s/os-volume_attachments' %
                              (server_id), volume_attachment
-                            ).body['volumeAttachment']
+                            )
 
     def put_server_volume(self, server_id, original_volume_id, volume_id):
         return self.api_put('/servers/%s/os-volume_attachments/%s' %

nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json.tpl

@@ -0,0 +1,7 @@
+{
+    "volumeAttachment": {
+        "volumeId": "%(volume_id)s",
+        "tag": "%(tag)s",
+        "delete_on_termination": true
+    }
+}

nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json.tpl

@@ -0,0 +1,22 @@
+{
+    "volumeAttachments": [
+        {
+            "device": "%(device)s",
+            "serverId": "%(uuid)s",
+            "tag": "%(tag)s",
+            "volumeId": "%(volume_id)s",
+            "delete_on_termination": true,
+            "attachment_id": "%(uuid)s",
+            "bdm_uuid": "%(uuid)s"
+        },
+        {
+            "device": "%(text)s",
+            "serverId": "%(uuid)s",
+            "tag": null,
+            "volumeId": "%(volume_id2)s",
+            "delete_on_termination": false,
+            "attachment_id": "%(uuid)s",
+            "bdm_uuid": "%(uuid)s"
+        }
+    ]
+}

nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json.tpl

@@ -0,0 +1,10 @@
+{
+    "volumeAttachment": {
+        "volumeId": "%(volume_id)s",
+        "id": "%(volume_id)s",
+        "serverId": "%(server_id)s",
+        "device": "%(device)s",
+        "tag": "%(tag)s",
+        "delete_on_termination": true
+    }
+}

nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-req.json.tpl

@@ -0,0 +1,5 @@
+{
+    "volumeAttachment": {
+        "volumeId": "%(new_volume_id)s"
+    }
+}

nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json.tpl

@@ -0,0 +1,11 @@
+{
+    "volumeAttachment": {
+        "device": "%(device)s",
+        "serverId": "%(uuid)s",
+        "tag": "%(tag)s",
+        "volumeId": "%(volume_id)s",
+        "delete_on_termination": true,
+        "attachment_id": "%(uuid)s",
+        "bdm_uuid": "%(uuid)s"
+    }
+}

nova/tests/functional/api_sample_tests/test_volume_attachments.py

@@ -159,3 +159,22 @@ class VolumeAttachmentsSampleV289(VolumeAttachmentsSampleV285):
     """
     microversion = '2.89'
     scenarios = [('v2_89', {'api_major_version': 'v2.1'})]
+
+
+class VolumeAttachmentsSampleV2101(VolumeAttachmentsSampleV289):
+    """Microversion 2.101 removes the body when creating an attachment"""
+    microversion = '2.101'
+    scenarios = [('v2_101', {'api_major_version': 'v2.1'})]
+
+    def test_attach_volume_to_server(self):
+        subs = {
+            'volume_id': self.OLD_VOLUME_ID,
+            'device': '/dev/sdb'
+        }
+        subs = self._get_vol_attachment_subs(subs)
+        response = self._do_post(
+            'servers/%s/os-volume_attachments' % self.server_id,
+            'attach-volume-to-server-req', subs)
+        self.assertEqual(202, response.status_code)
+        self.assertEqual("", response.text)
+        return subs

nova/tests/functional/integrated_helpers.py

@@ -585,14 +585,13 @@ class InstanceHelperMixin:
 
     def _attach_volume(self, server, volume_id):
         """attach a cinder volume to a server."""
-        attachment = self.api.post_server_volume(
+        self.api.post_server_volume(
             server['id'],
             {'volumeAttachment': {'volumeId': volume_id}}
         )
         self._wait_for_volume_attach(server['id'], volume_id)
         self.notifier.wait_for_versioned_notifications(
             'instance.volume_attach.end')
-        return attachment
 
     def _rebuild_server(self, server, image_uuid, expected_state='ACTIVE'):
         """Rebuild a server."""

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

@@ -266,14 +266,18 @@ class VolumeAttachTestsV21(test.NoDBTestCase):
         self.stub_out(
             'nova.compute.api.API.attach_volume',
             lambda self, context, instance, volume_id, device, tag=None,
-            supports_multiattach=False, delete_on_termination=False: None)
+            supports_multiattach=False, delete_on_termination=False,
+            needs_device_returned=True: None)
         body = {
             'volumeAttachment': {
                 'volumeId': FAKE_UUID_A,
                 'device': '/dev/fake'}}
         result = self.controller.create(self.req, FAKE_UUID, body=body)
-        self.assertEqual('00000000-aaaa-aaaa-aaaa-000000000000',
+        if api_version_request.is_supported(self.req, '2.101'):
-                         result['volumeAttachment']['id'])
+            self.assertEqual("", result.text)
+        else:
+            self.assertEqual('00000000-aaaa-aaaa-aaaa-000000000000',
+                             result['volumeAttachment']['id'])
 
     @mock.patch.object(compute_api.API, 'attach_volume',
                        side_effect=exception.VolumeTaggedAttachNotSupported())
@@ -332,9 +336,13 @@ class VolumeAttachTestsV21(test.NoDBTestCase):
                 'volumeId': FAKE_UUID_A,
                 'device': None}}
         result = self.controller.create(self.req, FAKE_UUID, body=body)
-        self.assertEqual('00000000-aaaa-aaaa-aaaa-000000000000',
+        if api_version_request.is_supported(self.req, '2.101'):
-                         result['volumeAttachment']['id'])
+            self.assertEqual("", result.text)
-        self.assertEqual('/dev/myfake', result['volumeAttachment']['device'])
+        else:
+            self.assertEqual('00000000-aaaa-aaaa-aaaa-000000000000',
+                             result['volumeAttachment']['id'])
+            self.assertEqual('/dev/myfake',
+                             result['volumeAttachment']['device'])
 
     @mock.patch.object(compute_api.API, 'attach_volume',
                        side_effect=exception.InstanceIsLocked(
@@ -348,11 +356,14 @@ class VolumeAttachTestsV21(test.NoDBTestCase):
                           self.req, FAKE_UUID, body=body)
         supports_multiattach = api_version_request.is_supported(
             self.req, '2.60')
+        needs_device_returned = not api_version_request.is_supported(
+            self.req, '2.101')
         mock_attach_volume.assert_called_once_with(
             self.req.environ['nova.context'],
             test.MatchType(objects.Instance), FAKE_UUID_A, '/dev/fake',
             supports_multiattach=supports_multiattach,
-            delete_on_termination=False, tag=None)
+            delete_on_termination=False, tag=None,
+            needs_device_returned=needs_device_returned)
 
     def test_attach_volume_bad_id(self):
         self.stub_out(
@@ -772,7 +783,7 @@ class VolumeAttachTestsV279(VolumeAttachTestsV275):
         mock_attach_volume.assert_called_once_with(
             req.environ['nova.context'], test.MatchType(objects.Instance),
             FAKE_UUID_A, None, tag=None, supports_multiattach=True,
-            delete_on_termination=False)
+            delete_on_termination=False, needs_device_returned=True)
 
     @mock.patch('nova.compute.api.API.attach_volume', return_value=None)
     def test_attach_volume_with_delete_on_termination_default_value(
@@ -784,11 +795,16 @@ class VolumeAttachTestsV279(VolumeAttachTestsV275):
         body = {'volumeAttachment': {'volumeId': FAKE_UUID_A}}
         req = self._get_req(body)
         result = self.controller.create(req, FAKE_UUID, body=body)
-        self.assertFalse(result['volumeAttachment']['delete_on_termination'])
+        needs_device_returned = not api_version_request.is_supported(
+            self.req, '2.101')
+        if needs_device_returned:
+            self.assertFalse(
+                result['volumeAttachment']['delete_on_termination'])
         mock_attach_volume.assert_called_once_with(
             req.environ['nova.context'], test.MatchType(objects.Instance),
             FAKE_UUID_A, None, tag=None, supports_multiattach=True,
-            delete_on_termination=False)
+            delete_on_termination=False,
+            needs_device_returned=needs_device_returned)
 
     def test_create_volume_attach_invalid_delete_on_termination_empty(self):
         body = {'volumeAttachment': {'volumeId': FAKE_UUID_A,
@@ -824,11 +840,16 @@ class VolumeAttachTestsV279(VolumeAttachTestsV275):
                                      'delete_on_termination': True}}
         req = self._get_req(body)
         result = self.controller.create(req, FAKE_UUID, body=body)
-        self.assertTrue(result['volumeAttachment']['delete_on_termination'])
+        needs_device_returned = not api_version_request.is_supported(
+            self.req, '2.101')
+        if needs_device_returned:
+            self.assertTrue(
+                result['volumeAttachment']['delete_on_termination'])
         mock_attach_volume.assert_called_once_with(
             req.environ['nova.context'], test.MatchType(objects.Instance),
             FAKE_UUID_A, None, tag=None, supports_multiattach=True,
-            delete_on_termination=True)
+            delete_on_termination=True,
+            needs_device_returned=needs_device_returned)
 
     def test_show_pre_v279(self):
         """Before microversion 2.79, show a detail of a volume attachment
@@ -1318,6 +1339,26 @@ class VolumeAttachTestsV289(VolumeAttachTestsV285):
         )
 
 
+class VolumeAttachTestsV2101(VolumeAttachTestsV289):
+    microversion = '2.101'
+
+    def setUp(self):
+        super().setUp()
+        self.controller = volume_attachments.VolumeAttachmentController()
+
+    @mock.patch('nova.compute.api.API.attach_volume', return_value=None)
+    def test_atttach_volume_v2101(self, mock_attach_volume):
+        body = {'volumeAttachment': {'volumeId': FAKE_UUID_A}}
+        req = self._get_req(body)
+        result = self.controller.create(req, FAKE_UUID, body=body)
+        self.assertEqual(202, result.status_int)
+        self.assertEqual("", result.text)
+        mock_attach_volume.assert_called_once_with(
+            req.environ['nova.context'], test.MatchType(objects.Instance),
+            FAKE_UUID_A, None, tag=None, supports_multiattach=True,
+            delete_on_termination=False, needs_device_returned=False)
+
+
 class SwapVolumeMultiattachTestCase(test.NoDBTestCase):
 
     @mock.patch('nova.api.openstack.common.get_instance')

nova/tests/unit/compute/test_api.py

@@ -412,33 +412,6 @@ class _ComputeAPIUnitTestMixIn(object):
         # 'get_pci_numa_policy_constraint' is only called in this method.
         mock_pci.assert_called_once()
 
-    @mock.patch('nova.objects.BlockDeviceMapping.save')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
-    def test_create_volume_bdm_call_reserve_dev_name(self, mock_reserve,
-                                                     mock_bdm_save):
-        bdm = objects.BlockDeviceMapping(
-                **fake_block_device.FakeDbBlockDeviceDict(
-                {
-                 'id': 1,
-                 'volume_id': 1,
-                 'source_type': 'volume',
-                 'destination_type': 'volume',
-                 'device_name': 'vda',
-                 'boot_index': 1,
-                 }))
-        mock_reserve.return_value = bdm
-        instance = self._create_instance_obj()
-        volume = {'id': '1', 'multiattach': False}
-        result = self.compute_api._create_volume_bdm(self.context,
-                                                     instance,
-                                                     'vda',
-                                                     volume,
-                                                     None,
-                                                     None)
-        self.assertTrue(mock_reserve.called)
-        self.assertEqual(result, bdm)
-        mock_bdm_save.assert_called_once_with()
-
     @mock.patch.object(objects.BlockDeviceMapping, 'create')
     def test_create_volume_bdm_local_creation(self, bdm_create):
         instance = self._create_instance_obj()
@@ -455,57 +428,42 @@ class _ComputeAPIUnitTestMixIn(object):
                  'disk_bus': None,
                  'device_type': None
                  }))
-        result = self.compute_api._create_volume_bdm(self.context,
+        result = self.compute_api._create_volume_bdm_locally(self.context,
                                                      instance,
                                                      '/dev/vda',
                                                      {'id': volume_id},
                                                      None,
-                                                     None,
+                                                     None)
-                                                     is_local_creation=True)
         self.assertEqual(result.instance_uuid, bdm.instance_uuid)
         self.assertIsNone(result.device_name)
         self.assertEqual(result.volume_id, bdm.volume_id)
         self.assertTrue(bdm_create.called)
 
-    @mock.patch.object(compute_api.API, '_record_action_start')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+    @mock.patch.object(nova.conductor.api.ComputeTaskAPI, 'attach_volume')
-    def test_attach_volume_new_flow(
+    def test_attach_volume_new_flow(self, mock_attach, mock_get_by_volume):
-        self, mock_attach, mock_get_by_volume, mock_reserve, mock_record
-    ):
         mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
             volume_id='fake-volume-id')
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
 
-        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
-        mock_reserve.return_value = fake_bdm
-
         mock_volume_api = mock.patch.object(self.compute_api, 'volume_api',
                                             mock.MagicMock(spec=cinder.API))
 
         with mock_volume_api as mock_v_api:
             mock_v_api.get.return_value = volume
-            mock_v_api.attachment_create.return_value = \
-                {'id': uuids.attachment_id}
             self.compute_api.attach_volume(
                 self.context, instance, volume['id'])
-            mock_v_api.check_availability_zone.assert_called_once_with(
-                self.context, volume, instance=instance)
-            mock_v_api.attachment_create.assert_called_once_with(self.context,
-                                                                 volume['id'],
-                                                                 instance.uuid)
             mock_attach.assert_called_once_with(self.context,
-                                                instance, fake_bdm)
+                instance, volume, None, None, None, tag=None,
+                supports_multiattach=False, delete_on_termination=False,
+                do_cast=True)
 
-    @mock.patch.object(compute_api.API, '_record_action_start')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+    @mock.patch.object(nova.conductor.api.ComputeTaskAPI, 'attach_volume')
     def test_tagged_volume_attach_new_flow(
-        self, mock_attach, mock_get_by_volume, mock_reserve, mock_record
+        self, mock_attach, mock_get_by_volume
     ):
         mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
             volume_id='fake-volume-id')
@@ -513,9 +471,6 @@ class _ComputeAPIUnitTestMixIn(object):
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
 
-        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
-        mock_reserve.return_value = fake_bdm
-
         mock_volume_api = mock.patch.object(self.compute_api, 'volume_api',
                                             mock.MagicMock(spec=cinder.API))
 
@@ -525,35 +480,22 @@ class _ComputeAPIUnitTestMixIn(object):
                 {'id': uuids.attachment_id}
             self.compute_api.attach_volume(
                 self.context, instance, volume['id'], tag='foo')
-            mock_reserve.assert_called_once_with(self.context, instance, None,
-                                                 volume['id'],
-                                                 device_type=None,
-                                                 disk_bus=None, tag='foo',
-                                                 multiattach=False)
-            mock_v_api.check_availability_zone.assert_called_once_with(
-                self.context, volume, instance=instance)
-            mock_v_api.attachment_create.assert_called_once_with(
-                self.context, volume['id'], instance.uuid)
             mock_attach.assert_called_once_with(self.context,
-                                                instance, fake_bdm)
+                instance, volume, None, None, None, tag='foo',
+                supports_multiattach=False, delete_on_termination=False,
+                do_cast=True)
 
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    @mock.patch.object(nova.conductor.api.ComputeTaskAPI, 'attach_volume')
-    @mock.patch.object(
-        objects.BlockDeviceMapping, 'get_by_volume_and_instance')
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
-    def test_attach_volume_reserve_bdm_timeout(
+    def test_attach_volume_timeout(self, mock_get_by_volume, mock_attach):
-            self, mock_get_by_volume, mock_get_by_volume_and_instance,
-            mock_reserve):
         mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
             volume_id='fake-volume-id')
 
-        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
-        mock_get_by_volume_and_instance.return_value = fake_bdm
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
 
-        mock_reserve.side_effect = oslo_exceptions.MessagingTimeout()
+        mock_attach.side_effect = oslo_exceptions.MessagingTimeout()
 
         mock_volume_api = mock.patch.object(self.compute_api, 'volume_api',
                                             mock.MagicMock(spec=cinder.API))
@@ -563,40 +505,6 @@ class _ComputeAPIUnitTestMixIn(object):
             self.assertRaises(oslo_exceptions.MessagingTimeout,
                                 self.compute_api.attach_volume,
                                 self.context, instance, volume['id'])
-            mock_get_by_volume_and_instance.assert_called_once_with(
-                self.context, volume['id'], instance.uuid)
-            fake_bdm.destroy.assert_called_once_with()
-
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
-    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
-    @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-    def test_attach_volume_attachment_create_fails(
-        self, mock_attach, mock_get_by_volume, mock_reserve
-    ):
-        mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
-            volume_id='fake-volume-id')
-        instance = self._create_instance_obj()
-        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
-                                         None, None, None, None, None)
-
-        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
-        mock_reserve.return_value = fake_bdm
-
-        mock_volume_api = mock.patch.object(self.compute_api, 'volume_api',
-                                            mock.MagicMock(spec=cinder.API))
-
-        with mock_volume_api as mock_v_api:
-            mock_v_api.get.return_value = volume
-            mock_v_api.attachment_create.side_effect = test.TestingException()
-            self.assertRaises(test.TestingException,
-                              self.compute_api.attach_volume,
-                              self.context, instance, volume['id'])
-            mock_v_api.check_availability_zone.assert_called_once_with(
-                self.context, volume, instance=instance)
-            mock_v_api.attachment_create.assert_called_once_with(
-                self.context, volume['id'], instance.uuid)
-            self.assertEqual(0, mock_attach.call_count)
-            fake_bdm.destroy.assert_called_once_with()
 
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
     def test_attach_volume_bdm_exists(self, mock_by_volume):
@@ -7949,19 +7857,6 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
         mock_record.assert_called_once_with(
             self.context, instance, instance_actions.DETACH_INTERFACE)
 
-    def test_check_attach_and_reserve_volume_multiattach_old_version(self):
-        """Tests that _check_attach_and_reserve_volume fails if trying
-        to use a multiattach volume with a microversion<2.60.
-        """
-        instance = self._create_instance_obj()
-        volume = {'id': uuids.volumeid, 'multiattach': True}
-        bdm = objects.BlockDeviceMapping(volume_id=uuids.volumeid,
-                                         instance_uuid=instance.uuid)
-        self.assertRaises(exception.MultiattachNotSupportedOldMicroversion,
-                          self.compute_api._check_attach_and_reserve_volume,
-                          self.context, volume, instance, bdm,
-                          supports_multiattach=False)
-
     @mock.patch('nova.volume.cinder.API.get',
                 return_value={'id': uuids.volumeid, 'multiattach': True})
     def test_attach_volume_shelved_offloaded_fails(

nova/tests/unit/compute/test_compute.py

@@ -2208,8 +2208,8 @@ class ComputeTestCase(BaseTestCase,
 
         bdms = []
 
-        def fake_rpc_reserve_block_device_name(self, context, instance, device,
+        def fake_rpc_attach_volume(self, context, instance, volume, device,
-                                               volume_id, **kwargs):
+                                   disk_bus, device_type, **kwargs):
             bdm = objects.BlockDeviceMapping(
                         **{'context': context,
                            'source_type': 'volume',
@@ -2229,9 +2229,9 @@ class ComputeTestCase(BaseTestCase,
         self.stub_out('nova.volume.cinder.API.terminate_connection',
                        fake_terminate_connection)
         self.stub_out('nova.volume.cinder.API.detach', fake_detach)
-        self.stub_out('nova.compute.rpcapi.ComputeAPI.'
+        self.stub_out('nova.conductor.rpcapi.ComputeTaskAPI.'
-                       'reserve_block_device_name',
+                       'attach_volume',
-                       fake_rpc_reserve_block_device_name)
+                       fake_rpc_attach_volume)
 
         self.compute_api.attach_volume(self.context, instance, 1,
                                        '/dev/vdc')
@@ -11783,13 +11783,6 @@ class ComputeAPITestCase(BaseTestCase):
             {'port_id': port_id, 'pci_req': pci_req})
 
     def test_attach_volume_new_flow(self):
-        fake_bdm = fake_block_device.FakeDbBlockDeviceDict(
-                {'source_type': 'volume', 'destination_type': 'volume',
-                 'volume_id': uuids.volume_id, 'device_name': '/dev/vdb'})
-        bdm = block_device_obj.BlockDeviceMapping()._from_db_object(
-                self.context,
-                block_device_obj.BlockDeviceMapping(),
-                fake_bdm)
         instance = self._create_fake_instance_obj()
         instance.id = 42
         fake_volume = {'id': uuids.volume, 'multiattach': False}
@@ -11798,46 +11791,23 @@ class ComputeAPITestCase(BaseTestCase):
             mock.patch.object(cinder.API, 'get', return_value=fake_volume),
             mock.patch.object(objects.BlockDeviceMapping,
                               'get_by_volume_and_instance'),
-            mock.patch.object(cinder.API, 'check_availability_zone'),
+            mock.patch.object(self.compute_api.compute_task_api,
-            mock.patch.object(cinder.API, 'attachment_create',
+                              'attach_volume')
-                              return_value={'id': uuids.attachment_id}),
+        ) as (mock_get, mock_no_bdm, mock_attach):
-            mock.patch.object(objects.BlockDeviceMapping, 'save'),
-            mock.patch.object(compute_rpcapi.ComputeAPI,
-                'reserve_block_device_name', return_value=bdm),
-            mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-        ) as (mock_get, mock_no_bdm,
-              mock_check_availability_zone, mock_attachment_create,
-              mock_bdm_save, mock_reserve_bdm, mock_attach):
             mock_no_bdm.side_effect = exception.VolumeBDMNotFound(
                     volume_id=uuids.volume)
             self.compute_api.attach_volume(
                     self.context, instance, uuids.volume,
                     '/dev/vdb', 'ide', 'cdrom')
 
-            mock_reserve_bdm.assert_called_once_with(
-                    self.context, instance, '/dev/vdb', uuids.volume,
-                    disk_bus='ide', device_type='cdrom', tag=None,
-                    multiattach=False)
             self.assertEqual(mock_get.call_args,
                              mock.call(self.context, uuids.volume))
-            self.assertEqual(mock_check_availability_zone.call_args,
+            mock_attach.assert_called_once_with(self.context, instance,
-                             mock.call(
+                fake_volume, '/dev/vdb', 'ide', 'cdrom', tag=None,
-                                 self.context, fake_volume, instance=instance))
+                supports_multiattach=False, delete_on_termination=False,
-            mock_attachment_create.assert_called_once_with(self.context,
+                do_cast=True)
-                                                           uuids.volume,
-                                                           instance.uuid)
-            a, kw = mock_attach.call_args
-            self.assertEqual(a[2].device_name, '/dev/vdb')
-            self.assertEqual(a[2].volume_id, uuids.volume_id)
 
     def test_attach_volume_no_device_new_flow(self):
-        fake_bdm = fake_block_device.FakeDbBlockDeviceDict(
-                {'source_type': 'volume', 'destination_type': 'volume',
-                 'device_name': '/dev/vdb', 'volume_id': uuids.volume_id})
-        bdm = block_device_obj.BlockDeviceMapping()._from_db_object(
-                self.context,
-                block_device_obj.BlockDeviceMapping(),
-                fake_bdm)
         instance = self._create_fake_instance_obj()
         instance.id = 42
         fake_volume = {'id': uuids.volume, 'multiattach': False}
@@ -11847,16 +11817,9 @@ class ComputeAPITestCase(BaseTestCase):
             mock.patch.object(objects.BlockDeviceMapping,
                               'get_by_volume_and_instance',
                               side_effect=exception.VolumeBDMNotFound),
-            mock.patch.object(cinder.API, 'check_availability_zone'),
+            mock.patch.object(self.compute_api.compute_task_api,
-            mock.patch.object(cinder.API, 'attachment_create',
+                              'attach_volume')
-                              return_value={'id': uuids.attachment_id}),
+        ) as (mock_get, mock_no_bdm, mock_attach):
-            mock.patch.object(objects.BlockDeviceMapping, 'save'),
-            mock.patch.object(compute_rpcapi.ComputeAPI,
-                'reserve_block_device_name', return_value=bdm),
-            mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-        ) as (mock_get, mock_no_bdm,
-              mock_check_availability_zone, mock_attachment_create,
-              mock_bdm_save, mock_reserve_bdm, mock_attach):
             mock_no_bdm.side_effect = exception.VolumeBDMNotFound(
                                           volume_id=uuids.volume)
 
@@ -11864,20 +11827,10 @@ class ComputeAPITestCase(BaseTestCase):
                     self.context, instance, uuids.volume,
                     device=None)
 
-            mock_reserve_bdm.assert_called_once_with(
-                    self.context, instance, None, uuids.volume,
-                    disk_bus=None, device_type=None, tag=None,
-                    multiattach=False)
             self.assertEqual(mock_get.call_args,
                              mock.call(self.context, uuids.volume))
-            self.assertEqual(mock_check_availability_zone.call_args,
-                             mock.call(
-                                 self.context, fake_volume, instance=instance))
-            mock_attachment_create.assert_called_once_with(self.context,
-                                                           uuids.volume,
-                                                           instance.uuid)
             a, kw = mock_attach.call_args
-            self.assertEqual(a[2].volume_id, uuids.volume_id)
+            self.assertEqual(a[2], fake_volume)
 
     def test_attach_volume_shelved_offloaded(self):
         instance = self._create_fake_instance_obj()
@@ -11891,10 +11844,10 @@ class ComputeAPITestCase(BaseTestCase):
                  'instance_uuid': instance.uuid}))
         with test.nested(
              mock.patch.object(compute.API,
-                               '_create_volume_bdm',
+                               '_create_volume_bdm_locally',
                                return_value=fake_bdm),
-             mock.patch.object(compute.API,
+             mock.patch.object(compute_utils,
-                               '_check_attach_and_reserve_volume'),
+                               'check_attach_and_reserve_volume'),
              mock.patch.object(cinder.API, 'attach'),
              mock.patch.object(compute_utils, 'EventReporter')
         ) as (mock_bdm_create, mock_attach_and_reserve, mock_attach,
@@ -11904,9 +11857,7 @@ class ComputeAPITestCase(BaseTestCase):
                     self.context, instance, volume,
                     '/dev/vdb', 'ide', 'cdrom', False)
             mock_attach_and_reserve.assert_called_once_with(self.context,
-                                                            volume,
+                self.compute_api.volume_api, volume, instance, fake_bdm)
-                                                            instance,
-                                                            fake_bdm)
             mock_attach.assert_called_once_with(self.context,
                                                 uuids.volume,
                                                 instance.uuid,
@@ -11932,10 +11883,10 @@ class ComputeAPITestCase(BaseTestCase):
 
         with test.nested(
              mock.patch.object(compute.API,
-                               '_create_volume_bdm',
+                               '_create_volume_bdm_locally',
                                return_value=fake_bdm),
-             mock.patch.object(compute.API,
+             mock.patch.object(compute_utils,
-                               '_check_attach_and_reserve_volume',
+                               'check_attach_and_reserve_volume',
                                side_effect=fake_check_attach_and_reserve),
              mock.patch.object(cinder.API, 'attachment_complete')
         ) as (mock_bdm_create, mock_attach_and_reserve, mock_attach_complete):
@@ -11944,9 +11895,7 @@ class ComputeAPITestCase(BaseTestCase):
                     self.context, instance, volume,
                     '/dev/vdb', 'ide', 'cdrom', False)
             mock_attach_and_reserve.assert_called_once_with(self.context,
-                                                            volume,
+                self.compute_api.volume_api, volume, instance, fake_bdm)
-                                                            instance,
-                                                            fake_bdm)
             mock_attach_complete.assert_called_once_with(
                 self.context, uuids.attachment_id)
 

nova/tests/unit/compute/test_utils.py

@@ -53,6 +53,7 @@ from nova.tests.unit import fake_instance
 from nova.tests.unit import fake_network
 from nova.tests.unit import fake_server_actions
 from nova.tests.unit.objects import test_flavor
+from nova.volume import cinder
 
 
 FAKE_IMAGE_REF = uuids.image_ref
@@ -1779,3 +1780,23 @@ class AcceleratorRequestTestCase(test.NoDBTestCase):
         compute_utils.delete_arqs_if_needed(self.context, instance, arq_uuids)
         mock_del_inst.assert_called_once_with(instance.uuid)
         mock_del_uuid.assert_called_once_with(arq_uuids)
+
+
+class CheckAttachAndReserverVolumeTestCase(test.NoDBTestCase):
+    def setUp(self):
+        super(CheckAttachAndReserverVolumeTestCase, self).setUp()
+        self.context = context.get_admin_context()
+
+    def test_check_attach_and_reserve_volume_multiattach_old_version(self):
+        """Tests that _check_attach_and_reserve_volume fails if trying
+        to use a multiattach volume with a microversion<2.60.
+        """
+        instance = fake_instance.fake_instance_obj(self.context)
+        volume = {'id': uuids.volumeid, 'multiattach': True}
+        bdm = objects.BlockDeviceMapping(volume_id=uuids.volumeid,
+                                         instance_uuid=instance.uuid)
+        mock_volume_api = mock.MagicMock(spec=cinder.API)
+        self.assertRaises(exception.MultiattachNotSupportedOldMicroversion,
+                          compute_utils.check_attach_and_reserve_volume,
+                          self.context, mock_volume_api, volume, instance, bdm,
+                          supports_multiattach=False)

nova/tests/unit/conductor/test_conductor.py

@@ -61,10 +61,12 @@ from nova.tests import fixtures
 from nova.tests.fixtures import cyborg as cyborg_fixture
 from nova.tests.unit.api.openstack import fakes
 from nova.tests.unit.compute import test_compute
+from nova.tests.unit import fake_block_device
 from nova.tests.unit import fake_build_request
 from nova.tests.unit import fake_instance
 from nova.tests.unit import fake_request_spec
 from nova.tests.unit import fake_server_actions
+from nova.tests.unit import fake_volume
 from nova.tests.unit import utils as test_utils
 from nova import utils
 from nova.volume import cinder
@@ -4540,6 +4542,220 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
             disk_over_commit=None, request_spec=reqspec)
         mock_execute.assert_called_once_with()
 
+    @mock.patch('nova.objects.BlockDeviceMapping.save')
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    def test_create_volume_bdm_call_reserve_dev_name(self, mock_reserve,
+                                                     mock_bdm_save):
+        bdm = objects.BlockDeviceMapping(
+                **fake_block_device.FakeDbBlockDeviceDict(
+                {
+                 'id': 1,
+                 'volume_id': 1,
+                 'source_type': 'volume',
+                 'destination_type': 'volume',
+                 'device_name': 'vda',
+                 'boot_index': 1,
+                 }))
+        mock_reserve.return_value = bdm
+        instance = fake_instance.fake_instance_obj(self.context)
+        volume = {'id': '1', 'multiattach': False}
+        result = self.conductor._create_volume_bdm(self.context,
+                                                   instance,
+                                                   'vda',
+                                                   volume,
+                                                   None,
+                                                   None)
+        self.assertTrue(mock_reserve.called)
+        self.assertEqual(result, bdm)
+        mock_bdm_save.assert_called_once_with()
+
+    def test_attach_volume_no_device_new_flow(self):
+        fake_bdm = fake_block_device.FakeDbBlockDeviceDict(
+                {'source_type': 'volume', 'destination_type': 'volume',
+                 'device_name': '/dev/vdb', 'volume_id': uuids.volume_id})
+        bdm = block_device_obj.BlockDeviceMapping()._from_db_object(
+                self.context,
+                block_device_obj.BlockDeviceMapping(),
+                fake_bdm)
+        instance = self._create_fake_instance_obj()
+        instance.id = 42
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         uuids.volume, None, None, None, None)
+
+        with test.nested(
+            mock.patch.object(objects.BlockDeviceMapping,
+                              'get_by_volume_and_instance',
+                              side_effect=exc.VolumeBDMNotFound),
+            mock.patch.object(cinder.API, 'check_availability_zone'),
+            mock.patch.object(cinder.API, 'attachment_create',
+                              return_value={'id': uuids.attachment_id}),
+            mock.patch.object(objects.BlockDeviceMapping, 'save'),
+            mock.patch.object(compute_rpcapi.ComputeAPI,
+                'reserve_block_device_name', return_value=bdm),
+            mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+        ) as (mock_no_bdm,
+              mock_check_availability_zone, mock_attachment_create,
+              mock_bdm_save, mock_reserve_bdm, mock_attach):
+            mock_no_bdm.side_effect = exc.VolumeBDMNotFound(
+                                          volume_id=uuids.volume)
+
+            self.conductor.attach_volume(
+                    self.context, instance, volume,
+                    device=None, disk_bus=None, device_type=None)
+
+            mock_reserve_bdm.assert_called_once_with(
+                    self.context, instance, None, uuids.volume,
+                    disk_bus=None, device_type=None, tag=None,
+                    multiattach=False)
+            self.assertEqual(mock_check_availability_zone.call_args,
+                             mock.call(
+                                 self.context, volume, instance=instance))
+            mock_attachment_create.assert_called_once_with(self.context,
+                                                           uuids.volume,
+                                                           instance.uuid)
+            a, kw = mock_attach.call_args
+            self.assertEqual(a[2].volume_id, uuids.volume_id)
+
+    def test_attach_volume_new_flow(self):
+        fake_bdm = fake_block_device.FakeDbBlockDeviceDict(
+                {'source_type': 'volume', 'destination_type': 'volume',
+                 'volume_id': uuids.volume_id, 'device_name': '/dev/vdb'})
+        bdm = block_device_obj.BlockDeviceMapping()._from_db_object(
+                self.context,
+                block_device_obj.BlockDeviceMapping(),
+                fake_bdm)
+        instance = self._create_fake_instance_obj()
+        instance.id = 42
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         uuids.volume, None, None, None, None)
+
+        with test.nested(
+            mock.patch.object(objects.BlockDeviceMapping,
+                              'get_by_volume_and_instance'),
+            mock.patch.object(cinder.API, 'check_availability_zone'),
+            mock.patch.object(cinder.API, 'attachment_create',
+                              return_value={'id': uuids.attachment_id}),
+            mock.patch.object(objects.BlockDeviceMapping, 'save'),
+            mock.patch.object(compute_rpcapi.ComputeAPI,
+                'reserve_block_device_name', return_value=bdm),
+            mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+        ) as (mock_no_bdm,
+              mock_check_availability_zone, mock_attachment_create,
+              mock_bdm_save, mock_reserve_bdm, mock_attach):
+            mock_no_bdm.side_effect = exc.VolumeBDMNotFound(
+                    volume_id=uuids.volume)
+            self.conductor.attach_volume(
+                    self.context, instance, volume,
+                    '/dev/vdb', 'ide', 'cdrom')
+
+            mock_reserve_bdm.assert_called_once_with(
+                    self.context, instance, '/dev/vdb', uuids.volume,
+                    disk_bus='ide', device_type='cdrom', tag=None,
+                    multiattach=False)
+            self.assertEqual(mock_check_availability_zone.call_args,
+                             mock.call(
+                                 self.context, volume, instance=instance))
+            mock_attachment_create.assert_called_once_with(self.context,
+                                                           uuids.volume,
+                                                           instance.uuid)
+            a, kw = mock_attach.call_args
+            self.assertEqual(a[2].device_name, '/dev/vdb')
+            self.assertEqual(a[2].volume_id, uuids.volume_id)
+
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+    def test_attach_volume_attachment_create_fails(
+        self, mock_attach, mock_get_by_volume, mock_reserve
+    ):
+        mock_get_by_volume.side_effect = exc.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+        instance = fake_instance.fake_instance_obj(self.context,
+                                                   vm_state=vm_states.ACTIVE)
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         None, None, None, None, None)
+
+        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
+        mock_reserve.return_value = fake_bdm
+
+        mock_volume_api = mock.patch.object(self.conductor, 'volume_api',
+                                            mock.MagicMock(spec=cinder.API))
+
+        with mock_volume_api as mock_v_api:
+            mock_v_api.attachment_create.side_effect = test.TestingException()
+            self.assertRaises(test.TestingException,
+                              self.conductor.attach_volume,
+                              self.context, instance, volume, None, None, None)
+            mock_v_api.check_availability_zone.assert_called_once_with(
+                self.context, volume, instance=instance)
+            mock_v_api.attachment_create.assert_called_once_with(
+                self.context, volume['id'], instance.uuid)
+            self.assertEqual(0, mock_attach.call_count)
+            fake_bdm.destroy.assert_called_once_with()
+
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    @mock.patch.object(
+        objects.BlockDeviceMapping, 'get_by_volume_and_instance')
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
+    def test_attach_volume_reserve_bdm_timeout(
+            self, mock_get_by_volume, mock_get_by_volume_and_instance,
+            mock_reserve):
+        mock_get_by_volume.side_effect = exc.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+
+        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
+        mock_get_by_volume_and_instance.return_value = fake_bdm
+        instance = fake_instance.fake_instance_obj(self.context,
+                                                   vm_state=vm_states.ACTIVE)
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         None, None, None, None, None)
+
+        mock_reserve.side_effect = messaging.exceptions.MessagingTimeout()
+
+        self.assertRaises(messaging.exceptions.MessagingTimeout,
+                            self.conductor.attach_volume,
+                            self.context, instance, volume,
+                            None, None, None)
+        mock_get_by_volume_and_instance.assert_called_once_with(
+            self.context, volume['id'], instance.uuid)
+        fake_bdm.destroy.assert_called_once_with()
+
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
+    def test_tagged_volume_attach_new_flow(
+        self, mock_attach, mock_get_by_volume, mock_reserve
+    ):
+        mock_get_by_volume.side_effect = exc.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+        instance = fake_instance.fake_instance_obj(self.context,
+                                                   vm_state=vm_states.ACTIVE)
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         None, None, None, None, None)
+
+        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
+        mock_reserve.return_value = fake_bdm
+
+        mock_volume_api = mock.patch.object(self.conductor, 'volume_api',
+                                            mock.MagicMock(spec=cinder.API))
+
+        with mock_volume_api as mock_v_api:
+            mock_v_api.attachment_create.return_value = \
+                {'id': uuids.attachment_id}
+            self.conductor.attach_volume(
+                self.context, instance, volume, None, None, None, tag='foo')
+            mock_reserve.assert_called_once_with(self.context, instance, None,
+                                                 volume['id'],
+                                                 device_type=None,
+                                                 disk_bus=None, tag='foo',
+                                                 multiattach=False)
+            mock_v_api.check_availability_zone.assert_called_once_with(
+                self.context, volume, instance=instance)
+            mock_v_api.attachment_create.assert_called_once_with(
+                self.context, volume['id'], instance.uuid)
+            mock_attach.assert_called_once_with(self.context,
+                                                instance, fake_bdm)
+
 
 class ConductorTaskRPCAPITestCase(_BaseTaskTestCase,
         test_compute.BaseTestCase):

releasenotes/notes/bp-async-volume-attachments-b2b9cd8a4cc54b30.yaml

@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    A new microversion has been added to allow fully-asynchronous attachment of
+    volumes to a server. Previously, calling ``POST
+    /servers/{server_id}/os-volume_attachments`` to attach a volume would
+    return HTTP 200 containing a ``volumeAttachment`` object, returning a
+    device name. With API version 2.101 this changes to HTTP 202. The reason
+    is, that the device name could not be assured by any of the current drivers
+    and reserving the device name could block the API thread for some time.