rbd-mirror: add mirror status removal on ImageReplayer shutdown

In a scenario where you have rbd-mirror daemons on both clusters. The
rbd-mirror daemon on the primary site will not properly cleanup his
status on image removal.

This commit add a path for direct removal at the shut_down of the
ImageReplayer to properly cleanup the metadata.

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@cern.ch>
(cherry picked from commit a538c5d279c90397d375668baddd65776d2462b0)

Conflicts:
        src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc
- Trivial conflict resolution; io_ctx exec has 1 less argument

diff --git a/src/test/rbd_mirror/test_mock_ImageReplayer.cc b/src/test/rbd_mirror/test_mock_ImageReplayer.cc
index 20693e3223b7b35b3b69f8753d8af4a0bca69253..57c2639a00ced2cda881268b60e699e5a1ed2749 100644 (file)
--- a/src/test/rbd_mirror/test_mock_ImageReplayer.cc
+++ b/src/test/rbd_mirror/test_mock_ImageReplayer.cc
@@ -63,7 +63,10 @@ struct MirrorStatusUpdater<librbd::MockTestImageCtx> {
   MOCK_METHOD3(set_mirror_image_status,
                void(const std::string&, const cls::rbd::MirrorImageSiteStatus&,
                     bool));
-  MOCK_METHOD2(remove_mirror_image_status, void(const std::string&, Context*));
+  MOCK_METHOD2(remove_refresh_mirror_image_status, void(const std::string&,
+                                                        Context*));
+  MOCK_METHOD3(remove_mirror_image_status, void(const std::string&, bool,
+                                                Context*));
 };
 
 template <>

diff --git a/src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc b/src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc
index 09805f78976c61f11452aa0ffdf26cf09eccf106..ebc6bd2363dc3fb0f0a218dd4bba3d1057dd6a97 100644 (file)
--- a/src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc
+++ b/src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc
@@ -186,6 +186,38 @@ public:
     }
   }
 
+  void expect_mirror_status_remove(const std::string& global_image_id, int r) {
+    EXPECT_CALL(*m_mock_local_io_ctx,
+                exec(RBD_MIRRORING, _, StrEq("rbd"),
+                     StrEq("mirror_image_status_remove"), _, _, _))
+      .WillOnce(WithArg<4>(Invoke(
+        [r, global_image_id](bufferlist& in_bl) {
+          auto bl_it = in_bl.cbegin();
+          std::string decode_global_image_id;
+          decode(decode_global_image_id, bl_it);
+          EXPECT_EQ(global_image_id, decode_global_image_id);
+
+          return r;
+        })));
+  }
+
+  void expect_mirror_status_removes(const std::set<std::string>& mirror_images,
+                                    int r) {
+    EXPECT_CALL(*m_mock_local_io_ctx, aio_operate(_, _, _, _, _))
+      .WillOnce(Invoke([this](auto&&... args) {
+          int r = m_mock_local_io_ctx->do_aio_operate(decltype(args)(args)...);
+          m_mock_local_io_ctx->aio_flush();
+          return r;
+        }));
+
+    for (auto global_image_id : mirror_images) {
+      expect_mirror_status_remove(global_image_id, r);
+      if (r < 0) {
+        break;
+      }
+    }
+  }
+
   void fire_timer_event(Context** timer_event,
                         Context** update_task) {
     expect_timer_add_event(timer_event);
@@ -385,6 +417,78 @@ TEST_F(TestMockMirrorStatusUpdater, OverwriteStatus) {
                                   *mock_mirror_status_watcher);
 }
 
+TEST_F(TestMockMirrorStatusUpdater, RemoveStatus) {
+  MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
+                                                     m_mock_threads, "");
+  MockMirrorStatusWatcher* mock_mirror_status_watcher =
+    new MockMirrorStatusWatcher();
+
+  InSequence seq;
+
+  Context* timer_event = nullptr;
+  init_mirror_status_updater(mock_mirror_status_updater,
+                             *mock_mirror_status_watcher, &timer_event);
+
+  C_SaferCond ctx;
+  mock_mirror_status_updater.set_mirror_image_status("1", {}, false);
+  expect_work_queue(false);
+  mock_mirror_status_updater.remove_mirror_image_status("1", false, &ctx);
+  ASSERT_EQ(0, ctx.wait());
+
+  Context* update_task = nullptr;
+  fire_timer_event(&timer_event, &update_task);
+
+  C_SaferCond remove_flush_ctx;
+  EXPECT_CALL(*m_mock_local_io_ctx, aio_operate(_, _, _, _, _))
+    .WillOnce(Invoke([this, &remove_flush_ctx](auto&&... args) {
+        int r = m_mock_local_io_ctx->do_aio_operate(decltype(args)(args)...);
+        m_mock_local_io_ctx->aio_flush();
+        remove_flush_ctx.complete(r);
+        return r;
+      }));
+  expect_mirror_status_remove("1", 0);
+  update_task->complete(0);
+  ASSERT_EQ(0, remove_flush_ctx.wait());
+
+  shut_down_mirror_status_updater(mock_mirror_status_updater,
+                                  *mock_mirror_status_watcher);
+}
+
+TEST_F(TestMockMirrorStatusUpdater, OverwriteRemoveStatus) {
+  MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
+                                                     m_mock_threads, "");
+  MockMirrorStatusWatcher* mock_mirror_status_watcher =
+    new MockMirrorStatusWatcher();
+
+  InSequence seq;
+
+  Context* timer_event = nullptr;
+  init_mirror_status_updater(mock_mirror_status_updater,
+                             *mock_mirror_status_watcher, &timer_event);
+
+  C_SaferCond ctx;
+  mock_mirror_status_updater.set_mirror_image_status("1", {}, false);
+  expect_work_queue(false);
+  mock_mirror_status_updater.remove_mirror_image_status("1", false, &ctx);
+  ASSERT_EQ(0, ctx.wait());
+  mock_mirror_status_updater.set_mirror_image_status(
+    "1", {"", cls::rbd::MIRROR_IMAGE_STATUS_STATE_REPLAYING, "description"},
+    false);
+
+
+  Context* update_task = nullptr;
+  fire_timer_event(&timer_event, &update_task);
+
+  expect_mirror_status_update(
+    {{"1", cls::rbd::MirrorImageSiteStatus{
+        "", cls::rbd::MIRROR_IMAGE_STATUS_STATE_REPLAYING, "description"}}},
+    "", 0);
+  update_task->complete(0);
+
+  shut_down_mirror_status_updater(mock_mirror_status_updater,
+                                  *mock_mirror_status_watcher);
+}
+
 TEST_F(TestMockMirrorStatusUpdater, OverwriteStatusInFlight) {
   MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
                                                      m_mock_threads, "");
@@ -447,7 +551,32 @@ TEST_F(TestMockMirrorStatusUpdater, ImmediateUpdate) {
                                   *mock_mirror_status_watcher);
 }
 
-TEST_F(TestMockMirrorStatusUpdater, RemoveIdleStatus) {
+TEST_F(TestMockMirrorStatusUpdater, RemoveImmediateUpdate) {
+  MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
+                                                     m_mock_threads, "");
+  MockMirrorStatusWatcher* mock_mirror_status_watcher =
+    new MockMirrorStatusWatcher();
+
+  InSequence seq;
+
+  Context* timer_event = nullptr;
+  init_mirror_status_updater(mock_mirror_status_updater,
+                             *mock_mirror_status_watcher, &timer_event);
+
+  mock_mirror_status_updater.set_mirror_image_status("1", {}, false);
+
+  C_SaferCond ctx;
+  expect_work_queue(true);
+  expect_work_queue(true);
+  expect_mirror_status_removes({"1"}, 0);
+  mock_mirror_status_updater.remove_mirror_image_status("1", true, &ctx);
+  ASSERT_EQ(0, ctx.wait());
+
+  shut_down_mirror_status_updater(mock_mirror_status_updater,
+                                  *mock_mirror_status_watcher);
+}
+
+TEST_F(TestMockMirrorStatusUpdater, RemoveRefreshIdleStatus) {
   MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
                                                      m_mock_threads, "");
   MockMirrorStatusWatcher* mock_mirror_status_watcher =
@@ -463,14 +592,14 @@ TEST_F(TestMockMirrorStatusUpdater, RemoveIdleStatus) {
 
   C_SaferCond ctx;
   expect_work_queue(true);
-  mock_mirror_status_updater.remove_mirror_image_status("1", &ctx);
+  mock_mirror_status_updater.remove_refresh_mirror_image_status("1", &ctx);
   ASSERT_EQ(0, ctx.wait());
 
   shut_down_mirror_status_updater(mock_mirror_status_updater,
                                   *mock_mirror_status_watcher);
 }
 
-TEST_F(TestMockMirrorStatusUpdater, RemoveInFlightStatus) {
+TEST_F(TestMockMirrorStatusUpdater, RemoveRefreshInFlightStatus) {
   MockMirrorStatusUpdater mock_mirror_status_updater(m_local_io_ctx,
                                                      m_mock_threads, "");
   MockMirrorStatusWatcher* mock_mirror_status_watcher =
@@ -491,7 +620,8 @@ TEST_F(TestMockMirrorStatusUpdater, RemoveInFlightStatus) {
   EXPECT_CALL(*m_mock_local_io_ctx, aio_operate(_, _, _, _, _))
     .WillOnce(Invoke(
       [this, &mock_mirror_status_updater, &on_removed](auto&&... args) {
-        mock_mirror_status_updater.remove_mirror_image_status("1", &on_removed);
+        mock_mirror_status_updater.remove_refresh_mirror_image_status(
+            "1", &on_removed);
 
         int r = m_mock_local_io_ctx->do_aio_operate(decltype(args)(args)...);
         m_mock_local_io_ctx->aio_flush();

diff --git a/src/tools/rbd_mirror/ImageReplayer.cc b/src/tools/rbd_mirror/ImageReplayer.cc
index 74f4d4bf1be2c5cff8b56cd6b25d0152a8e3885a..f045b1bfd9858bfe6b34b9a49549318e5267284a 100644 (file)
--- a/src/tools/rbd_mirror/ImageReplayer.cc
+++ b/src/tools/rbd_mirror/ImageReplayer.cc
@@ -310,6 +310,7 @@ void ImageReplayer<I>::start(Context *on_finish, bool manual, bool restart)
       m_manual_stop = false;
       m_delete_requested = false;
       m_restart_requested = false;
+      m_status_removed = false;
 
       if (on_finish != nullptr) {
         ceph_assert(m_on_start_finish == nullptr);
@@ -928,6 +929,7 @@ void ImageReplayer<I>::handle_shut_down(int r) {
       dout(0) << "remote image no longer exists: scheduling deletion" << dendl;
       unregister_asok_hook = true;
       std::swap(delete_requested, m_delete_requested);
+      m_delete_in_progress = true;
     }
 
     std::swap(resync_requested, m_resync_requested);
@@ -963,23 +965,12 @@ void ImageReplayer<I>::handle_shut_down(int r) {
     return;
   }
 
-  if (m_local_status_updater->exists(m_global_image_id)) {
-    dout(15) << "removing local mirror image status" << dendl;
-    auto ctx = new LambdaContext([this, r](int) {
-        handle_shut_down(r);
-      });
-    m_local_status_updater->remove_mirror_image_status(m_global_image_id, ctx);
-    return;
-  }
-
-  if (m_remote_image_peer.mirror_status_updater != nullptr &&
-      m_remote_image_peer.mirror_status_updater->exists(m_global_image_id)) {
-    dout(15) << "removing remote mirror image status" << dendl;
+  if (!m_status_removed) {
     auto ctx = new LambdaContext([this, r](int) {
-        handle_shut_down(r);
-      });
-    m_remote_image_peer.mirror_status_updater->remove_mirror_image_status(
-      m_global_image_id, ctx);
+      m_status_removed = true;
+      handle_shut_down(r);
+    });
+    remove_image_status(m_delete_in_progress, ctx);
     return;
   }
 
@@ -1135,6 +1126,48 @@ void ImageReplayer<I>::reregister_admin_socket_hook() {
   register_admin_socket_hook();
 }
 
+template <typename I>
+void ImageReplayer<I>::remove_image_status(bool force, Context *on_finish)
+{
+  auto ctx = new LambdaContext([this, force, on_finish](int) {
+    remove_image_status_remote(force, on_finish);
+  });
+
+  if (m_local_status_updater->exists(m_global_image_id)) {
+    dout(15) << "removing local mirror image status" << dendl;
+    if (force) {
+      m_local_status_updater->remove_mirror_image_status(
+        m_global_image_id, true, ctx);
+    } else {
+      m_local_status_updater->remove_refresh_mirror_image_status(
+        m_global_image_id, ctx);
+    }
+    return;
+  }
+
+  ctx->complete(0);
+}
+
+template <typename I>
+void ImageReplayer<I>::remove_image_status_remote(bool force, Context *on_finish)
+{
+  if (m_remote_image_peer.mirror_status_updater != nullptr &&
+      m_remote_image_peer.mirror_status_updater->exists(m_global_image_id)) {
+    dout(15) << "removing remote mirror image status" << dendl;
+    if (force) {
+      m_remote_image_peer.mirror_status_updater->remove_mirror_image_status(
+        m_global_image_id, true, on_finish);
+    } else {
+      m_remote_image_peer.mirror_status_updater->remove_refresh_mirror_image_status(
+        m_global_image_id, on_finish);
+    }
+    return;
+  }
+  if (on_finish) {
+    on_finish->complete(0);
+  }
+}
+
 template <typename I>
 std::ostream &operator<<(std::ostream &os, const ImageReplayer<I> &replayer)
 {

diff --git a/src/tools/rbd_mirror/ImageReplayer.h b/src/tools/rbd_mirror/ImageReplayer.h
index 9569f025bea2c906d8d3c9b5d66034e255a0f3ce..1fbf8db94d3dddb5601a72d16e78eec40488025a 100644 (file)
--- a/src/tools/rbd_mirror/ImageReplayer.h
+++ b/src/tools/rbd_mirror/ImageReplayer.h
@@ -204,10 +204,13 @@ private:
   BootstrapProgressContext m_progress_cxt;
 
   bool m_finished = false;
+  bool m_delete_in_progress = false;
   bool m_delete_requested = false;
   bool m_resync_requested = false;
   bool m_restart_requested = false;
 
+  bool m_status_removed = false;
+
   image_replayer::StateBuilder<ImageCtxT>* m_state_builder = nullptr;
   image_replayer::Replayer* m_replayer = nullptr;
   ReplayerListener* m_replayer_listener = nullptr;
@@ -258,6 +261,8 @@ private:
   void register_admin_socket_hook();
   void unregister_admin_socket_hook();
   void reregister_admin_socket_hook();
+  void remove_image_status(bool force, Context *on_finish);
+  void remove_image_status_remote(bool force, Context *on_finish);
 
 };
 

diff --git a/src/tools/rbd_mirror/MirrorStatusUpdater.cc b/src/tools/rbd_mirror/MirrorStatusUpdater.cc
index 9d27734c3d0262ae14a89a8512fd758bc5de2744..98dc50c2f3c761acf91dbea30a8f8a124923cf45 100644 (file)
--- a/src/tools/rbd_mirror/MirrorStatusUpdater.cc
+++ b/src/tools/rbd_mirror/MirrorStatusUpdater.cc
@@ -189,18 +189,33 @@ void MirrorStatusUpdater<I>::set_mirror_image_status(
   }
 }
 
+template <typename I>
+void MirrorStatusUpdater<I>::remove_refresh_mirror_image_status(
+    const std::string& global_image_id,
+    Context* on_finish) {
+  if (try_remove_mirror_image_status(global_image_id, false, false,
+                                     on_finish)) {
+    m_threads->work_queue->queue(on_finish, 0);
+  }
+}
+
 template <typename I>
 void MirrorStatusUpdater<I>::remove_mirror_image_status(
-    const std::string& global_image_id, Context* on_finish) {
-  if (try_remove_mirror_image_status(global_image_id, on_finish)) {
+    const std::string& global_image_id, bool immediate_update,
+    Context* on_finish) {
+  if (try_remove_mirror_image_status(global_image_id, true, immediate_update,
+                                     on_finish)) {
     m_threads->work_queue->queue(on_finish, 0);
   }
 }
 
 template <typename I>
 bool MirrorStatusUpdater<I>::try_remove_mirror_image_status(
-    const std::string& global_image_id, Context* on_finish) {
-  dout(15) << "global_image_id=" << global_image_id << dendl;
+    const std::string& global_image_id, bool queue_update,
+    bool immediate_update, Context* on_finish) {
+  dout(15) << "global_image_id=" << global_image_id << ", "
+           << "queue_update=" << queue_update << ", "
+           << "immediate_update=" << immediate_update << dendl;
 
   std::unique_lock locker(m_lock);
   if ((m_update_in_flight &&
@@ -209,8 +224,10 @@ bool MirrorStatusUpdater<I>::try_remove_mirror_image_status(
        m_update_global_image_ids.count(global_image_id) > 0)) {
     // if update is scheduled/in-progress, wait for it to complete
     on_finish = new LambdaContext(
-      [this, global_image_id, on_finish](int r) {
-        if (try_remove_mirror_image_status(global_image_id, on_finish)) {
+      [this, global_image_id, queue_update, immediate_update,
+             on_finish](int r) {
+        if (try_remove_mirror_image_status(global_image_id, queue_update,
+                                           immediate_update, on_finish)) {
           on_finish->complete(0);
         }
       });
@@ -219,6 +236,13 @@ bool MirrorStatusUpdater<I>::try_remove_mirror_image_status(
   }
 
   m_global_image_status.erase(global_image_id);
+  if (queue_update) {
+    m_update_global_image_ids.insert(global_image_id);
+    if (immediate_update) {
+      queue_update_task(std::move(locker));
+    }
+  }
+
   return true;
 }
 
@@ -314,6 +338,8 @@ void MirrorStatusUpdater<I>::update_task(int r) {
 
       auto status_it = global_image_status.find(global_image_id);
       if (status_it == global_image_status.end()) {
+        librbd::cls_client::mirror_image_status_remove(&op, global_image_id);
+        ++op_count;
         continue;
       }
 

diff --git a/src/tools/rbd_mirror/MirrorStatusUpdater.h b/src/tools/rbd_mirror/MirrorStatusUpdater.h
index 60ae68ce2c7cd30fb6f254b64f1d30199b091a5d..783b818fc56e84d2ee52f33d478b1371aaa2f1cf 100644 (file)
--- a/src/tools/rbd_mirror/MirrorStatusUpdater.h
+++ b/src/tools/rbd_mirror/MirrorStatusUpdater.h
@@ -44,7 +44,9 @@ public:
       const cls::rbd::MirrorImageSiteStatus& mirror_image_site_status,
       bool immediate_update);
   void remove_mirror_image_status(const std::string& global_image_id,
-                                  Context* on_finish);
+                                  bool immediate_update, Context* on_finish);
+  void remove_refresh_mirror_image_status(const std::string& global_image_id,
+                                          Context* on_finish);
 
 private:
   /**
@@ -90,6 +92,7 @@ private:
   GlobalImageIds m_updating_global_image_ids;
 
   bool try_remove_mirror_image_status(const std::string& global_image_id,
+                                      bool queue_update, bool immediate_update,
                                       Context* on_finish);
 
   void init_mirror_status_watcher(Context* on_finish);