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 <prasanna.kalever@redhat.com>

diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc
index 4d6e2f134a864d4c0ead8cd3c5253cb5686aecf9..9fa41145b14cc57bfb6ae26aebdca723c1fcf801 100644 (file)
--- a/src/librbd/api/Mirror.cc
+++ b/src/librbd/api/Mirror.cc
@@ -3431,6 +3431,21 @@ int Mirror<I>::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<I>::group_image_add(IoCtx &group_ioctx,
 
   std::vector<uint64_t> quiesce_requests;
   std::vector<I *> 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<I>::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 7a14b8b7c95fb9b53b7a2255f0f2badd5b994f21..184ef1a3fd3e2e491a56790a42b1a3aeaf628652 100644 (file)
--- 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 36a7bd54e17bab29213f7b2bf76c099c067406f9..932b9db6e8ba83c39f9606357d4ecbc48d8e0935 100644 (file)
--- a/src/tools/rbd_mirror/ImageMap.cc
+++ b/src/tools/rbd_mirror/ImageMap.cc
@@ -416,7 +416,7 @@ void ImageMap<I>::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<I>::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 2f7676baf1bbdf0a26fac4c913e4d7bf2c785e7f..36589a930dd05cac32b4b5b1aa31dbe3986d647b 100644 (file)
--- a/src/tools/rbd_mirror/ImageReplayer.cc
+++ b/src/tools/rbd_mirror/ImageReplayer.cc
@@ -909,13 +909,12 @@ void ImageReplayer<I>::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 558920d73c531b03b6e5b599ebb002442c99b2b4..ca4a82c46f5df49b448bb1ebe54e06dafb336735 100644 (file)
--- 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 6113a0a1a37ec9d1eb27928d3a50ca40e24fc093..adae79034801dc0eb25f1e9ee952f3a1f4a4d646 100644 (file)
--- a/src/tools/rbd_mirror/group_replayer/Replayer.cc
+++ b/src/tools/rbd_mirror/group_replayer/Replayer.cc
@@ -730,6 +730,7 @@ void Replayer<I>::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<cls::rbd::MirrorSnapshotNamespace>(
       &snap_info.snapshot_namespace);
@@ -779,43 +780,50 @@ void Replayer<I>::mirror_regular_snapshot(
       remote_group_snap_name,
       cls::rbd::GROUP_SNAPSHOT_STATE_INCOMPLETE};
 
-  std::vector<cls::rbd::ImageSnapshotSpec> local_image_snap_specs;
-  local_image_snap_specs = std::vector<cls::rbd::ImageSnapshotSpec>(
-      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<cls::rbd::ImageSnapshotSpec> 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);
+        }
       }
     }
   }