From 2ae4a57e160f21201b9dfb4c8adb02f2826c7106 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Wed, 16 Apr 2025 13:15:19 +0200 Subject: [PATCH] librbd: disallow "rbd trash mv" if image is in a group Removing an image that is a member of a group has always been disallowed. However, moving an image that is a member of a group to trash is currently allowed and this is deceptive -- the only reason for a user to move an image to trash should be the intent to remove it. More importantly, group APIs operate in terms of image names -- there are no corresponding variants that would operate in terms of image IDs. For example, even though internally GroupImageSpec struct stores an image ID, the public rbd_group_image_info_t struct insists on an image name. When rbd_group_image_list() encounters a trashed member image (i.e. one that doesn't have a name), it just fails with ENOENT and no listing gets produced at all until the offending image is restored from trash. Something like this can be very hard to debug for an average user, so let's make rbd_trash_move() fail with EMLINK the same way as rbd_remove() does in this scenario. The one case where moving a member image to trash makes sense is live migration where the source image gets trashed to be almost immediately replaced by the destination image as part of preparing migration. Fixes: https://tracker.ceph.com/issues/71026 Signed-off-by: Ilya Dryomov --- PendingReleaseNotes | 4 ++++ src/librbd/api/Trash.cc | 7 +++++++ src/test/pybind/test_rbd.py | 7 ++----- src/tools/rbd/action/Trash.cc | 16 +++++++++++++--- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 46082039bd78e..d572eb9070d4a 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -129,6 +129,10 @@ * RGW: Added support for the `RestrictPublicBuckets` property of the S3 `PublicAccessBlock` configuration. +* RBD: Moving an image that is a member of a group to trash is no longer + allowed. `rbd trash mv` command now behaves the same way as `rbd rm` in this + scenario. + >=19.2.1 * CephFS: Command `fs subvolume create` now allows tagging subvolumes through option diff --git a/src/librbd/api/Trash.cc b/src/librbd/api/Trash.cc index c0924d9f5f620..bc7523285af15 100644 --- a/src/librbd/api/Trash.cc +++ b/src/librbd/api/Trash.cc @@ -217,6 +217,13 @@ int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, ictx->state->close(); return -EBUSY; } + if (ictx->group_spec.is_valid() && + source != RBD_TRASH_IMAGE_SOURCE_MIGRATION) { + lderr(cct) << "image is in a group - not moving to trash" << dendl; + ictx->image_lock.unlock_shared(); + ictx->state->close(); + return -EMLINK; + } ictx->image_lock.unlock_shared(); if (mirror_r >= 0 && diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index ebbe37d5612cd..9ea5cd0749ed9 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -3069,13 +3069,10 @@ class TestGroups(object): def test_group_image_list_move_to_trash(self): eq([], list(self.group.list_images())) - with Image(ioctx, image_name) as image: - image_id = image.id() self.group.add_image(ioctx, image_name) eq([image_name], [img['name'] for img in self.group.list_images()]) - RBD().trash_move(ioctx, image_name, 0) - eq([], list(self.group.list_images())) - RBD().trash_restore(ioctx, image_id, image_name) + assert_raises(ImageMemberOfGroup, RBD().trash_move, ioctx, image_name, 0) + eq([image_name], [img['name'] for img in self.group.list_images()]) def test_group_image_list_remove(self): # need a closed image to get ImageMemberOfGroup instead of ImageBusy diff --git a/src/tools/rbd/action/Trash.cc b/src/tools/rbd/action/Trash.cc index 3e7c039102d7c..319280013100b 100644 --- a/src/tools/rbd/action/Trash.cc +++ b/src/tools/rbd/action/Trash.cc @@ -97,8 +97,15 @@ int execute_move(const po::variables_map &vm, librbd::RBD rbd; r = rbd.trash_move(io_ctx, image_name.c_str(), dt); if (r < 0) { - std::cerr << "rbd: deferred delete error: " << cpp_strerror(r) - << std::endl; + if (r == -EMLINK) { + std::cerr << "rbd: error: image belongs to a group" + << std::endl + << "Remove the image from the group and try again." + << std::endl; + } else { + std::cerr << "rbd: deferred delete error: " << cpp_strerror(r) + << std::endl; + } return r; } @@ -162,7 +169,10 @@ int execute_remove(const po::variables_map &vm, << "waiting 30s for the crashed client to timeout." << std::endl; } else if (r == -EMLINK) { - std::cerr << std::endl + // moving to trash an image that belongs to a group is no longer + // allowed, this is to handle any image that was trashed earlier + std::cerr << "rbd: error: image belongs to a group" + << std::endl << "Remove the image from the group and try again." << std::endl; } else if (r == -EPERM) { -- 2.39.5