diff --git a/api-ref/source/os-volume-attachments.inc b/api-ref/source/os-volume-attachments.inc index 359d4d16e8..659ffea589 100644 --- a/api-ref/source/os-volume-attachments.inc +++ b/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 diff --git a/doc/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json b/doc/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json new file mode 100644 index 0000000000..b4429e12e9 --- /dev/null +++ b/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 + } +} \ No newline at end of file diff --git a/doc/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json b/doc/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json new file mode 100644 index 0000000000..9935969fbf --- /dev/null +++ b/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" + } + ] +} diff --git a/doc/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json b/doc/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json new file mode 100644 index 0000000000..a2e17f2b6f --- /dev/null +++ b/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 + } +} \ No newline at end of file diff --git a/doc/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json b/doc/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json new file mode 100644 index 0000000000..eda615f996 --- /dev/null +++ b/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" + } +} diff --git a/doc/api_samples/versions/v21-version-get-resp.json b/doc/api_samples/versions/v21-version-get-resp.json index 03bed55570..77e8a6b2b6 100644 --- a/doc/api_samples/versions/v21-version-get-resp.json +++ b/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" } -} \ No newline at end of file +} diff --git a/doc/api_samples/versions/versions-get-resp.json b/doc/api_samples/versions/versions-get-resp.json index 4ca27da2e6..19d85f22f9 100644 --- a/doc/api_samples/versions/versions-get-resp.json +++ b/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" } diff --git a/nova/api/openstack/api_version_request.py b/nova/api/openstack/api_version_request.py index e77bb27507..827c5c1253 100644 --- a/nova/api/openstack/api_version_request.py +++ b/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 diff --git a/nova/api/openstack/compute/rest_api_version_history.rst b/nova/api/openstack/compute/rest_api_version_history.rst index 0747b3ce3a..da988200a9 100644 --- a/nova/api/openstack/compute/rest_api_version_history.rst +++ b/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. diff --git a/nova/api/openstack/compute/schemas/volume_attachments.py b/nova/api/openstack/compute/schemas/volume_attachments.py index 60f2f8b51c..ad83951b17 100644 --- a/nova/api/openstack/compute/schemas/volume_attachments.py +++ b/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'} diff --git a/nova/api/openstack/compute/volume_attachments.py b/nova/api/openstack/compute/volume_attachments.py index e5034e25fb..ea07e67f09 100644 --- a/nova/api/openstack/compute/volume_attachments.py +++ b/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 - # 'bdm_uuid' and 'attachment_id' and stop returning 'id' + # NOTE(stephenfin): We forgot to apply 2.89 here, but starting with + # 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): diff --git a/nova/compute/api.py b/nova/compute/api.py index b846ef7abc..0c87b7c05f 100644 --- a/nova/compute/api.py +++ b/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( - context, volume, instance, bdm, supports_multiattach, - validate_az=False) + compute_utils.check_attach_and_reserve_volume( + context, self.volume_api, volume, instance, bdm, + 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, - disk_bus, device_type, is_local_creation=False, - tag=None, delete_on_termination=False): + def _create_volume_bdm_locally(self, context, instance, device, volume, + disk_bus, device_type, 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 - # name as we do not have a way to check that the name specified is - # a valid one. - # We leave the setting of that value when the actual attach - # happens on the compute manager - # NOTE(artom) Local attach (to a shelved-offload instance) cannot - # support device tagging because we have no way to call the compute - # manager to check that it supports device tagging. In fact, we - # don't even know which computer manager the instance will - # eventually end up on when it's unshelved. - volume_bdm = objects.BlockDeviceMapping( - context=context, - source_type='volume', destination_type='volume', - instance_uuid=instance.uuid, boot_index=None, - volume_id=volume_id, - device_name=None, guest_format=None, - disk_bus=disk_bus, device_type=device_type, - delete_on_termination=delete_on_termination) - 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() + # 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 + # a valid one. + # We leave the setting of that value when the actual attach + # happens on the compute manager + # NOTE(artom) Local attach (to a shelved-offload instance) cannot + # support device tagging because we have no way to call the compute + # manager to check that it supports device tagging. In fact, we + # don't even know which computer manager the instance will + # eventually end up on when it's unshelved. + volume_bdm = objects.BlockDeviceMapping( + context=context, + source_type='volume', destination_type='volume', + instance_uuid=instance.uuid, boot_index=None, + volume_id=volume_id, + device_name=None, guest_format=None, + disk_bus=disk_bus, device_type=device_type, + delete_on_termination=delete_on_termination) + volume_bdm.create() 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, - volume_bdm) + compute_utils.check_attach_and_reserve_volume(context, + 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, - disk_bus, device_type, tag=tag, - supports_multiattach=supports_multiattach, - delete_on_termination=delete_on_termination) + return self.compute_task_api.attach_volume(context, instance, + volume, device, disk_bus, device_type, tag=tag, + supports_multiattach=supports_multiattach, + 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. diff --git a/nova/compute/utils.py b/nova/compute/utils.py index b8117cdb7b..a1f20be053 100644 --- a/nova/compute/utils.py +++ b/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() diff --git a/nova/conductor/api.py b/nova/conductor/api.py index 843c8ce3a3..2a6d6866ac 100644 --- a/nova/conductor/api.py +++ b/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) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index 5e3a90c6be..b7e834ef59 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -260,7 +260,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() @@ -2240,3 +2240,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 diff --git a/nova/conductor/rpcapi.py b/nova/conductor/rpcapi.py index a5f0cf0094..03017b277e 100644 --- a/nova/conductor/rpcapi.py +++ b/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) diff --git a/nova/tests/functional/api/client.py b/nova/tests/functional/api/client.py index 905176fe04..e12917585e 100644 --- a/nova/tests/functional/api/client.py +++ b/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' % diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/attach-volume-to-server-req.json.tpl new file mode 100644 index 0000000000..92c13861bb --- /dev/null +++ b/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 + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/list-volume-attachments-resp.json.tpl new file mode 100644 index 0000000000..67bfd78237 --- /dev/null +++ b/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" + } + ] +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-attachment-delete-flag-req.json.tpl new file mode 100644 index 0000000000..2aba36133c --- /dev/null +++ b/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 + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/update-volume-req.json.tpl new file mode 100644 index 0000000000..85c1244b14 --- /dev/null +++ b/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" + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volume_attachments/v2.101/volume-attachment-detail-resp.json.tpl new file mode 100644 index 0000000000..190bead8be --- /dev/null +++ b/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" + } +} diff --git a/nova/tests/functional/api_sample_tests/test_volume_attachments.py b/nova/tests/functional/api_sample_tests/test_volume_attachments.py index d2edecfcef..0649395554 100644 --- a/nova/tests/functional/api_sample_tests/test_volume_attachments.py +++ b/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 diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index a00e713b4e..0adbe8b768 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/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.""" diff --git a/nova/tests/unit/api/openstack/compute/test_volume_attachments.py b/nova/tests/unit/api/openstack/compute/test_volume_attachments.py index 9d189be5f4..a433557f90 100644 --- a/nova/tests/unit/api/openstack/compute/test_volume_attachments.py +++ b/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', - result['volumeAttachment']['id']) + if api_version_request.is_supported(self.req, '2.101'): + 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', - result['volumeAttachment']['id']) - self.assertEqual('/dev/myfake', result['volumeAttachment']['device']) + if api_version_request.is_supported(self.req, '2.101'): + self.assertEqual("", result.text) + 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') diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index f2fed17b24..72d7a99e6d 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/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, - is_local_creation=True) + None) 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') - def test_attach_volume_new_flow( - self, mock_attach, mock_get_by_volume, mock_reserve, mock_record - ): + @mock.patch.object(nova.conductor.api.ComputeTaskAPI, 'attach_volume') + def test_attach_volume_new_flow(self, mock_attach, mock_get_by_volume): 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( - objects.BlockDeviceMapping, 'get_by_volume_and_instance') + @mock.patch.object(nova.conductor.api.ComputeTaskAPI, 'attach_volume') @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): + def test_attach_volume_timeout(self, mock_get_by_volume, mock_attach): 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( diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 9afb81ec77..18c4f33d3a 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/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, - volume_id, **kwargs): + def fake_rpc_attach_volume(self, context, instance, volume, device, + 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.' - 'reserve_block_device_name', - fake_rpc_reserve_block_device_name) + self.stub_out('nova.conductor.rpcapi.ComputeTaskAPI.' + 'attach_volume', + fake_rpc_attach_volume) self.compute_api.attach_volume(self.context, instance, 1, '/dev/vdc') @@ -11782,13 +11782,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} @@ -11797,46 +11790,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(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_get, mock_no_bdm, - mock_check_availability_zone, mock_attachment_create, - mock_bdm_save, mock_reserve_bdm, mock_attach): + mock.patch.object(self.compute_api.compute_task_api, + 'attach_volume') + ) as (mock_get, mock_no_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.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].device_name, '/dev/vdb') - self.assertEqual(a[2].volume_id, uuids.volume_id) + mock_attach.assert_called_once_with(self.context, instance, + fake_volume, '/dev/vdb', 'ide', 'cdrom', tag=None, + supports_multiattach=False, delete_on_termination=False, + do_cast=True) 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} @@ -11846,16 +11816,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(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_get, mock_no_bdm, - mock_check_availability_zone, mock_attachment_create, - mock_bdm_save, mock_reserve_bdm, mock_attach): + mock.patch.object(self.compute_api.compute_task_api, + 'attach_volume') + ) as (mock_get, mock_no_bdm, mock_attach): mock_no_bdm.side_effect = exception.VolumeBDMNotFound( volume_id=uuids.volume) @@ -11863,20 +11826,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() @@ -11890,10 +11843,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, - '_check_attach_and_reserve_volume'), + mock.patch.object(compute_utils, + '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, @@ -11903,9 +11856,7 @@ class ComputeAPITestCase(BaseTestCase): self.context, instance, volume, '/dev/vdb', 'ide', 'cdrom', False) mock_attach_and_reserve.assert_called_once_with(self.context, - volume, - instance, - fake_bdm) + self.compute_api.volume_api, volume, instance, fake_bdm) mock_attach.assert_called_once_with(self.context, uuids.volume, instance.uuid, @@ -11931,10 +11882,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, - '_check_attach_and_reserve_volume', + mock.patch.object(compute_utils, + '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): @@ -11943,9 +11894,7 @@ class ComputeAPITestCase(BaseTestCase): self.context, instance, volume, '/dev/vdb', 'ide', 'cdrom', False) mock_attach_and_reserve.assert_called_once_with(self.context, - volume, - instance, - fake_bdm) + self.compute_api.volume_api, volume, instance, fake_bdm) mock_attach_complete.assert_called_once_with( self.context, uuids.attachment_id) diff --git a/nova/tests/unit/compute/test_utils.py b/nova/tests/unit/compute/test_utils.py index 3f1ab11451..6e10399979 100644 --- a/nova/tests/unit/compute/test_utils.py +++ b/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) diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 16f624ee4a..0653d69e5e 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/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 @@ -4564,6 +4566,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): diff --git a/releasenotes/notes/bp-async-volume-attachments-b2b9cd8a4cc54b30.yaml b/releasenotes/notes/bp-async-volume-attachments-b2b9cd8a4cc54b30.yaml new file mode 100644 index 0000000000..156a129915 --- /dev/null +++ b/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.