From fcdaace9bd5d45a6241597e0a44bf9cbb8f21480 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Fri, 9 Aug 2024 12:03:12 +0530 Subject: [PATCH] rbd-mirror: fix the below bugs * fix braces in the imageMap update_images_added & update_images_removed * do not allow image add from non-primary * `down+unknown` status shown on querying individual images which are part of group enabled for mirroring * `mirror pool status` shows down+unknown status * fix imageMap being overwritten when multiple images are enabled for mirroring * fix misleading error msg when getting status of a non-mirror enabled group Signed-off-by: Prasanna Kumar Kalever --- src/librbd/api/Mirror.cc | 27 ++++++-- src/tools/rbd/action/MirrorGroup.cc | 7 +- src/tools/rbd_mirror/ImageMap.cc | 4 +- src/tools/rbd_mirror/ImageReplayer.cc | 11 ++- src/tools/rbd_mirror/Types.cc | 4 +- .../rbd_mirror/group_replayer/Replayer.cc | 68 +++++++++++-------- 6 files changed, 71 insertions(+), 50 deletions(-) diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc index 4d6e2f134a864..9fa41145b14cc 100644 --- a/src/librbd/api/Mirror.cc +++ b/src/librbd/api/Mirror.cc @@ -3431,6 +3431,21 @@ int Mirror::group_image_add(IoCtx &group_ioctx, return 0; } + cls::rbd::MirrorSnapshotState state; + int r = get_last_mirror_snapshot_state(group_ioctx, group_id, &state); + if (r == -ENOENT) { + state = cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED; + r = 0; + } + if (r < 0) { + return r; + } + + if (state == cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY) { + lderr(cct) << "group is not primary" << dendl; + return -EBUSY; + } + std::string group_snap_id = librbd::util::generate_image_id(group_ioctx); cls::rbd::GroupSnapshot group_snap{ group_snap_id, @@ -3441,10 +3456,10 @@ int Mirror::group_image_add(IoCtx &group_ioctx, std::vector quiesce_requests; std::vector image_ctxs; - int r = prepare_group_images(group_ioctx, group_id, &image_ctxs, - &group_snap, quiesce_requests, - cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, - flags); + r = prepare_group_images(group_ioctx, group_id, &image_ctxs, + &group_snap, quiesce_requests, + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, + flags); if (r != 0) { return r; } @@ -3896,8 +3911,8 @@ int Mirror::group_get_info(librados::IoCtx& io_ctx, int r = cls_client::dir_get_id(&io_ctx, RBD_GROUP_DIRECTORY, group_name, &group_id); if (r < 0) { - lderr(cct) << "error getting id of group " << group_name << ": " - << cpp_strerror(r) << dendl; + ldout(cct, 20) << "error getting id of group " << group_name << ": " + << cpp_strerror(r) << dendl; return r; } diff --git a/src/tools/rbd/action/MirrorGroup.cc b/src/tools/rbd/action/MirrorGroup.cc index 7a14b8b7c95fb..184ef1a3fd3e2 100644 --- a/src/tools/rbd/action/MirrorGroup.cc +++ b/src/tools/rbd/action/MirrorGroup.cc @@ -64,15 +64,14 @@ int validate_mirroring_enabled(librados::IoCtx io_ctx, librbd::mirror_group_info_t info; int r = rbd.mirror_group_get_info(io_ctx, group_name.c_str(), &info, sizeof(info)); - if (r < 0) { + if (r < 0 && r != -ENOENT) { std::cerr << "rbd: failed to get mirror info for group " << group_name << ": " << cpp_strerror(r) << std::endl; return r; } - if (info.state != RBD_MIRROR_GROUP_ENABLED) { - std::cerr << "rbd: mirroring not enabled for group: " - << group_name << std::endl; + if (r == -ENOENT || info.state != RBD_MIRROR_GROUP_ENABLED) { + std::cerr << "rbd: mirroring not enabled on the group" << std::endl; return -EINVAL; } diff --git a/src/tools/rbd_mirror/ImageMap.cc b/src/tools/rbd_mirror/ImageMap.cc index 36a7bd54e17ba..932b9db6e8ba8 100644 --- a/src/tools/rbd_mirror/ImageMap.cc +++ b/src/tools/rbd_mirror/ImageMap.cc @@ -416,7 +416,7 @@ void ImageMap::update_images_added( const std::string &mirror_uuid, const MirrorEntities &entities) { dout(5) << "mirror_uuid=" << mirror_uuid << ", " - << "entities=[" << entities << "]" << dendl; + << "entities={ " << entities << " }" << dendl; ceph_assert(ceph_mutex_is_locked(m_lock)); for (auto &entity : entities) { @@ -435,7 +435,7 @@ void ImageMap::update_images_removed( const std::string &mirror_uuid, const MirrorEntities &entities) { dout(5) << "mirror_uuid=" << mirror_uuid << ", " - << "entities=[" << entities << "]" << dendl; + << "entities={ " << entities << " }" << dendl; ceph_assert(ceph_mutex_is_locked(m_lock)); Updates to_remove; diff --git a/src/tools/rbd_mirror/ImageReplayer.cc b/src/tools/rbd_mirror/ImageReplayer.cc index 2f7676baf1bbd..36589a930dd05 100644 --- a/src/tools/rbd_mirror/ImageReplayer.cc +++ b/src/tools/rbd_mirror/ImageReplayer.cc @@ -909,13 +909,12 @@ void ImageReplayer::set_mirror_image_status_update( m_local_group_ctx->global_group_id, m_remote_image_peer.io_ctx.get_id(), m_global_image_id, status, force); } - } else { - m_local_status_updater->set_mirror_image_status(m_global_image_id, status, - force); - if (m_remote_image_peer.mirror_status_updater != nullptr) { - m_remote_image_peer.mirror_status_updater->set_mirror_image_status( + } + m_local_status_updater->set_mirror_image_status(m_global_image_id, status, + force); + if (m_remote_image_peer.mirror_status_updater != nullptr) { + m_remote_image_peer.mirror_status_updater->set_mirror_image_status( m_global_image_id, status, force); - } } m_in_flight_op_tracker.finish_op(); diff --git a/src/tools/rbd_mirror/Types.cc b/src/tools/rbd_mirror/Types.cc index 558920d73c531..ca4a82c46f5df 100644 --- a/src/tools/rbd_mirror/Types.cc +++ b/src/tools/rbd_mirror/Types.cc @@ -23,8 +23,8 @@ std::ostream &operator<<(std::ostream &os, const MirrorEntityType &type) { } std::ostream &operator<<(std::ostream &os, const MirrorEntity &entity) { - return os << "type=" << entity.type << ", global_id=" << entity.global_id - << ", count=" << entity.count; + return os << "[type=" << entity.type << ", global_id=" << entity.global_id + << ", count=" << entity.count << "]"; } std::ostream& operator<<(std::ostream& os, diff --git a/src/tools/rbd_mirror/group_replayer/Replayer.cc b/src/tools/rbd_mirror/group_replayer/Replayer.cc index 6113a0a1a37ec..adae79034801d 100644 --- a/src/tools/rbd_mirror/group_replayer/Replayer.cc +++ b/src/tools/rbd_mirror/group_replayer/Replayer.cc @@ -730,6 +730,7 @@ void Replayer::update_image_snapshot( if (r < 0) { derr << "failed getting snap info for snap id: " << spec.snap_id << ", : " << cpp_strerror(r) << dendl; + return; } auto mirror_ns = std::get_if( &snap_info.snapshot_namespace); @@ -779,43 +780,50 @@ void Replayer::mirror_regular_snapshot( remote_group_snap_name, cls::rbd::GROUP_SNAPSHOT_STATE_INCOMPLETE}; - std::vector local_image_snap_specs; - local_image_snap_specs = std::vector( - local_images->size(), cls::rbd::ImageSnapshotSpec()); - for (auto& image : *local_images) { - std::string image_header_oid = librbd::util::header_name( - image.spec.image_id); - ::SnapContext snapc; - int r = librbd::cls_client::get_snapcontext(&m_local_io_ctx, - image_header_oid, &snapc); - if (r < 0) { - derr << "get snap context failed: " << cpp_strerror(r) << dendl; - on_finish->complete(r); - return; - } + auto itr = std::find_if( + m_remote_group_snaps.begin(), m_remote_group_snaps.end(), + [remote_group_snap_id](const cls::rbd::GroupSnapshot &s) { + return s.id == remote_group_snap_id; + }); - auto image_snap_name = ".group." + std::to_string(image.spec.pool_id) + - "_" + m_remote_group_id + "_" + remote_group_snap_id; - // stored in reverse order - for (auto snap_id : snapc.snaps) { - cls::rbd::SnapshotInfo snap_info; - r = librbd::cls_client::snapshot_get(&m_local_io_ctx, image_header_oid, - snap_id, &snap_info); + std::vector local_image_snap_specs; + if (itr != m_remote_group_snaps.end()) { + local_image_snap_specs.reserve(itr->snaps.size()); + for (auto& image : *local_images) { + std::string image_header_oid = librbd::util::header_name( + image.spec.image_id); + ::SnapContext snapc; + int r = librbd::cls_client::get_snapcontext(&m_local_io_ctx, + image_header_oid, &snapc); if (r < 0) { - derr << "failed getting snap info for snap id: " << snap_id - << ", : " << cpp_strerror(r) << dendl; + derr << "get snap context failed: " << cpp_strerror(r) << dendl; on_finish->complete(r); return; } - // extract { pool_id, snap_id, image_id } - if (snap_info.name == image_snap_name) { - cls::rbd::ImageSnapshotSpec snap_spec; - snap_spec.pool = image.spec.pool_id; - snap_spec.image_id = image.spec.image_id; - snap_spec.snap_id = snap_info.id; + auto image_snap_name = ".group." + std::to_string(image.spec.pool_id) + + "_" + m_remote_group_id + "_" + remote_group_snap_id; + // stored in reverse order + for (auto snap_id : snapc.snaps) { + cls::rbd::SnapshotInfo snap_info; + r = librbd::cls_client::snapshot_get(&m_local_io_ctx, image_header_oid, + snap_id, &snap_info); + if (r < 0) { + derr << "failed getting snap info for snap id: " << snap_id + << ", : " << cpp_strerror(r) << dendl; + on_finish->complete(r); + return; + } + + // extract { pool_id, snap_id, image_id } + if (snap_info.name == image_snap_name) { + cls::rbd::ImageSnapshotSpec snap_spec; + snap_spec.pool = image.spec.pool_id; + snap_spec.image_id = image.spec.image_id; + snap_spec.snap_id = snap_info.id; - local_image_snap_specs.push_back(snap_spec); + local_image_snap_specs.push_back(snap_spec); + } } } } -- 2.39.5