From 616fa0b3985a4f997962b56a0eb18e996c30bbea Mon Sep 17 00:00:00 2001 From: Arthur Outhenin-Chalandre Date: Mon, 7 Jun 2021 14:58:03 +0200 Subject: [PATCH] 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 (cherry picked from commit a538c5d279c90397d375668baddd65776d2462b0) Conflicts: src/test/rbd_mirror/test_mock_MirrorStatusUpdater.cc - Trivial conflict resolution; io_ctx exec has 1 less argument --- .../rbd_mirror/test_mock_ImageReplayer.cc | 5 +- .../test_mock_MirrorStatusUpdater.cc | 138 +++++++++++++++++- src/tools/rbd_mirror/ImageReplayer.cc | 65 +++++++-- src/tools/rbd_mirror/ImageReplayer.h | 5 + src/tools/rbd_mirror/MirrorStatusUpdater.cc | 38 ++++- src/tools/rbd_mirror/MirrorStatusUpdater.h | 5 +- 6 files changed, 228 insertions(+), 28 deletions(-) diff --git a/src/test/rbd_mirror/test_mock_ImageReplayer.cc b/src/test/rbd_mirror/test_mock_ImageReplayer.cc index 20693e3223b7b..57c2639a00ced 100644 --- a/src/test/rbd_mirror/test_mock_ImageReplayer.cc +++ b/src/test/rbd_mirror/test_mock_ImageReplayer.cc @@ -63,7 +63,10 @@ struct MirrorStatusUpdater { 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 09805f78976c6..ebc6bd2363dc3 100644 --- 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& 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 74f4d4bf1be2c..f045b1bfd9858 100644 --- a/src/tools/rbd_mirror/ImageReplayer.cc +++ b/src/tools/rbd_mirror/ImageReplayer.cc @@ -310,6 +310,7 @@ void ImageReplayer::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::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::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::reregister_admin_socket_hook() { register_admin_socket_hook(); } +template +void ImageReplayer::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 +void ImageReplayer::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 std::ostream &operator<<(std::ostream &os, const ImageReplayer &replayer) { diff --git a/src/tools/rbd_mirror/ImageReplayer.h b/src/tools/rbd_mirror/ImageReplayer.h index 9569f025bea2c..1fbf8db94d3dd 100644 --- 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* 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 9d27734c3d026..98dc50c2f3c76 100644 --- a/src/tools/rbd_mirror/MirrorStatusUpdater.cc +++ b/src/tools/rbd_mirror/MirrorStatusUpdater.cc @@ -189,18 +189,33 @@ void MirrorStatusUpdater::set_mirror_image_status( } } +template +void MirrorStatusUpdater::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 void MirrorStatusUpdater::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 bool MirrorStatusUpdater::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::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::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::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 60ae68ce2c7cd..783b818fc56e8 100644 --- 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); -- 2.39.5