librbd: let Mirror::group_get_info() return ENOENT only if group DNE

Getting ENOENT when the group doesn't exist is very much desired,
especially considering the fact that groups can't be opened like images
are and that group APIs operate in terms of group names.  However, in
case the group exists but mirroring is disabled, it's better to remain
consistent with Mirror::image_get_info() and return a partially formed
struct where state is set to RBD_MIRROR_GROUP_DISABLED -- this is the
behavior that API users have grown to rely on.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc
index 83e72fa4f4c7625f44dd4a00452e44ed4d0416b1..567cade34936c2c3923f2b86ae285d5a0cce1f0a 100644 (file)
--- a/src/librbd/api/Mirror.cc
+++ b/src/librbd/api/Mirror.cc
@@ -604,7 +604,7 @@ struct C_GroupSnapshotCreate : public Context {
   }
 
   void finish(int r) override {
-    if (r < 0 && r != -ENOENT) {
+    if (r < 0) {
       on_finish->complete(r);
       return;
     }

diff --git a/src/librbd/mirror/GroupGetInfoRequest.cc b/src/librbd/mirror/GroupGetInfoRequest.cc
index 625b2470f4360f6b2b739eaddfde9282d680111d..166d53c4aaa0c551db0a77af2cb9dbdb46e27f0b 100644 (file)
--- a/src/librbd/mirror/GroupGetInfoRequest.cc
+++ b/src/librbd/mirror/GroupGetInfoRequest.cc
@@ -32,6 +32,7 @@ void GroupGetInfoRequest<I>::send() {
   }
 
   if (m_group_id.empty()) {
+    m_enoent_is_group_dne = true;
     get_id();
   } else {
     get_info();
@@ -109,8 +110,9 @@ void GroupGetInfoRequest<I>::handle_get_info(int r) {
   }
 
   if (r == -ENOENT) {
-    ldout(cct, 20) << "mirroring is disabled" << dendl;
-    finish(r);
+    ldout(cct, 20) << "mirroring is disabled, enoent_is_group_dne="
+                   << m_enoent_is_group_dne << dendl;
+    finish(m_enoent_is_group_dne ? 0 : -ENOENT);
     return;
   }
   if (r < 0) {

diff --git a/src/librbd/mirror/GroupGetInfoRequest.h b/src/librbd/mirror/GroupGetInfoRequest.h
index 70d86cc60a7b1a2c594fb4ceb16e817d72e0055a..2c6eeb3d2c18592f6d149f9a977f616d1f258657 100644 (file)
--- a/src/librbd/mirror/GroupGetInfoRequest.h
+++ b/src/librbd/mirror/GroupGetInfoRequest.h
@@ -75,6 +75,7 @@ private:
   Context *m_on_finish;
 
   bufferlist m_outbl;
+  bool m_enoent_is_group_dne = false;
 
   void get_id();
   void handle_get_id(int r);

diff --git a/src/pybind/mgr/rbd_support/mirror_group_snapshot_schedule.py b/src/pybind/mgr/rbd_support/mirror_group_snapshot_schedule.py
index 4ac01362e5eee28bac46aafa630c73dd42c90f17..f8bff8524f8e0d6f510497589e13ea090434d7df 100644 (file)
--- a/src/pybind/mgr/rbd_support/mirror_group_snapshot_schedule.py
+++ b/src/pybind/mgr/rbd_support/mirror_group_snapshot_schedule.py
@@ -20,13 +20,11 @@ def namespace_validator(ioctx: rados.Ioctx) -> None:
 
 
 def group_validator(group: rbd.Group) -> None:
-    try:
-        info = group.mirror_group_get_info()
-    except rbd.ObjectNotFound:
-        raise rbd.InvalidArgument("Error getting mirror group info")
-    if (info['state'] != rbd.RBD_MIRROR_GROUP_ENABLED
-            or info['image_mode'] != rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT):
-        raise rbd.InvalidArgument("Group not enabled for snapshot mirroring")
+    info = group.mirror_group_get_info()
+    if info['state'] != rbd.RBD_MIRROR_GROUP_ENABLED:
+        raise rbd.InvalidArgument("Mirroring is not enabled")
+    if info['image_mode'] != rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT:
+        raise rbd.InvalidArgument("Invalid mirror image mode")
 
 
 class GroupSpec(NamedTuple):

diff --git a/src/pybind/mgr/rbd_support/schedule.py b/src/pybind/mgr/rbd_support/schedule.py
index 45032ec07f72ebfd170077343ae442911b1b0db5..46ea3f987004fffad15e0c3430ba97e707e80ec4 100644 (file)
--- a/src/pybind/mgr/rbd_support/schedule.py
+++ b/src/pybind/mgr/rbd_support/schedule.py
@@ -168,8 +168,8 @@ class LevelSpec:
                                         group_name))
                                 except rbd.InvalidArgument:
                                     raise ValueError(
-                                        "group {} not enabled for snapshot mirroring".format(
-                                            group_name))
+                                        "group {} is not in snapshot mirror mode".format(
+                                            group_id))
                             else:
                                 image_name = match.group(3)
                                 try:

diff --git a/src/tools/rbd/action/Group.cc b/src/tools/rbd/action/Group.cc
index 55f2a0b6ef7034a94cdc7eef6d1d88961bc33c32..933f808738dd589c4ce67b4bf36d58732080e07f 100644 (file)
--- a/src/tools/rbd/action/Group.cc
+++ b/src/tools/rbd/action/Group.cc
@@ -315,7 +315,7 @@ int execute_info(const po::variables_map &vm,
   mirror_group_info.state = RBD_MIRROR_GROUP_DISABLED;
   r = rbd.mirror_group_get_info(io_ctx, group_name.c_str(), &mirror_group_info,
                                 sizeof(mirror_group_info));
-  if (r < 0 && r != -ENOENT) {
+  if (r < 0) {
     return r;
   }
 

diff --git a/src/tools/rbd/action/MirrorGroup.cc b/src/tools/rbd/action/MirrorGroup.cc
index 4aaeefdb087c071ea87de4f9ee337fe3f859ef9a..fdb22f1ece2c707c2af836a2eb521ae8bf7d7e36 100644 (file)
--- a/src/tools/rbd/action/MirrorGroup.cc
+++ b/src/tools/rbd/action/MirrorGroup.cc
@@ -61,24 +61,16 @@ namespace {
 int validate_mirroring_enabled(librados::IoCtx io_ctx,
                                const std::string group_name) {
   librbd::RBD rbd;
-  std::string group_id;
-  int r = rbd.group_get_id(io_ctx, group_name.c_str(), &group_id);
-  if (r < 0) {
-    std::cerr << "rbd: failed to get mirror info for group: "
-              << cpp_strerror(r) << std::endl;
-    return r;
-  }
-
   librbd::mirror_group_info_t info;
-  r = rbd.mirror_group_get_info(io_ctx, group_name.c_str(), &info,
-                                sizeof(info));
-  if (r < 0 && r != -ENOENT) {
+  int r = rbd.mirror_group_get_info(io_ctx, group_name.c_str(), &info,
+                                    sizeof(info));
+  if (r < 0) {
     std::cerr << "rbd: failed to get mirror info for group: "
               << cpp_strerror(r) << std::endl;
     return r;
   }
 
-  if (r == -ENOENT || info.state != RBD_MIRROR_GROUP_ENABLED) {
+  if (info.state != RBD_MIRROR_GROUP_ENABLED) {
     std::cerr << "rbd: mirroring not enabled on the group" << std::endl;
     return -EINVAL;
   }