From: Mahati Chamarthy Date: Thu, 6 Dec 2018 23:44:59 +0000 (-0800) Subject: rbd: move image to trash as a first step X-Git-Tag: v14.1.0~1^2~8 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=4297eff693a2c4dd0e1a00c00561545103b559e7;p=ceph-ci.git rbd: move image to trash as a first step ... while removing an Image Fixes: http://tracker.ceph.com/issues/24226 Signed-off-by: Mahati Chamarthy --- diff --git a/src/include/rbd/librbd.h b/src/include/rbd/librbd.h index c872eb67630..149e59dcc9a 100644 --- a/src/include/rbd/librbd.h +++ b/src/include/rbd/librbd.h @@ -237,7 +237,8 @@ enum { typedef enum { RBD_TRASH_IMAGE_SOURCE_USER = 0, RBD_TRASH_IMAGE_SOURCE_MIRRORING = 1, - RBD_TRASH_IMAGE_SOURCE_MIGRATION = 2 + RBD_TRASH_IMAGE_SOURCE_MIGRATION = 2, + RBD_TRASH_IMAGE_SOURCE_REMOVE = 3 } rbd_trash_image_source_t; typedef struct { diff --git a/src/librbd/api/Image.cc b/src/librbd/api/Image.cc index 8c3b63d2eaa..1d36070eb0a 100644 --- a/src/librbd/api/Image.cc +++ b/src/librbd/api/Image.cc @@ -690,10 +690,49 @@ int Image::remove(IoCtx& io_ctx, const std::string &image_name, ConfigProxy config(cct->_conf); Config::apply_pool_overrides(io_ctx, &config); - if (config.get_val("rbd_move_to_trash_on_remove")) { - return Trash::move( - io_ctx, RBD_TRASH_IMAGE_SOURCE_USER, image_name, - config.get_val("rbd_move_to_trash_on_remove_expire_seconds")); + std::string image_id; + int r = cls_client::dir_get_id(&io_ctx, RBD_DIRECTORY, image_name, + &image_id); + if (r < 0 && r != -ENOENT) { + lderr(cct) << "failed to retrieve image id: " << cpp_strerror(r) << dendl; + return r; + } + + if(r == 0) { + auto expire_seconds = config.get_val( + "rbd_move_to_trash_on_remove_expire_seconds"); + + if (config.get_val("rbd_move_to_trash_on_remove")) { + return Trash::move_by_id( + io_ctx, RBD_TRASH_IMAGE_SOURCE_USER, image_id, image_name, + expire_seconds, true); + } else { + r = Trash::move_by_id( + io_ctx, RBD_TRASH_IMAGE_SOURCE_REMOVE, image_id, image_name, + expire_seconds, true); + + if (r == -ENOENT) { + // check if it already exists in trash from an aborted + // remove attempt + std::vector trash_entries; + r = Trash::list(io_ctx, trash_entries); + if (r < 0) { + return r; + } + + for (auto& entry : trash_entries) { + if (entry.name == image_name && + entry.source == RBD_TRASH_IMAGE_SOURCE_REMOVE) { + return Trash::remove(io_ctx, image_id, force, prog_ctx); + } + } + return -ENOENT; + } else if (r < 0) { + return r; + } + + return Trash::remove(io_ctx, image_id, force, prog_ctx); + } } } diff --git a/src/librbd/api/Trash.cc b/src/librbd/api/Trash.cc index 83f420585c2..effc7a583dc 100644 --- a/src/librbd/api/Trash.cc +++ b/src/librbd/api/Trash.cc @@ -20,6 +20,8 @@ #include "librbd/mirror/EnableRequest.h" #include "librbd/trash/MoveRequest.h" #include +#include "librbd/journal/DisabledPolicy.h" +#include "librbd/image/ListWatchersRequest.h" #define dout_subsys ceph_subsys_rbd #undef dout_prefix @@ -116,27 +118,18 @@ int enable_mirroring(IoCtx &io_ctx, const std::string &image_id) { } // anonymous namespace template -int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, - const std::string &image_name, uint64_t delay) { +int Trash::move_by_id(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, + const std::string &image_id, const std::string &image_name, + uint64_t delay, bool check_for_watchers) { CephContext *cct((CephContext *)io_ctx.cct()); - ldout(cct, 20) << "trash_move " << &io_ctx << " " << image_name + ldout(cct, 20) << &io_ctx << " " << (image_id.empty() ? image_name : image_id) << dendl; - // try to get image id from the directory - std::string image_id; - int r = cls_client::dir_get_id(&io_ctx, RBD_DIRECTORY, image_name, - &image_id); - if (r < 0 && r != -ENOENT) { - lderr(cct) << "failed to retrieve image id: " << cpp_strerror(r) << dendl; - return r; - } - ImageCtx *ictx = new ImageCtx((image_id.empty() ? image_name : ""), image_id, nullptr, io_ctx, false); - r = ictx->state->open(OPEN_FLAG_SKIP_OPEN_PARENT); - if (r == -ENOENT) { - return r; - } else if (r < 0) { + int r = ictx->state->open(OPEN_FLAG_SKIP_OPEN_PARENT); + + if (r < 0 && r != -ENOENT) { lderr(cct) << "failed to open image: " << cpp_strerror(r) << dendl; return r; } else if (ictx->old_format) { @@ -145,49 +138,84 @@ int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, return -EOPNOTSUPP; } - image_id = ictx->id; - ictx->owner_lock.get_read(); - if (ictx->exclusive_lock != nullptr) { - ictx->exclusive_lock->block_requests(0); + std::string id = ictx->id; + + if (id.empty()) { + return r; + } + + if (r == 0) { + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + RWLock::WLocker snap_locker(ictx->snap_lock); + ictx->set_journal_policy(new journal::DisabledPolicy()); + } + + ictx->owner_lock.get_read(); + if (ictx->exclusive_lock != nullptr) { + ictx->exclusive_lock->block_requests(0); + + r = ictx->operations->prepare_image_update(false); + if (r < 0) { + lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl; + ictx->owner_lock.put_read(); + ictx->state->close(); + return -EBUSY; + } + } + ictx->owner_lock.put_read(); + + if (!ictx->migration_info.empty()) { + lderr(cct) << "cannot move migrating image to trash" << dendl; + ictx->state->close(); + return -EINVAL; + } - r = ictx->operations->prepare_image_update(false); + r = disable_mirroring(ictx); if (r < 0) { - lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl; - ictx->owner_lock.put_read(); ictx->state->close(); - return -EBUSY; + return r; } - } - ictx->owner_lock.put_read(); - if (!ictx->migration_info.empty()) { - lderr(cct) << "cannot move migrating image to trash" << dendl; - ictx->state->close(); - return -EINVAL; - } + if (check_for_watchers) { + std::list t_watchers; + int flags = librbd::image::LIST_WATCHERS_FILTER_OUT_MY_INSTANCE | + librbd::image::LIST_WATCHERS_FILTER_OUT_MIRROR_INSTANCES; + C_SaferCond t_on_list_watchers; + auto list_req = librbd::image::ListWatchersRequest::create( + *ictx, flags, &t_watchers, &t_on_list_watchers); + list_req->send(); + r = t_on_list_watchers.wait(); + if (r < 0) { + lderr(cct) << "failed listing watchers:" << cpp_strerror(r) << dendl; + ictx->state->close(); + return r; + } + if (!t_watchers.empty()) { + lderr(cct) << "image has watchers - not moving" << dendl; + ictx->state->close(); + return -EBUSY; + } + } - r = disable_mirroring(ictx); - if (r < 0) { - return r; + ictx->state->close(); } utime_t delete_time{ceph_clock_now()}; utime_t deferment_end_time{delete_time}; deferment_end_time += delay; cls::rbd::TrashImageSpec trash_image_spec{ - static_cast(source), ictx->name, + static_cast(source), image_name, delete_time, deferment_end_time}; trash_image_spec.state = cls::rbd::TRASH_IMAGE_STATE_MOVING; C_SaferCond ctx; - auto req = trash::MoveRequest::create(io_ctx, ictx->id, trash_image_spec, + auto req = trash::MoveRequest::create(io_ctx, id, trash_image_spec, &ctx); req->send(); r = ctx.wait(); - ictx->state->close(); trash_image_spec.state = cls::rbd::TRASH_IMAGE_STATE_NORMAL; - int ret = cls_client::trash_state_set(&io_ctx, image_id, + int ret = cls_client::trash_state_set(&io_ctx, id, trash_image_spec.state, cls::rbd::TRASH_IMAGE_STATE_MOVING); if (ret < 0 && ret != -EOPNOTSUPP) { @@ -200,7 +228,7 @@ int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, } C_SaferCond notify_ctx; - TrashWatcher::notify_image_added(io_ctx, image_id, trash_image_spec, + TrashWatcher::notify_image_added(io_ctx, id, trash_image_spec, ¬ify_ctx); r = notify_ctx.wait(); if (r < 0) { @@ -211,6 +239,27 @@ int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, return 0; } +template +int Trash::move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, + const std::string &image_name, uint64_t delay, + bool check_for_watchers) { + CephContext *cct((CephContext *)io_ctx.cct()); + ldout(cct, 20) << &io_ctx << " " << image_name + << dendl; + + // try to get image id from the directory + std::string image_id; + int r = cls_client::dir_get_id(&io_ctx, RBD_DIRECTORY, image_name, + &image_id); + if (r < 0 && r != -ENOENT) { + lderr(cct) << "failed to retrieve image id: " << cpp_strerror(r) << dendl; + return r; + } + + return Trash::move_by_id(io_ctx, source, image_id, image_name, delay, + check_for_watchers); +} + template int Trash::get(IoCtx &io_ctx, const std::string &id, trash_image_info_t *info) { diff --git a/src/librbd/api/Trash.h b/src/librbd/api/Trash.h index 23382453f14..2c128b03209 100644 --- a/src/librbd/api/Trash.h +++ b/src/librbd/api/Trash.h @@ -21,8 +21,12 @@ namespace api { template struct Trash { + static int move_by_id(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, + const std::string &image_id, const std::string &image_name, + uint64_t delay, bool check_for_watchers=false); static int move(librados::IoCtx &io_ctx, rbd_trash_image_source_t source, - const std::string &image_name, uint64_t delay); + const std::string &image_name, uint64_t delay, + bool check_for_watchers=false); static int get(librados::IoCtx &io_ctx, const std::string &id, trash_image_info_t *info); static int list(librados::IoCtx &io_ctx, diff --git a/src/pybind/rbd/rbd.pyx b/src/pybind/rbd/rbd.pyx index 9688f24ae0f..d0b79b64caf 100644 --- a/src/pybind/rbd/rbd.pyx +++ b/src/pybind/rbd/rbd.pyx @@ -188,6 +188,7 @@ cdef extern from "rbd/librbd.h" nogil: _RBD_TRASH_IMAGE_SOURCE_USER "RBD_TRASH_IMAGE_SOURCE_USER", _RBD_TRASH_IMAGE_SOURCE_MIRRORING "RBD_TRASH_IMAGE_SOURCE_MIRRORING", _RBD_TRASH_IMAGE_SOURCE_MIGRATION "RBD_TRASH_IMAGE_SOURCE_MIGRATION" + _RBD_TRASH_IMAGE_SOURCE_REMOVE "RBD_TRASH_IMAGE_SOURCE_REMOVE" ctypedef struct rbd_trash_image_info_t: char *id diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index e7a731d6a33..c6404f931cd 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -672,6 +672,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) { librbd::ImageCtx *ictx; librbd::ImageCtx *ictx2 = nullptr; std::string clone_name = get_temp_image_name(); + std::string image_id; ASSERT_EQ(0, open_image(m_image_name, &ictx)); ASSERT_EQ(0, snap_create(*ictx, "snap")); @@ -682,7 +683,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) { } librbd::NoOpProgressContext no_op; - ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, clone_name, "", no_op)); + ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, "", image_id, no_op)); ASSERT_EQ(0, ictx->operations->snap_unprotect(cls::rbd::UserSnapshotNamespace(), "snap")); }; @@ -691,6 +692,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) { clone_name.c_str(), ictx->features, &order, 0, 0)); ASSERT_EQ(0, open_image(clone_name, &ictx2)); + image_id = ictx2->id; MockRefreshImageCtx mock_image_ctx(*ictx2); MockRefreshParentRequest *mock_refresh_parent_request = new MockRefreshParentRequest(); @@ -725,6 +727,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChildDontOpenParent) { librbd::ImageCtx *ictx; librbd::ImageCtx *ictx2 = nullptr; + std::string image_id; std::string clone_name = get_temp_image_name(); ASSERT_EQ(0, open_image(m_image_name, &ictx)); @@ -736,7 +739,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChildDontOpenParent) { } librbd::NoOpProgressContext no_op; - ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, clone_name, "", no_op)); + ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, "", image_id, no_op)); ASSERT_EQ(0, ictx->operations->snap_unprotect(cls::rbd::UserSnapshotNamespace(), "snap")); }; @@ -745,6 +748,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChildDontOpenParent) { clone_name.c_str(), ictx->features, &order, 0, 0)); ASSERT_EQ(0, open_image(clone_name, &ictx2)); + image_id = ictx2->id; MockRefreshImageCtx mock_image_ctx(*ictx2); MockExclusiveLock mock_exclusive_lock; diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index afd4a543fef..acf1a222728 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -3377,6 +3377,7 @@ TEST_F(TestLibRBD, ListChildren) librados::IoCtx ioctx3; ASSERT_EQ(0, _rados.ioctx_create(pool_name2.c_str(), ioctx3)); + ASSERT_EQ(0, rbd_close(image3)); ASSERT_EQ(0, rbd.trash_move(ioctx3, child_name3.c_str(), 0)); test_list_children(parent, 2, pool_name2.c_str(), child_name1.c_str(), pool_name1.c_str(), child_name2.c_str()); @@ -3420,7 +3421,6 @@ TEST_F(TestLibRBD, ListChildren) child_id3, pool_name2.c_str(), child_name3.c_str(), false, child_id4, pool_name2.c_str(), child_name4.c_str(), false); - ASSERT_EQ(0, rbd_close(image3)); ASSERT_EQ(0, rbd_remove(ioctx2, child_name3.c_str())); test_list_children(parent, 2, pool_name1.c_str(), child_name2.c_str(), @@ -3562,6 +3562,7 @@ TEST_F(TestLibRBD, ListChildrenTiered) librados::IoCtx ioctx3; ASSERT_EQ(0, _rados.ioctx_create(pool_name2.c_str(), ioctx3)); + ASSERT_EQ(0, rbd_close(image3)); ASSERT_EQ(0, rbd.trash_move(ioctx3, child_name3.c_str(), 0)); test_list_children(parent, 2, pool_name2.c_str(), child_name1.c_str(), pool_name1.c_str(), child_name2.c_str()); @@ -3605,7 +3606,6 @@ TEST_F(TestLibRBD, ListChildrenTiered) child_id3, pool_name2.c_str(), child_name3.c_str(), false, child_id4, pool_name2.c_str(), child_name4.c_str(), false); - ASSERT_EQ(0, rbd_close(image3)); ASSERT_EQ(0, rbd_remove(ioctx2, child_name3.c_str())); test_list_children(parent, 2, pool_name1.c_str(), child_name2.c_str(), @@ -7206,7 +7206,7 @@ TEST_F(TestLibRBD, Migration) { ASSERT_EQ(status.state, RBD_IMAGE_MIGRATION_STATE_PREPARED); rbd_migration_status_cleanup(&status); - ASSERT_EQ(-EBUSY, rbd_remove(ioctx, name.c_str())); + ASSERT_EQ(-EINVAL, rbd_remove(ioctx, name.c_str())); ASSERT_EQ(-EINVAL, rbd_trash_move(ioctx, name.c_str(), 0)); ASSERT_EQ(0, rbd_migration_execute(ioctx, name.c_str())); @@ -7223,7 +7223,7 @@ TEST_F(TestLibRBD, Migration) { ASSERT_EQ(0, rbd_migration_prepare(ioctx, name.c_str(), ioctx, new_name.c_str(), image_options)); - ASSERT_EQ(-EBUSY, rbd_remove(ioctx, new_name.c_str())); + ASSERT_EQ(-EINVAL, rbd_remove(ioctx, new_name.c_str())); ASSERT_EQ(-EINVAL, rbd_trash_move(ioctx, new_name.c_str(), 0)); ASSERT_EQ(0, rbd_migration_abort(ioctx, name.c_str())); @@ -7269,7 +7269,7 @@ TEST_F(TestLibRBD, MigrationPP) { ASSERT_NE(status.dest_image_id, ""); ASSERT_EQ(status.state, RBD_IMAGE_MIGRATION_STATE_PREPARED); - ASSERT_EQ(-EBUSY, rbd.remove(ioctx, name.c_str())); + ASSERT_EQ(-EINVAL, rbd.remove(ioctx, name.c_str())); ASSERT_EQ(-EINVAL, rbd.trash_move(ioctx, name.c_str(), 0)); ASSERT_EQ(0, rbd.migration_execute(ioctx, name.c_str())); @@ -7285,7 +7285,7 @@ TEST_F(TestLibRBD, MigrationPP) { ASSERT_EQ(0, rbd.migration_prepare(ioctx, name.c_str(), ioctx, new_name.c_str(), image_options)); - ASSERT_EQ(-EBUSY, rbd.remove(ioctx, new_name.c_str())); + ASSERT_EQ(-EINVAL, rbd.remove(ioctx, new_name.c_str())); ASSERT_EQ(-EINVAL, rbd.trash_move(ioctx, new_name.c_str(), 0)); ASSERT_EQ(0, rbd.migration_abort(ioctx, name.c_str())); diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc index 4b2211e24da..71306ba5157 100644 --- a/src/test/librbd/test_mirroring.cc +++ b/src/test/librbd/test_mirroring.cc @@ -23,6 +23,7 @@ #include "librbd/io/ImageRequest.h" #include "librbd/io/ImageRequestWQ.h" #include "librbd/journal/Types.h" +#include "librbd/api/Image.h" #include "journal/Journaler.h" #include "journal/Settings.h" #include @@ -294,9 +295,12 @@ public: std::string image_id; ASSERT_EQ(0, image.get_id(&image_id)); + image.close(); ASSERT_EQ(0, m_rbd.trash_move(m_ioctx, image_name.c_str(), 100)); + ASSERT_EQ(0, m_rbd.open_by_id(m_ioctx, image, image_id.c_str(), NULL)); + librbd::mirror_image_info_t mirror_image; ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, sizeof(mirror_image))); ASSERT_EQ(mirror_image.state, RBD_MIRROR_IMAGE_DISABLED); @@ -714,12 +718,17 @@ TEST_F(TestMirroring, RemoveBootstrapped) ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_POOL)); uint64_t features = RBD_FEATURE_EXCLUSIVE_LOCK | RBD_FEATURE_JOURNALING; + //uint64_t features = 0; int order = 20; ASSERT_EQ(0, m_rbd.create2(m_ioctx, image_name.c_str(), 4096, features, &order)); librbd::Image image; ASSERT_EQ(0, m_rbd.open(m_ioctx, image, image_name.c_str())); - ASSERT_EQ(-EBUSY, m_rbd.remove(m_ioctx, image_name.c_str())); + std::string image_id; + ASSERT_EQ(0, image.get_id(&image_id)); + + librbd::NoOpProgressContext no_op; + ASSERT_EQ(-EBUSY, librbd::api::Image<>::remove(m_ioctx, "", image_id, no_op)); // simulate the image is open by rbd-mirror bootstrap uint64_t handle; @@ -740,7 +749,7 @@ TEST_F(TestMirroring, RemoveBootstrapped) ASSERT_EQ(0, m_ioctx.create(RBD_MIRRORING, false)); ASSERT_EQ(0, m_ioctx.watch2(RBD_MIRRORING, &handle, &watcher)); // now remove should succeed - ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str())); + ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, "", image_id, no_op)); ASSERT_EQ(0, m_ioctx.unwatch2(handle)); ASSERT_TRUE(watcher.m_notified); ASSERT_EQ(0, image.close()); diff --git a/src/tools/rbd/action/Trash.cc b/src/tools/rbd/action/Trash.cc index 37609324a48..f932f73a242 100644 --- a/src/tools/rbd/action/Trash.cc +++ b/src/tools/rbd/action/Trash.cc @@ -266,6 +266,9 @@ int do_list(librbd::RBD &rbd, librados::IoCtx& io_ctx, bool long_flag, case RBD_TRASH_IMAGE_SOURCE_MIGRATION: del_source = "MIGRATION"; break; + case RBD_TRASH_IMAGE_SOURCE_REMOVE: + del_source = "REMOVE"; + break; } std::string time_str = ctime(&entry.deletion_time);