From d22ca3d9780bb0e6033fe9d604d915313af798d7 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 22 Dec 2020 09:46:04 -0500 Subject: [PATCH] librbd/migration: require snapshot when importing from native source Since we cannot mark the source image read-only when running in import-only migration mode, we should require the user to provide a snapshot to ensure that data cannot change while the migration is running. Signed-off-by: Jason Dillaman --- qa/workunits/rbd/cli_migration.sh | 17 +++-- src/librbd/migration/NativeFormat.cc | 94 ++++++++++++++++++++++++++++ src/librbd/migration/NativeFormat.h | 5 ++ src/test/cli/rbd/help.t | 62 +++++++++--------- src/test/pybind/test_rbd.py | 10 ++- src/tools/rbd/action/Migration.cc | 26 ++++++-- 6 files changed, 170 insertions(+), 44 deletions(-) diff --git a/qa/workunits/rbd/cli_migration.sh b/qa/workunits/rbd/cli_migration.sh index e1a4d2df8ce..5f24f8ac2ce 100755 --- a/qa/workunits/rbd/cli_migration.sh +++ b/qa/workunits/rbd/cli_migration.sh @@ -75,13 +75,17 @@ test_import_native_format() { local base_image=$1 local dest_image=$2 + rbd migration prepare --import-only "rbd/${base_image}@2" ${dest_image} + rbd migration abort ${dest_image} + local pool_id=$(ceph osd pool ls detail --format xml | xmlstarlet sel -t -v "//pools/pool[pool_name='rbd']/pool_id") cat > ${TEMPDIR}/spec.json <::open(Context* on_finish) { return; } + auto& snap_name_val = m_json_object[SNAP_NAME_KEY]; + if (snap_name_val.type() == json_spirit::str_type) { + m_snap_name = snap_name_val.get_str(); + } else if (snap_name_val.type() != json_spirit::null_type) { + lderr(cct) << "invalid snap name" << dendl; + on_finish->complete(-EINVAL); + return; + } + + auto& snap_id_val = m_json_object[SNAP_ID_KEY]; + if (!m_snap_name.empty() && snap_id_val.type() != json_spirit::null_type) { + lderr(cct) << "cannot specify both snap name and snap id" << dendl; + on_finish->complete(-EINVAL); + return; + } else if (snap_id_val.type() == json_spirit::str_type) { + try { + m_snap_id = boost::lexical_cast(snap_id_val.get_str()); + } catch (boost::bad_lexical_cast &) { + } + } else if (snap_id_val.type() == json_spirit::int_type) { + m_snap_id = snap_id_val.get_uint64(); + } + + if (snap_id_val.type() != json_spirit::null_type && + m_snap_id == CEPH_NOSNAP) { + lderr(cct) << "invalid snap id" << dendl; + on_finish->complete(-EINVAL); + return; + } + + // snapshot is required for import to keep source read-only + if (m_import_only && m_snap_name.empty() && m_snap_id == CEPH_NOSNAP) { + lderr(cct) << "snapshot required for import" << dendl; + on_finish->complete(-EINVAL); + return; + } + // TODO add support for external clusters librados::IoCtx io_ctx; int r = util::create_ioctx(m_image_ctx->md_ctx, "source image", @@ -153,9 +193,63 @@ void NativeFormat::open(Context* on_finish) { } // open the source RBD image + on_finish = new LambdaContext([this, on_finish](int r) { + handle_open(r, on_finish); }); m_image_ctx->state->open(flags, on_finish); } +template +void NativeFormat::handle_open(int r, Context* on_finish) { + auto cct = m_image_ctx->cct; + ldout(cct, 10) << "r=" << r << dendl; + + if (r < 0) { + lderr(cct) << "failed to open image: " << cpp_strerror(r) << dendl; + on_finish->complete(r); + return; + } + + if (m_snap_id == CEPH_NOSNAP && m_snap_name.empty()) { + on_finish->complete(0); + return; + } + + if (!m_snap_name.empty()) { + std::shared_lock image_locker{m_image_ctx->image_lock}; + m_snap_id = m_image_ctx->get_snap_id(cls::rbd::UserSnapshotNamespace{}, + m_snap_name); + } + + if (m_snap_id == CEPH_NOSNAP) { + lderr(cct) << "failed to locate snapshot " << m_snap_name << dendl; + on_finish = new LambdaContext([on_finish](int) { + on_finish->complete(-ENOENT); }); + m_image_ctx->state->close(on_finish); + return; + } + + on_finish = new LambdaContext([this, on_finish](int r) { + handle_snap_set(r, on_finish); }); + m_image_ctx->state->snap_set(m_snap_id, on_finish); +} + +template +void NativeFormat::handle_snap_set(int r, Context* on_finish) { + auto cct = m_image_ctx->cct; + ldout(cct, 10) << "r=" << r << dendl; + + if (r < 0) { + lderr(cct) << "failed to set snapshot " << m_snap_id << ": " + << cpp_strerror(r) << dendl; + on_finish = new LambdaContext([r, on_finish](int) { + on_finish->complete(r); }); + m_image_ctx->state->close(on_finish); + return; + } + + on_finish->complete(0); +} + template void NativeFormat::close(Context* on_finish) { auto cct = m_image_ctx->cct; diff --git a/src/librbd/migration/NativeFormat.h b/src/librbd/migration/NativeFormat.h index 0b2406ecbc8..e58c041214e 100644 --- a/src/librbd/migration/NativeFormat.h +++ b/src/librbd/migration/NativeFormat.h @@ -66,6 +66,11 @@ private: std::string m_pool_namespace; std::string m_image_name; std::string m_image_id; + std::string m_snap_name; + uint64_t m_snap_id = CEPH_NOSNAP; + + void handle_open(int r, Context* on_finish); + void handle_snap_set(int r, Context* on_finish); }; diff --git a/src/test/cli/rbd/help.t b/src/test/cli/rbd/help.t index fcc3cd9126b..fb9f1b4737c 100644 --- a/src/test/cli/rbd/help.t +++ b/src/test/cli/rbd/help.t @@ -1491,7 +1491,7 @@ [--source-spec-path ] [--source-spec ] [--pool ] [--namespace ] [--image ] - [--dest-pool ] + [--snap ] [--dest-pool ] [--dest-namespace ] [--dest ] [--image-format ] [--new-format] [--order ] @@ -1504,40 +1504,44 @@ [--journal-splay-width ] [--journal-object-size ] [--journal-pool ] [--flatten] - + Prepare image migration. Positional arguments - source image specification - (example: [/[/]]) - destination image specification - (example: [/[/]]) + source image or snapshot specification + (example: + [/[/]][@]) + destination image specification + (example: + [/[/]]) Optional arguments - --import-only only import data from source - --source-spec-path arg source-spec file (or '-' for stdin) - --source-spec arg source-spec - -p [ --pool ] arg source pool name - --namespace arg source namespace name - --image arg source image name - --dest-pool arg destination pool name - --dest-namespace arg destination namespace name - --dest arg destination image name - --image-format arg image format [default: 2] - --object-size arg object size in B/K/M [4K <= object size <= 32M] - --image-feature arg image features - [layering(+), exclusive-lock(+*), object-map(+*), - deep-flatten(+-), journaling(*)] - --image-shared shared image - --stripe-unit arg stripe unit in B/K/M - --stripe-count arg stripe count - --data-pool arg data pool - --mirror-image-mode arg mirror image mode [journal or snapshot] - --journal-splay-width arg number of active journal objects - --journal-object-size arg size of journal objects [4K <= size <= 64M] - --journal-pool arg pool for journal objects - --flatten fill clone with parent data (make it independent) + --import-only only import data from source + --source-spec-path arg source-spec file (or '-' for stdin) + --source-spec arg source-spec + -p [ --pool ] arg source pool name + --namespace arg source namespace name + --image arg source image name + --snap arg source snapshot name + --dest-pool arg destination pool name + --dest-namespace arg destination namespace name + --dest arg destination image name + --image-format arg image format [default: 2] + --object-size arg object size in B/K/M [4K <= object size <= 32M] + --image-feature arg image features + [layering(+), exclusive-lock(+*), + object-map(+*), deep-flatten(+-), journaling(*)] + --image-shared shared image + --stripe-unit arg stripe unit in B/K/M + --stripe-count arg stripe count + --data-pool arg data pool + --mirror-image-mode arg mirror image mode [journal or snapshot] + --journal-splay-width arg number of active journal objects + --journal-object-size arg size of journal objects [4K <= size <= 64M] + --journal-pool arg pool for journal objects + --flatten fill clone with parent data (make it independent) Image Features: (*) supports enabling/disabling on existing images diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index 74cde0764d6..503c72c979c 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -2644,13 +2644,15 @@ class TestMigration(object): create_image() with Image(ioctx, image_name) as image: image_id = image.id() + image.create_snap('snap') source_spec = json.dumps( {'type': 'native', 'pool_id': ioctx.get_pool_id(), 'pool_namespace': '', 'image_name': image_name, - 'image_id': image_id}) + 'image_id': image_id, + 'snap_name': 'snap'}) dst_image_name = get_temp_image_name() RBD().migration_prepare_import(source_spec, ioctx, dst_image_name, features=63, order=23, stripe_unit=1<<23, @@ -2667,6 +2669,12 @@ class TestMigration(object): RBD().migration_execute(ioctx, dst_image_name) RBD().migration_commit(ioctx, dst_image_name) + + with Image(ioctx, image_name) as image: + image.remove_snap('snap') + with Image(ioctx, dst_image_name) as image: + image.remove_snap('snap') + RBD().remove(ioctx, dst_image_name) RBD().remove(ioctx, image_name) diff --git a/src/tools/rbd/action/Migration.cc b/src/tools/rbd/action/Migration.cc index eef82d1a1b3..e4e12276820 100644 --- a/src/tools/rbd/action/Migration.cc +++ b/src/tools/rbd/action/Migration.cc @@ -138,7 +138,8 @@ void get_prepare_arguments(po::options_description *positional, "source-spec file (or '-' for stdin)") ("source-spec", po::value(), "source-spec"); - at::add_image_spec_options(positional, options, at::ARGUMENT_MODIFIER_SOURCE); + at::add_image_or_snap_spec_options(positional, options, + at::ARGUMENT_MODIFIER_SOURCE); at::add_image_spec_options(positional, options, at::ARGUMENT_MODIFIER_DEST); at::add_create_image_options(options, true); at::add_flatten_option(options); @@ -146,13 +147,18 @@ void get_prepare_arguments(po::options_description *positional, int execute_prepare(const po::variables_map &vm, const std::vector &ceph_global_init_args) { + bool import_only = vm["import-only"].as(); + size_t arg_index = 0; std::string pool_name; std::string namespace_name; std::string image_name; + std::string snap_name; int r = utils::get_pool_image_snapshot_names( vm, at::ARGUMENT_MODIFIER_SOURCE, &arg_index, &pool_name, &namespace_name, - &image_name, nullptr, true, utils::SNAPSHOT_PRESENCE_NONE, + &image_name, import_only ? &snap_name : nullptr, true, + import_only ? utils::SNAPSHOT_PRESENCE_PERMITTED : + utils::SNAPSHOT_PRESENCE_NONE, utils::SPEC_VALIDATION_NONE); if (r < 0) { return r; @@ -169,7 +175,6 @@ int execute_prepare(const po::variables_map &vm, return r; } - bool import_only = vm["import-only"].as(); std::string source_spec; if (vm.count("source-spec") && vm.count("source-spec-path")) { std::cerr << "rbd: cannot specify both source-image-spec and " @@ -223,12 +228,18 @@ int execute_prepare(const po::variables_map &vm, } if (import_only && source_spec.empty()) { + if (snap_name.empty()) { + std::cerr << "rbd: snapshot name was not specified" << std::endl; + return -EINVAL; + } + std::stringstream ss; ss << R"({)" << R"("type":"native",)" << R"("pool_id":)" << io_ctx.get_id() << R"(,)" << R"("pool_namespace":")" << io_ctx.get_namespace() << R"(",)" - << R"("image_name":")" << image_name << R"(")" + << R"("image_name":")" << image_name << R"(",)" + << R"("snap_name":")" << snap_name << R"(")" << R"(})"; source_spec = ss.str(); @@ -238,12 +249,19 @@ int execute_prepare(const po::variables_map &vm, } io_ctx = dst_io_ctx; image_name = dst_image_name; + snap_name = ""; } else if (!import_only && !source_spec.empty()) { std::cerr << "rbd: --import-only must be used in combination with " << "source-spec/source-spec-path" << std::endl; return -EINVAL; } + if (!snap_name.empty()) { + std::cerr << "rbd: snapshot name specified for a command that doesn't " + << "use it" << std::endl; + return -EINVAL; + } + librbd::ImageOptions opts; r = utils::get_image_options(vm, true, &opts); if (r < 0) {