From 73f1292f5c64df819c38b6a8254849b263769c54 Mon Sep 17 00:00:00 2001 From: Vinay Bhaskar Varada Date: Wed, 19 Feb 2025 13:54:15 +0530 Subject: [PATCH] rbd-mirror: fix crashes in unittest_rbd_mirror Signed-off-by: Vinay Bhaskar Varada --- .../snapshot/test_mock_Replayer.cc | 7 ++--- .../rbd_mirror/test_mock_ImageReplayer.cc | 3 ++ .../rbd_mirror/test_mock_InstanceReplayer.cc | 9 ++++-- src/tools/rbd_mirror/ImageMap.cc | 12 +++++-- src/tools/rbd_mirror/ImageMap.h | 1 + src/tools/rbd_mirror/ImageReplayer.cc | 31 +++---------------- src/tools/rbd_mirror/InstanceReplayer.cc | 8 ++--- src/tools/rbd_mirror/InstanceReplayer.h | 1 + 8 files changed, 33 insertions(+), 39 deletions(-) diff --git a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc index c4de9c50214b5..49b1399642107 100644 --- a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc +++ b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc @@ -158,15 +158,14 @@ struct CreateNonPrimaryRequest { static CreateNonPrimaryRequest* s_instance; static CreateNonPrimaryRequest* create(MockTestImageCtx *image_ctx, bool demoted, + const std::string group_id, + const std::string group_snap_id, const std::string &primary_mirror_uuid, uint64_t primary_snap_id, const SnapSeqs& snap_seqs, - uint64_t local_group_pool_id, - const std::string &local_group_id, - const std::string &local_group_snap_id, const ImageState &image_state, uint64_t *snap_id, - Context *on_finish) { + Context *on_finish){ ceph_assert(s_instance != nullptr); s_instance->demoted = demoted; s_instance->primary_mirror_uuid = primary_mirror_uuid; diff --git a/src/test/rbd_mirror/test_mock_ImageReplayer.cc b/src/test/rbd_mirror/test_mock_ImageReplayer.cc index 0a0f2245477c3..7b8c17c6c7b11 100644 --- a/src/test/rbd_mirror/test_mock_ImageReplayer.cc +++ b/src/test/rbd_mirror/test_mock_ImageReplayer.cc @@ -177,6 +177,9 @@ struct MockReplayer : public Replayer { MOCK_CONST_METHOD0(is_resync_requested, bool()); MOCK_CONST_METHOD0(get_error_code, int()); MOCK_CONST_METHOD0(get_error_description, std::string()); + MOCK_METHOD1(prune_snapshot, void(uint64_t snapshot_id)); + MOCK_METHOD1(set_remote_snap_id_end_limit, void(uint64_t snapshot_id)); + MOCK_METHOD0(get_remote_snap_id_end_limit, uint64_t()); }; template <> diff --git a/src/test/rbd_mirror/test_mock_InstanceReplayer.cc b/src/test/rbd_mirror/test_mock_InstanceReplayer.cc index d5fae11501b39..57138c56942fc 100644 --- a/src/test/rbd_mirror/test_mock_InstanceReplayer.cc +++ b/src/test/rbd_mirror/test_mock_InstanceReplayer.cc @@ -89,7 +89,7 @@ struct GroupReplayer { } MOCK_METHOD0(destroy, void()); - MOCK_METHOD4(start, void(Context *, bool, bool, bool)); + MOCK_METHOD3(start, void(Context *, bool, bool)); MOCK_METHOD2(stop, void(Context *, bool)); MOCK_METHOD2(restart, void(Context*, bool)); MOCK_METHOD0(flush, void()); @@ -197,7 +197,8 @@ public: void expect_work_queue(MockThreads &mock_threads) { EXPECT_CALL(*mock_threads.work_queue, queue(_, _)) - .WillOnce(Invoke([this](Context *ctx, int r) { + .Times(testing::AtLeast(1)) + .WillRepeatedly(Invoke([this](Context *ctx, int r) { m_threads->work_queue->queue(ctx, r); })); } @@ -224,7 +225,8 @@ public: void expect_cancel_event(MockThreads &mock_threads, bool canceled) { EXPECT_CALL(*mock_threads.timer, cancel_event(_)) - .WillOnce(Return(canceled)); + .Times(testing::AtLeast(1)) + .WillRepeatedly(Return(canceled)); } }; @@ -434,6 +436,7 @@ TEST_F(TestMockInstanceReplayer, Reacquire) { expect_work_queue(mock_threads); expect_cancel_event(mock_threads, true); + expect_work_queue(mock_threads); EXPECT_CALL(mock_image_replayer, is_stopped()).WillOnce(Return(true)); expect_work_queue(mock_threads); expect_work_queue(mock_threads); diff --git a/src/tools/rbd_mirror/ImageMap.cc b/src/tools/rbd_mirror/ImageMap.cc index 932b9db6e8ba8..9ec991a6d9f67 100644 --- a/src/tools/rbd_mirror/ImageMap.cc +++ b/src/tools/rbd_mirror/ImageMap.cc @@ -211,6 +211,14 @@ void ImageMap::schedule_update_task() { schedule_update_task(m_threads->timer_lock, after); } +template +void ImageMap::schedule_update_task(const ceph::mutex &timer_lock) { + ceph_assert(ceph_mutex_is_locked(m_threads->timer_lock)); + CephContext *cct = reinterpret_cast(m_ioctx.cct()); + double after = cct->_conf.get_val("rbd_mirror_image_policy_update_throttle_interval"); + schedule_update_task(m_threads->timer_lock, after); +} + template void ImageMap::schedule_update_task(const ceph::mutex &timer_lock, double after) { @@ -259,7 +267,7 @@ void ImageMap::rebalance() { } } - schedule_update_task(); + schedule_update_task(m_threads->timer_lock); } template @@ -422,7 +430,7 @@ void ImageMap::update_images_added( for (auto &entity : entities) { auto global_id = GlobalId(entity.type, entity.global_id); auto result = m_peer_map[global_id].insert(mirror_uuid); - if ((result.second || entity.type == MIRROR_ENTITY_TYPE_GROUP)) { + if ((result.second && m_peer_map[global_id].size() == 1) || entity.type == MIRROR_ENTITY_TYPE_GROUP) { if (m_policy->add_entity(global_id, entity.count)) { schedule_action(global_id); } diff --git a/src/tools/rbd_mirror/ImageMap.h b/src/tools/rbd_mirror/ImageMap.h index aac7299ef3b37..1cd99cc49de2a 100644 --- a/src/tools/rbd_mirror/ImageMap.h +++ b/src/tools/rbd_mirror/ImageMap.h @@ -148,6 +148,7 @@ private: void schedule_action(const image_map::GlobalId &global_id); void schedule_update_task(); + void schedule_update_task(const ceph::mutex &timer_lock); void schedule_update_task(const ceph::mutex &timer_lock, double after); void process_updates(); void update_image_mapping(Updates&& map_updates, diff --git a/src/tools/rbd_mirror/ImageReplayer.cc b/src/tools/rbd_mirror/ImageReplayer.cc index 9ec215145bcd1..3c8317ac9a1fa 100644 --- a/src/tools/rbd_mirror/ImageReplayer.cc +++ b/src/tools/rbd_mirror/ImageReplayer.cc @@ -1025,14 +1025,6 @@ void ImageReplayer::handle_shut_down(int r) { return; } - if (!m_status_removed) { - auto ctx = new LambdaContext([this, r](int) { - m_status_removed = true; - handle_shut_down(r); - }); - remove_image_status(m_delete_in_progress, ctx); - return; - } if (m_local_group_ctx != nullptr) { if (m_local_status_updater->mirror_group_image_exists( @@ -1062,25 +1054,12 @@ void ImageReplayer::handle_shut_down(int r) { return; } } else { - if (m_local_status_updater->mirror_image_exists(m_global_image_id)) { - dout(15) << "removing local mirror image status" << dendl; + if (!m_status_removed) { auto ctx = new LambdaContext([this, r](int) { - handle_shut_down(r); - }); - m_local_status_updater->remove_mirror_image_status(m_global_image_id, - true, ctx); - return; - } - - if (m_remote_image_peer.mirror_status_updater != nullptr && - m_remote_image_peer.mirror_status_updater->mirror_image_exists( - m_global_image_id)) { - dout(15) << "removing remote mirror image status" << dendl; - 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, true, ctx); + m_status_removed = true; + handle_shut_down(r); + }); + remove_image_status(m_delete_in_progress, ctx); return; } } diff --git a/src/tools/rbd_mirror/InstanceReplayer.cc b/src/tools/rbd_mirror/InstanceReplayer.cc index ad49a35a57f6e..01e61a09e43a2 100644 --- a/src/tools/rbd_mirror/InstanceReplayer.cc +++ b/src/tools/rbd_mirror/InstanceReplayer.cc @@ -607,9 +607,9 @@ void InstanceReplayer::handle_stop_image_replayers(int r) { } m_image_replayers.clear(); - std::swap(on_finish, m_on_shut_down); } - if (on_finish) { + if (--m_shutdown_counter == 0 ) { + std::swap(on_finish, m_on_shut_down); on_finish->complete(r); } } @@ -803,9 +803,9 @@ void InstanceReplayer::handle_stop_group_replayers(int r) { } m_group_replayers.clear(); - std::swap(on_finish, m_on_shut_down); } - if (on_finish) { + if (--m_shutdown_counter == 0) { + std::swap(on_finish, m_on_shut_down); on_finish->complete(r); } } diff --git a/src/tools/rbd_mirror/InstanceReplayer.h b/src/tools/rbd_mirror/InstanceReplayer.h index f83a2ba688f8a..93907e0a1034f 100644 --- a/src/tools/rbd_mirror/InstanceReplayer.h +++ b/src/tools/rbd_mirror/InstanceReplayer.h @@ -125,6 +125,7 @@ private: Context *m_image_state_check_task = nullptr; Context *m_group_state_check_task = nullptr; Context *m_on_shut_down = nullptr; + std::atomic m_shutdown_counter{2}; bool m_manual_stop = false; bool m_blocklisted = false; -- 2.39.5