rbd-mirror: fix races in snapshot-based mirroring deletion propagation

When remote image is deleted, rbd-mirror can encounter three cases:

  1) no remote image id
  2) no remote mirror metadata
  3) MIRROR_IMAGE_STATE_DISABLING in remote mirror metadata

Commit d4c66ac5c615 ("rbd-mirror: fix issue with snapshot-based
mirroring deletion propagation") fixed case 1.  Cases 2 and 3 remained
broken because for both of them finalize_snapshot_state_builder() would
populate not only remote_mirror_peer_uuid but also remote_image_id,
thus disabling ENOLINK logic in handle_prepare_remote_image() and
handle_bootstrap().  Commit ff60aec2d9ef ("rbd-mirror: fix bootstrap
sequence while the image is removed") touched on case 3, but it made
a difference only for journal-based mirroring.

Stop calling finalize_snapshot_state_builder() on errors.  Instead,
align with journal-based mirroring by filling remote_mirror_peer_uuid
together with remote_mirror_uuid.

Fixes: https://tracker.ceph.com/issues/53963
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc
index e49b40821821a5190fdcadbf304dc091dd553c58..2c4b550a6a44b4696f63e747d275958dab513331 100644 (file)
--- a/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc
+++ b/src/test/rbd_mirror/image_replayer/test_mock_PrepareRemoteImageRequest.cc
@@ -586,6 +586,229 @@ TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, RegisterClientError) {
   ASSERT_EQ(-EINVAL, ctx.wait());
 }
 
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorImageIdDNEJournal) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request, "", -ENOENT);
+
+  MockJournalStateBuilder mock_journal_state_builder;
+  MockStateBuilder* mock_state_builder = &mock_journal_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid", ""},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL,
+           mock_journal_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_journal_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("", mock_journal_state_builder.remote_image_id);
+}
+
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorImageIdDNESnapshot) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request, "", -ENOENT);
+
+  MockSnapshotStateBuilder mock_snapshot_state_builder;
+  MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid",
+                                                    "remote mirror peer uuid"},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT,
+           mock_snapshot_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_snapshot_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("remote mirror peer uuid",
+            mock_snapshot_state_builder.remote_mirror_peer_uuid);
+  ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id);
+}
+
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDNEJournal) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request,
+                             "remote image id", 0);
+
+  MockGetMirrorInfoRequest mock_get_mirror_info_request;
+  expect_get_mirror_info(mock_get_mirror_info_request,
+                         {cls::rbd::MIRROR_IMAGE_MODE_JOURNAL,
+                          "global image id",
+                          cls::rbd::MIRROR_IMAGE_STATE_ENABLED},
+                         librbd::mirror::PROMOTION_STATE_PRIMARY,
+                         "remote mirror uuid", -ENOENT);
+
+  MockJournalStateBuilder mock_journal_state_builder;
+  MockStateBuilder* mock_state_builder = &mock_journal_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid", ""},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL,
+           mock_journal_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_journal_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("", mock_journal_state_builder.remote_image_id);
+}
+
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDNESnapshot) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request,
+                             "remote image id", 0);
+
+  MockGetMirrorInfoRequest mock_get_mirror_info_request;
+  expect_get_mirror_info(mock_get_mirror_info_request,
+                         {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT,
+                          "global image id",
+                          cls::rbd::MIRROR_IMAGE_STATE_ENABLED},
+                         librbd::mirror::PROMOTION_STATE_PRIMARY,
+                         "remote mirror uuid", -ENOENT);
+
+  MockSnapshotStateBuilder mock_snapshot_state_builder;
+  MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid",
+                                                    "remote mirror peer uuid"},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT,
+           mock_snapshot_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_snapshot_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("remote mirror peer uuid",
+            mock_snapshot_state_builder.remote_mirror_peer_uuid);
+  ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id);
+}
+
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDisablingJournal) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request,
+                             "remote image id", 0);
+
+  MockGetMirrorInfoRequest mock_get_mirror_info_request;
+  expect_get_mirror_info(mock_get_mirror_info_request,
+                         {cls::rbd::MIRROR_IMAGE_MODE_JOURNAL,
+                          "global image id",
+                          cls::rbd::MIRROR_IMAGE_STATE_DISABLING},
+                         librbd::mirror::PROMOTION_STATE_PRIMARY,
+                         "remote mirror uuid", 0);
+
+  MockJournalStateBuilder mock_journal_state_builder;
+  expect_get_mirror_image_mode(mock_journal_state_builder,
+                               cls::rbd::MIRROR_IMAGE_MODE_JOURNAL);
+  MockStateBuilder* mock_state_builder = &mock_journal_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid", ""},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_JOURNAL,
+           mock_journal_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_journal_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("", mock_journal_state_builder.remote_image_id);
+}
+
+TEST_F(TestMockImageReplayerPrepareRemoteImageRequest, MirrorInfoDisablingSnapshot) {
+  MockThreads mock_threads(m_threads);
+
+  InSequence seq;
+  MockGetMirrorImageIdRequest mock_get_mirror_image_id_request;
+  expect_get_mirror_image_id(mock_get_mirror_image_id_request,
+                             "remote image id", 0);
+
+  MockGetMirrorInfoRequest mock_get_mirror_info_request;
+  expect_get_mirror_info(mock_get_mirror_info_request,
+                         {cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT,
+                          "global image id",
+                          cls::rbd::MIRROR_IMAGE_STATE_DISABLING},
+                         librbd::mirror::PROMOTION_STATE_PRIMARY,
+                         "remote mirror uuid", 0);
+
+  MockSnapshotStateBuilder mock_snapshot_state_builder;
+  expect_get_mirror_image_mode(mock_snapshot_state_builder,
+                               cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT);
+  MockStateBuilder* mock_state_builder = &mock_snapshot_state_builder;
+  C_SaferCond ctx;
+  auto req = MockPrepareRemoteImageRequest::create(&mock_threads,
+                                                   m_local_io_ctx,
+                                                   m_remote_io_ctx,
+                                                   "global image id",
+                                                   "local mirror uuid",
+                                                   {"remote mirror uuid",
+                                                    "remote mirror peer uuid"},
+                                                   nullptr,
+                                                   &mock_state_builder,
+                                                   &ctx);
+  req->send();
+
+  ASSERT_EQ(-ENOENT, ctx.wait());
+  ASSERT_EQ(cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT,
+           mock_snapshot_state_builder.mirror_image_mode);
+  ASSERT_EQ("remote mirror uuid",
+            mock_snapshot_state_builder.remote_mirror_uuid);
+  ASSERT_EQ("remote mirror peer uuid",
+            mock_snapshot_state_builder.remote_mirror_peer_uuid);
+  ASSERT_EQ("", mock_snapshot_state_builder.remote_image_id);
+}
+
 } // namespace image_replayer
 } // namespace mirror
 } // namespace rbd

diff --git a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc
index 08d3dd7806fc3d7234f89884e82ed1cf29f2ae6b..b1e37ed084b80c1131c5cee8f6191be9b19d35bd 100644 (file)
--- a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc
+++ b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc
@@ -38,6 +38,10 @@ template <typename I>
 void PrepareRemoteImageRequest<I>::send() {
   if (*m_state_builder != nullptr) {
     (*m_state_builder)->remote_mirror_uuid = m_remote_pool_meta.mirror_uuid;
+    auto state_builder = dynamic_cast<snapshot::StateBuilder<I>*>(*m_state_builder);
+    if (state_builder) {
+      state_builder->remote_mirror_peer_uuid = m_remote_pool_meta.mirror_peer_uuid;
+    }
   }
 
   get_remote_image_id();
@@ -62,7 +66,6 @@ void PrepareRemoteImageRequest<I>::handle_get_remote_image_id(int r) {
            << "remote_image_id=" << m_remote_image_id << dendl;
 
   if (r == -ENOENT) {
-    finalize_snapshot_state_builder(r);
     finish(r);
     return;
   } else if (r < 0) {
@@ -93,7 +96,6 @@ void PrepareRemoteImageRequest<I>::handle_get_mirror_info(int r) {
 
   if (r == -ENOENT) {
     dout(10) << "image " << m_global_image_id << " not mirrored" << dendl;
-    finalize_snapshot_state_builder(r);
     finish(r);
     return;
   } else if (r < 0) {
@@ -130,7 +132,7 @@ void PrepareRemoteImageRequest<I>::handle_get_mirror_info(int r) {
     get_client();
     break;
   case cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT:
-    finalize_snapshot_state_builder(0);
+    finalize_snapshot_state_builder();
     finish(0);
     break;
   default:
@@ -252,26 +254,21 @@ void PrepareRemoteImageRequest<I>::finalize_journal_state_builder(
 }
 
 template <typename I>
-void PrepareRemoteImageRequest<I>::finalize_snapshot_state_builder(int r) {
+void PrepareRemoteImageRequest<I>::finalize_snapshot_state_builder() {
   snapshot::StateBuilder<I>* state_builder = nullptr;
   if (*m_state_builder != nullptr) {
     state_builder = dynamic_cast<snapshot::StateBuilder<I>*>(*m_state_builder);
-  } else if (r >= 0) {
+    ceph_assert(state_builder != nullptr);
+  } else {
     state_builder = snapshot::StateBuilder<I>::create(m_global_image_id);
     *m_state_builder = state_builder;
   }
 
-  if (state_builder == nullptr) {
-    // local image prepare failed or is using journal-based mirroring
-    return;
-  }
-
   dout(10) << "remote_mirror_uuid=" << m_remote_pool_meta.mirror_uuid << ", "
            << "remote_mirror_peer_uuid="
            << m_remote_pool_meta.mirror_peer_uuid << ", "
            << "remote_image_id=" << m_remote_image_id << ", "
            << "remote_promotion_state=" << m_promotion_state << dendl;
-  ceph_assert(state_builder != nullptr);
   state_builder->remote_mirror_uuid = m_remote_pool_meta.mirror_uuid;
   state_builder->remote_mirror_peer_uuid = m_remote_pool_meta.mirror_peer_uuid;
   state_builder->remote_image_id = m_remote_image_id;

diff --git a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h
index 0d0129d55826bb98ec44bb13699a298ef75cccf6..483cfc00195f93c0d00f3f7213732bbfab60891a 100644 (file)
--- a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h
+++ b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.h
@@ -139,7 +139,7 @@ private:
 
   void finalize_journal_state_builder(cls::journal::ClientState client_state,
                                       const MirrorPeerClientMeta& client_meta);
-  void finalize_snapshot_state_builder(int r);
+  void finalize_snapshot_state_builder();
 
   void finish(int r);
 };