From: Jason Dillaman Date: Mon, 9 Mar 2020 22:32:03 +0000 (-0400) Subject: librbd: re-use mirror promote state machine when disabling X-Git-Tag: v15.1.1~31^2~9 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=672968f9c753f5a6f0e85ea872ddad639b6c01f3;p=ceph.git librbd: re-use mirror promote state machine when disabling The promote state machine will handle remove the non-primary feature bit and will ensure an interrupted disable operation doesn't leave things in an inconsistent state. Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/mirror/DisableRequest.cc b/src/librbd/mirror/DisableRequest.cc index 4a7d914fce4d..2d2dd874a80a 100644 --- a/src/librbd/mirror/DisableRequest.cc +++ b/src/librbd/mirror/DisableRequest.cc @@ -8,6 +8,7 @@ #include "cls/journal/cls_journal_client.h" #include "journal/Journaler.h" #include "librbd/ImageCtx.h" +#include "librbd/ImageState.h" #include "librbd/Journal.h" #include "librbd/Operations.h" #include "librbd/Utils.h" @@ -15,6 +16,7 @@ #include "librbd/mirror/GetInfoRequest.h" #include "librbd/mirror/ImageRemoveRequest.h" #include "librbd/mirror/ImageStateUpdateRequest.h" +#include "librbd/mirror/snapshot/PromoteRequest.h" #define dout_subsys ceph_subsys_rbd #undef dout_prefix @@ -109,32 +111,6 @@ Context *DisableRequest::handle_image_state_update(int *result) { return m_on_finish; } - if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) { - // remove mirroring snapshots - - bool removing_snapshots = false; - { - std::lock_guard locker{m_lock}; - std::shared_lock image_locker{m_image_ctx->image_lock}; - - for (auto &it : m_image_ctx->snap_info) { - auto &snap_info = it.second; - auto type = cls::rbd::get_snap_namespace_type( - snap_info.snap_namespace); - if (type == cls::rbd::SNAPSHOT_NAMESPACE_TYPE_MIRROR) { - send_remove_snap("", snap_info.snap_namespace, snap_info.name); - removing_snapshots = true; - } - } - } - - if (!removing_snapshots) { - send_remove_mirror_image(); - } - - return nullptr; - } - send_promote_image(); return nullptr; } @@ -142,21 +118,29 @@ Context *DisableRequest::handle_image_state_update(int *result) { template void DisableRequest::send_promote_image() { if (m_is_primary) { - send_get_clients(); + clean_mirror_state(); return; } CephContext *cct = m_image_ctx->cct; ldout(cct, 10) << dendl; - // Not primary -- shouldn't have the journal open - ceph_assert(m_image_ctx->journal == nullptr); - - using klass = DisableRequest; - Context *ctx = util::create_context_callback< - klass, &klass::handle_promote_image>(this); - auto req = journal::PromoteRequest::create(m_image_ctx, true, ctx); - req->send(); + auto ctx = util::create_context_callback< + DisableRequest, &DisableRequest::handle_promote_image>(this); + if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_JOURNAL) { + // Not primary -- shouldn't have the journal open + ceph_assert(m_image_ctx->journal == nullptr); + + auto req = journal::PromoteRequest::create(m_image_ctx, true, ctx); + req->send(); + } else if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) { + auto req = mirror::snapshot::PromoteRequest::create( + m_image_ctx, m_mirror_image.global_image_id, ctx); + req->send(); + } else { + lderr(cct) << "unknown image mirror mode: " << m_mirror_image.mode << dendl; + ctx->complete(-EOPNOTSUPP); + } } template @@ -169,10 +153,52 @@ Context *DisableRequest::handle_promote_image(int *result) { return m_on_finish; } - send_get_clients(); + send_refresh_image(); return nullptr; } +template +void DisableRequest::send_refresh_image() { + if (!m_image_ctx->state->is_refresh_required()) { + clean_mirror_state(); + return; + } + + CephContext *cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + auto ctx = util::create_context_callback< + DisableRequest, + &DisableRequest::handle_refresh_image>(this); + m_image_ctx->state->refresh(ctx); +} + +template +Context *DisableRequest::handle_refresh_image(int* result) { + CephContext *cct = m_image_ctx->cct; + ldout(cct, 10) << "r=" << *result << dendl; + + if (*result < 0) { + lderr(cct) << "failed to refresh image: " << cpp_strerror(*result) << dendl; + return m_on_finish; + } + + clean_mirror_state(); + return nullptr; +} + +template +void DisableRequest::clean_mirror_state() { + CephContext *cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) { + remove_mirror_snapshots(); + } else { + send_get_clients(); + } +} + template void DisableRequest::send_get_clients() { CephContext *cct = m_image_ctx->cct; @@ -258,6 +284,33 @@ Context *DisableRequest::handle_get_clients(int *result) { return nullptr; } +template +void DisableRequest::remove_mirror_snapshots() { + CephContext *cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + // remove snapshot-based mirroring snapshots + bool removing_snapshots = false; + { + std::lock_guard locker{m_lock}; + std::shared_lock image_locker{m_image_ctx->image_lock}; + + for (auto &it : m_image_ctx->snap_info) { + auto &snap_info = it.second; + auto type = cls::rbd::get_snap_namespace_type( + snap_info.snap_namespace); + if (type == cls::rbd::SNAPSHOT_NAMESPACE_TYPE_MIRROR) { + send_remove_snap("", snap_info.snap_namespace, snap_info.name); + removing_snapshots = true; + } + } + } + + if (!removing_snapshots) { + send_remove_mirror_image(); + } +} + template void DisableRequest::send_remove_snap( const std::string &client_id, diff --git a/src/librbd/mirror/DisableRequest.h b/src/librbd/mirror/DisableRequest.h index 39cb2539f662..f45d1a14cee7 100644 --- a/src/librbd/mirror/DisableRequest.h +++ b/src/librbd/mirror/DisableRequest.h @@ -50,6 +50,9 @@ private: * PROMOTE_IMAGE (skip if primary) * * | * * v * + * REFRESH_IMAGE (skip if necessary) * + * | * + * v * * GET_CLIENTS <----------------------------------------\ * * * * * | | (unregister clients) | * (on error) * | |/----------------------------\ | * @@ -101,9 +104,16 @@ private: void send_promote_image(); Context *handle_promote_image(int *result); + void send_refresh_image(); + Context* handle_refresh_image(int* result); + + void clean_mirror_state(); + void send_get_clients(); Context *handle_get_clients(int *result); + void remove_mirror_snapshots(); + void send_remove_snap(const std::string &client_id, const cls::rbd::SnapshotNamespace &snap_namespace, const std::string &snap_name); diff --git a/src/test/librbd/mirror/test_mock_DisableRequest.cc b/src/test/librbd/mirror/test_mock_DisableRequest.cc index 37eed983b7b8..ea9936a66bcd 100644 --- a/src/test/librbd/mirror/test_mock_DisableRequest.cc +++ b/src/test/librbd/mirror/test_mock_DisableRequest.cc @@ -4,6 +4,7 @@ #include "test/librbd/test_mock_fixture.h" #include "test/librbd/test_support.h" #include "test/librbd/mock/MockImageCtx.h" +#include "test/librbd/mock/MockImageState.h" #include "test/librbd/mock/MockOperations.h" #include "test/librados_test_stub/MockTestMemIoCtxImpl.h" #include "test/librados_test_stub/MockTestMemRadosClient.h" @@ -12,6 +13,7 @@ #include "librbd/mirror/GetInfoRequest.h" #include "librbd/mirror/ImageRemoveRequest.h" #include "librbd/mirror/ImageStateUpdateRequest.h" +#include "librbd/mirror/snapshot/PromoteRequest.h" namespace librbd { @@ -49,7 +51,6 @@ PromoteRequest *PromoteRequest struct GetInfoRequest { cls::rbd::MirrorImage *mirror_image; @@ -124,6 +125,30 @@ GetInfoRequest *GetInfoRequest *ImageRemoveRequest::s_instance = nullptr; ImageStateUpdateRequest *ImageStateUpdateRequest::s_instance = nullptr; +namespace snapshot { + +template <> +struct PromoteRequest { + Context *on_finish = nullptr; + static PromoteRequest *s_instance; + static PromoteRequest *create(librbd::MockTestImageCtx*, + const std::string& global_image_id, + Context *on_finish) { + ceph_assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + return s_instance; + } + + PromoteRequest() { + s_instance = this; + } + + MOCK_METHOD0(send, void()); +}; + +PromoteRequest *PromoteRequest::s_instance = nullptr; + +} // namespace snapshot } // namespace mirror } // namespace librbd @@ -150,10 +175,11 @@ class TestMockMirrorDisableRequest : public TestMockFixture { public: typedef DisableRequest MockDisableRequest; typedef Journal MockJournal; - typedef journal::PromoteRequest MockPromoteRequest; + typedef journal::PromoteRequest MockJournalPromoteRequest; typedef mirror::GetInfoRequest MockGetInfoRequest; typedef mirror::ImageRemoveRequest MockImageRemoveRequest; typedef mirror::ImageStateUpdateRequest MockImageStateUpdateRequest; + typedef mirror::snapshot::PromoteRequest MockSnapshotPromoteRequest; void expect_get_mirror_info(MockTestImageCtx &mock_image_ctx, MockGetInfoRequest &mock_get_info_request, @@ -222,11 +248,30 @@ public: } void expect_journal_promote(MockTestImageCtx &mock_image_ctx, - MockPromoteRequest &mock_promote_request, int r) { + MockJournalPromoteRequest &mock_promote_request, + int r) { + EXPECT_CALL(mock_promote_request, send()) + .WillOnce(FinishRequest(&mock_promote_request, r, &mock_image_ctx)); + } + + void expect_snapshot_promote(MockTestImageCtx &mock_image_ctx, + MockSnapshotPromoteRequest &mock_promote_request, + int r) { EXPECT_CALL(mock_promote_request, send()) .WillOnce(FinishRequest(&mock_promote_request, r, &mock_image_ctx)); } + void expect_is_refresh_required(MockTestImageCtx &mock_image_ctx, + bool refresh_required) { + EXPECT_CALL(*mock_image_ctx.state, is_refresh_required()) + .WillOnce(Return(refresh_required)); + } + + void expect_refresh_image(MockTestImageCtx &mock_image_ctx, int r) { + EXPECT_CALL(*mock_image_ctx.state, refresh(_)) + .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue)); + } + void expect_snap_remove(MockTestImageCtx &mock_image_ctx, const std::string &snap_name, int r) { EXPECT_CALL(*mock_image_ctx.operations, snap_remove(_, StrEq(snap_name), _)) @@ -322,7 +367,7 @@ TEST_F(TestMockMirrorDisableRequest, SuccessNonPrimary) { ASSERT_EQ(0, open_image(m_image_name, &ictx)); MockTestImageCtx mock_image_ctx(*ictx); - MockPromoteRequest mock_promote_request; + MockJournalPromoteRequest mock_promote_request; expect_op_work_queue(mock_image_ctx); @@ -337,6 +382,7 @@ TEST_F(TestMockMirrorDisableRequest, SuccessNonPrimary) { expect_mirror_image_state_update( mock_image_ctx, mock_image_state_update_request, 0); expect_journal_promote(mock_image_ctx, mock_promote_request, 0); + expect_is_refresh_required(mock_image_ctx, false); expect_journal_client_list(mock_image_ctx, {}, 0); MockImageRemoveRequest mock_image_remove_request; expect_mirror_image_remove( @@ -424,13 +470,11 @@ TEST_F(TestMockMirrorDisableRequest, MirrorImageSetError) { } TEST_F(TestMockMirrorDisableRequest, JournalPromoteError) { - REQUIRE_FEATURE(RBD_FEATURE_JOURNALING); - librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); MockTestImageCtx mock_image_ctx(*ictx); - MockPromoteRequest mock_promote_request; + MockJournalPromoteRequest mock_promote_request; expect_op_work_queue(mock_image_ctx); @@ -559,6 +603,62 @@ TEST_F(TestMockMirrorDisableRequest, JournalClientUnregisterError) { ASSERT_EQ(-EINVAL, ctx.wait()); } +TEST_F(TestMockMirrorDisableRequest, SnapshotPromoteError) { + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + MockSnapshotPromoteRequest mock_promote_request; + + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + + MockGetInfoRequest mock_get_info_request; + expect_get_mirror_info( + mock_image_ctx, mock_get_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, "global id", + cls::rbd::MIRROR_IMAGE_STATE_ENABLED}, PROMOTION_STATE_NON_PRIMARY, 0); + MockImageStateUpdateRequest mock_image_state_update_request; + expect_mirror_image_state_update( + mock_image_ctx, mock_image_state_update_request, 0); + expect_snapshot_promote(mock_image_ctx, mock_promote_request, -EPERM); + + C_SaferCond ctx; + auto req = new MockDisableRequest(&mock_image_ctx, true, true, &ctx); + req->send(); + ASSERT_EQ(-EPERM, ctx.wait()); +} + +TEST_F(TestMockMirrorDisableRequest, RefreshError) { + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + MockSnapshotPromoteRequest mock_promote_request; + + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + + MockGetInfoRequest mock_get_info_request; + expect_get_mirror_info( + mock_image_ctx, mock_get_info_request, + {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT, "global id", + cls::rbd::MIRROR_IMAGE_STATE_ENABLED}, PROMOTION_STATE_NON_PRIMARY, 0); + MockImageStateUpdateRequest mock_image_state_update_request; + expect_mirror_image_state_update( + mock_image_ctx, mock_image_state_update_request, 0); + expect_snapshot_promote(mock_image_ctx, mock_promote_request, 0); + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh_image(mock_image_ctx, -EPERM); + + C_SaferCond ctx; + auto req = new MockDisableRequest(&mock_image_ctx, true, true, &ctx); + req->send(); + ASSERT_EQ(-EPERM, ctx.wait()); +} + TEST_F(TestMockMirrorDisableRequest, MirrorImageRemoveError) { REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);