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 <idryomov@gmail.com>

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 46082039bd78efb6b2d7ef778fdee357dead6ec4..d572eb9070d4a3a53172dcb801cdede8f2aded89 100644 (file)
--- 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 c0924d9f5f620031fd96aa990f9c37376249b998..bc7523285af15f8a17645bb418774146e63dcef8 100644 (file)
--- a/src/librbd/api/Trash.cc
+++ b/src/librbd/api/Trash.cc
@@ -217,6 +217,13 @@ int Trash<I>::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 ebbe37d5612cd19b0bcb17e7a3792c09947702f1..9ea5cd0749ed9decba85122fe9e19d92195eef6f 100644 (file)
--- 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 3e7c039102d7c2410fadcbeba5d12d6d2fa3dcf0..319280013100b2ac4bb2e900d1de42e96127894d 100644 (file)
--- 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) {