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 <dillaman@redhat.com>
(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

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 e72c13c509459f8095bebd2fdb0d9c3715c5fd99..7d9e26b60f862dd55bfaa202546e713588d53c02 100644 (file)
--- 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<MockTestImageCtx> {
+  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<MockTestImageCtx>* TrashWatcher<MockTestImageCtx>::s_instance = nullptr;
+
+namespace trash {
 
 template <>
 struct RemoveRequest<librbd::MockTestImageCtx> {
@@ -32,17 +51,13 @@ struct RemoveRequest<librbd::MockTestImageCtx> {
   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<librbd::MockTestImageCtx> {
 
 RemoveRequest<librbd::MockTestImageCtx>* RemoveRequest<librbd::MockTestImageCtx>::s_instance = nullptr;
 
-} // namespace image
+} // namespace trash
 } // namespace librbd
 
 namespace rbd {
@@ -111,7 +126,19 @@ class TestMockImageDeleterTrashRemoveRequest : public TestMockFixture {
 public:
   typedef TrashRemoveRequest<librbd::MockTestImageCtx> MockTrashRemoveRequest;
   typedef SnapshotPurgeRequest<librbd::MockTestImageCtx> MockSnapshotPurgeRequest;
-  typedef librbd::image::RemoveRequest<librbd::MockTestImageCtx> MockImageRemoveRequest;
+  typedef librbd::TrashWatcher<librbd::MockTestImageCtx> MockTrashWatcher;
+  typedef librbd::trash::RemoveRequest<librbd::MockTestImageCtx> 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 bd1ae214ed876d1498e5217edbae5a2205f0dada..9aa1ace5cf3a2329c5c56e612d6916312f34fe69 100644 (file)
--- 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 <typename I>
 void TrashRemoveRequest<I>::send() {
   *m_error_result = ERROR_RESULT_RETRY;
 
+  get_trash_image_spec();
+}
+
+template <typename I>
+void TrashRemoveRequest<I>::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<I>,
+    &TrashRemoveRequest<I>::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 <typename I>
+void TrashRemoveRequest<I>::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<I>::remove_image() {
   auto ctx = create_context_callback<
     TrashRemoveRequest<I>,
     &TrashRemoveRequest<I>::handle_remove_image>(this);
-  auto req = librbd::image::RemoveRequest<I>::create(
-    m_io_ctx, "", m_image_id, true, true, m_progress_ctx, m_op_work_queue,
+  auto req = librbd::trash::RemoveRequest<I>::create(
+    m_io_ctx, m_image_id, m_op_work_queue, true, m_progress_ctx,
     ctx);
   req->send();
 }
@@ -136,6 +177,27 @@ void TrashRemoveRequest<I>::handle_remove_image(int r) {
     return;
   }
 
+  notify_trash_removed();
+}
+
+template <typename I>
+void TrashRemoveRequest<I>::notify_trash_removed() {
+  dout(10) << dendl;
+
+  Context *ctx = create_context_callback<
+    TrashRemoveRequest<I>,
+    &TrashRemoveRequest<I>::handle_notify_trash_removed>(this);
+  librbd::TrashWatcher<I>::notify_image_removed(m_io_ctx, m_image_id, ctx);
+}
+
+template <typename I>
+void TrashRemoveRequest<I>::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 c2aef03801721f73ae321a27c354bc0c73167f49..c2c97f1a052184ee3bc492e29241cfed82c71de4 100644 (file)
--- 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 <string>
@@ -47,13 +48,19 @@ private:
    * <start>
    *    |
    *    v
+   * GET_TRASH_IMAGE_SPEC
+   *    |
+   *    v
    * GET_SNAP_CONTEXT
    *    |
    *    v
    * PURGE_SNAPSHOTS
    *    |
    *    v
-   * REMOVE_IMAGE
+   * TRASH_REMOVE
+   *    |
+   *    v
+   * NOTIFY_TRASH_REMOVE
    *    |
    *    v
    * <finish>
@@ -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);
 
 };