From 5e7b3c3de65f8c8f49b8c47e187c0b6d8d15fd2d Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Tue, 29 Apr 2025 20:12:42 +0200 Subject: [PATCH] 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 --- src/librbd/api/Mirror.cc | 2 +- src/librbd/mirror/GroupGetInfoRequest.cc | 6 ++++-- src/librbd/mirror/GroupGetInfoRequest.h | 1 + .../mirror_group_snapshot_schedule.py | 12 +++++------- src/pybind/mgr/rbd_support/schedule.py | 4 ++-- src/tools/rbd/action/Group.cc | 2 +- src/tools/rbd/action/MirrorGroup.cc | 16 ++++------------ 7 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc index 83e72fa4f4c..567cade3493 100644 --- 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 625b2470f43..166d53c4aaa 100644 --- a/src/librbd/mirror/GroupGetInfoRequest.cc +++ b/src/librbd/mirror/GroupGetInfoRequest.cc @@ -32,6 +32,7 @@ void GroupGetInfoRequest::send() { } if (m_group_id.empty()) { + m_enoent_is_group_dne = true; get_id(); } else { get_info(); @@ -109,8 +110,9 @@ void GroupGetInfoRequest::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 70d86cc60a7..2c6eeb3d2c1 100644 --- 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 4ac01362e5e..f8bff8524f8 100644 --- 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 45032ec07f7..46ea3f98700 100644 --- 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 55f2a0b6ef7..933f808738d 100644 --- 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 4aaeefdb087..fdb22f1ece2 100644 --- 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; } -- 2.39.5