From d634a1df5b19d61955f2f94c7cc29bd4f3b678c8 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Fri, 21 Jan 2022 13:41:46 +0100 Subject: [PATCH] rbd-mirror: fix races in snapshot-based mirroring deletion propagation When remote image is deleted, rbd-mirror can encounter three cases: 1) no remote image id 2) no remote mirror metadata 3) MIRROR_IMAGE_STATE_DISABLING in remote mirror metadata Commit d4c66ac5c615 ("rbd-mirror: fix issue with snapshot-based mirroring deletion propagation") fixed case 1. Cases 2 and 3 remained broken because for both of them finalize_snapshot_state_builder() would populate not only remote_mirror_peer_uuid but also remote_image_id, thus disabling ENOLINK logic in handle_prepare_remote_image() and handle_bootstrap(). Commit ff60aec2d9ef ("rbd-mirror: fix bootstrap sequence while the image is removed") touched on case 3, but it made a difference only for journal-based mirroring. Stop calling finalize_snapshot_state_builder() on errors. Instead, align with journal-based mirroring by filling remote_mirror_peer_uuid together with remote_mirror_uuid. Fixes: https://tracker.ceph.com/issues/53963 Signed-off-by: Ilya Dryomov --- .../test_mock_PrepareRemoteImageRequest.cc | 223 ++++++++++++++++++ .../PrepareRemoteImageRequest.cc | 19 +- .../PrepareRemoteImageRequest.h | 2 +- 3 files changed, 232 insertions(+), 12 deletions(-) diff --git a/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc index e49b40821821a..2c4b550a6a44b 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc @@ -586,6 +586,229 @@ TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, RegisterClientError) { ASSERT_EQ(-EINVAL, ctx.wait()); } +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorImageIdDNEJournal) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, "", -ENOENT); + + MockJournalStateBuilder mock_journal_state_builder; + MockStateBuilder* mock_state_builder = &mock_journal_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", ""}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL, + mock_journal_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_journal_state_builder.remote_mirror_uuid); + ASSERT_EQ("", mock_journal_state_builder.remote_image_id); +} + +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorImageIdDNESnapshot) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, "", -ENOENT); + + MockSnapshotStateBuilder mock_snapshot_state_builder; + MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", + "remote mirror peer uuid"}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, + mock_snapshot_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_snapshot_state_builder.remote_mirror_uuid); + ASSERT_EQ("remote mirror peer uuid", + mock_snapshot_state_builder.remote_mirror_peer_uuid); + ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id); +} + +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDNEJournal) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, + "remote image id", 0); + + MockGetMirrorInfoRequest mock_get_mirror_info_request; + expect_get_mirror_info(mock_get_mirror_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_JOURNAL, + "global image id", + cls::rbd::MIRROR_IMAGE_STATE_ENABLED}, + librbd::mirror::PROMOTION_STATE_PRIMARY, + "remote mirror uuid", -ENOENT); + + MockJournalStateBuilder mock_journal_state_builder; + MockStateBuilder* mock_state_builder = &mock_journal_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", ""}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL, + mock_journal_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_journal_state_builder.remote_mirror_uuid); + ASSERT_EQ("", mock_journal_state_builder.remote_image_id); +} + +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDNESnapshot) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, + "remote image id", 0); + + MockGetMirrorInfoRequest mock_get_mirror_info_request; + expect_get_mirror_info(mock_get_mirror_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, + "global image id", + cls::rbd::MIRROR_IMAGE_STATE_ENABLED}, + librbd::mirror::PROMOTION_STATE_PRIMARY, + "remote mirror uuid", -ENOENT); + + MockSnapshotStateBuilder mock_snapshot_state_builder; + MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", + "remote mirror peer uuid"}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, + mock_snapshot_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_snapshot_state_builder.remote_mirror_uuid); + ASSERT_EQ("remote mirror peer uuid", + mock_snapshot_state_builder.remote_mirror_peer_uuid); + ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id); +} + +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDisablingJournal) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, + "remote image id", 0); + + MockGetMirrorInfoRequest mock_get_mirror_info_request; + expect_get_mirror_info(mock_get_mirror_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_JOURNAL, + "global image id", + cls::rbd::MIRROR_IMAGE_STATE_DISABLING}, + librbd::mirror::PROMOTION_STATE_PRIMARY, + "remote mirror uuid", 0); + + MockJournalStateBuilder mock_journal_state_builder; + expect_get_mirror_image_mode(mock_journal_state_builder, + cls::rbd::MIRROR_IMAGE_MODE_JOURNAL); + MockStateBuilder* mock_state_builder = &mock_journal_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", ""}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL, + mock_journal_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_journal_state_builder.remote_mirror_uuid); + ASSERT_EQ("", mock_journal_state_builder.remote_image_id); +} + +TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDisablingSnapshot) { + MockThreads mock_threads(m_threads); + + InSequence seq; + MockGetMirrorImageIdRequest mock_get_mirror_image_id_request; + expect_get_mirror_image_id(mock_get_mirror_image_id_request, + "remote image id", 0); + + MockGetMirrorInfoRequest mock_get_mirror_info_request; + expect_get_mirror_info(mock_get_mirror_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, + "global image id", + cls::rbd::MIRROR_IMAGE_STATE_DISABLING}, + librbd::mirror::PROMOTION_STATE_PRIMARY, + "remote mirror uuid", 0); + + MockSnapshotStateBuilder mock_snapshot_state_builder; + expect_get_mirror_image_mode(mock_snapshot_state_builder, + cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT); + MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder; + C_SaferCond ctx; + auto req = MockPrepareRemoteImageRequest::create(&mock_threads, + m_local_io_ctx, + m_remote_io_ctx, + "global image id", + "local mirror uuid", + {"remote mirror uuid", + "remote mirror peer uuid"}, + nullptr, + &mock_state_builder, + &ctx); + req->send(); + + ASSERT_EQ(-ENOENT, ctx.wait()); + ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, + mock_snapshot_state_builder.mirror_image_mode); + ASSERT_EQ("remote mirror uuid", + mock_snapshot_state_builder.remote_mirror_uuid); + ASSERT_EQ("remote mirror peer uuid", + mock_snapshot_state_builder.remote_mirror_peer_uuid); + ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id); +} + } // namespace image_replayer } // namespace mirror } // namespace rbd diff --git a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc index 08d3dd7806fc3..b1e37ed084b80 100644 --- a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc @@ -38,6 +38,10 @@ template void PrepareRemoteImageRequest::send() { if (*m_state_builder != nullptr) { (*m_state_builder)->remote_mirror_uuid = m_remote_pool_meta.mirror_uuid; + auto state_builder = dynamic_cast*>(*m_state_builder); + if (state_builder) { + state_builder->remote_mirror_peer_uuid = m_remote_pool_meta.mirror_peer_uuid; + } } get_remote_image_id(); @@ -62,7 +66,6 @@ void PrepareRemoteImageRequest::handle_get_remote_image_id(int r) { << "remote_image_id=" << m_remote_image_id << dendl; if (r == -ENOENT) { - finalize_snapshot_state_builder(r); finish(r); return; } else if (r < 0) { @@ -93,7 +96,6 @@ void PrepareRemoteImageRequest::handle_get_mirror_info(int r) { if (r == -ENOENT) { dout(10) << "image " << m_global_image_id << " not mirrored" << dendl; - finalize_snapshot_state_builder(r); finish(r); return; } else if (r < 0) { @@ -130,7 +132,7 @@ void PrepareRemoteImageRequest::handle_get_mirror_info(int r) { get_client(); break; case cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT: - finalize_snapshot_state_builder(0); + finalize_snapshot_state_builder(); finish(0); break; default: @@ -252,26 +254,21 @@ void PrepareRemoteImageRequest::finalize_journal_state_builder( } template -void PrepareRemoteImageRequest::finalize_snapshot_state_builder(int r) { +void PrepareRemoteImageRequest::finalize_snapshot_state_builder() { snapshot::StateBuilder* state_builder = nullptr; if (*m_state_builder != nullptr) { state_builder = dynamic_cast*>(*m_state_builder); - } else if (r >= 0) { + ceph_assert(state_builder != nullptr); + } else { state_builder = snapshot::StateBuilder::create(m_global_image_id); *m_state_builder = state_builder; } - if (state_builder == nullptr) { - // local image prepare failed or is using journal-based mirroring - return; - } - dout(10) << "remote_mirror_uuid=" << m_remote_pool_meta.mirror_uuid << ", " << "remote_mirror_peer_uuid=" << m_remote_pool_meta.mirror_peer_uuid << ", " << "remote_image_id=" << m_remote_image_id << ", " << "remote_promotion_state=" << m_promotion_state << dendl; - ceph_assert(state_builder != nullptr); state_builder->remote_mirror_uuid = m_remote_pool_meta.mirror_uuid; state_builder->remote_mirror_peer_uuid = m_remote_pool_meta.mirror_peer_uuid; state_builder->remote_image_id = m_remote_image_id; diff --git a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h index 0d0129d55826b..483cfc00195f9 100644 --- a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h +++ b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h @@ -139,7 +139,7 @@ private: void finalize_journal_state_builder(cls::journal::ClientState client_state, const MirrorPeerClientMeta& client_meta); - void finalize_snapshot_state_builder(int r); + void finalize_snapshot_state_builder(); void finish(int r); }; -- 2.39.5