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>
(cherry picked from commit 2ae4a57e160f21201b9dfb4c8adb02f2826c7106)

Conflicts:
	PendingReleaseNotes [ moved to >=19.2.2 section ]

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index e42f74333a9dd5fbf7a0c5669bd7856e434eb316..ad0baf308194615ff946c2fc53c030e3d315e7b6 100644 (file)
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -64,6 +64,10 @@
    - https://tracker.ceph.com/issues/67179
    - https://tracker.ceph.com/issues/66867
 
+* 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.2
 
 * MGR: MGR's always-on modulues/plugins can now be force-disabled. This can be

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 6d9e5802d952a958f5851b349c14e33d57601489..d62dce1a8dd77aa56cb2694f4031d6772fdd9059 100644 (file)
--- a/src/test/pybind/test_rbd.py
+++ b/src/test/pybind/test_rbd.py
@@ -2971,13 +2971,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) {