From: Prasanna Kumar Kalever Date: Mon, 6 Mar 2023 09:58:03 +0000 (+0530) Subject: librbd: remove previous incomplete primary snapshot after successfully creating a... X-Git-Tag: v18.1.0~95^2~11 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=ef2b3be27e55e130907defa34971ecfbc36b51e1;p=ceph.git librbd: remove previous incomplete primary snapshot after successfully creating a new one Problem: ------- At a high level, creating a primary snapshot consists of three steps: 1. actually creating a snapshot in the mirror namespace 2. generating a set of image state objects with additional metadata for the snapshot 3. marking the snapshot as complete after the image state objects are written out Depending on the circumstances, a request to create a primary snapshot can be forwarded to rbd-mirror daemon. If that happens and rbd-mirror daemon gets axed for some practical reason after completing steps (1) and/or (2) but before completing step (3), we are left with a permanently incomplete primary snapshot because upon retrying that primary snapshot creation request, librbd notices that such snapshot already exists. It does not check whether this "pre-existing" snapshot is complete. Solution: -------- As part of the next mirror snapshot create (say triggered by the scheduler) the unlink_peer() is called, it checks if there exists any incomplete snapshot and delete them accordingly. Fixes: https://tracker.ceph.com/issues/58887 Signed-off-by: Prasanna Kumar Kalever (cherry picked from commit 165c9a4e163c5edfa77c900f61c680cc944b2b5d) --- diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc index 36cd3bddfc794..5a6115cd4f4ec 100644 --- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc +++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc @@ -209,14 +209,15 @@ void CreatePrimaryRequest::unlink_peer() { // or if it's not linked with any peer (happens if mirroring is enabled // on a pool with no peers configured or if UnlinkPeerRequest gets // interrupted) - if (info->mirror_peer_uuids.size() == 0) { + if (!info->mirror_peer_uuids.empty() && + info->mirror_peer_uuids.count(peer) == 0) { + continue; + } + if (info->mirror_peer_uuids.empty() || !info->complete) { peer_uuid = peer; snap_id = snap_it.first; break; } - if (info->mirror_peer_uuids.count(peer) == 0) { - continue; - } count++; if (count == max_snapshots) { unlink_snap_id = snap_it.first; diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc index 0966810ea8d8b..f2169671bb688 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc @@ -157,7 +157,10 @@ public: if (r != 0) { return; } - snap_create(mock_image_ctx, ns, snap_name); + auto mirror_ns = + std::get(ns); + mirror_ns.complete = true; + snap_create(mock_image_ctx, mirror_ns, snap_name); }), WithArg<4>(CompleteContext( r, mock_image_ctx.image_ctx->op_work_queue)) @@ -167,10 +170,10 @@ public: void expect_unlink_peer(MockTestImageCtx &mock_image_ctx, MockUnlinkPeerRequest &mock_unlink_peer_request, uint64_t snap_id, const std::string &peer_uuid, - bool is_linked, int r) { + bool is_linked, bool complete, int r) { EXPECT_CALL(mock_unlink_peer_request, send()) .WillOnce(Invoke([&mock_image_ctx, &mock_unlink_peer_request, - snap_id, peer_uuid, is_linked, r]() { + snap_id, peer_uuid, is_linked, complete, r]() { ASSERT_EQ(mock_unlink_peer_request.mirror_peer_uuid, peer_uuid); ASSERT_EQ(mock_unlink_peer_request.snap_id, snap_id); @@ -181,6 +184,7 @@ public: std::get_if( &it->second.snap_namespace); ASSERT_NE(nullptr, info); + ASSERT_EQ(complete, info->complete); ASSERT_EQ(is_linked, info->mirror_peer_uuids.erase( peer_uuid)); if (info->mirror_peer_uuids.empty()) { @@ -288,6 +292,40 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, CreateSnapshotError) { ASSERT_EQ(-EINVAL, ctx.wait()); } +TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkIncomplete) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + ictx->config.set_val("rbd_mirroring_max_mirroring_snapshots", "3"); + + MockTestImageCtx mock_image_ctx(*ictx); + cls::rbd::MirrorSnapshotNamespace ns{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"uuid"}, "", CEPH_NOSNAP}; + ns.complete = false; + snap_create(mock_image_ctx, ns, "mirror_snap"); + + InSequence seq; + + expect_clone_md_ctx(mock_image_ctx); + MockUtils mock_utils; + expect_can_create_primary_snapshot(mock_utils, false, false, true); + expect_get_mirror_peers(mock_image_ctx, + {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph", + "mirror", "mirror uuid"}}, 0); + expect_create_snapshot(mock_image_ctx, 0); + MockUnlinkPeerRequest mock_unlink_peer_request; + auto it = mock_image_ctx.snap_info.rbegin(); + auto snap_id = it->first; + expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid", + true, false, 0); + C_SaferCond ctx; + auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, + 0U, 0U, nullptr, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) { REQUIRE_FORMAT_V2(); @@ -299,6 +337,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) { for (int i = 0; i < 3; i++) { cls::rbd::MirrorSnapshotNamespace ns{ cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"uuid"}, "", CEPH_NOSNAP}; + ns.complete = true; snap_create(mock_image_ctx, ns, "mirror_snap"); } @@ -315,7 +354,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) { auto it = mock_image_ctx.snap_info.rbegin(); auto snap_id = it->first; expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid", - true, 0); + true, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, 0U, 0U, nullptr, &ctx); @@ -333,6 +372,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) { MockTestImageCtx mock_image_ctx(*ictx); cls::rbd::MirrorSnapshotNamespace ns{ cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {}, "", CEPH_NOSNAP}; + ns.complete = true; snap_create(mock_image_ctx, ns, "mirror_snap"); InSequence seq; @@ -347,9 +387,8 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) { MockUnlinkPeerRequest mock_unlink_peer_request; auto it = mock_image_ctx.snap_info.rbegin(); auto snap_id = it->first; - std::list peer_uuids = {"uuid"}; expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid", - false, 0); + false, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, @@ -370,6 +409,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkMultiplePeers) { cls::rbd::MirrorSnapshotNamespace ns{ cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"uuid1", "uuid2"}, "", CEPH_NOSNAP}; + ns.complete = true; snap_create(mock_image_ctx, ns, "mirror_snap"); } @@ -388,9 +428,9 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkMultiplePeers) { auto it = mock_image_ctx.snap_info.rbegin(); auto snap_id = it->first; expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid1", - true, 0); + true, true, 0); expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid2", - true, 0); + true, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, 0U, 0U, nullptr, &ctx);