From 6357026abba61f77bb298564752870c406bf438e Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Sat, 9 Jul 2022 13:35:04 +0200 Subject: [PATCH] rbd-mirror: remove bogus completed_non_primary_snapshots_exist check This check was added in commit ecd3778a6f9a ("rbd-mirror: ensure that the last non-primary snapshot cannot be pruned") as an additional safeguard against pruning an incomplete non-primary snapshot in case there is no predecessor mirror snapshot. However it still fires if the predecessor is there but happens to be a primary demotion snapshot. A bogus "incomplete local non-primary snapshot" error is reported and the replayer gets stuck. Remove completed_non_primary_snapshots_exist tracking as the presence of the predecessor in the incomplete non-primary snapshot pruning arm is already ensured by "m_local_snap_id_start > 0" condition. Fixes: https://tracker.ceph.com/issues/56516 Signed-off-by: Ilya Dryomov (cherry picked from commit a581509381ba84b49c906a1fe440ca3ddcab418c) --- .../snapshot/test_mock_Replayer.cc | 237 ++++++++++++++++++ .../image_replayer/snapshot/Replayer.cc | 15 +- 2 files changed, 239 insertions(+), 13 deletions(-) diff --git a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc index 8cac24c4d6ae5..253d528c06aea 100644 --- a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc +++ b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc @@ -1139,6 +1139,115 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedSyncDelta) { mock_remote_image_ctx)); } +TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedSyncDeltaDemote) { + librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx}; + librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx}; + + MockThreads mock_threads(m_threads); + expect_work_queue_repeatedly(mock_threads); + + MockReplayerListener mock_replayer_listener; + expect_notification(mock_threads, mock_replayer_listener); + + InSequence seq; + + MockInstanceWatcher mock_instance_watcher; + MockImageMeta mock_image_meta; + MockStateBuilder mock_state_builder(mock_local_image_ctx, + mock_remote_image_ctx, + mock_image_meta); + MockReplayer mock_replayer{&mock_threads, &mock_instance_watcher, + "local mirror uuid", &m_pool_meta_cache, + &mock_state_builder, &mock_replayer_listener}; + m_pool_meta_cache.set_remote_pool_meta( + m_remote_io_ctx.get_id(), + {"remote mirror uuid", "remote mirror peer uuid"}); + + librbd::UpdateWatchCtx* update_watch_ctx = nullptr; + ASSERT_EQ(0, init_entry_replayer(mock_replayer, mock_threads, + mock_local_image_ctx, + mock_remote_image_ctx, + mock_replayer_listener, + mock_image_meta, + &update_watch_ctx)); + + // inject an incomplete sync snapshot with last_copied_object_number > 0 + // after a primary demotion snapshot + mock_remote_image_ctx.snap_info = { + {1U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "local mirror uuid", 11, true, 0, + {{11, CEPH_NOSNAP}}}, + 0, {}, 0, 0, {}}}, + {2U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"}, + "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}}; + mock_local_image_ctx.snap_info = { + {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + {12U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "remote mirror uuid", + 2, false, 123, {{2, CEPH_NOSNAP}}}, + 0, {}, 0, 0, {}}}}; + + // re-sync snap2 + expect_load_image_meta(mock_image_meta, false, 0); + expect_is_refresh_required(mock_local_image_ctx, false); + expect_is_refresh_required(mock_remote_image_ctx, false); + MockGetImageStateRequest mock_get_image_state_request; + expect_get_image_state(mock_get_image_state_request, 12, 0); + expect_notify_sync_request(mock_instance_watcher, mock_local_image_ctx.id, 0); + MockImageCopyRequest mock_image_copy_request; + expect_image_copy(mock_image_copy_request, 1, 2, 11, + librbd::deep_copy::ObjectNumber{123U}, + {{2, CEPH_NOSNAP}}, 0); + MockApplyImageStateRequest mock_apply_state_request; + expect_apply_image_state(mock_apply_state_request, 0); + expect_mirror_image_snapshot_set_copy_progress( + mock_local_image_ctx, 12, true, 123, 0); + expect_notify_update(mock_local_image_ctx); + MockUnlinkPeerRequest mock_unlink_peer_request; + expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid", + 0); + expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); + + // idle + expect_load_image_meta(mock_image_meta, false, 0); + expect_is_refresh_required(mock_local_image_ctx, true); + expect_refresh( + mock_local_image_ctx, { + {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + {12U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "remote mirror uuid", + 2, true, 0, {}}, + 0, {}, 0, 0, {}}}, + }, 0); + expect_is_refresh_required(mock_remote_image_ctx, true); + expect_refresh( + mock_remote_image_ctx, { + {2U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"}, + "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + }, 0); + + // wake-up replayer + update_watch_ctx->handle_notify(); + + // wait for sync to complete + ASSERT_EQ(0, wait_for_notification(2)); + + ASSERT_EQ(0, shut_down_entry_replayer(mock_replayer, mock_threads, + mock_local_image_ctx, + mock_remote_image_ctx)); +} + TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedPendingSyncInitial) { librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx}; librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx}; @@ -1363,6 +1472,134 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedPendingSyncDelta) { mock_remote_image_ctx)); } +TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedPendingSyncDeltaDemote) { + librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx}; + librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx}; + + MockThreads mock_threads(m_threads); + expect_work_queue_repeatedly(mock_threads); + + MockReplayerListener mock_replayer_listener; + expect_notification(mock_threads, mock_replayer_listener); + + InSequence seq; + + MockInstanceWatcher mock_instance_watcher; + MockImageMeta mock_image_meta; + MockStateBuilder mock_state_builder(mock_local_image_ctx, + mock_remote_image_ctx, + mock_image_meta); + MockReplayer mock_replayer{&mock_threads, &mock_instance_watcher, + "local mirror uuid", &m_pool_meta_cache, + &mock_state_builder, &mock_replayer_listener}; + m_pool_meta_cache.set_remote_pool_meta( + m_remote_io_ctx.get_id(), + {"remote mirror uuid", "remote mirror peer uuid"}); + + librbd::UpdateWatchCtx* update_watch_ctx = nullptr; + ASSERT_EQ(0, init_entry_replayer(mock_replayer, mock_threads, + mock_local_image_ctx, + mock_remote_image_ctx, + mock_replayer_listener, + mock_image_meta, + &update_watch_ctx)); + + // inject an incomplete sync snapshot with last_copied_object_number == 0 + // after a primary demotion snapshot + mock_remote_image_ctx.snap_info = { + {1U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "local mirror uuid", 11, true, 0, + {{11, CEPH_NOSNAP}}}, + 0, {}, 0, 0, {}}}, + {2U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"}, + "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}}; + mock_local_image_ctx.snap_info = { + {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + {12U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "remote mirror uuid", + 2, false, 0, {{2, CEPH_NOSNAP}}}, + 0, {}, 0, 0, {}}}}; + + // prune non-primary snap2 + expect_load_image_meta(mock_image_meta, false, 0); + expect_is_refresh_required(mock_local_image_ctx, false); + expect_is_refresh_required(mock_remote_image_ctx, false); + expect_prune_non_primary_snapshot(mock_local_image_ctx, 12, 0); + + // sync snap2 + expect_load_image_meta(mock_image_meta, false, 0); + expect_is_refresh_required(mock_local_image_ctx, true); + expect_refresh( + mock_local_image_ctx, { + {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + }, 0); + expect_is_refresh_required(mock_remote_image_ctx, false); + MockSnapshotCopyRequest mock_snapshot_copy_request; + expect_snapshot_copy(mock_snapshot_copy_request, 1, 2, 11, + {{2, CEPH_NOSNAP}}, 0); + MockGetImageStateRequest mock_get_image_state_request; + expect_get_image_state(mock_get_image_state_request, 2, 0); + MockCreateNonPrimaryRequest mock_create_non_primary_request; + expect_create_non_primary_request(mock_create_non_primary_request, + false, "remote mirror uuid", 2, + {{2, CEPH_NOSNAP}}, 13, 0); + expect_notify_sync_request(mock_instance_watcher, mock_local_image_ctx.id, 0); + MockImageCopyRequest mock_image_copy_request; + expect_image_copy(mock_image_copy_request, 1, 2, 11, {}, + {{2, CEPH_NOSNAP}}, 0); + MockApplyImageStateRequest mock_apply_state_request; + expect_apply_image_state(mock_apply_state_request, 0); + expect_mirror_image_snapshot_set_copy_progress( + mock_local_image_ctx, 13, true, 0, 0); + expect_notify_update(mock_local_image_ctx); + MockUnlinkPeerRequest mock_unlink_peer_request; + expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid", + 0); + expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); + + // idle + expect_load_image_meta(mock_image_meta, false, 0); + expect_is_refresh_required(mock_local_image_ctx, true); + expect_refresh( + mock_local_image_ctx, { + {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, + {"remote mirror peer uuid"}, "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + {13U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "remote mirror uuid", + 2, true, 0, {}}, + 0, {}, 0, 0, {}}}, + }, 0); + expect_is_refresh_required(mock_remote_image_ctx, true); + expect_refresh( + mock_remote_image_ctx, { + {2U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"}, + "", CEPH_NOSNAP, true, 0, {}}, + 0, {}, 0, 0, {}}}, + }, 0); + + // wake-up replayer + update_watch_ctx->handle_notify(); + + // wait for sync to complete + ASSERT_EQ(0, wait_for_notification(2)); + + ASSERT_EQ(0, shut_down_entry_replayer(mock_replayer, mock_threads, + mock_local_image_ctx, + mock_remote_image_ctx)); +} + TEST_F(TestMockImageReplayerSnapshotReplayer, RemoteImageDemoted) { librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx}; librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx}; diff --git a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc index afabe6769c672..0270a11708edd 100644 --- a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc +++ b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc @@ -441,7 +441,6 @@ void Replayer::scan_local_mirror_snapshots( std::set prune_snap_ids; - bool completed_non_primary_snapshots_exist = false; auto local_image_ctx = m_state_builder->local_image_ctx; std::shared_lock image_locker{local_image_ctx->image_lock}; for (auto snap_info_it = local_image_ctx->snap_info.begin(); @@ -462,8 +461,7 @@ void Replayer::scan_local_mirror_snapshots( if (mirror_ns->complete) { // if remote has new snapshots, we would sync from here m_local_snap_id_start = local_snap_id; - m_local_snap_id_end = CEPH_NOSNAP; - completed_non_primary_snapshots_exist = true; + ceph_assert(m_local_snap_id_end == CEPH_NOSNAP); if (mirror_ns->mirror_peer_uuids.empty()) { // no other peer will attempt to sync to this snapshot so store as @@ -472,15 +470,6 @@ void Replayer::scan_local_mirror_snapshots( } } else if (mirror_ns->last_copied_object_number == 0 && m_local_snap_id_start > 0) { - // shouldn't be possible, but ensure that pruning this snapshot - // wouldn't leave this image w/o any non-primary snapshots - if (!completed_non_primary_snapshots_exist) { - derr << "incomplete local non-primary snapshot" << dendl; - handle_replay_complete(locker, -EINVAL, - "incomplete local non-primary snapshot"); - return; - } - // snapshot might be missing image state, object-map, etc, so just // delete and re-create it if we haven't started copying data // objects. Also only prune this snapshot since we will need the @@ -500,7 +489,7 @@ void Replayer::scan_local_mirror_snapshots( } else if (mirror_ns->is_primary()) { if (mirror_ns->complete) { m_local_snap_id_start = local_snap_id; - m_local_snap_id_end = CEPH_NOSNAP; + ceph_assert(m_local_snap_id_end == CEPH_NOSNAP); } else { derr << "incomplete local primary snapshot" << dendl; handle_replay_complete(locker, -EINVAL, -- 2.39.5