From d202faded6223d993d7f8c3d3f4f15e278a197a1 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 11 Sep 2019 16:30:16 -0400 Subject: [PATCH] rbd-mirror: prevent restored trash images from being deleted after delay The image deleter wasn't verifying whether or not an image was still in the trash prior to deleting the image. This not only would incorrectly remove any restored images but it will also leave the image id object and entry within the directory. Fixes: https://tracker.ceph.com/issues/41780 Signed-off-by: Jason Dillaman (cherry picked from commit f091a31d5252bba76598fffdc997275ca531621d) Conflicts: src/test/rbd_mirror/image_deleter/test_mock_TrashRemoveRequest.cc: removed state test cases src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.h/cc: removed state validation --- .../test_mock_TrashRemoveRequest.cc | 146 ++++++++++++++++-- .../image_deleter/TrashRemoveRequest.cc | 68 +++++++- .../image_deleter/TrashRemoveRequest.h | 16 +- 3 files changed, 213 insertions(+), 17 deletions(-) diff --git a/src/test/rbd_mirror/image_deleter/test_mock_TrashRemoveRequest.cc b/src/test/rbd_mirror/image_deleter/test_mock_TrashRemoveRequest.cc index e72c13c509459..7d9e26b60f862 100644 --- a/src/test/rbd_mirror/image_deleter/test_mock_TrashRemoveRequest.cc +++ b/src/test/rbd_mirror/image_deleter/test_mock_TrashRemoveRequest.cc @@ -4,8 +4,9 @@ #include "test/rbd_mirror/test_mock_fixture.h" #include "cls/rbd/cls_rbd_types.h" #include "librbd/ImageCtx.h" +#include "librbd/TrashWatcher.h" #include "librbd/Utils.h" -#include "librbd/image/RemoveRequest.h" +#include "librbd/trash/RemoveRequest.h" #include "tools/rbd_mirror/Threads.h" #include "tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.h" #include "tools/rbd_mirror/image_deleter/TrashRemoveRequest.h" @@ -24,7 +25,25 @@ struct MockTestImageCtx : public librbd::MockImageCtx { } // anonymous namespace -namespace image { +template<> +struct TrashWatcher { + static TrashWatcher* s_instance; + static void notify_image_removed(librados::IoCtx&, + const std::string& image_id, Context *ctx) { + ceph_assert(s_instance != nullptr); + s_instance->notify_image_removed(image_id, ctx); + } + + MOCK_METHOD2(notify_image_removed, void(const std::string&, Context*)); + + TrashWatcher() { + s_instance = this; + } +}; + +TrashWatcher* TrashWatcher::s_instance = nullptr; + +namespace trash { template <> struct RemoveRequest { @@ -32,17 +51,13 @@ struct RemoveRequest { Context *on_finish = nullptr; static RemoveRequest *create(librados::IoCtx &io_ctx, - const std::string &image_name, const std::string &image_id, + ContextWQ *work_queue, bool force, - bool remove_from_trash, librbd::ProgressContext &progress_ctx, - ContextWQ *work_queue, Context *on_finish) { assert(s_instance != nullptr); - EXPECT_TRUE(image_name.empty()); EXPECT_TRUE(force); - EXPECT_TRUE(remove_from_trash); s_instance->construct(image_id); s_instance->on_finish = on_finish; return s_instance; @@ -58,7 +73,7 @@ struct RemoveRequest { RemoveRequest* RemoveRequest::s_instance = nullptr; -} // namespace image +} // namespace trash } // namespace librbd namespace rbd { @@ -111,7 +126,19 @@ class TestMockImageDeleterTrashRemoveRequest : public TestMockFixture { public: typedef TrashRemoveRequest MockTrashRemoveRequest; typedef SnapshotPurgeRequest MockSnapshotPurgeRequest; - typedef librbd::image::RemoveRequest MockImageRemoveRequest; + typedef librbd::TrashWatcher MockTrashWatcher; + typedef librbd::trash::RemoveRequest MockLibrbdTrashRemoveRequest; + + void expect_trash_get(const cls::rbd::TrashImageSpec& trash_spec, int r) { + using ceph::encode; + EXPECT_CALL(get_mock_io_ctx(m_local_io_ctx), + exec(StrEq(RBD_TRASH), _, StrEq("rbd"), + StrEq("trash_get"), _, _, _)) + .WillOnce(WithArg<5>(Invoke([trash_spec, r](bufferlist* bl) { + encode(trash_spec, *bl); + return r; + }))); + } void expect_get_snapcontext(const std::string& image_id, const ::SnapContext &snapc, int r) { @@ -137,7 +164,7 @@ public: })); } - void expect_image_remove(MockImageRemoveRequest &image_remove_request, + void expect_image_remove(MockLibrbdTrashRemoveRequest &image_remove_request, const std::string &image_id, int r) { EXPECT_CALL(image_remove_request, construct(image_id)); EXPECT_CALL(image_remove_request, send()) @@ -146,18 +173,83 @@ public: image_remove_request.on_finish, r); })); } + + void expect_notify_image_removed(MockTrashWatcher& mock_trash_watcher, + const std::string& image_id) { + EXPECT_CALL(mock_trash_watcher, notify_image_removed(image_id, _)) + .WillOnce(WithArg<1>(Invoke([this](Context *ctx) { + m_threads->work_queue->queue(ctx, 0); + }))); + } + }; TEST_F(TestMockImageDeleterTrashRemoveRequest, Success) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, 0); MockSnapshotPurgeRequest mock_snapshot_purge_request; expect_snapshot_purge(mock_snapshot_purge_request, "image id", 0); - MockImageRemoveRequest mock_image_remove_request; + MockLibrbdTrashRemoveRequest mock_image_remove_request; expect_image_remove(mock_image_remove_request, "image id", 0); + MockTrashWatcher mock_trash_watcher; + expect_notify_image_removed(mock_trash_watcher, "image id"); + + C_SaferCond ctx; + ErrorResult error_result; + auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id", + &error_result, + m_threads->work_queue, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashDNE) { + InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, -ENOENT); + + C_SaferCond ctx; + ErrorResult error_result; + auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id", + &error_result, + m_threads->work_queue, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashError) { + InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, -EPERM); + + C_SaferCond ctx; + ErrorResult error_result; + auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id", + &error_result, + m_threads->work_queue, &ctx); + req->send(); + ASSERT_EQ(-EPERM, ctx.wait()); +} + +TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashSourceIncorrect) { + InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_USER, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + C_SaferCond ctx; ErrorResult error_result; auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id", @@ -169,11 +261,19 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, Success) { TEST_F(TestMockImageDeleterTrashRemoveRequest, GetSnapContextDNE) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, -ENOENT); - MockImageRemoveRequest mock_image_remove_request; + MockLibrbdTrashRemoveRequest mock_image_remove_request; expect_image_remove(mock_image_remove_request, "image id", 0); + MockTrashWatcher mock_trash_watcher; + expect_notify_image_removed(mock_trash_watcher, "image id"); + C_SaferCond ctx; ErrorResult error_result; auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id", @@ -185,6 +285,11 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, GetSnapContextDNE) { TEST_F(TestMockImageDeleterTrashRemoveRequest, GetSnapContextError) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, -EINVAL); C_SaferCond ctx; @@ -198,6 +303,11 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, GetSnapContextError) { TEST_F(TestMockImageDeleterTrashRemoveRequest, PurgeSnapshotBusy) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, 0); MockSnapshotPurgeRequest mock_snapshot_purge_request; @@ -215,6 +325,11 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, PurgeSnapshotBusy) { TEST_F(TestMockImageDeleterTrashRemoveRequest, PurgeSnapshotError) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, 0); MockSnapshotPurgeRequest mock_snapshot_purge_request; @@ -231,12 +346,17 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, PurgeSnapshotError) { TEST_F(TestMockImageDeleterTrashRemoveRequest, RemoveError) { InSequence seq; + + cls::rbd::TrashImageSpec trash_image_spec{ + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}}; + expect_trash_get(trash_image_spec, 0); + expect_get_snapcontext("image id", {1, {1}}, 0); MockSnapshotPurgeRequest mock_snapshot_purge_request; expect_snapshot_purge(mock_snapshot_purge_request, "image id", 0); - MockImageRemoveRequest mock_image_remove_request; + MockLibrbdTrashRemoveRequest mock_image_remove_request; expect_image_remove(mock_image_remove_request, "image id", -EINVAL); C_SaferCond ctx; diff --git a/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.cc b/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.cc index bd1ae214ed876..9aa1ace5cf3a2 100644 --- a/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.cc +++ b/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.cc @@ -9,8 +9,9 @@ #include "cls/rbd/cls_rbd_client.h" #include "librbd/ImageCtx.h" #include "librbd/Journal.h" +#include "librbd/TrashWatcher.h" #include "librbd/Utils.h" -#include "librbd/image/RemoveRequest.h" +#include "librbd/trash/RemoveRequest.h" #include "tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.h" #define dout_context g_ceph_context @@ -30,6 +31,46 @@ template void TrashRemoveRequest::send() { *m_error_result = ERROR_RESULT_RETRY; + get_trash_image_spec(); +} + +template +void TrashRemoveRequest::get_trash_image_spec() { + dout(10) << dendl; + + librados::ObjectReadOperation op; + librbd::cls_client::trash_get_start(&op, m_image_id); + + auto aio_comp = create_rados_callback< + TrashRemoveRequest, + &TrashRemoveRequest::handle_get_trash_image_spec>(this); + m_out_bl.clear(); + int r = m_io_ctx.aio_operate(RBD_TRASH, aio_comp, &op, &m_out_bl); + ceph_assert(r == 0); + aio_comp->release(); +} + +template +void TrashRemoveRequest::handle_get_trash_image_spec(int r) { + dout(10) << "r=" << r << dendl; + + if (r == 0) { + auto bl_it = m_out_bl.begin(); + r = librbd::cls_client::trash_get_finish(&bl_it, &m_trash_image_spec); + } + + if (r == -ENOENT || (r >= 0 && m_trash_image_spec.source != + cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING)) { + dout(10) << "image id " << m_image_id << " not in mirroring trash" << dendl; + finish(0); + return; + } else if (r < 0) { + derr << "error getting image id " << m_image_id << " info from trash: " + << cpp_strerror(r) << dendl; + finish(r); + return; + } + get_snap_context(); } @@ -111,8 +152,8 @@ void TrashRemoveRequest::remove_image() { auto ctx = create_context_callback< TrashRemoveRequest, &TrashRemoveRequest::handle_remove_image>(this); - auto req = librbd::image::RemoveRequest::create( - m_io_ctx, "", m_image_id, true, true, m_progress_ctx, m_op_work_queue, + auto req = librbd::trash::RemoveRequest::create( + m_io_ctx, m_image_id, m_op_work_queue, true, m_progress_ctx, ctx); req->send(); } @@ -136,6 +177,27 @@ void TrashRemoveRequest::handle_remove_image(int r) { return; } + notify_trash_removed(); +} + +template +void TrashRemoveRequest::notify_trash_removed() { + dout(10) << dendl; + + Context *ctx = create_context_callback< + TrashRemoveRequest, + &TrashRemoveRequest::handle_notify_trash_removed>(this); + librbd::TrashWatcher::notify_image_removed(m_io_ctx, m_image_id, ctx); +} + +template +void TrashRemoveRequest::handle_notify_trash_removed(int r) { + dout(10) << "r=" << r << dendl; + + if (r < 0) { + derr << "failed to notify trash watchers: " << cpp_strerror(r) << dendl; + } + finish(0); } diff --git a/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.h b/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.h index c2aef03801721..c2c97f1a05218 100644 --- a/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.h +++ b/src/tools/rbd_mirror/image_deleter/TrashRemoveRequest.h @@ -6,6 +6,7 @@ #include "include/rados/librados.hpp" #include "include/buffer.h" +#include "cls/rbd/cls_rbd_types.h" #include "librbd/internal.h" #include "tools/rbd_mirror/image_deleter/Types.h" #include @@ -47,13 +48,19 @@ private: * * | * v + * GET_TRASH_IMAGE_SPEC + * | + * v * GET_SNAP_CONTEXT * | * v * PURGE_SNAPSHOTS * | * v - * REMOVE_IMAGE + * TRASH_REMOVE + * | + * v + * NOTIFY_TRASH_REMOVE * | * v * @@ -68,9 +75,13 @@ private: Context *m_on_finish; ceph::bufferlist m_out_bl; + cls::rbd::TrashImageSpec m_trash_image_spec; bool m_has_snapshots = false; librbd::NoOpProgressContext m_progress_ctx; + void get_trash_image_spec(); + void handle_get_trash_image_spec(int r); + void get_snap_context(); void handle_get_snap_context(int r); @@ -80,6 +91,9 @@ private: void remove_image(); void handle_remove_image(int r); + void notify_trash_removed(); + void handle_notify_trash_removed(int r); + void finish(int r); }; -- 2.39.5