From: Jason Dillaman Date: Wed, 19 Jul 2017 20:13:23 +0000 (-0400) Subject: rbd-mirror: pre-register image id before creating image X-Git-Tag: v13.0.0~218^2~9 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=028847cb33519bb6d14504c11a52d8b713f166a9;p=ceph-ci.git rbd-mirror: pre-register image id before creating image This fixes a potential race condition that could occur previously if rbd-mirror daemon failed between creating an image and recording the image id to the remote journal. Fixes: http://tracker.ceph.com/issues/15764 Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/Utils.h b/src/librbd/Utils.h index f75d65c43de..19c63447943 100644 --- a/src/librbd/Utils.h +++ b/src/librbd/Utils.h @@ -97,6 +97,11 @@ struct C_AsyncCallback : public Context { std::string generate_image_id(librados::IoCtx &ioctx); +template +inline std::string generate_image_id(librados::IoCtx &ioctx) { + return generate_image_id(ioctx); +} + const std::string group_header_name(const std::string &group_id); const std::string id_obj_name(const std::string &name); const std::string header_name(const std::string &image_id); diff --git a/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc index 4ed498370a7..c372a4b9fc3 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc @@ -36,6 +36,18 @@ struct TypeTraits { }; } // namespace journal + +namespace util { + +static std::string s_image_id; + +template <> +std::string generate_image_id(librados::IoCtx&) { + assert(!s_image_id.empty()); + return s_image_id; +} + +} // namespace util } // namespace librbd namespace rbd { @@ -114,7 +126,6 @@ struct CloseImageRequest { template<> struct CreateImageRequest { static CreateImageRequest* s_instance; - std::string *local_image_id = nullptr; Context *on_finish = nullptr; static CreateImageRequest* create(librados::IoCtx &local_io_ctx, @@ -122,12 +133,12 @@ struct CreateImageRequest { const std::string &global_image_id, const std::string &remote_mirror_uuid, const std::string &local_image_name, + const std::string &local_image_id, librbd::MockTestImageCtx *remote_image_ctx, - std::string *local_image_id, Context *on_finish) { assert(s_instance != nullptr); - s_instance->local_image_id = local_image_id; s_instance->on_finish = on_finish; + s_instance->construct(local_image_id); return s_instance; } @@ -139,6 +150,7 @@ struct CreateImageRequest { s_instance = nullptr; } + MOCK_METHOD1(construct, void(const std::string&)); MOCK_METHOD0(send, void()); }; @@ -407,9 +419,9 @@ public: void expect_create_image(MockCreateImageRequest &mock_create_image_request, const std::string &image_id, int r) { + EXPECT_CALL(mock_create_image_request, construct(image_id)); EXPECT_CALL(mock_create_image_request, send()) - .WillOnce(Invoke([this, &mock_create_image_request, image_id, r]() { - *mock_create_image_request.local_image_id = image_id; + .WillOnce(Invoke([this, &mock_create_image_request, r]() { m_threads->work_queue->queue(mock_create_image_request.on_finish, r); })); } @@ -924,10 +936,19 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrimaryRemote) { MockIsPrimaryRequest mock_is_primary_request; expect_is_primary(mock_is_primary_request, true, 0); - // create the local image + // update client state back to syncing librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); mock_local_image_ctx.journal = &mock_journal; + librbd::util::s_image_id = mock_local_image_ctx.id; + mirror_peer_client_meta = {mock_local_image_ctx.id}; + mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_SYNCING; + client_data.client_meta = mirror_peer_client_meta; + client.data.clear(); + ::encode(client_data, client.data); + expect_journaler_update_client(mock_journaler, client_data, 0); + + // create the local image MockCreateImageRequest mock_create_image_request; expect_create_image(mock_create_image_request, mock_local_image_ctx.id, 0); @@ -937,14 +958,6 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrimaryRemote) { mock_local_image_ctx.id, &mock_local_image_ctx, 0); expect_is_resync_requested(mock_journal, false, 0); - // update client state back to syncing - mirror_peer_client_meta = {mock_local_image_ctx.id}; - mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_SYNCING; - client_data.client_meta = mirror_peer_client_meta; - client.data.clear(); - ::encode(client_data, client.data); - expect_journaler_update_client(mock_journaler, client_data, 0); - // sync the remote image to the local image MockImageSync mock_image_sync; expect_image_sync(mock_image_sync, 0); @@ -1012,10 +1025,19 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrimaryRemoteLocalDeleted) { // test if remote image is primary expect_is_primary(mock_is_primary_request, true, 0); - // create the missing local image + // update client state back to syncing librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); mock_local_image_ctx.journal = &mock_journal; + librbd::util::s_image_id = mock_local_image_ctx.id; + mirror_peer_client_meta = {mock_local_image_ctx.id}; + mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_SYNCING; + client_data.client_meta = mirror_peer_client_meta; + client.data.clear(); + ::encode(client_data, client.data); + expect_journaler_update_client(mock_journaler, client_data, 0); + + // create the missing local image MockCreateImageRequest mock_create_image_request; expect_create_image(mock_create_image_request, mock_local_image_ctx.id, 0); @@ -1024,14 +1046,6 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrimaryRemoteLocalDeleted) { mock_local_image_ctx.id, &mock_local_image_ctx, 0); expect_is_resync_requested(mock_journal, false, 0); - // update client state back to syncing - mirror_peer_client_meta = {mock_local_image_ctx.id}; - mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_SYNCING; - client_data.client_meta = mirror_peer_client_meta; - client.data.clear(); - ::encode(client_data, client.data); - expect_journaler_update_client(mock_journaler, client_data, 0); - // sync the remote image to the local image MockImageSync mock_image_sync; expect_image_sync(mock_image_sync, 0); diff --git a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc index 9abbbcbfdf0..df0c4b2eac6 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc @@ -313,13 +313,13 @@ public: MockCreateImageRequest *create_request(const std::string &global_image_id, const std::string &remote_mirror_uuid, const std::string &local_image_name, + const std::string &local_image_id, librbd::MockTestImageCtx &mock_remote_image_ctx, - std::string *local_image_id, Context *on_finish) { return new MockCreateImageRequest(m_local_io_ctx, m_threads->work_queue, global_image_id, remote_mirror_uuid, - local_image_name, &mock_remote_image_ctx, - local_image_id, on_finish); + local_image_name, local_image_id, + &mock_remote_image_ctx, on_finish); } librbd::ImageCtx *m_remote_image_ctx; @@ -333,14 +333,11 @@ TEST_F(TestMockImageReplayerCreateImageRequest, Create) { expect_create_image(mock_create_request, m_local_io_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", - mock_remote_image_ctx, - &local_image_id, &ctx); + "image name", "101241a7c4c9", + mock_remote_image_ctx, &ctx); request->send(); ASSERT_EQ(0, ctx.wait()); - ASSERT_FALSE(local_image_id.empty()); } TEST_F(TestMockImageReplayerCreateImageRequest, CreateError) { @@ -351,11 +348,9 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CreateError) { expect_create_image(mock_create_request, m_local_io_ctx, -EINVAL); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", - mock_remote_image_ctx, - &local_image_id, &ctx); + "image name", "101241a7c4c9", + mock_remote_image_ctx, &ctx); request->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -396,14 +391,12 @@ TEST_F(TestMockImageReplayerCreateImageRequest, Clone) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(0, ctx.wait()); - ASSERT_FALSE(local_image_id.empty()); } TEST_F(TestMockImageReplayerCreateImageRequest, CloneGetGlobalImageIdError) { @@ -423,11 +416,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneGetGlobalImageIdError) { expect_get_parent_global_image_id(m_remote_io_ctx, "global uuid", -ENOENT); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -450,11 +442,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneGetLocalParentImageIdError) expect_mirror_image_get_image_id(m_local_io_ctx, "local parent id", -ENOENT); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -482,11 +473,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneOpenRemoteParentError) { m_remote_image_ctx->id, mock_remote_parent_image_ctx, -ENOENT); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -524,11 +514,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneOpenLocalParentError) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -568,11 +557,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneSnapSetError) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -613,11 +601,10 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneError) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -658,14 +645,12 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneLocalParentCloseError) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, 0); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(0, ctx.wait()); - ASSERT_FALSE(local_image_id.empty()); } TEST_F(TestMockImageReplayerCreateImageRequest, CloneRemoteParentCloseError) { @@ -704,14 +689,12 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneRemoteParentCloseError) { expect_close_image(mock_close_image_request, mock_remote_parent_image_ctx, -EINVAL); C_SaferCond ctx; - std::string local_image_id; MockCreateImageRequest *request = create_request("global uuid", "remote uuid", - "image name", + "image name", "101241a7c4c9", mock_remote_clone_image_ctx, - &local_image_id, &ctx); + &ctx); request->send(); ASSERT_EQ(0, ctx.wait()); - ASSERT_FALSE(local_image_id.empty()); } } // namespace image_replayer diff --git a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc index 8fdf1b270b9..1b2359a7df0 100644 --- a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc @@ -237,6 +237,7 @@ void BootstrapRequest::handle_register_client(int r) { return; } + m_client = {}; *m_client_meta = librbd::journal::MirrorPeerClientMeta(m_local_image_id); is_primary(); } @@ -276,7 +277,7 @@ void BootstrapRequest::handle_is_primary(int r) { } if (m_local_image_id.empty()) { - create_local_image(); + update_client_image(); return; } @@ -389,7 +390,7 @@ void BootstrapRequest::handle_open_local_image(int r) { return; } - update_client_image(); + get_remote_tags(); } template @@ -419,52 +420,14 @@ void BootstrapRequest::handle_unregister_client(int r) { register_client(); } -template -void BootstrapRequest::create_local_image() { - dout(20) << dendl; - - m_local_image_id = ""; - update_progress("CREATE_LOCAL_IMAGE"); - - m_remote_image_ctx->snap_lock.get_read(); - std::string image_name = m_remote_image_ctx->name; - m_remote_image_ctx->snap_lock.put_read(); - - Context *ctx = create_context_callback< - BootstrapRequest, &BootstrapRequest::handle_create_local_image>( - this); - CreateImageRequest *request = CreateImageRequest::create( - m_local_io_ctx, m_work_queue, m_global_image_id, m_remote_mirror_uuid, - image_name, m_remote_image_ctx, &m_local_image_id, ctx); - request->send(); -} - -template -void BootstrapRequest::handle_create_local_image(int r) { - dout(20) << ": r=" << r << dendl; - - if (r < 0) { - derr << ": failed to create local image: " << cpp_strerror(r) << dendl; - m_ret_val = r; - close_remote_image(); - return; - } - - open_local_image(); -} - template void BootstrapRequest::update_client_image() { - if (m_client_meta->image_id == (*m_local_image_ctx)->id) { - // already registered local image with remote journal - get_remote_tags(); - return; - } - m_local_image_id = (*m_local_image_ctx)->id; - dout(20) << dendl; update_progress("UPDATE_CLIENT_IMAGE"); + assert(m_local_image_id.empty()); + m_local_image_id = librbd::util::generate_image_id(m_local_io_ctx); + librbd::journal::MirrorPeerClientMeta client_meta{m_local_image_id}; client_meta.state = librbd::journal::MIRROR_PEER_STATE_SYNCING; @@ -498,7 +461,39 @@ void BootstrapRequest::handle_update_client_image(int r) { *m_client_meta = {m_local_image_id}; m_client_meta->state = librbd::journal::MIRROR_PEER_STATE_SYNCING; - get_remote_tags(); + create_local_image(); +} + +template +void BootstrapRequest::create_local_image() { + dout(20) << dendl; + update_progress("CREATE_LOCAL_IMAGE"); + + m_remote_image_ctx->snap_lock.get_read(); + std::string image_name = m_remote_image_ctx->name; + m_remote_image_ctx->snap_lock.put_read(); + + Context *ctx = create_context_callback< + BootstrapRequest, &BootstrapRequest::handle_create_local_image>( + this); + CreateImageRequest *request = CreateImageRequest::create( + m_local_io_ctx, m_work_queue, m_global_image_id, m_remote_mirror_uuid, + image_name, m_local_image_id, m_remote_image_ctx, ctx); + request->send(); +} + +template +void BootstrapRequest::handle_create_local_image(int r) { + dout(20) << ": r=" << r << dendl; + + if (r < 0) { + derr << ": failed to create local image: " << cpp_strerror(r) << dendl; + m_ret_val = r; + close_remote_image(); + return; + } + + open_local_image(); } template diff --git a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.h b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.h index 668dd8375d4..6c67222d4ab 100644 --- a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.h +++ b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.h @@ -108,6 +108,9 @@ private: * IS_PRIMARY * * * * * * * * * * * * * * * * * * * * * * | * | * * | * | (remote image primary, no local image id) * * | + * \----> UPDATE_CLIENT_IMAGE * * * * * * * * * * * * | + * | | * * | + * | v * * | * \----> CREATE_LOCAL_IMAGE * * * * * * * * * * * * * | * | | * * | * | v * * | @@ -120,9 +123,6 @@ private: * | | \-----------------------*---*---/ * | | * * * | v (skip if not needed) * * - * | UPDATE_CLIENT_IMAGE * * * * * * * - * | | * * * - * | v (skip if not needed) * * * * | GET_REMOTE_TAGS * * * * * * * * * * | | * * * * | v (skip if not needed) v * * diff --git a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc index 2c7a0f367e3..5db89b4d1cc 100644 --- a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc @@ -33,14 +33,14 @@ CreateImageRequest::CreateImageRequest(librados::IoCtx &local_io_ctx, const std::string &global_image_id, const std::string &remote_mirror_uuid, const std::string &local_image_name, + const std::string &local_image_id, I *remote_image_ctx, - std::string *local_image_id, Context *on_finish) : m_local_io_ctx(local_io_ctx), m_work_queue(work_queue), m_global_image_id(global_image_id), m_remote_mirror_uuid(remote_mirror_uuid), - m_local_image_name(local_image_name), m_remote_image_ctx(remote_image_ctx), - m_local_image_id(local_image_id), m_on_finish(on_finish) { + m_local_image_name(local_image_name), m_local_image_id(local_image_id), + m_remote_image_ctx(remote_image_ctx), m_on_finish(on_finish) { } template @@ -75,10 +75,8 @@ void CreateImageRequest::create_image() { image_options.set(RBD_IMAGE_OPTION_STRIPE_COUNT, m_remote_image_ctx->stripe_count); - *m_local_image_id = librbd::util::generate_image_id(m_local_io_ctx);; - librbd::image::CreateRequest *req = librbd::image::CreateRequest::create( - m_local_io_ctx, m_local_image_name, *m_local_image_id, + m_local_io_ctx, m_local_image_name, m_local_image_id, m_remote_image_ctx->size, image_options, m_global_image_id, m_remote_mirror_uuid, false, m_remote_image_ctx->op_work_queue, ctx); req->send(); @@ -290,14 +288,12 @@ void CreateImageRequest::clone_image() { opts.set(RBD_IMAGE_OPTION_STRIPE_UNIT, m_remote_image_ctx->stripe_unit); opts.set(RBD_IMAGE_OPTION_STRIPE_COUNT, m_remote_image_ctx->stripe_count); - *m_local_image_id = librbd::util::generate_image_id(m_local_io_ctx);; - using klass = CreateImageRequest; Context *ctx = create_context_callback(this); librbd::image::CloneRequest *req = librbd::image::CloneRequest::create( m_local_parent_image_ctx, m_local_io_ctx, m_local_image_name, - *m_local_image_id, opts, m_global_image_id, m_remote_mirror_uuid, + m_local_image_id, opts, m_global_image_id, m_remote_mirror_uuid, m_remote_image_ctx->op_work_queue, ctx); req->send(); } diff --git a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h index 683a6d9a2cf..b45deb66770 100644 --- a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h +++ b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h @@ -26,20 +26,20 @@ public: const std::string &global_image_id, const std::string &remote_mirror_uuid, const std::string &local_image_name, + const std::string &local_image_id, ImageCtxT *remote_image_ctx, - std::string *local_image_id, Context *on_finish) { return new CreateImageRequest(local_io_ctx, work_queue, global_image_id, remote_mirror_uuid, local_image_name, - remote_image_ctx, local_image_id, on_finish); + local_image_id, remote_image_ctx, on_finish); } CreateImageRequest(librados::IoCtx &local_io_ctx, ContextWQ *work_queue, const std::string &global_image_id, const std::string &remote_mirror_uuid, const std::string &local_image_name, + const std::string &local_image_id, ImageCtxT *remote_image_ctx, - std::string *local_image_id, Context *on_finish); void send(); @@ -86,8 +86,8 @@ private: std::string m_global_image_id; std::string m_remote_mirror_uuid; std::string m_local_image_name; + std::string m_local_image_id; ImageCtxT *m_remote_image_ctx; - std::string *m_local_image_id; Context *m_on_finish; librados::IoCtx m_remote_parent_io_ctx;