]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
rbd-mirror: generally skip replay/resync if remote image is not primary
authorIlya Dryomov <idryomov@gmail.com>
Mon, 20 Jun 2022 12:19:41 +0000 (14:19 +0200)
committerIlya Dryomov <idryomov@gmail.com>
Wed, 22 Jun 2022 12:35:00 +0000 (14:35 +0200)
Replay and resync should generally be skipped if the remote image is
not primary.

If this is not done for replay, snapshot-based mirroring can run into
a livelock if the primary image is demoted while a mirror snapshot is
being synced.  On the demote site, rbd-mirror would pick up the just
demoted image, grab the exclusive lock on it and idle waiting for a new
mirror snapshot to be created.  On the (still) non-primary site,
rbd-mirror would eventually finish syncing that mirror snapshot and
attempt to unlink from it on the demote site.  These attempts would
fail with EROFS due to exclusive lock being held in the "refuse proxied
maintenance operations" mode, blocking forward progress (syncing of the
demotion snapshot so that the non-primary image can be orderly promoted
to primary, etc).

If this is not done for resync, data loss can ensue as the just demoted
image would be immediately trashed, underneath the non-primary site that
is still syncing.

Currently this is done in PrepareReplayRequest only for journal-based
mirroring.  Note that it is conditional: if the local image is linked
to the remote image, proceeding is desirable.

Generalize this check, consolidate it with a related check in
PrepareRemoteImageRequest and move the result to BootstrapRequest to
cover both "local image does not exist" and "local image is unlinked"
cases for both modes.

Fixes: https://tracker.ceph.com/issues/54448
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 79d28e63cb47e9bacbb2fd8a5ddc3a092377731b)

src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc
src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc
src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc
src/tools/rbd_mirror/image_replayer/StateBuilder.cc
src/tools/rbd_mirror/image_replayer/StateBuilder.h
src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc

index 9f75e69e609a60f193251905947f0747adcb15f5..c570f69ebf520c1c47dfe0ae407598b43a54f859 100644 (file)
@@ -293,35 +293,6 @@ TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, ResyncRequestedError) {
   ASSERT_EQ(-EINVAL, ctx.wait());
 }
 
-TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, UnlinkedRemoteNonPrimary) {
-  InSequence seq;
-
-  librbd::MockJournal mock_journal;
-  librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);
-  mock_local_image_ctx.journal = &mock_journal;
-
-  // check initial state
-  expect_is_resync_requested(mock_journal, false, 0);
-  expect_journal_get_tag_tid(mock_journal, 345);
-  expect_journal_get_tag_data(mock_journal, {"blah"});
-
-  C_SaferCond ctx;
-  ::journal::MockJournaler mock_remote_journaler;
-  librbd::journal::MirrorPeerClientMeta mirror_peer_client_meta;
-  mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_REPLAYING;
-  mirror_peer_client_meta.image_id = mock_local_image_ctx.id;
-  MockStateBuilder mock_state_builder(mock_local_image_ctx,
-                                      mock_remote_journaler,
-                                      mirror_peer_client_meta);
-  bool resync_requested;
-  bool syncing;
-  auto request = create_request(
-    mock_state_builder, librbd::mirror::PROMOTION_STATE_NON_PRIMARY,
-    "local mirror uuid", &resync_requested, &syncing, &ctx);
-  request->send();
-  ASSERT_EQ(-EREMOTEIO, ctx.wait());
-}
-
 TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, Syncing) {
   InSequence seq;
 
@@ -558,37 +529,6 @@ TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, UpdateClientError) {
   ASSERT_FALSE(syncing);
 }
 
-TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, NonPrimaryRemoteNotTagOwner) {
-  InSequence seq;
-
-  librbd::MockJournal mock_journal;
-  librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);
-  mock_local_image_ctx.journal = &mock_journal;
-
-  // check initial state
-  expect_is_resync_requested(mock_journal, false, 0);
-  expect_journal_get_tag_tid(mock_journal, 345);
-  expect_journal_get_tag_data(mock_journal, {librbd::Journal<>::LOCAL_MIRROR_UUID,
-                                             librbd::Journal<>::ORPHAN_MIRROR_UUID,
-                                             true, 344, 0});
-
-  C_SaferCond ctx;
-  ::journal::MockJournaler mock_remote_journaler;
-  librbd::journal::MirrorPeerClientMeta mirror_peer_client_meta;
-  mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_REPLAYING;
-  mirror_peer_client_meta.image_id = mock_local_image_ctx.id;
-  MockStateBuilder mock_state_builder(mock_local_image_ctx,
-                                      mock_remote_journaler,
-                                      mirror_peer_client_meta);
-  bool resync_requested;
-  bool syncing;
-  auto request = create_request(
-    mock_state_builder, librbd::mirror::PROMOTION_STATE_NON_PRIMARY,
-    "local mirror uuid", &resync_requested, &syncing, &ctx);
-  request->send();
-  ASSERT_EQ(-EREMOTEIO, ctx.wait());
-}
-
 TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, RemoteDemotePromote) {
   InSequence seq;
 
index 53a69a84eefafdf4cca5b377d06c9c777338f61d..4edd1e760bf6a5e3998cbec92b9b4b776c87d48f 100644 (file)
@@ -232,6 +232,7 @@ struct StateBuilder<librbd::MockTestImageCtx> {
 
   MOCK_CONST_METHOD0(is_disconnected, bool());
   MOCK_CONST_METHOD0(is_local_primary, bool());
+  MOCK_CONST_METHOD0(is_remote_primary, bool());
   MOCK_CONST_METHOD0(is_linked, bool());
 
   MOCK_CONST_METHOD0(replay_requires_remote_image, bool());
@@ -355,6 +356,17 @@ public:
       .WillOnce(Return(is_primary));
   }
 
+  void expect_is_remote_primary(MockStateBuilder& mock_state_builder,
+                                bool is_primary) {
+    EXPECT_CALL(mock_state_builder, is_remote_primary())
+      .WillOnce(Return(is_primary));
+  }
+
+  void expect_is_linked(MockStateBuilder& mock_state_builder, bool is_linked) {
+    EXPECT_CALL(mock_state_builder, is_linked())
+      .WillOnce(Return(is_linked));
+  }
+
   void expect_is_disconnected(MockStateBuilder& mock_state_builder,
                               bool is_disconnected) {
     EXPECT_CALL(mock_state_builder, is_disconnected())
@@ -492,6 +504,107 @@ TEST_F(TestMockImageReplayerBootstrapRequest, Success) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
+
+  // open the remote image
+  librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
+  MockOpenImageRequest mock_open_image_request;
+  expect_open_image(mock_open_image_request, m_remote_io_ctx,
+                    mock_remote_image_ctx.id, mock_remote_image_ctx, 0);
+
+  // open the local image
+  librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);
+  MockOpenLocalImageRequest mock_open_local_image_request;
+  expect_open_local_image(mock_open_local_image_request, m_local_io_ctx,
+                          mock_local_image_ctx.id, &mock_local_image_ctx, 0);
+
+  // prepare replay
+  expect_prepare_replay(mock_state_builder, false, false, 0);
+  expect_is_disconnected(mock_state_builder, false);
+
+  // close remote image
+  expect_replay_requires_remote_image(mock_state_builder, false);
+  expect_close_remote_image(mock_state_builder, 0);
+
+  C_SaferCond ctx;
+  MockThreads mock_threads(m_threads);
+  MockInstanceWatcher mock_instance_watcher;
+  MockBootstrapRequest *request = create_request(
+    &mock_threads, &mock_instance_watcher, "global image id",
+    "local mirror uuid", &ctx);
+  request->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
+TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalDNE) {
+  InSequence seq;
+
+  // prepare local image
+  MockStateBuilder mock_state_builder;
+  MockPrepareLocalImageRequest mock_prepare_local_image_request;
+  expect_send(mock_prepare_local_image_request, mock_state_builder,
+              m_local_image_ctx->id, m_local_image_ctx->name, -ENOENT);
+
+  // prepare remote image
+  MockPrepareRemoteImageRequest mock_prepare_remote_image_request;
+  expect_send(mock_prepare_remote_image_request, mock_state_builder,
+              "remote mirror uuid", m_remote_image_ctx->id, 0);
+  expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, false);
+
+  C_SaferCond ctx;
+  MockThreads mock_threads(m_threads);
+  MockInstanceWatcher mock_instance_watcher;
+  MockBootstrapRequest *request = create_request(
+    &mock_threads, &mock_instance_watcher, "global image id",
+    "local mirror uuid", &ctx);
+  request->send();
+  ASSERT_EQ(-EREMOTEIO, ctx.wait());
+}
+
+TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalUnlinked) {
+  InSequence seq;
+
+  // prepare local image
+  MockStateBuilder mock_state_builder;
+  MockPrepareLocalImageRequest mock_prepare_local_image_request;
+  expect_send(mock_prepare_local_image_request, mock_state_builder,
+              m_local_image_ctx->id, m_local_image_ctx->name, 0);
+
+  // prepare remote image
+  MockPrepareRemoteImageRequest mock_prepare_remote_image_request;
+  expect_send(mock_prepare_remote_image_request, mock_state_builder,
+              "remote mirror uuid", m_remote_image_ctx->id, 0);
+  expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, false);
+  expect_is_linked(mock_state_builder, false);
+
+  C_SaferCond ctx;
+  MockThreads mock_threads(m_threads);
+  MockInstanceWatcher mock_instance_watcher;
+  MockBootstrapRequest *request = create_request(
+    &mock_threads, &mock_instance_watcher, "global image id",
+    "local mirror uuid", &ctx);
+  request->send();
+  ASSERT_EQ(-EREMOTEIO, ctx.wait());
+}
+
+TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalLinked) {
+  InSequence seq;
+
+  // prepare local image
+  MockStateBuilder mock_state_builder;
+  MockPrepareLocalImageRequest mock_prepare_local_image_request;
+  expect_send(mock_prepare_local_image_request, mock_state_builder,
+              m_local_image_ctx->id, m_local_image_ctx->name, 0);
+
+  // prepare remote image
+  MockPrepareRemoteImageRequest mock_prepare_remote_image_request;
+  expect_send(mock_prepare_remote_image_request, mock_state_builder,
+              "remote mirror uuid", m_remote_image_ctx->id, 0);
+  expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, false);
+  expect_is_linked(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -537,6 +650,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImageError) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -579,6 +693,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImageDNE) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -632,6 +747,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImagePrimary) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -674,6 +790,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, CreateLocalImageError) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -712,6 +829,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayError) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -756,6 +874,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayResyncRequested) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -801,6 +920,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplaySyncing) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -850,6 +970,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayDisconnected) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -895,6 +1016,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ImageSyncError) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -944,6 +1066,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ImageSyncCanceled) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -990,6 +1113,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, CloseRemoteImageError) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
@@ -1035,6 +1159,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ReplayRequiresRemoteImage) {
   expect_send(mock_prepare_remote_image_request, mock_state_builder,
               "remote mirror uuid", m_remote_image_ctx->id, 0);
   expect_is_local_primary(mock_state_builder, false);
+  expect_is_remote_primary(mock_state_builder, true);
 
   // open the remote image
   librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
index a34f39c15b34a6770384589f3dfe2755d79af04b..0cf52f8d57d64021cb07f866bbabb3d0447b94a1 100644 (file)
@@ -177,10 +177,6 @@ void BootstrapRequest<I>::handle_prepare_remote_image(int r) {
     dout(5) << "local image is primary" << dendl;
     finish(-ENOMSG);
     return;
-  } else if (r == -EREMOTEIO) {
-    dout(10) << "remote-image is non-primary" << cpp_strerror(r) << dendl;
-    finish(r);
-    return;
   } else if (r == -ENOENT || state_builder == nullptr) {
     dout(10) << "remote image does not exist";
     if (state_builder != nullptr) {
@@ -209,6 +205,23 @@ void BootstrapRequest<I>::handle_prepare_remote_image(int r) {
     return;
   }
 
+  if (!state_builder->is_remote_primary()) {
+    ceph_assert(!state_builder->remote_image_id.empty());
+    if (state_builder->local_image_id.empty()) {
+      dout(10) << "local image does not exist and remote image is not primary"
+               << dendl;
+      finish(-EREMOTEIO);
+      return;
+    } else if (!state_builder->is_linked()) {
+      dout(10) << "local image is unlinked and remote image is not primary"
+               << dendl;
+      finish(-EREMOTEIO);
+      return;
+    }
+    // if the local image is linked to the remote image, we ignore that
+    // the remote image is not primary so that we can replay demotion
+  }
+
   open_remote_image();
 }
 
@@ -314,9 +327,7 @@ void BootstrapRequest<I>::handle_prepare_replay(int r) {
   dout(10) << "r=" << r << dendl;
 
   if (r < 0) {
-    if (r != -EREMOTEIO) {
-      derr << "failed to prepare local replay: " << cpp_strerror(r) << dendl;
-    }
+    derr << "failed to prepare local replay: " << cpp_strerror(r) << dendl;
     m_ret_val = r;
     close_remote_image();
     return;
index b1e37ed084b80c1131c5cee8f6191be9b19d35bd..02dfb4f1cfbff0aca2d543016c78c8fb3a61e8a3 100644 (file)
@@ -116,15 +116,6 @@ void PrepareRemoteImageRequest<I>::handle_get_mirror_info(int r) {
     dout(5) << "remote image mirroring is being disabled" << dendl;
     finish(-ENOENT);
     return;
-  } else if (m_promotion_state != librbd::mirror::PROMOTION_STATE_PRIMARY &&
-             (state_builder == nullptr ||
-              state_builder->local_image_id.empty() ||
-              state_builder->local_promotion_state ==
-                librbd::mirror::PROMOTION_STATE_UNKNOWN)) {
-    // no local image and remote isn't primary -- don't sync it
-    dout(5) << "remote image is not primary -- not syncing" << dendl;
-    finish(-EREMOTEIO);
-    return;
   }
 
   switch (m_mirror_image.mode) {
index c8649ab2fa83e87a0f7427d2dcbab043baa47fea..55fb3509d19d20fa477e2abaaf3f5f05db2a889b 100644 (file)
@@ -44,6 +44,15 @@ bool StateBuilder<I>::is_local_primary() const {
   return false;
 }
 
+template <typename I>
+bool StateBuilder<I>::is_remote_primary() const {
+  if (remote_promotion_state == librbd::mirror::PROMOTION_STATE_PRIMARY) {
+    ceph_assert(!remote_image_id.empty());
+    return true;
+  }
+  return false;
+}
+
 template <typename I>
 bool StateBuilder<I>::is_linked() const {
   if (local_promotion_state == librbd::mirror::PROMOTION_STATE_NON_PRIMARY) {
index 4c839a3efcee2effbeb5be536793b8ecf3f0f5a4..51cf8668c1eff3c612e007211a9520173672f49b 100644 (file)
@@ -44,6 +44,7 @@ public:
   virtual bool is_disconnected() const = 0;
 
   bool is_local_primary() const;
+  bool is_remote_primary() const;
   bool is_linked() const;
 
   virtual cls::rbd::MirrorImageMode get_mirror_image_mode() const = 0;
index 69631f27587ede2a4a9264a323b0bcfa347a829e..c8a96a4ad364c7d1fa4607f091e480b61ce83ff6 100644 (file)
@@ -68,17 +68,6 @@ void PrepareReplayRequest<I>::send() {
            << "local tag data=" << m_local_tag_data << dendl;
   image_locker.unlock();
 
-  if (m_local_tag_data.mirror_uuid != m_state_builder->remote_mirror_uuid &&
-      m_remote_promotion_state != librbd::mirror::PROMOTION_STATE_PRIMARY) {
-    // if the local mirror is not linked to the (now) non-primary image,
-    // stop the replay. Otherwise, we ignore that the remote is non-primary
-    // so that we can replay the demotion
-    dout(5) << "remote image is not primary -- skipping image replay"
-            << dendl;
-    finish(-EREMOTEIO);
-    return;
-  }
-
   if (*m_resync_requested) {
     finish(0);
     return;