From b7964a3c5a47a5c9c2456b8a3a1735462eb437e3 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Mon, 28 Nov 2022 16:31:55 +0100 Subject: [PATCH] sma: forbid deleting devices with attached volumes It makes it possible for stateless clients to send CreateDevice / DeleteDevice each time a volume is attached / detached. Deleting a device with attached volumes results in FAILED_PRECONDITION error, which a client can simply ignore. The device will be deleted during the final DeleteDevice call, once all volumes are detached. We limit this behavior to device types that support AttachVolume / DetachVolume methods, otherwise it would be impossible to delete such devices. Signed-off-by: Konrad Sztyber Change-Id: I244b2b09455ec1430970c70f3fbb739cc9069754 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15670 Reviewed-by: Jing Yan Reviewed-by: Filip Szufnarowski Reviewed-by: Tomasz Zawadzki Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- proto/sma.proto | 5 ++++- python/spdk/sma/device/device.py | 4 +++- python/spdk/sma/device/vhost_blk.py | 2 +- python/spdk/sma/sma.py | 6 +++++- python/spdk/sma/volume/volume.py | 6 ++++++ test/sma/crypto.sh | 1 + test/sma/discovery.sh | 13 ++++++++++++- 7 files changed, 32 insertions(+), 5 deletions(-) diff --git a/proto/sma.proto b/proto/sma.proto index abc427482..ea02471f3 100644 --- a/proto/sma.proto +++ b/proto/sma.proto @@ -172,7 +172,10 @@ service StorageManagementAgent { // volumes (e.g. an NVMeoF subsystem). rpc CreateDevice (CreateDeviceRequest) returns (CreateDeviceResponse) {} - // Deletes a device + // Deletes a device. It is only allowed to delete a device with volumes still + // attached if that device doesn't support attaching volumes through + // AttachVolume (e.g. virtio-blk). In other cases, it is forbidden and + // FAILED_PRECONDITION status will be returned. rpc DeleteDevice (DeleteDeviceRequest) returns (DeleteDeviceResponse) {} // Attaches a volume to a specified device making it available through that diff --git a/python/spdk/sma/device/device.py b/python/spdk/sma/device/device.py index edcf4ac66..20d757f7f 100644 --- a/python/spdk/sma/device/device.py +++ b/python/spdk/sma/device/device.py @@ -12,10 +12,12 @@ class DeviceException(Exception): class DeviceManager: - def __init__(self, name, protocol, client): + def __init__(self, name, protocol, client, allow_delete_volumes=False): self._client = client self.protocol = protocol self.name = name + # Configures whether the device allows deleting a device with attached volumes + self.allow_delete_volumes = allow_delete_volumes def init(self, config): pass diff --git a/python/spdk/sma/device/vhost_blk.py b/python/spdk/sma/device/vhost_blk.py index bc5469b8c..7b655a045 100644 --- a/python/spdk/sma/device/vhost_blk.py +++ b/python/spdk/sma/device/vhost_blk.py @@ -20,7 +20,7 @@ from .device import DeviceException, DeviceManager class VhostBlkDeviceManager(DeviceManager): def __init__(self, client): - super().__init__('vhost_blk', 'virtio_blk', client) + super().__init__('vhost_blk', 'virtio_blk', client, allow_delete_volumes=True) def init(self, config): self._buses = config.get('buses', []) diff --git a/python/spdk/sma/sma.py b/python/spdk/sma/sma.py index 529bc5c78..e0460e6ec 100644 --- a/python/spdk/sma/sma.py +++ b/python/spdk/sma/sma.py @@ -97,8 +97,12 @@ class StorageManagementAgent(pb2_grpc.StorageManagementAgentServicer): if device is None: raise DeviceException(grpc.StatusCode.NOT_FOUND, 'Invalid device handle') + if not device.allow_delete_volumes and self._volume_mgr.has_volumes(request.handle): + raise DeviceException(grpc.StatusCode.FAILED_PRECONDITION, + 'Device has attached volumes') device.delete_device(request) - # Remove all volumes attached to that device + # Either there are no volumes attached to this device or we're allowed to delete it + # with volumes still attached self._volume_mgr.disconnect_device_volumes(request.handle) except DeviceException as ex: context.set_details(ex.message) diff --git a/python/spdk/sma/volume/volume.py b/python/spdk/sma/volume/volume.py index 9ae568346..8c0226b4b 100644 --- a/python/spdk/sma/volume/volume.py +++ b/python/spdk/sma/volume/volume.py @@ -320,3 +320,9 @@ class VolumeManager: volumes = [i for i, v in self._volumes.items() if v.device_handle == device_handle] for volume_id in volumes: self._disconnect_volume(volume_id) + + @_locked + def has_volumes(self, device_handle): + """Checks whether a given device has volumes attached to it""" + return next(filter(lambda v: v.device_handle == device_handle, + self._volumes.values()), None) is not None diff --git a/test/sma/crypto.sh b/test/sma/crypto.sh index 99ba6b890..a9e97cb5b 100755 --- a/test/sma/crypto.sh +++ b/test/sma/crypto.sh @@ -242,6 +242,7 @@ delete_device $device device=$(create_device $uuid AES_CBC $key0 | jq -r '.handle') verify_crypto_volume $localnqn $uuid +detach_volume $device $uuid delete_device $device # Try to create a device with incorrect volume crypto params, check that it fails and everything diff --git a/test/sma/discovery.sh b/test/sma/discovery.sh index 435440e2a..345ef18f7 100755 --- a/test/sma/discovery.sh +++ b/test/sma/discovery.sh @@ -260,7 +260,15 @@ detach_volume $device_id $t1uuid $rpc_py bdev_nvme_get_discovery_info | jq -r '.[].trid.trsvcid' | grep $t1dscport $rpc_py bdev_nvme_get_discovery_info | jq -r '.[].trid.trsvcid' | grep $t2dscport1 -# Delete the device and verify that this also causes the volumes to be disconnected +# Try to delete the device and verify that it fails if it has volumes attached to it +NOT delete_device $device_id + +[[ $($rpc_py bdev_nvme_get_discovery_info | jq -r '. | length') -eq 2 ]] +$rpc_py bdev_nvme_get_discovery_info | jq -r '.[].trid.trsvcid' | grep $t1dscport +$rpc_py bdev_nvme_get_discovery_info | jq -r '.[].trid.trsvcid' | grep $t2dscport1 + +# After detaching the second volume, it should be possible to delete the device +detach_volume $device_id $t2uuid delete_device $device_id [[ $($rpc_py bdev_nvme_get_discovery_info | jq -r '. | length') -eq 0 ]] @@ -299,6 +307,9 @@ $rpc_py nvmf_get_subsystems $localnqn | jq -r '.[].namespaces[].uuid' | grep $t2 $rpc_py nvmf_get_subsystems $localnqn | jq -r '.[].namespaces[].uuid' | grep $t2uuid2 # Reset the device +detach_volume $device_id $t1uuid +detach_volume $device_id $t2uuid +detach_volume $device_id $t2uuid2 delete_device $device_id [[ $($rpc_py bdev_nvme_get_discovery_info | jq -r '. | length') -eq 0 ]]