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 <[email protected]>

Author: idryomov

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

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 &&

src/test/pybind/test_rbd.py

@@ -3069,13 +3069,10 @@ def test_group_image_list(self):
 
     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

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) {