From 733d4133df8d0e13c48f45416658ec71ffff5f04 Mon Sep 17 00:00:00 2001 From: zhangbailin Date: Tue, 12 Nov 2019 20:00:54 +0800 Subject: [PATCH] Allow PUT volume attachments API to modify delete_on_termination Allow PUT /servers/{server_id}/os-volume_attachments/{volume_id}`` to support specifying ``delete_on_termination`` field in the request body. This allows updating the attached volume's flag that controls whether or not it is automatically deleted when the instance is deleted. When we request 'volumeId' and 'delete_on_termination' in the requst body to swap volume, since the new microversion it will be support updating the swapping volume's delete flag. Co-Authored-By: Dan Smith Blueprint: destroy-instance-with-datavolume Change-Id: I6ccac4e17f56b40e67c79d40f32558ef414685ea --- api-ref/source/os-volume-attachments.inc | 39 +- api-ref/source/parameters.yaml | 39 +- .../v2.85/attach-volume-to-server-req.json | 7 + .../v2.85/attach-volume-to-server-resp.json | 10 + .../v2.85/list-volume-attachments-resp.json | 20 ++ ...ate-volume-attachment-delete-flag-req.json | 6 + .../os-volumes/v2.85/update-volume-req.json | 5 + .../v2.85/volume-attachment-detail-resp.json | 10 + .../versions/v21-version-get-resp.json | 2 +- .../versions/versions-get-resp.json | 2 +- nova/api/openstack/api_version_request.py | 6 +- .../compute/rest_api_version_history.rst | 7 + nova/api/openstack/compute/schemas/volumes.py | 31 +- nova/api/openstack/compute/volumes.py | 71 +++- nova/api/validation/parameter_types.py | 5 + .../attach-volume-to-server-req.json.tpl | 7 + .../attach-volume-to-server-resp.json.tpl | 10 + .../list-volume-attachments-resp.json.tpl | 20 ++ ...volume-attachment-delete-flag-req.json.tpl | 10 + .../v2.85/update-volume-req.json.tpl | 5 + .../volume-attachment-detail-resp.json.tpl | 10 + .../api_sample_tests/test_volumes.py | 30 ++ .../api/openstack/compute/test_volumes.py | 332 +++++++++++++++++- ...ance-with-datavolume-4c71b12e005832b0.yaml | 8 + 24 files changed, 653 insertions(+), 39 deletions(-) create mode 100644 doc/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json create mode 100644 doc/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json create mode 100644 doc/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json create mode 100644 doc/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json create mode 100644 doc/api_samples/os-volumes/v2.85/update-volume-req.json create mode 100644 doc/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/update-volume-req.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json.tpl create mode 100644 releasenotes/notes/bp-destroy-instance-with-datavolume-4c71b12e005832b0.yaml diff --git a/api-ref/source/os-volume-attachments.inc b/api-ref/source/os-volume-attachments.inc index 3deb96d05a42..5ca5432b71ed 100644 --- a/api-ref/source/os-volume-attachments.inc +++ b/api-ref/source/os-volume-attachments.inc @@ -177,19 +177,25 @@ Update a volume attachment. .. note:: This action only valid when the server is in ACTIVE, PAUSED and RESIZED state, or a conflict(409) error will be returned. -.. warning:: This API is typically meant to only be used as part of a larger - orchestrated volume migration operation initiated in the block - storage service via the ``os-retype`` or ``os-migrate_volume`` - volume actions. Direct usage of this API is not recommended and - may result in needing to hard reboot the server to update details - within the guest such as block storage serial IDs. Furthermore, - this API is only implemented by `certain compute drivers`_. +.. warning:: When updating volumeId, this API is typically meant to + only be used as part of a larger orchestrated volume + migration operation initiated in the block storage + service via the ``os-retype`` or ``os-migrate_volume`` + volume actions. Direct usage of this API to update + volumeId is not recommended and may result in needing to + hard reboot the server to update details within the guest + such as block storage serial IDs. Furthermore, updating + volumeId via this API is only implemented by `certain + compute drivers`_. .. _certain compute drivers: https://docs.openstack.org/nova/latest/user/support-matrix.html#operation_swap_volume -Policy defaults enable only users with the administrative role to perform -this operation. Cloud providers can change these permissions through the -``policy.json`` file. +Policy default role is 'rule:system_admin_or_owner', its scope is +[system, project], which allow project members or system admins to +change the fields of an attached volume of a server. Policy defaults +enable only users with the administrative role to change ``volumeId`` +via this operation. Cloud providers can change these permissions +through the ``policy.json`` file. Updating, or what is commonly referred to as "swapping", volume attachments with volumes that have more than one read/write attachment, is not supported. @@ -207,10 +213,19 @@ Request - volume_id: volume_id_swap_src - volumeAttachment: volumeAttachment_put - volumeId: volumeId_swap + - delete_on_termination: delete_on_termination_put_req + - device: attachment_device_put_req + - serverId: attachment_server_id_put_req + - tag: device_tag_bdm_attachment_put_req + - id: attachment_id_put_req -**Example Update a volume attachment: JSON request** +.. note:: Other than ``volumeId``, as of v2.85 only + ``delete_on_termination`` may be changed from the current + value. -.. literalinclude:: ../../doc/api_samples/os-volumes/update-volume-req.json +**Example Update a volume attachment (v2.85): JSON request** + +.. literalinclude:: ../../doc/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json :language: javascript Response diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 4dff3fcf2b73..263835d84848 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -1776,12 +1776,26 @@ associate_host: in: body required: true type: string +attachment_device_put_req: + description: | + Name of the device in the attachment object, such as, ``/dev/vdb``. + in: body + required: false + type: string + min_version: 2.85 attachment_device_resp: description: | Name of the device in the attachment object, such as, ``/dev/vdb``. in: body required: false type: string +attachment_id_put_req: + description: | + The UUID of the attachment. + in: body + required: false + type: string + min_version: 2.85 attachment_id_required: description: | The UUID of the attachment. @@ -1794,6 +1808,13 @@ attachment_id_resp: in: body required: false type: string +attachment_server_id_put_req: + description: | + The UUID of the server. + in: body + required: false + type: string + min_version: 2.85 attachment_server_id_resp: description: | The UUID of the server. @@ -2294,6 +2315,14 @@ delete_on_termination_attachments_resp: required: true type: boolean min_version: 2.79 +delete_on_termination_put_req: + description: | + A flag indicating if the attached volume will be deleted when the server is + deleted. + in: body + required: false + type: boolean + min_version: 2.85 deleted: description: | A boolean indicates whether this aggregate is deleted or not, if it has @@ -2384,6 +2413,13 @@ device_tag_bdm_attachment: required: false type: string min_version: 2.49 +device_tag_bdm_attachment_put_req: + description: | + The device tag applied to the volume block device or ``null``. + in: body + required: true + type: string + min_version: 2.85 device_tag_bdm_attachment_resp: description: | The device tag applied to the volume block device or ``null``. @@ -7370,7 +7406,8 @@ volumeAttachment_post: volumeAttachment_put: description: | A dictionary representation of a volume attachment containing the field - ``volumeId`` which is the UUID of the replacement volume. + ``volumeId`` which is the UUID of the replacement volume, and other fields + to update in the attachment. in: body required: true type: object diff --git a/doc/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json b/doc/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json new file mode 100644 index 000000000000..b4429e12e968 --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/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-volumes/v2.85/attach-volume-to-server-resp.json b/doc/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json new file mode 100644 index 000000000000..3a60cdc0d095 --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json @@ -0,0 +1,10 @@ +{ + "volumeAttachment": { + "delete_on_termination": true, + "device": "/dev/sdb", + "id": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113", + "serverId": "09b3b9d1-b8c5-48e1-841d-62c3ef967a88", + "tag": "foo", + "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113" + } +} \ No newline at end of file diff --git a/doc/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json b/doc/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json new file mode 100644 index 000000000000..ffe7c0baf1eb --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json @@ -0,0 +1,20 @@ +{ + "volumeAttachments": [ + { + "delete_on_termination": false, + "device": "/dev/sdc", + "id": "227cc671-f30b-4488-96fd-7d0bf13648d8", + "serverId": "d5e4ae35-ac0e-4311-a8c5-0ee863e951d9", + "tag": null, + "volumeId": "227cc671-f30b-4488-96fd-7d0bf13648d8" + }, + { + "delete_on_termination": true, + "device": "/dev/sdb", + "id": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113", + "serverId": "d5e4ae35-ac0e-4311-a8c5-0ee863e951d9", + "tag": "foo", + "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113" + } + ] +} \ No newline at end of file diff --git a/doc/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json b/doc/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json new file mode 100644 index 000000000000..30105458e7cd --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json @@ -0,0 +1,6 @@ +{ + "volumeAttachment": { + "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113", + "delete_on_termination": true + } +} diff --git a/doc/api_samples/os-volumes/v2.85/update-volume-req.json b/doc/api_samples/os-volumes/v2.85/update-volume-req.json new file mode 100644 index 000000000000..e5ad47aa3cce --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/update-volume-req.json @@ -0,0 +1,5 @@ +{ + "volumeAttachment": { + "volumeId": "227cc671-f30b-4488-96fd-7d0bf13648d8" + } +} \ No newline at end of file diff --git a/doc/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json b/doc/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json new file mode 100644 index 000000000000..4a54243c2b1c --- /dev/null +++ b/doc/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json @@ -0,0 +1,10 @@ +{ + "volumeAttachment": { + "delete_on_termination": true, + "device": "/dev/sdb", + "id": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113", + "serverId": "2aad99d3-7aa4-41e9-b4e6-3f960b115d68", + "tag": "foo", + "volumeId": "a07f71dc-8151-4e7d-a0cc-cd24a3f11113" + } +} \ No newline at end of file diff --git a/doc/api_samples/versions/v21-version-get-resp.json b/doc/api_samples/versions/v21-version-get-resp.json index 5e5c77561fbc..2d1760aa312f 100644 --- a/doc/api_samples/versions/v21-version-get-resp.json +++ b/doc/api_samples/versions/v21-version-get-resp.json @@ -19,7 +19,7 @@ } ], "status": "CURRENT", - "version": "2.84", + "version": "2.85", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z" } diff --git a/doc/api_samples/versions/versions-get-resp.json b/doc/api_samples/versions/versions-get-resp.json index 8eb238e31930..d0a29d7ff181 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.84", + "version": "2.85", "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 3d2f0549ee88..b6e354e32caf 100644 --- a/nova/api/openstack/api_version_request.py +++ b/nova/api/openstack/api_version_request.py @@ -227,6 +227,10 @@ REST_API_VERSION_HISTORY = """REST API Version History: * 2.83 - Allow more filter parameters for ``GET /servers/detail`` and ``GET /servers`` for non-admin. * 2.84 - Adds ``details`` field to instance action events. + * 2.85 - Add support for + ``PUT /servers/{server_id}/os-volume_attachments/{volume_id}`` + which supports specifying the ``delete_on_termination`` field in + the request body to change the attached volume's flag. """ # The minimum and maximum versions of the API supported @@ -235,7 +239,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.84" +_MAX_API_VERSION = "2.85" 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 28c567e652c2..cb51613d6a6a 100644 --- a/nova/api/openstack/compute/rest_api_version_history.rst +++ b/nova/api/openstack/compute/rest_api_version_history.rst @@ -1113,3 +1113,10 @@ The ``GET /servers/{server_id}/os-instance-actions/{request_id}`` API returns a ``details`` parameter for each failed event with a fault message, similar to the server ``fault.message`` parameter in ``GET /servers/{server_id}`` for a server with status ``ERROR``. + +2.85 +---- + +Adds the ability to specify ``delete_on_termination`` in the +``PUT /servers/{server_id}/os-volume_attachments/{volume_id}`` API, which +allows changing the behavior of volume deletion on instance deletion. diff --git a/nova/api/openstack/compute/schemas/volumes.py b/nova/api/openstack/compute/schemas/volumes.py index 2848f7f2ebb9..6ac52354c1b3 100644 --- a/nova/api/openstack/compute/schemas/volumes.py +++ b/nova/api/openstack/compute/schemas/volumes.py @@ -74,7 +74,7 @@ create_volume_attachment = { # NOTE: The validation pattern from match_device() in # nova/block_device.py. 'pattern': '(^/dev/x{0,1}[a-z]{0,1}d{0,1})([a-z]+)[0-9]*$' - } + }, }, 'required': ['volumeId'], 'additionalProperties': False, @@ -95,6 +95,35 @@ update_volume_attachment = copy.deepcopy(create_volume_attachment) del update_volume_attachment['properties']['volumeAttachment'][ 'properties']['device'] +# NOTE(brinzhang): Allow attachment_id, serverId, device, tag, and +# delete_on_termination to be specified for RESTfulness, even though +# we will not allow updating all of them. +update_volume_attachment_v285 = { + 'type': 'object', + 'properties': { + 'volumeAttachment': { + 'type': 'object', + 'properties': { + 'volumeId': parameter_types.volume_id, + 'device': { + 'type': ['string', 'null'], + # NOTE: The validation pattern from match_device() in + # nova/block_device.py. + 'pattern': '(^/dev/x{0,1}[a-z]{0,1}d{0,1})([a-z]+)[0-9]*$' + }, + 'tag': parameter_types.tag, + 'delete_on_termination': parameter_types.boolean, + 'serverId': parameter_types.server_id, + 'id': parameter_types.attachment_id + }, + 'required': ['volumeId'], + 'additionalProperties': False, + }, + }, + 'required': ['volumeAttachment'], + 'additionalProperties': False, +} + index_query = { 'type': 'object', 'properties': { diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 3fa49356a397..6129fa2d5477 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -391,15 +391,8 @@ class VolumeAttachmentController(wsgi.Controller): attachment['delete_on_termination'] = delete_on_termination return {'volumeAttachment': attachment} - @wsgi.response(202) - @wsgi.expected_errors((400, 404, 409)) - @validation.schema(volumes_schema.update_volume_attachment) - def update(self, req, server_id, id, body): + def _update_volume_swap(self, req, instance, id, body): context = req.environ['nova.context'] - instance = common.get_instance(self.compute_api, context, server_id) - context.can(va_policies.POLICY_ROOT % 'update', - target={'project_id': instance.project_id}) - old_volume_id = id try: old_volume = self.volume_api.get(context, old_volume_id) @@ -431,7 +424,67 @@ class VolumeAttachmentController(wsgi.Controller): raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, - 'swap_volume', server_id) + 'swap_volume', instance.uuid) + + def _update_volume_regular(self, req, instance, id, body): + context = req.environ['nova.context'] + att = body['volumeAttachment'] + # NOTE(danms): We may be doing an update of regular parameters in + # the midst of a swap operation, so to find the original BDM, we need + # to use the old volume ID, which is the one in the path. + volume_id = id + + try: + bdm = objects.BlockDeviceMapping.get_by_volume_and_instance( + context, volume_id, instance.uuid) + + # NOTE(danms): The attachment id is just the (current) volume id + if 'id' in att and att['id'] != volume_id: + raise exc.HTTPBadRequest(explanation='The id property is ' + 'not mutable') + if 'serverId' in att and att['serverId'] != instance.uuid: + raise exc.HTTPBadRequest(explanation='The serverId property ' + 'is not mutable') + if 'device' in att and att['device'] != bdm.device_name: + raise exc.HTTPBadRequest(explanation='The device property is ' + 'not mutable') + if 'tag' in att and att['tag'] != bdm.tag: + raise exc.HTTPBadRequest(explanation='The tag property is ' + 'not mutable') + if 'delete_on_termination' in att: + bdm.delete_on_termination = att['delete_on_termination'] + bdm.save() + except exception.VolumeBDMNotFound as e: + raise exc.HTTPNotFound(explanation=e.format_message()) + + @wsgi.response(202) + @wsgi.expected_errors((400, 404, 409)) + @validation.schema(volumes_schema.update_volume_attachment, '2.0', '2.84') + @validation.schema(volumes_schema.update_volume_attachment_v285, + min_version='2.85') + def update(self, req, server_id, id, body): + context = req.environ['nova.context'] + instance = common.get_instance(self.compute_api, context, server_id) + # TODO(danms): For now, use the existing admin-only policy for update. + # Later, split off the swap_volume permission and check the correct + # policy based on what is being asked by the client. + context.can(va_policies.POLICY_ROOT % 'update', + target={'project_id': instance.project_id}) + + attachment = body['volumeAttachment'] + volume_id = attachment['volumeId'] + only_swap = not api_version_request.is_supported(req, '2.85') + if only_swap: + # NOTE(danms): Original behavior is always call swap on PUT + # FIXME(danms): Check the swap volume policy here + self._update_volume_swap(req, instance, id, body) + else: + # NOTE(danms): New behavior is update any supported attachment + # properties first, and then call swap if volumeId differs + # FIXME(danms): Check the volume attachment update policy here + self._update_volume_regular(req, instance, id, body) + if id != volume_id: + self._update_volume_swap(req, instance, id, body) @wsgi.response(202) @wsgi.expected_errors((400, 403, 404, 409)) diff --git a/nova/api/validation/parameter_types.py b/nova/api/validation/parameter_types.py index caeb2d7eb055..144a1f64cfe3 100644 --- a/nova/api/validation/parameter_types.py +++ b/nova/api/validation/parameter_types.py @@ -336,6 +336,11 @@ volume_id = { } +attachment_id = { + 'type': 'string', 'format': 'uuid' +} + + volume_type = { 'type': ['string', 'null'], 'minLength': 0, 'maxLength': 255 } diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-req.json.tpl new file mode 100644 index 000000000000..92c13861bb6f --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/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-volumes/v2.85/attach-volume-to-server-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json.tpl new file mode 100644 index 000000000000..34eacc94d233 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/attach-volume-to-server-resp.json.tpl @@ -0,0 +1,10 @@ +{ + "volumeAttachment": { + "device": "%(device)s", + "id": "%(volume_id)s", + "serverId": "%(uuid)s", + "tag": "%(tag)s", + "volumeId": "%(volume_id)s", + "delete_on_termination": true + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json.tpl new file mode 100644 index 000000000000..700cf5e759f2 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/list-volume-attachments-resp.json.tpl @@ -0,0 +1,20 @@ +{ + "volumeAttachments": [ + { + "device": "%(device)s", + "id": "%(volume_id)s", + "serverId": "%(uuid)s", + "tag": "%(tag)s", + "volumeId": "%(volume_id)s", + "delete_on_termination": true + }, + { + "device": "%(text)s", + "id": "%(volume_id2)s", + "serverId": "%(uuid)s", + "tag": null, + "volumeId": "%(volume_id2)s", + "delete_on_termination": false + } + ] +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/update-volume-attachment-delete-flag-req.json.tpl new file mode 100644 index 000000000000..2aba36133c5e --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/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-volumes/v2.85/update-volume-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/update-volume-req.json.tpl new file mode 100644 index 000000000000..85c1244b1438 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/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-volumes/v2.85/volume-attachment-detail-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json.tpl new file mode 100644 index 000000000000..34eacc94d233 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-volumes/v2.85/volume-attachment-detail-resp.json.tpl @@ -0,0 +1,10 @@ +{ + "volumeAttachment": { + "device": "%(device)s", + "id": "%(volume_id)s", + "serverId": "%(uuid)s", + "tag": "%(tag)s", + "volumeId": "%(volume_id)s", + "delete_on_termination": true + } +} diff --git a/nova/tests/functional/api_sample_tests/test_volumes.py b/nova/tests/functional/api_sample_tests/test_volumes.py index 0b5827a0a885..4c48a3e164ee 100644 --- a/nova/tests/functional/api_sample_tests/test_volumes.py +++ b/nova/tests/functional/api_sample_tests/test_volumes.py @@ -287,3 +287,33 @@ class VolumeAttachmentsSampleV279(VolumeAttachmentsSampleV270): """ microversion = '2.79' scenarios = [('v2_79', {'api_major_version': 'v2.1'})] + + +class UpdateVolumeAttachmentsSampleV285(VolumeAttachmentsSampleV279): + """Microversion 2.85 adds the ``PUT + /servers/{server_id}/os-volume_attachments/{volume_id}`` + support for specifying ``delete_on_termination`` field in the request + body to re-config the attached volume whether to delete when the instance + is deleted. + """ + microversion = '2.85' + scenarios = [('v2_85', {'api_major_version': 'v2.1'})] + + def test_volume_attachment_update(self): + subs = self.test_attach_volume_to_server() + attached_volume_id = subs['volume_id'] + subs['server_id'] = self.server_id + response = self._do_put('servers/%s/os-volume_attachments/%s' + % (self.server_id, attached_volume_id), + 'update-volume-attachment-delete-flag-req', + subs) + self.assertEqual(202, response.status_code) + self.assertEqual('', response.text) + + # Make sure the attached volume was changed + attachments = self.api.api_get( + '/servers/%s/os-volume_attachments' % self.server_id).body[ + 'volumeAttachments'] + self.assertEqual(1, len(attachments)) + self.assertEqual(self.server_id, attachments[0]['serverId']) + self.assertTrue(attachments[0]['delete_on_termination']) diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index c8f5a0beec95..0b04f23f96c5 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -39,6 +39,7 @@ import nova.conf from nova import context from nova import exception from nova import objects +from nova.objects import block_device as block_device_obj from nova import test from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_block_device @@ -731,8 +732,9 @@ class VolumeAttachTestsV21(test.NoDBTestCase): side_effect=exception.InstanceIsLocked( instance_uuid=uuids.instance)) def test_swap_volume_for_locked_server(self, mock_swap_volume): - self.assertRaises(webob.exc.HTTPConflict, self._test_swap, - self.attachments) + with mock.patch.object(self.attachments, '_update_volume_regular'): + self.assertRaises(webob.exc.HTTPConflict, self._test_swap, + self.attachments) mock_swap_volume.assert_called_once_with( self.req.environ['nova.context'], test.MatchType(objects.Instance), {'attach_status': 'attached', @@ -771,8 +773,9 @@ class VolumeAttachTestsV21(test.NoDBTestCase): mock_get.side_effect = [ None, exception.VolumeNotFound(volume_id=FAKE_UUID_C)] body = {'volumeAttachment': {'volumeId': FAKE_UUID_C}} - self.assertRaises(exc.HTTPBadRequest, self._test_swap, - self.attachments, body=body) + with mock.patch.object(self.attachments, '_update_volume_regular'): + self.assertRaises(exc.HTTPBadRequest, self._test_swap, + self.attachments, body=body) mock_get.assert_has_calls([ mock.call(self.req.environ['nova.context'], FAKE_UUID_A), mock.call(self.req.environ['nova.context'], FAKE_UUID_C)]) @@ -796,17 +799,30 @@ class VolumeAttachTestsV21(test.NoDBTestCase): @mock.patch.object(compute_api.API, 'swap_volume', side_effect=exception.VolumeBDMNotFound( volume_id=FAKE_UUID_B)) - def test_swap_volume_for_bdm_not_found(self, mock_swap_volume): + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance', + side_effect=exception.VolumeBDMNotFound( + volume_id=FAKE_UUID_A)) + def test_swap_volume_for_bdm_not_found(self, mock_bdm, mock_swap_volume): self.assertRaises(webob.exc.HTTPNotFound, self._test_swap, self.attachments) - mock_swap_volume.assert_called_once_with( - self.req.environ['nova.context'], test.MatchType(objects.Instance), - {'attach_status': 'attached', - 'status': 'in-use', - 'id': FAKE_UUID_A}, - {'attach_status': 'detached', - 'status': 'available', - 'id': FAKE_UUID_B}) + if mock_bdm.called: + # New path includes regular PUT procedure + mock_bdm.assert_called_once_with(self.req.environ['nova.context'], + FAKE_UUID_A, uuids.instance) + mock_swap_volume.assert_not_called() + else: + # Old path is pure swap-volume + mock_bdm.assert_not_called() + mock_swap_volume.assert_called_once_with( + self.req.environ['nova.context'], + test.MatchType(objects.Instance), + {'attach_status': 'attached', + 'status': 'in-use', + 'id': FAKE_UUID_A}, + {'attach_status': 'detached', + 'status': 'available', + 'id': FAKE_UUID_B}) def _test_list_with_invalid_filter(self, url): req = self._build_request(url) @@ -1149,6 +1165,296 @@ class VolumeAttachTestsV279(VolumeAttachTestsV2_75): self.assertNotIn('delete_on_termination', result['volumeAttachments']) +class UpdateVolumeAttachTests(VolumeAttachTestsV279): + microversion = '2.85' + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + def test_swap_volume(self, mock_save_bdm, mock_get_bdm): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_bdm.return_value = vol_bdm + # On the newer microversion, this test will try to look up the + # BDM to check for update of other fields. + super(UpdateVolumeAttachTests, self).test_swap_volume() + + def test_swap_volume_with_extra_arg(self): + # NOTE(danms): Override this from parent because now device + # is checked for unchanged-ness. + body = {'volumeAttachment': {'volumeId': FAKE_UUID_A, + 'device': '/dev/fake0', + 'notathing': 'foo'}} + + self.assertRaises(self.validation_error, + self._test_swap, + self.attachments, + body=body) + + @mock.patch.object(compute_api.API, 'swap_volume') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + def test_update_volume(self, mock_bdm_save, + mock_get_vol_and_inst, mock_swap): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'tag': 'fake-tag', + 'delete_on_termination': True, + 'device': '/dev/fake0', + }} + self.attachments.update(self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + mock_swap.assert_not_called() + mock_bdm_save.assert_called_once() + self.assertTrue(vol_bdm['delete_on_termination']) + + @mock.patch.object(compute_api.API, 'swap_volume') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + def test_update_volume_swap(self, mock_bdm_save, + mock_get_vol_and_inst, mock_swap): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_B, + 'tag': 'fake-tag', + 'delete_on_termination': True, + }} + self.attachments.update(self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + mock_bdm_save.assert_called_once() + self.assertTrue(vol_bdm['delete_on_termination']) + # Swap volume is tested elsewhere, just make sure that we did + # attempt to call it in addition to updating the BDM + self.assertTrue(mock_swap.called) + + @mock.patch.object(compute_api.API, 'swap_volume') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + def test_update_volume_swap_only_old_microversion( + self, mock_bdm_save, mock_get_vol_and_inst, mock_swap): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_B, + }} + req = self._get_req(body, microversion='2.84') + self.attachments.update(req, FAKE_UUID, + FAKE_UUID_A, body=body) + mock_swap.assert_called_once() + mock_bdm_save.assert_not_called() + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance', + side_effect=exception.VolumeBDMNotFound( + volume_id=FAKE_UUID_A)) + def test_update_volume_with_invalid_volume_id(self, mock_mr): + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'delete_on_termination': True, + }} + self.assertRaises(exc.HTTPNotFound, + self.attachments.update, + self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_update_volume_with_changed_attachment_id(self, + mock_get_vol_and_inst): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'id': uuids.attachment_id2, + }} + self.assertRaises(exc.HTTPBadRequest, + self.attachments.update, + self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_update_volume_with_changed_serverId(self, + mock_get_vol_and_inst): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'serverId': uuids.server_id, + }} + self.assertRaises(exc.HTTPBadRequest, + self.attachments.update, + self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_update_volume_with_changed_device(self, mock_get_vol_and_inst): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'device': '/dev/sdz', + }} + self.assertRaises(exc.HTTPBadRequest, + self.attachments.update, + self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + + def test_update_volume_with_device_name_old_microversion(self): + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'device': '/dev/fake0', + }} + req = self._get_req(body, microversion='2.84') + ex = self.assertRaises(exception.ValidationError, + self.attachments.update, + req, FAKE_UUID, + FAKE_UUID_A, body=body) + self.assertIn('Additional properties are not allowed', + six.text_type(ex)) + + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_update_volume_with_changed_tag(self, mock_get_vol_and_inst): + vol_bdm = objects.BlockDeviceMapping( + self.context, + id=1, + instance_uuid=FAKE_UUID, + volume_id=FAKE_UUID_A, + source_type='volume', + destination_type='volume', + delete_on_termination=False, + connection_info=None, + tag='fake-tag', + device_name='/dev/fake0', + attachment_id=uuids.attachment_id) + mock_get_vol_and_inst.return_value = vol_bdm + + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'tag': 'icanhaznewtag', + }} + self.assertRaises(exc.HTTPBadRequest, + self.attachments.update, + self.req, FAKE_UUID, + FAKE_UUID_A, body=body) + + def test_update_volume_with_tag_old_microversion(self): + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'tag': 'fake-tag', + }} + req = self._get_req(body, microversion='2.84') + ex = self.assertRaises(exception.ValidationError, + self.attachments.update, + req, FAKE_UUID, + FAKE_UUID_A, body=body) + self.assertIn('Additional properties are not allowed', + six.text_type(ex)) + + def test_update_volume_with_delete_flag_old_microversion(self): + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'delete_on_termination': True, + }} + req = self._get_req(body, microversion='2.84') + ex = self.assertRaises(exception.ValidationError, + self.attachments.update, + req, FAKE_UUID, + FAKE_UUID_A, body=body) + self.assertIn('Additional properties are not allowed', + six.text_type(ex)) + + class SwapVolumeMultiattachTestCase(test.NoDBTestCase): @mock.patch('nova.api.openstack.common.get_instance') diff --git a/releasenotes/notes/bp-destroy-instance-with-datavolume-4c71b12e005832b0.yaml b/releasenotes/notes/bp-destroy-instance-with-datavolume-4c71b12e005832b0.yaml new file mode 100644 index 000000000000..8abb0210d236 --- /dev/null +++ b/releasenotes/notes/bp-destroy-instance-with-datavolume-4c71b12e005832b0.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + With microversion 2.85 add new API + ``PUT /servers/{server_id}/os-volume_attachments/{volume_id}`` which + support for specifying ``delete_on_termination`` field in the request + body to re-config the attached volume whether to delete when the instance + is deleted.