From: Jason Dillaman Date: Tue, 20 Oct 2020 22:45:04 +0000 (-0400) Subject: librbd: switch to migration source-spec builder X-Git-Tag: v16.1.0~702^2~16 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=5869d13fb5f55ca24a55a95977c5ca22214d7227;p=ceph.git librbd: switch to migration source-spec builder Remove all the hard-coded references to the NativeFormat handler and allow the RawFormat+FileStream handlers to be instantiated. Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/CMakeLists.txt b/src/librbd/CMakeLists.txt index 53eb6853f398..ac4b196def1e 100644 --- a/src/librbd/CMakeLists.txt +++ b/src/librbd/CMakeLists.txt @@ -130,6 +130,7 @@ set(librbd_internal_srcs migration/NativeFormat.cc migration/OpenSourceImageRequest.cc migration/RawFormat.cc + migration/SourceSpecBuilder.cc mirror/DemoteRequest.cc mirror/DisableRequest.cc mirror/EnableRequest.cc diff --git a/src/librbd/migration/FileStream.cc b/src/librbd/migration/FileStream.cc index e2920e27a144..501167235031 100644 --- a/src/librbd/migration/FileStream.cc +++ b/src/librbd/migration/FileStream.cc @@ -20,7 +20,11 @@ namespace librbd { namespace migration { -static std::string FILE_PATH {"file_path"}; +namespace { + +const std::string FILE_PATH {"file_path"}; + +} // anonymous namespace #ifdef BOOST_ASIO_HAS_POSIX_STREAM_DESCRIPTOR @@ -135,6 +139,7 @@ void FileStream::open(Context* on_finish) { if (file_path_value.type() != json_spirit::str_type) { lderr(m_cct) << "failed to locate '" << FILE_PATH << "' key" << dendl; on_finish->complete(-EINVAL); + return; } auto& file_path = file_path_value.get_str(); diff --git a/src/librbd/migration/OpenSourceImageRequest.cc b/src/librbd/migration/OpenSourceImageRequest.cc index 47188bccc662..f12d7cb518f5 100644 --- a/src/librbd/migration/OpenSourceImageRequest.cc +++ b/src/librbd/migration/OpenSourceImageRequest.cc @@ -9,6 +9,7 @@ #include "librbd/io/ImageDispatcher.h" #include "librbd/migration/ImageDispatch.h" #include "librbd/migration/NativeFormat.h" +#include "librbd/migration/SourceSpecBuilder.h" #define dout_subsys ceph_subsys_rbd #undef dout_prefix @@ -40,8 +41,8 @@ void OpenSourceImageRequest::open_source() { ldout(cct, 10) << dendl; // note that all source image ctx properties are placeholders - *m_src_image_ctx = I::create("", "", m_src_snap_id, - m_dst_image_ctx->md_ctx, true); + *m_src_image_ctx = I::create("", "", m_src_snap_id, m_dst_image_ctx->md_ctx, + true); (*m_src_image_ctx)->child = m_dst_image_ctx; auto source_spec = m_migration_info.source_spec; @@ -52,19 +53,24 @@ void OpenSourceImageRequest::open_source() { m_migration_info.image_name, m_migration_info.image_id); } - // TODO use factory once multiple sources are available - - json_spirit::mValue json_root; - json_spirit::mObject json_source_spec_object; - if(json_spirit::read(source_spec, json_root)) { - try { - json_source_spec_object = json_root.get_obj(); - } catch (std::runtime_error&) { - } + SourceSpecBuilder source_spec_builder{*m_src_image_ctx}; + json_spirit::mObject source_spec_object; + int r = source_spec_builder.parse_source_spec(source_spec, + &source_spec_object); + if (r < 0) { + lderr(cct) << "failed to parse migration source-spec:" << cpp_strerror(r) + << dendl; + finish(r); + return; } - m_format = std::unique_ptr(NativeFormat::create( - *m_src_image_ctx, json_source_spec_object)); + r = source_spec_builder.build_format(source_spec_object, &m_format); + if (r < 0) { + lderr(cct) << "failed to build migration format handler: " + << cpp_strerror(r) << dendl; + finish(r); + return; + } auto ctx = util::create_context_callback< OpenSourceImageRequest, diff --git a/src/librbd/migration/RawFormat.cc b/src/librbd/migration/RawFormat.cc index 529a4da37738..24ba7ef31c9e 100644 --- a/src/librbd/migration/RawFormat.cc +++ b/src/librbd/migration/RawFormat.cc @@ -3,15 +3,14 @@ #include "librbd/migration/RawFormat.h" #include "common/dout.h" +#include "common/errno.h" #include "librbd/ImageCtx.h" #include "librbd/io/AioCompletion.h" #include "librbd/io/ReadResult.h" #include "librbd/migration/FileStream.h" +#include "librbd/migration/SourceSpecBuilder.h" #include "librbd/migration/StreamInterface.h" -static const std::string STREAM{"stream"}; -static const std::string STREAM_TYPE{"type"}; - #define dout_subsys ceph_subsys_rbd #undef dout_prefix #define dout_prefix *_dout << "librbd::migration::RawFormat: " << this \ @@ -22,8 +21,10 @@ namespace migration { template RawFormat::RawFormat( - I* image_ctx, const json_spirit::mObject& json_object) - : m_image_ctx(image_ctx), m_json_object(json_object) { + I* image_ctx, const json_spirit::mObject& json_object, + const SourceSpecBuilder* source_spec_builder) + : m_image_ctx(image_ctx), m_json_object(json_object), + m_source_spec_builder(source_spec_builder) { } template @@ -31,30 +32,14 @@ void RawFormat::open(Context* on_finish) { auto cct = m_image_ctx->cct; ldout(cct, 10) << dendl; - // TODO switch to stream builder when more available - auto& stream_value = m_json_object[STREAM]; - if (stream_value.type() != json_spirit::obj_type) { - lderr(cct) << "missing stream section" << dendl; - on_finish->complete(-EINVAL); - return; - } - - auto& stream_obj = stream_value.get_obj(); - auto& stream_type_value = stream_obj[STREAM_TYPE]; - if (stream_type_value.type() != json_spirit::str_type) { - lderr(cct) << "missing stream type value" << dendl; - on_finish->complete(-EINVAL); - return; - } - - auto& stream_type = stream_type_value.get_str(); - if (stream_type != "file") { - lderr(cct) << "unknown stream type '" << stream_type << "'" << dendl; - on_finish->complete(-EINVAL); + int r = m_source_spec_builder->build_stream(m_json_object, &m_stream); + if (r < 0) { + lderr(cct) << "failed to build migration stream handler" << cpp_strerror(r) + << dendl; + on_finish->complete(r); return; } - m_stream.reset(FileStream::create(m_image_ctx, stream_obj)); m_stream->open(on_finish); } diff --git a/src/librbd/migration/RawFormat.h b/src/librbd/migration/RawFormat.h index 3bf1c3474d5c..ab185a5b596b 100644 --- a/src/librbd/migration/RawFormat.h +++ b/src/librbd/migration/RawFormat.h @@ -19,17 +19,20 @@ struct ImageCtx; namespace migration { +template struct SourceSpecBuilder; struct StreamInterface; template class RawFormat : public FormatInterface { public: - static RawFormat* create(ImageCtxT* image_ctx, - const json_spirit::mObject& json_object) { - return new RawFormat(image_ctx, json_object); + static RawFormat* create( + ImageCtxT* image_ctx, const json_spirit::mObject& json_object, + const SourceSpecBuilder* source_spec_builder) { + return new RawFormat(image_ctx, json_object, source_spec_builder); } - RawFormat(ImageCtxT* image_ctx, const json_spirit::mObject& json_object); + RawFormat(ImageCtxT* image_ctx, const json_spirit::mObject& json_object, + const SourceSpecBuilder* source_spec_builder); RawFormat(const RawFormat&) = delete; RawFormat& operator=(const RawFormat&) = delete; @@ -53,6 +56,7 @@ public: private: ImageCtxT* m_image_ctx; json_spirit::mObject m_json_object; + const SourceSpecBuilder* m_source_spec_builder; std::unique_ptr m_stream; diff --git a/src/librbd/migration/SourceSpecBuilder.cc b/src/librbd/migration/SourceSpecBuilder.cc new file mode 100644 index 000000000000..2b8d0a3885fa --- /dev/null +++ b/src/librbd/migration/SourceSpecBuilder.cc @@ -0,0 +1,110 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab + +#include "librbd/migration/SourceSpecBuilder.h" +#include "common/dout.h" +#include "librbd/ImageCtx.h" +#include "librbd/migration/FileStream.h" +#include "librbd/migration/NativeFormat.h" +#include "librbd/migration/RawFormat.h" + +#define dout_subsys ceph_subsys_rbd +#undef dout_prefix +#define dout_prefix *_dout << "librbd::migration::SourceSpecBuilder: " << this \ + << " " << __func__ << ": " + +namespace librbd { +namespace migration { + +namespace { + +const std::string STREAM_KEY{"stream"}; +const std::string TYPE_KEY{"type"}; + +} // anonymous namespace + +template +int SourceSpecBuilder::parse_source_spec( + const std::string& source_spec, + json_spirit::mObject* source_spec_object) const { + auto cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + json_spirit::mValue json_root; + if(json_spirit::read(source_spec, json_root)) { + try { + *source_spec_object = json_root.get_obj(); + return 0; + } catch (std::runtime_error&) { + } + } + + lderr(cct) << "invalid source-spec JSON" << dendl; + return -EBADMSG; +} + +template +int SourceSpecBuilder::build_format( + const json_spirit::mObject& source_spec_object, + std::unique_ptr* format) const { + auto cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + auto type_value_it = source_spec_object.find(TYPE_KEY); + if (type_value_it == source_spec_object.end() || + type_value_it->second.type() != json_spirit::str_type) { + lderr(cct) << "failed to locate format type value" << dendl; + return -EINVAL; + } + + auto& type = type_value_it->second.get_str(); + if (type == "native") { + format->reset(NativeFormat::create(m_image_ctx, source_spec_object)); + } else if (type == "raw") { + format->reset(RawFormat::create(m_image_ctx, source_spec_object, this)); + } else { + lderr(cct) << "unknown or unsupported format type '" << type << "'" + << dendl; + return -ENOSYS; + } + return 0; +} + +template +int SourceSpecBuilder::build_stream( + const json_spirit::mObject& source_spec_object, + std::unique_ptr* stream) const { + auto cct = m_image_ctx->cct; + ldout(cct, 10) << dendl; + + auto stream_value_it = source_spec_object.find(STREAM_KEY); + if (stream_value_it == source_spec_object.end() || + stream_value_it->second.type() != json_spirit::obj_type) { + lderr(cct) << "failed to locate stream object" << dendl; + return -EINVAL; + } + + auto& stream_obj = stream_value_it->second.get_obj(); + auto type_value_it = stream_obj.find(TYPE_KEY); + if (type_value_it == stream_obj.end() || + type_value_it->second.type() != json_spirit::str_type) { + lderr(cct) << "failed to locate stream type value" << dendl; + return -EINVAL; + } + + auto& type = type_value_it->second.get_str(); + if (type == "file") { + stream->reset(FileStream::create(m_image_ctx, stream_obj)); + } else { + lderr(cct) << "unknown or unsupported stream type '" << type << "'" + << dendl; + return -ENOSYS; + } + + return 0; +} + +} // namespace migration +} // namespace librbd + +template class librbd::migration::SourceSpecBuilder; diff --git a/src/librbd/migration/SourceSpecBuilder.h b/src/librbd/migration/SourceSpecBuilder.h new file mode 100644 index 000000000000..be32c5dc031c --- /dev/null +++ b/src/librbd/migration/SourceSpecBuilder.h @@ -0,0 +1,49 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab + +#ifndef CEPH_LIBRBD_MIGRATION_SOURCE_SPEC_BUILDER_H +#define CEPH_LIBRBD_MIGRATION_SOURCE_SPEC_BUILDER_H + +#include "include/int_types.h" +#include +#include +#include +#include + +struct Context; + +namespace librbd { + +struct ImageCtx; + +namespace migration { + +struct FormatInterface; +struct StreamInterface; + +template +class SourceSpecBuilder { +public: + SourceSpecBuilder(ImageCtxT* image_ctx) : m_image_ctx(image_ctx) { + } + + int parse_source_spec(const std::string& source_spec, + json_spirit::mObject* source_spec_object) const; + + int build_format(const json_spirit::mObject& format_object, + std::unique_ptr* format) const; + + int build_stream(const json_spirit::mObject& source_spec_object, + std::unique_ptr* stream) const; + +private: + ImageCtxT* m_image_ctx; + +}; + +} // namespace migration +} // namespace librbd + +extern template class librbd::migration::SourceSpecBuilder; + +#endif // CEPH_LIBRBD_MIGRATION_SOURCE_SPEC_BUILDER_H diff --git a/src/test/librbd/migration/test_mock_RawFormat.cc b/src/test/librbd/migration/test_mock_RawFormat.cc index 413370564016..c67694c63f94 100644 --- a/src/test/librbd/migration/test_mock_RawFormat.cc +++ b/src/test/librbd/migration/test_mock_RawFormat.cc @@ -3,10 +3,12 @@ #include "test/librbd/test_mock_fixture.h" #include "test/librbd/test_support.h" +#include "test/librbd/mock/migration/MockStreamInterface.h" #include "include/rbd_types.h" #include "common/ceph_mutex.h" #include "librbd/migration/FileStream.h" #include "librbd/migration/RawFormat.h" +#include "librbd/migration/SourceSpecBuilder.h" #include "gtest/gtest.h" #include "gmock/gmock.h" #include "json_spirit/json_spirit.h" @@ -24,31 +26,13 @@ struct MockTestImageCtx : public MockImageCtx { namespace migration { template<> -struct FileStream : public StreamInterface { - static FileStream* s_instance; - static FileStream* create(librbd::MockTestImageCtx*, - const json_spirit::mObject&) { - ceph_assert(s_instance != nullptr); - return s_instance; - } - - MOCK_METHOD1(open, void(Context*)); - MOCK_METHOD1(close, void(Context*)); - - MOCK_METHOD2(get_size, void(uint64_t*, Context*)); +struct SourceSpecBuilder { - MOCK_METHOD3(read, void(const io::Extents&, bufferlist*, Context*)); - void read(io::Extents&& byte_extents, bufferlist* bl, Context* on_finish) { - read(byte_extents, bl, on_finish); - } + MOCK_CONST_METHOD2(build_stream, int(const json_spirit::mObject&, + std::unique_ptr*)); - FileStream() { - s_instance = this; - } }; -FileStream* FileStream::s_instance = nullptr; - } // namespace migration } // namespace librbd @@ -67,7 +51,7 @@ using ::testing::Invoke; class TestMockMigrationRawFormat : public TestMockFixture { public: typedef RawFormat MockRawFormat; - typedef FileStream MockFileStream; + typedef SourceSpecBuilder MockSourceSpecBuilder; librbd::ImageCtx *m_image_ctx; @@ -81,29 +65,39 @@ public: json_object["stream"] = stream_obj; } - void expect_stream_open(MockFileStream& mock_file_stream, int r) { - EXPECT_CALL(mock_file_stream, open(_)) + void expect_build_stream(MockSourceSpecBuilder& mock_source_spec_builder, + MockStreamInterface* mock_stream_interface, int r) { + EXPECT_CALL(mock_source_spec_builder, build_stream(_, _)) + .WillOnce(WithArgs<1>(Invoke([mock_stream_interface, r] + (std::unique_ptr* ptr) { + ptr->reset(mock_stream_interface); + return r; + }))); + } + + void expect_stream_open(MockStreamInterface& mock_stream_interface, int r) { + EXPECT_CALL(mock_stream_interface, open(_)) .WillOnce(Invoke([r](Context* ctx) { ctx->complete(r); })); } - void expect_stream_close(MockFileStream& mock_file_stream, int r) { - EXPECT_CALL(mock_file_stream, close(_)) + void expect_stream_close(MockStreamInterface& mock_stream_interface, int r) { + EXPECT_CALL(mock_stream_interface, close(_)) .WillOnce(Invoke([r](Context* ctx) { ctx->complete(r); })); } - void expect_stream_get_size(MockFileStream& mock_file_stream, + void expect_stream_get_size(MockStreamInterface& mock_stream_interface, uint64_t size, int r) { - EXPECT_CALL(mock_file_stream, get_size(_, _)) + EXPECT_CALL(mock_stream_interface, get_size(_, _)) .WillOnce(Invoke([size, r](uint64_t* out_size, Context* ctx) { *out_size = size; ctx->complete(r); })); } - void expect_stream_read(MockFileStream& mock_file_stream, + void expect_stream_read(MockStreamInterface& mock_stream_interface, const io::Extents& byte_extents, const bufferlist& bl, int r) { - EXPECT_CALL(mock_file_stream, read(byte_extents, _, _)) + EXPECT_CALL(mock_stream_interface, read(byte_extents, _, _)) .WillOnce(WithArgs<1, 2>(Invoke([bl, r] (bufferlist* out_bl, Context* ctx) { *out_bl = bl; @@ -118,12 +112,17 @@ TEST_F(TestMockMigrationRawFormat, OpenClose) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; + + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); + + expect_stream_open(*mock_stream_interface, 0); - expect_stream_close(*mock_file_stream, 0); + expect_stream_close(*mock_stream_interface, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); @@ -138,12 +137,17 @@ TEST_F(TestMockMigrationRawFormat, GetSnapshots) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; - expect_stream_close(*mock_file_stream, 0); + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + expect_stream_open(*mock_stream_interface, 0); + + expect_stream_close(*mock_stream_interface, 0); + + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); @@ -164,14 +168,19 @@ TEST_F(TestMockMigrationRawFormat, GetImageSize) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; + + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); - expect_stream_get_size(*mock_file_stream, 123, 0); + expect_stream_open(*mock_stream_interface, 0); - expect_stream_close(*mock_file_stream, 0); + expect_stream_get_size(*mock_stream_interface, 123, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + expect_stream_close(*mock_stream_interface, 0); + + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); @@ -192,12 +201,17 @@ TEST_F(TestMockMigrationRawFormat, GetImageSizeSnapshot) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; + + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); - expect_stream_close(*mock_file_stream, 0); + expect_stream_open(*mock_stream_interface, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + expect_stream_close(*mock_stream_interface, 0); + + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); @@ -217,16 +231,21 @@ TEST_F(TestMockMigrationRawFormat, Read) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; + + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); + + expect_stream_open(*mock_stream_interface, 0); bufferlist expect_bl; expect_bl.append(std::string(123, '1')); - expect_stream_read(*mock_file_stream, {{123, 123}}, expect_bl, 0); + expect_stream_read(*mock_stream_interface, {{123, 123}}, expect_bl, 0); - expect_stream_close(*mock_file_stream, 0); + expect_stream_close(*mock_stream_interface, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); @@ -251,12 +270,17 @@ TEST_F(TestMockMigrationRawFormat, ListSnaps) { MockTestImageCtx mock_image_ctx(*m_image_ctx); InSequence seq; - auto mock_file_stream = new MockFileStream(); - expect_stream_open(*mock_file_stream, 0); + MockSourceSpecBuilder mock_source_spec_builder; + + auto mock_stream_interface = new MockStreamInterface(); + expect_build_stream(mock_source_spec_builder, mock_stream_interface, 0); + + expect_stream_open(*mock_stream_interface, 0); - expect_stream_close(*mock_file_stream, 0); + expect_stream_close(*mock_stream_interface, 0); - MockRawFormat mock_raw_format(&mock_image_ctx, json_object); + MockRawFormat mock_raw_format(&mock_image_ctx, json_object, + &mock_source_spec_builder); C_SaferCond ctx1; mock_raw_format.open(&ctx1); diff --git a/src/test/librbd/mock/migration/MockStreamInterface.h b/src/test/librbd/mock/migration/MockStreamInterface.h new file mode 100644 index 000000000000..36df86638dee --- /dev/null +++ b/src/test/librbd/mock/migration/MockStreamInterface.h @@ -0,0 +1,29 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab + +#ifndef CEPH_TEST_LIBRBD_MOCK_MIGRATION_MOCK_STREAM_INTERFACE_H +#define CEPH_TEST_LIBRBD_MOCK_MIGRATION_MOCK_STREAM_INTERFACE_H + +#include "include/buffer.h" +#include "gmock/gmock.h" +#include "librbd/migration/StreamInterface.h" + +namespace librbd { +namespace migration { + +struct MockStreamInterface : public StreamInterface { + MOCK_METHOD1(open, void(Context*)); + MOCK_METHOD1(close, void(Context*)); + + MOCK_METHOD2(get_size, void(uint64_t*, Context*)); + + MOCK_METHOD3(read, void(const io::Extents&, bufferlist*, Context*)); + void read(io::Extents&& byte_extents, bufferlist* bl, Context* on_finish) { + read(byte_extents, bl, on_finish); + } +}; + +} // namespace migration +} // namespace librbd + +#endif // CEPH_TEST_LIBRBD_MOCK_MIGRATION_MOCK_STREAM_INTERFACE_H