librbd: propagate ENOENT for non-existent groups and images

Calling `list_images()` on a non-existent group falsely succeeded and
returned an empty iterator.This happened because `Group<I>::image_list`
ignored the negative error code returned by `group_image_list`, allowing
execution to proceed blindly. Furthermore, the C API `rbd_group_image_list`
intercepted `-ENOENT` and forced a success (0) return value.

Similarly other public C APIs `rbd_snap_list` and `rbd_group_snap_list`
also intercepts -ENOENT and forces a success (0) return value which
is incorrect.

Fix these behavior by:
- Propagating errors properly inside `Group<I>::image_list`.
- Removing the incorrect `-ENOENT` masking in `rbd_group_image_list`,
`rbd_snap_list`, rbd_group_snap_list`

Now `list_images()` consistently raise an `ObjectNotFound` error when
invoked on a non-existent group and public C APIs propagate ENOENT
without masking it into 0.
Also Extended the `TestGroups` test fixture with `self.dne_group` to
validate the expected error paths across all relevant APIs.

Fixes: https://tracker.ceph.com/issues/78108
Signed-off-by: VinayBhaskar-V <vvarada@redhat.com>

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 9e873fab60c23ec9f9173882f988b3e4a787ec69..9658c2f9964e1aa966fb1e93bfa991e32342aa96 100644 (file)
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -269,6 +269,10 @@
   rather rigid and also challenging to disseminate in some environments.  The
   key can be embedded in the migration spec or just referenced from there while
   stored in the MON config-key store.
+* RBD: `Group::list_images` python API will now correctly raise an ObjectNotFound
+  error when invoked on a non-existent group. The C APIs `rbd_group_image_list`,
+  `rbd_snap_list`, `rbd_group_snap_list` and C++ API `group_image_list` will now
+  return ENOENT instead of masking the error and returning 0.
 * BlueStore: The experimental PMEM device backend has been removed, together
   with the ``pmem`` value of the ``bdev_type`` option and the DML/DSA offload
   path. The hardware it targeted, Intel Optane DC persistent memory, was

diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index 36c963941f1aa744b97873585d1884562b2b275c..c45471be6d47f4e6443ca08d5bee0c2194994bd6 100644 (file)
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -822,7 +822,10 @@ int Group<I>::image_list(librados::IoCtx& group_ioctx,
 
   std::vector<cls::rbd::GroupImageStatus> image_ids;
 
-  group_image_list(group_ioctx, group_name, &image_ids);
+  int r = group_image_list(group_ioctx, group_name, &image_ids);
+  if (r < 0) {
+    return r;
+  }
 
   for (auto image_id : image_ids) {
     IoCtx ioctx;

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index fac2c5a29497ff05d93c44660565e1fe771a9c09..923261dd98d056dc1cb9ff7d76088372c413fa8a 100644 (file)
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -5700,10 +5700,6 @@ extern "C" int rbd_snap_list(rbd_image_t image, rbd_snap_info_t *snaps,
   memset(snaps, 0, sizeof(*snaps) * *max_snaps);
 
   int r = librbd::api::Snapshot<>::list(ictx, cpp_snaps);
-  if (r == -ENOENT) {
-    tracepoint(librbd, snap_list_exit, 0, *max_snaps);
-    return 0;
-  }
   if (r < 0) {
     tracepoint(librbd, snap_list_exit, r, *max_snaps);
     return r;
@@ -7266,12 +7262,6 @@ extern "C" int rbd_group_image_list(rados_ioctx_t group_p,
   int r = librbd::api::Group<>::image_list(group_ioctx, group_name,
                                            &cpp_images);
 
-  if (r == -ENOENT) {
-    tracepoint(librbd, group_image_list_exit, 0);
-    *image_size = 0;
-    return 0;
-  }
-
   if (r < 0) {
     tracepoint(librbd, group_image_list_exit, r);
     return r;
@@ -7447,12 +7437,6 @@ extern "C" int rbd_group_snap_list(rados_ioctx_t group_p,
   int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, true, false,
                                           &cpp_snaps);
 
-  if (r == -ENOENT) {
-    *snaps_size = 0;
-    tracepoint(librbd, group_snap_list_exit, 0);
-    return 0;
-  }
-
   if (r < 0) {
     tracepoint(librbd, group_snap_list_exit, r);
     return r;

diff --git a/src/test/librbd/test_Groups.cc b/src/test/librbd/test_Groups.cc
index 702128de181c521f5469e97190bde3c930ffe70d..122cda57bc5d0ecc952917674594db445cde02c1 100644 (file)
--- a/src/test/librbd/test_Groups.cc
+++ b/src/test/librbd/test_Groups.cc
@@ -164,6 +164,10 @@ TEST_F(TestGroup, add_image)
   ASSERT_EQ(1U, num_images);
 
   rbd_group_image_info_t images[1];
+  ASSERT_EQ(-ENOENT, rbd_group_image_list(ioctx, "group_does_not_exist",
+                                          images,
+                                          sizeof(rbd_group_image_info_t),
+                                          &num_images));
   ASSERT_EQ(1, rbd_group_image_list(ioctx, group_name, images,
                                     sizeof(rbd_group_image_info_t),
                                     &num_images));
@@ -233,6 +237,9 @@ TEST_F(TestGroup, add_imagePP)
                 RBD_OPERATION_FEATURE_GROUP);
 
   std::vector<librbd::group_image_info_t> images;
+  ASSERT_EQ(-ENOENT, rbd.group_image_list(ioctx, "group_does_not_exist",
+                                          &images,
+                                          sizeof(librbd::group_image_info_t)));
   ASSERT_EQ(0, rbd.group_image_list(ioctx, group_name, &images,
                                     sizeof(librbd::group_image_info_t)));
   ASSERT_EQ(1U, images.size());
@@ -342,6 +349,9 @@ TEST_F(TestGroup, add_snapshot)
   ASSERT_EQ(1U, num_snaps);
 
   rbd_group_snap_info_t snaps[1];
+  ASSERT_EQ(-ENOENT, rbd_group_snap_list(ioctx, "group_does_not_exist", snaps,
+                                         sizeof(rbd_group_snap_info_t),
+                                         &num_snaps));
   ASSERT_EQ(1, rbd_group_snap_list(ioctx, group_name, snaps,
                                    sizeof(rbd_group_snap_info_t),
                                    &num_snaps));
@@ -437,6 +447,8 @@ TEST_F(TestGroup, add_snapshotPP)
   ASSERT_EQ(0, rbd.group_snap_create(ioctx, group_name, snap_name));
 
   std::vector<librbd::group_snap_info_t> snaps;
+  ASSERT_EQ(-ENOENT, rbd.group_snap_list(ioctx, "group_does_not_exist", &snaps,
+                                         sizeof(librbd::group_snap_info_t)));
   ASSERT_EQ(0, rbd.group_snap_list(ioctx, group_name, &snaps,
                                    sizeof(librbd::group_snap_info_t)));
   ASSERT_EQ(1U, snaps.size());

diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py
index 3bcadcb8481c8eacd3ba2d27239179f3a9c5ddee..8c90f0810c058c91472b21b3f7515d396b36068b 100644 (file)
--- a/src/test/pybind/test_rbd.py
+++ b/src/test/pybind/test_rbd.py
@@ -3087,12 +3087,11 @@ class TestGroups(object):
             RBD().remove(ioctx, name)
 
     def test_group_image_add(self):
+        assert_raises(ObjectNotFound, self.dne_group.add_image, ioctx, image_name)
         self.group.add_image(ioctx, image_name)
 
-    def test_group_image_list_empty(self):
-        eq([], list(self.group.list_images()))
-
     def test_group_image_list(self):
+        assert_raises(ObjectNotFound, self.dne_group.list_images)
         eq([], list(self.group.list_images()))
         self.group.add_image(ioctx, image_name)
         eq([image_name], [img['name'] for img in self.group.list_images()])
@@ -3114,6 +3113,7 @@ class TestGroups(object):
         eq([image_name], [img['name'] for img in self.group.list_images()])
 
     def test_group_get_id(self):
+        assert_raises(ObjectNotFound, self.dne_group.id)
         id = self.group.id()
         assert isinstance(id, str)
         assert len(id) > 0
@@ -3142,6 +3142,7 @@ class TestGroups(object):
             eq(group_name, group['name'])
 
         eq([image_name], [img['name'] for img in self.group.list_images()])
+        assert_raises(ObjectNotFound, self.dne_group.remove_image, ioctx, image_name)
         self.group.remove_image(ioctx, image_name)
         eq([], list(self.group.list_images()))
         with Image(ioctx, image_name) as image:
@@ -3157,6 +3158,7 @@ class TestGroups(object):
         assert_raises(ObjectNotFound, self.group.get_snap_info, "")
 
         self.group.create_snap(snap_name)
+        assert_raises(ObjectNotFound, self.dne_group.get_snap_info, snap_name)
         snap_info_dict = self.group.get_snap_info(snap_name)
         image_names = []
         assert sorted(snap_info_dict.keys()) == self.gp_snap_keys
@@ -3194,6 +3196,7 @@ class TestGroups(object):
         global snap_name
         assert_raises(ObjectNotFound, self.dne_group.list_snaps)
         eq([], list(self.group.list_snaps()))
+        assert_raises(ObjectNotFound, self.dne_group.create_snap, snap_name)
         self.group.create_snap(snap_name)
         eq([snap_name], [snap['name'] for snap in self.group.list_snaps()])
 
@@ -3203,6 +3206,7 @@ class TestGroups(object):
             eq(group_name, info['group_name'])
             eq(snap_name, info['group_snap_name'])
 
+        assert_raises(ObjectNotFound, self.dne_group.remove_snap, snap_name)
         self.group.remove_snap(snap_name)
         eq([], list(self.group.list_snaps()))
 
@@ -3270,6 +3274,7 @@ class TestGroups(object):
         eq([], list(self.group.list_snaps()))
         self.group.create_snap(snap_name)
         eq([snap_name], [snap['name'] for snap in self.group.list_snaps()])
+        assert_raises(ObjectNotFound, self.dne_group.rename_snap, snap_name, new_snap_name)
         self.group.rename_snap(snap_name, new_snap_name)
         eq([new_snap_name], [snap['name'] for snap in self.group.list_snaps()])
         self.group.remove_snap(new_snap_name)
@@ -3465,6 +3470,9 @@ class TestGroups(object):
             image_snaps = list(image.list_snaps())
             assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP]
 
+        # no group exists
+        assert_raises(ObjectNotFound, self.dne_group.rollback_to_snap, snap_name1)
+
         # group = []
         assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
         assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)