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)

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 0da7988394aed210a489205dfd76f7bbd05afa53..be875704741cf3e647bbc53304401124cc24de38 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) {
     ceph_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,32 @@ 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_trash_state_set(const std::string& image_id, int r) {
+    bufferlist in_bl;
+    encode(image_id, in_bl);
+    encode(cls::rbd::TRASH_IMAGE_STATE_REMOVING, in_bl);
+    encode(cls::rbd::TRASH_IMAGE_STATE_NORMAL, in_bl);
+
+    EXPECT_CALL(get_mock_io_ctx(m_local_io_ctx),
+                exec(StrEq(RBD_TRASH), _, StrEq("rbd"),
+                     StrEq("trash_state_set"),
+                     ContentsEqual(in_bl), _, _))
+      .WillOnce(Return(r));
+  }
 
   void expect_get_snapcontext(const std::string& image_id,
                               const ::SnapContext &snapc, int r) {
@@ -137,7 +177,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 +186,121 @@ 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_trash_state_set("image id", 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",
+                                            &error_result,
+                                            m_threads->work_queue, &ctx);
+  req->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
+TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashStateIncorrect) {
+  InSequence seq;
+
+  cls::rbd::TrashImageSpec trash_image_spec{
+    cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}};
+  trash_image_spec.state = cls::rbd::TRASH_IMAGE_STATE_RESTORING;
+  expect_trash_get(trash_image_spec, 0);
+
+  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(-EBUSY, ctx.wait());
+  ASSERT_EQ(ERROR_RESULT_RETRY_IMMEDIATELY, error_result);
+}
+
+TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashSetStateDNE) {
+  InSequence seq;
+
+  cls::rbd::TrashImageSpec trash_image_spec{
+    cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}};
+  expect_trash_get(trash_image_spec, 0);
+
+  expect_trash_state_set("image id", -ENOENT);
+
   C_SaferCond ctx;
   ErrorResult error_result;
   auto req = MockTrashRemoveRequest::create(m_local_io_ctx, "image id",
@@ -167,13 +310,41 @@ TEST_F(TestMockImageDeleterTrashRemoveRequest, Success) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockImageDeleterTrashRemoveRequest, TrashSetStateError) {
+  InSequence seq;
+
+  cls::rbd::TrashImageSpec trash_image_spec{
+    cls::rbd::TRASH_IMAGE_SOURCE_MIRRORING, "image name", {}, {}};
+  expect_trash_get(trash_image_spec, 0);
+
+  expect_trash_state_set("image id", -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, 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_trash_state_set("image id", 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 +356,13 @@ 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_trash_state_set("image id", 0);
+
   expect_get_snapcontext("image id", {1, {1}}, -EINVAL);
 
   C_SaferCond ctx;
@@ -198,6 +376,13 @@ 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_trash_state_set("image id", 0);
+
   expect_get_snapcontext("image id", {1, {1}}, 0);
 
   MockSnapshotPurgeRequest mock_snapshot_purge_request;
@@ -215,6 +400,13 @@ 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_trash_state_set("image id", 0);
+
   expect_get_snapcontext("image id", {1, {1}}, 0);
 
   MockSnapshotPurgeRequest mock_snapshot_purge_request;
@@ -231,12 +423,19 @@ 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_trash_state_set("image id", 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 c08a0a8c0a5fae7a630c68f048106d2576dab87f..e7c725dc9c2279c6531d9dc0c1822a04b6308253 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,95 @@ 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.cbegin();
+    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;
+  }
+
+  if (m_trash_image_spec.state != cls::rbd::TRASH_IMAGE_STATE_NORMAL &&
+      m_trash_image_spec.state != cls::rbd::TRASH_IMAGE_STATE_REMOVING) {
+    dout(10) << "image " << m_image_id << " is not in an expected trash state: "
+             << m_trash_image_spec.state << dendl;
+    *m_error_result = ERROR_RESULT_RETRY_IMMEDIATELY;
+    finish(-EBUSY);
+    return;
+  }
+
+  set_trash_state();
+}
+
+template <typename I>
+void TrashRemoveRequest<I>::set_trash_state() {
+  if (m_trash_image_spec.state == cls::rbd::TRASH_IMAGE_STATE_REMOVING) {
+    get_snap_context();
+    return;
+  }
+
+  dout(10) << dendl;
+
+  librados::ObjectWriteOperation op;
+  librbd::cls_client::trash_state_set(&op, m_image_id,
+                                      cls::rbd::TRASH_IMAGE_STATE_REMOVING,
+                                      cls::rbd::TRASH_IMAGE_STATE_NORMAL);
+
+  auto aio_comp = create_rados_callback<
+    TrashRemoveRequest<I>,
+    &TrashRemoveRequest<I>::handle_set_trash_state>(this);
+  int r = m_io_ctx.aio_operate(RBD_TRASH, aio_comp, &op);
+  ceph_assert(r == 0);
+  aio_comp->release();
+}
+
+template <typename I>
+void TrashRemoveRequest<I>::handle_set_trash_state(int r) {
+  dout(10) << "r=" << r << dendl;
+
+  if (r == -ENOENT) {
+    dout(10) << "image id " << m_image_id << " not in mirroring trash" << dendl;
+    finish(0);
+    return;
+  } else if (r < 0 && r != -EOPNOTSUPP) {
+    derr << "error setting trash image state for image id " << m_image_id
+         << ": " << cpp_strerror(r) << dendl;
+    finish(r);
+    return;
+  }
+
   get_snap_context();
 }
 
@@ -111,8 +201,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 +226,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..d2295e8ed2ab2c23b0b0e379f2cf9205192a99b7 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,22 @@ private:
    * <start>
    *    |
    *    v
+   * GET_TRASH_IMAGE_SPEC
+   *    |
+   *    v
+   * SET_TRASH_STATE
+   *    |
+   *    v
    * GET_SNAP_CONTEXT
    *    |
    *    v
    * PURGE_SNAPSHOTS
    *    |
    *    v
-   * REMOVE_IMAGE
+   * TRASH_REMOVE
+   *    |
+   *    v
+   * NOTIFY_TRASH_REMOVE
    *    |
    *    v
    * <finish>
@@ -68,9 +78,16 @@ 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 set_trash_state();
+  void handle_set_trash_state(int r);
+
   void get_snap_context();
   void handle_get_snap_context(int r);
 
@@ -80,6 +97,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);
 
 };