From: Arthur Outhenin-Chalandre Date: Fri, 25 Jun 2021 08:15:23 +0000 (+0200) Subject: rbd-mirror: remove mirror image at shut_down when there is no images X-Git-Tag: v15.2.16~33^2~8 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=bc7c434b142178167a4e0617b94a3b479b485887;p=ceph.git rbd-mirror: remove mirror image at shut_down when there is no images Some cases makes the ImageReplayer to be eternally restarted if there is no local and remote images. If both images are absent and that the local image id exists, the ImageReplayer shutdown will request a mirror image removal. Signed-off-by: Arthur Outhenin-Chalandre (cherry picked from commit 0c1c7fb886fcaaff5f00937cf62cf69feb8d4deb) Conflicts: src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc Added a condition to handle the case where m_image_ctx is null on close_image and handle_close_image in the TrashMoveRequest. This fix is not needed in newer versions of Ceph as ImageCtx no longer needs to be destroyed explicitely with a destroy method after Octopus. --- diff --git a/src/test/rbd_mirror/test_mock_ImageReplayer.cc b/src/test/rbd_mirror/test_mock_ImageReplayer.cc index 57c2639a00c..e75fa0ac490 100644 --- a/src/test/rbd_mirror/test_mock_ImageReplayer.cc +++ b/src/test/rbd_mirror/test_mock_ImageReplayer.cc @@ -283,10 +283,11 @@ public: void expect_send(MockBootstrapRequest& mock_bootstrap_request, MockStateBuilder& mock_state_builder, librbd::MockTestImageCtx& mock_local_image_ctx, - bool do_resync, int r) { + bool do_resync, bool set_local_image, int r) { EXPECT_CALL(mock_bootstrap_request, send()) .WillOnce(Invoke([this, &mock_bootstrap_request, &mock_state_builder, - &mock_local_image_ctx, do_resync, r]() { + &mock_local_image_ctx, set_local_image, do_resync, + r]() { if (r == 0 || r == -ENOLINK) { mock_state_builder.local_image_id = mock_local_image_ctx.id; mock_state_builder.remote_image_id = m_remote_image_ctx->id; @@ -296,9 +297,15 @@ public: mock_state_builder.local_image_ctx = &mock_local_image_ctx; *mock_bootstrap_request.do_resync = do_resync; } - if (r < 0) { + if (r < 0 && r != -ENOENT) { mock_state_builder.remote_image_id = ""; } + if (r == -ENOENT) { + *mock_bootstrap_request.state_builder = &mock_state_builder; + } + if (set_local_image) { + mock_state_builder.local_image_id = mock_local_image_ctx.id; + } mock_bootstrap_request.on_finish->complete(r); })); } @@ -405,7 +412,7 @@ TEST_F(TestMockImageReplayer, StartStop) { MockBootstrapRequest mock_bootstrap_request; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, 0); @@ -447,8 +454,42 @@ TEST_F(TestMockImageReplayer, LocalImagePrimary) { MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, -ENOMSG); + false, false, -ENOMSG); + + expect_mirror_image_status_exists(false); + + create_image_replayer(mock_threads); + + C_SaferCond start_ctx; + m_image_replayer->start(&start_ctx); + ASSERT_EQ(0, start_ctx.wait()); +} + +TEST_F(TestMockImageReplayer, MetadataCleanup) { + // START + + create_local_image(); + librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + + MockThreads mock_threads(m_threads); + expect_work_queue_repeatedly(mock_threads); + expect_add_event_after_repeatedly(mock_threads); + + MockImageDeleter mock_image_deleter; + MockBootstrapRequest mock_bootstrap_request; + MockReplayer mock_replayer; + + expect_get_replay_status(mock_replayer); + expect_set_mirror_image_status_repeatedly(); + + InSequence seq; + + MockStateBuilder mock_state_builder; + expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, + false, true, -ENOLINK); + expect_close(mock_state_builder, 0); + expect_trash_move(mock_image_deleter, "global image id", false, 0); expect_mirror_image_status_exists(false); create_image_replayer(mock_threads); @@ -475,7 +516,7 @@ TEST_F(TestMockImageReplayer, BootstrapRemoteDeleted) { MockBootstrapRequest mock_bootstrap_request; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, -ENOLINK); + false, false, -ENOLINK); expect_close(mock_state_builder, 0); @@ -506,7 +547,7 @@ TEST_F(TestMockImageReplayer, BootstrapResyncRequested) { MockBootstrapRequest mock_bootstrap_request; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - true, 0); + true, false, 0); expect_close(mock_state_builder, 0); @@ -536,7 +577,7 @@ TEST_F(TestMockImageReplayer, BootstrapError) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, -EINVAL); + false, false, -EINVAL); expect_mirror_image_status_exists(false); @@ -599,7 +640,7 @@ TEST_F(TestMockImageReplayer, StopError) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, 0); @@ -638,7 +679,7 @@ TEST_F(TestMockImageReplayer, ReplayerError) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, -EINVAL); @@ -675,7 +716,7 @@ TEST_F(TestMockImageReplayer, ReplayerResync) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, 0); @@ -718,7 +759,7 @@ TEST_F(TestMockImageReplayer, ReplayerInterrupted) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, 0); @@ -766,7 +807,7 @@ TEST_F(TestMockImageReplayer, ReplayerRenamed) { InSequence seq; MockStateBuilder mock_state_builder; expect_send(mock_bootstrap_request, mock_state_builder, mock_local_image_ctx, - false, 0); + false, false, 0); expect_create_replayer(mock_state_builder, mock_replayer); expect_init(mock_replayer, 0); diff --git a/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc b/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc index 5a2dc9c7243..4372e5ebe69 100644 --- a/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc +++ b/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc @@ -180,6 +180,13 @@ template void TrashMoveRequest::handle_open_image(int r) { dout(10) << "r=" << r << dendl; + if (r == -ENOENT) { + dout(5) << "mirror image does not exist, removing orphaned metadata" << dendl; + m_image_ctx = nullptr; + remove_mirror_image(); + return; + } + if (r < 0) { derr << "failed to open image: " << cpp_strerror(r) << dendl; m_image_ctx->destroy(); @@ -337,6 +344,10 @@ template void TrashMoveRequest::close_image() { dout(10) << dendl; + if (m_image_ctx == nullptr) { + handle_close_image(0); + return; + } Context *ctx = create_context_callback< TrashMoveRequest, &TrashMoveRequest::handle_close_image>(this); m_image_ctx->state->close(ctx); @@ -346,8 +357,10 @@ template void TrashMoveRequest::handle_close_image(int r) { dout(10) << "r=" << r << dendl; - m_image_ctx->destroy(); - m_image_ctx = nullptr; + if (m_image_ctx != nullptr) { + m_image_ctx->destroy(); + m_image_ctx = nullptr; + } if (r < 0) { derr << "failed to close image: " << cpp_strerror(r) << dendl; diff --git a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc index 7241618be2b..ca77d87f8fa 100644 --- a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc @@ -194,10 +194,10 @@ void BootstrapRequest::handle_prepare_remote_image(int r) { // TODO need to support multiple remote images if (state_builder != nullptr && state_builder->remote_image_id.empty() && - !state_builder->local_image_id.empty() && - state_builder->is_linked()) { - // local image exists and is non-primary and linked to the missing - // remote image + (state_builder->local_image_id.empty() || + state_builder->is_linked())) { + // both images doesn't exist or local image exists and is non-primary + // and linked to the missing remote image finish(-ENOLINK); } else { finish(-ENOENT); diff --git a/src/tools/rbd_mirror/image_replayer/StateBuilder.h b/src/tools/rbd_mirror/image_replayer/StateBuilder.h index d055c84e020..807fe7f9162 100644 --- a/src/tools/rbd_mirror/image_replayer/StateBuilder.h +++ b/src/tools/rbd_mirror/image_replayer/StateBuilder.h @@ -81,13 +81,13 @@ public: std::string global_image_id; - std::string local_image_id; + std::string local_image_id{}; librbd::mirror::PromotionState local_promotion_state = librbd::mirror::PROMOTION_STATE_PRIMARY; ImageCtxT* local_image_ctx = nullptr; std::string remote_mirror_uuid; - std::string remote_image_id; + std::string remote_image_id{}; librbd::mirror::PromotionState remote_promotion_state = librbd::mirror::PROMOTION_STATE_NON_PRIMARY; ImageCtxT* remote_image_ctx = nullptr;