From 83844531b15f2dce2e46a1c96f3ae2277a3b879c Mon Sep 17 00:00:00 2001 From: Christopher Hoffman Date: Wed, 19 Apr 2023 15:26:27 +0000 Subject: [PATCH] librbd: localize snap_remove op for mirror snapshots A client may attempt a lock request not quickly enough to obtain exclusive lock for operations when another competing client responds quicker. This can happen when a peer site has different performance characteristics or latency. Instead of relying on this unpredictable behavior, localize operation to primary cluster. Fixes: https://tracker.ceph.com/issues/59393 Signed-off-by: Christopher Hoffman (cherry picked from commit ac552c9b4d65198db8038d397a3060d5a030917d) --- src/cls/rbd/cls_rbd.cc | 18 --- src/librbd/api/Mirror.cc | 2 +- .../mirror/snapshot/CreatePrimaryRequest.cc | 3 +- .../mirror/snapshot/UnlinkPeerRequest.cc | 81 +++++------ .../mirror/snapshot/UnlinkPeerRequest.h | 17 ++- src/test/cls_rbd/test_cls_rbd.cc | 8 +- .../test_mock_CreatePrimaryRequest.cc | 19 ++- .../snapshot/test_mock_UnlinkPeerRequest.cc | 136 ++++++++++++++++-- src/test/librbd/test_mirroring.cc | 4 +- .../snapshot/test_mock_Replayer.cc | 25 ++-- .../image_replayer/snapshot/Replayer.cc | 2 +- 11 files changed, 211 insertions(+), 104 deletions(-) diff --git a/src/cls/rbd/cls_rbd.cc b/src/cls/rbd/cls_rbd.cc index b4efc23ceb60b..229f85d490958 100644 --- a/src/cls/rbd/cls_rbd.cc +++ b/src/cls/rbd/cls_rbd.cc @@ -5720,24 +5720,6 @@ int image_snapshot_unlink_peer(cls_method_context_t hctx, return -ENOENT; } - if (mirror_ns->mirror_peer_uuids.size() == 1) { - // if this is the last peer to unlink and we have at least one additional - // newer mirror snapshot, return a special error to inform the caller it - // should remove the snapshot instead. - auto search_lambda = [snap_id](const cls_rbd_snap& snap_meta) { - if (snap_meta.id > snap_id && - std::holds_alternative( - snap_meta.snapshot_namespace)) { - return -EEXIST; - } - return 0; - }; - r = image::snapshot::iterate(hctx, search_lambda); - if (r == -EEXIST) { - return -ERESTART; - } - } - mirror_ns->mirror_peer_uuids.erase(mirror_peer_uuid); r = image::snapshot::write(hctx, snap_key, std::move(snap)); diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc index ee5897eddfc30..2cfad0d327532 100644 --- a/src/librbd/api/Mirror.cc +++ b/src/librbd/api/Mirror.cc @@ -1654,7 +1654,7 @@ int Mirror::peer_site_remove(librados::IoCtx& io_ctx, for (auto snap_id : snap_ids) { C_SaferCond cond; auto req = mirror::snapshot::UnlinkPeerRequest::create( - img_ctx, snap_id, uuid, &cond); + img_ctx, snap_id, uuid, true, &cond); req->send(); r = cond.wait(); if (r == -ENOENT) { diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc index ae8c045e16ba3..fd6f9b5023d2a 100644 --- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc +++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc @@ -239,7 +239,8 @@ void CreatePrimaryRequest::unlink_peer() { auto ctx = create_context_callback< CreatePrimaryRequest, &CreatePrimaryRequest::handle_unlink_peer>(this); - auto req = UnlinkPeerRequest::create(m_image_ctx, snap_id, peer_uuid, ctx); + auto req = UnlinkPeerRequest::create(m_image_ctx, snap_id, peer_uuid, true, + ctx); req->send(); } diff --git a/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc b/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc index b28591c00e8bf..35313f6277981 100644 --- a/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc +++ b/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc @@ -62,18 +62,19 @@ void UnlinkPeerRequest::unlink_peer() { m_image_ctx->image_lock.lock_shared(); int r = -ENOENT; - cls::rbd::MirrorSnapshotNamespace* mirror_ns = nullptr; - m_newer_mirror_snapshots = false; + cls::rbd::SnapshotNamespace snap_namespace; + std::string snap_name; + bool have_newer_mirror_snapshot = false; for (auto snap_it = m_image_ctx->snap_info.find(m_snap_id); snap_it != m_image_ctx->snap_info.end(); ++snap_it) { if (snap_it->first == m_snap_id) { r = 0; - mirror_ns = std::get_if( - &snap_it->second.snap_namespace); + snap_namespace = snap_it->second.snap_namespace; + snap_name = snap_it->second.name; } else if (std::holds_alternative( snap_it->second.snap_namespace)) { ldout(cct, 15) << "located newer mirror snapshot" << dendl; - m_newer_mirror_snapshots = true; + have_newer_mirror_snapshot = true; break; } } @@ -85,6 +86,8 @@ void UnlinkPeerRequest::unlink_peer() { return; } + auto mirror_ns = std::get_if( + &snap_namespace); if (mirror_ns == nullptr) { lderr(cct) << "not mirror snapshot (snap_id=" << m_snap_id << ")" << dendl; m_image_ctx->image_lock.unlock_shared(); @@ -94,12 +97,32 @@ void UnlinkPeerRequest::unlink_peer() { // if there is or will be no more peers in the mirror snapshot and we have // a more recent mirror snapshot, remove the older one - if ((mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) == 0) || - (mirror_ns->mirror_peer_uuids.size() <= 1U && m_newer_mirror_snapshots)) { + if ((mirror_ns->mirror_peer_uuids.empty() || + (mirror_ns->mirror_peer_uuids.size() == 1 && + mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) != 0)) && + have_newer_mirror_snapshot) { + if (m_allow_remove) { + m_image_ctx->image_lock.unlock_shared(); + remove_snapshot(snap_namespace, snap_name); + return; + } else { + ldout(cct, 15) << "skipping removal of snapshot: snap_id=" << m_snap_id + << ", mirror_peer_uuid=" << m_mirror_peer_uuid + << ", mirror_peer_uuids=" << mirror_ns->mirror_peer_uuids + << dendl; + } + } + + if (mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) == 0) { + ldout(cct, 15) << "no peer to unlink: snap_id=" << m_snap_id + << ", mirror_peer_uuid=" << m_mirror_peer_uuid + << ", mirror_peer_uuids=" << mirror_ns->mirror_peer_uuids + << dendl; m_image_ctx->image_lock.unlock_shared(); - remove_snapshot(); + finish(0); return; } + m_image_ctx->image_lock.unlock_shared(); ldout(cct, 15) << "snap_id=" << m_snap_id << ", " @@ -120,6 +143,10 @@ void UnlinkPeerRequest::handle_unlink_peer(int r) { ldout(cct, 15) << "r=" << r << dendl; if (r == -ERESTART || r == -ENOENT) { + if (r == -ERESTART) { + ldout(cct, 15) << "unlinking last peer not supported" << dendl; + m_allow_remove = true; + } refresh_image(); return; } @@ -161,44 +188,12 @@ void UnlinkPeerRequest::handle_notify_update(int r) { } template -void UnlinkPeerRequest::remove_snapshot() { +void UnlinkPeerRequest::remove_snapshot( + const cls::rbd::SnapshotNamespace& snap_namespace, + const std::string& snap_name) { CephContext *cct = m_image_ctx->cct; ldout(cct, 15) << dendl; - cls::rbd::SnapshotNamespace snap_namespace; - std::string snap_name; - int r = 0; - { - std::shared_lock image_locker{m_image_ctx->image_lock}; - - auto snap_info = m_image_ctx->get_snap_info(m_snap_id); - if (!snap_info) { - r = -ENOENT; - } else { - snap_namespace = snap_info->snap_namespace; - snap_name = snap_info->name; - } - } - - if (r == -ENOENT) { - ldout(cct, 15) << "failed to locate snapshot " << m_snap_id << dendl; - finish(0); - return; - } - - auto info = std::get( - snap_namespace); - - info.mirror_peer_uuids.erase(m_mirror_peer_uuid); - if (!info.mirror_peer_uuids.empty() || !m_newer_mirror_snapshots) { - ldout(cct, 15) << "skipping removal of snapshot: " - << "snap_id=" << m_snap_id << ": " - << "mirror_peer_uuid=" << m_mirror_peer_uuid << ", " - << "mirror_peer_uuids=" << info.mirror_peer_uuids << dendl; - finish(0); - return; - } - auto ctx = create_context_callback< UnlinkPeerRequest, &UnlinkPeerRequest::handle_remove_snapshot>(this); m_image_ctx->operations->snap_remove(snap_namespace, snap_name, ctx); diff --git a/src/librbd/mirror/snapshot/UnlinkPeerRequest.h b/src/librbd/mirror/snapshot/UnlinkPeerRequest.h index 9ef47269d8760..192b40d6e5e67 100644 --- a/src/librbd/mirror/snapshot/UnlinkPeerRequest.h +++ b/src/librbd/mirror/snapshot/UnlinkPeerRequest.h @@ -5,6 +5,7 @@ #define CEPH_LIBRBD_MIRROR_SNAPSHOT_UNLINK_PEER_REQUEST_H #include "include/buffer.h" +#include "cls/rbd/cls_rbd_client.h" #include #include @@ -23,15 +24,17 @@ class UnlinkPeerRequest { public: static UnlinkPeerRequest *create(ImageCtxT *image_ctx, uint64_t snap_id, const std::string &mirror_peer_uuid, - Context *on_finish) { + bool allow_remove, Context *on_finish) { return new UnlinkPeerRequest(image_ctx, snap_id, mirror_peer_uuid, - on_finish); + allow_remove, on_finish); } UnlinkPeerRequest(ImageCtxT *image_ctx, uint64_t snap_id, - const std::string &mirror_peer_uuid, Context *on_finish) + const std::string &mirror_peer_uuid, bool allow_remove, + Context *on_finish) : m_image_ctx(image_ctx), m_snap_id(snap_id), - m_mirror_peer_uuid(mirror_peer_uuid), m_on_finish(on_finish) { + m_mirror_peer_uuid(mirror_peer_uuid), m_allow_remove(allow_remove), + m_on_finish(on_finish) { } void send(); @@ -67,10 +70,9 @@ private: ImageCtxT *m_image_ctx; uint64_t m_snap_id; std::string m_mirror_peer_uuid; + bool m_allow_remove; Context *m_on_finish; - bool m_newer_mirror_snapshots = false; - void refresh_image(); void handle_refresh_image(int r); @@ -80,7 +82,8 @@ private: void notify_update(); void handle_notify_update(int r); - void remove_snapshot(); + void remove_snapshot(const cls::rbd::SnapshotNamespace& snap_namespace, + const std::string& snap_name); void handle_remove_snapshot(int r); void finish(int r); diff --git a/src/test/cls_rbd/test_cls_rbd.cc b/src/test/cls_rbd/test_cls_rbd.cc index 4d6f9bd89b008..2f553a0f43385 100644 --- a/src/test/cls_rbd/test_cls_rbd.cc +++ b/src/test/cls_rbd/test_cls_rbd.cc @@ -2275,14 +2275,14 @@ TEST_F(TestClsRbd, mirror_snapshot) { ASSERT_EQ(1U, sn->mirror_peer_uuids.size()); ASSERT_EQ(1U, sn->mirror_peer_uuids.count("peer2")); - ASSERT_EQ(-ERESTART, - mirror_image_snapshot_unlink_peer(&ioctx, oid, 1, "peer2")); + ASSERT_EQ(0, mirror_image_snapshot_unlink_peer(&ioctx, oid, 1, "peer2")); + ASSERT_EQ(-ENOENT, mirror_image_snapshot_unlink_peer(&ioctx, oid, 1, + "peer2")); ASSERT_EQ(0, snapshot_get(&ioctx, oid, 1, &snap)); sn = std::get_if( &snap.snapshot_namespace); ASSERT_NE(nullptr, sn); - ASSERT_EQ(1U, sn->mirror_peer_uuids.size()); - ASSERT_EQ(1U, sn->mirror_peer_uuids.count("peer2")); + ASSERT_EQ(0U, sn->mirror_peer_uuids.size()); ASSERT_EQ(0, snapshot_get(&ioctx, oid, 2, &snap)); auto nsn = std::get_if( diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc index 1875678c91ac5..22692e15f2804 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc @@ -58,15 +58,18 @@ template <> struct UnlinkPeerRequest { uint64_t snap_id = CEPH_NOSNAP; std::string mirror_peer_uuid; + bool allow_remove; Context* on_finish = nullptr; static UnlinkPeerRequest* s_instance; static UnlinkPeerRequest *create(MockTestImageCtx *image_ctx, uint64_t snap_id, const std::string &mirror_peer_uuid, + bool allow_remove, Context *on_finish) { ceph_assert(s_instance != nullptr); s_instance->snap_id = snap_id; s_instance->mirror_peer_uuid = mirror_peer_uuid; + s_instance->allow_remove = allow_remove; s_instance->on_finish = on_finish; return s_instance; } @@ -175,13 +178,15 @@ 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, bool complete, int r) { + bool is_linked, bool complete, bool allow_remove, + int r) { EXPECT_CALL(mock_unlink_peer_request, send()) .WillOnce(Invoke([&mock_image_ctx, &mock_unlink_peer_request, - snap_id, peer_uuid, is_linked, complete, r]() { + snap_id, peer_uuid, is_linked, complete, allow_remove, r]() { ASSERT_EQ(mock_unlink_peer_request.mirror_peer_uuid, peer_uuid); ASSERT_EQ(mock_unlink_peer_request.snap_id, snap_id); + ASSERT_EQ(mock_unlink_peer_request.allow_remove, allow_remove); if (r == 0) { auto it = mock_image_ctx.snap_info.find(snap_id); ASSERT_NE(it, mock_image_ctx.snap_info.end()); @@ -325,7 +330,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkIncomplete) { 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); + true, false, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, 0U, 0U, nullptr, &ctx); @@ -362,7 +367,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, true, 0); + true, true, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, 0U, 0U, nullptr, &ctx); @@ -397,7 +402,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) { 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", - false, true, 0); + false, true, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, @@ -438,9 +443,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, true, 0); + true, true, true, 0); expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid2", - true, true, 0); + true, true, true, 0); C_SaferCond ctx; auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP, 0U, 0U, nullptr, &ctx); diff --git a/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc index eebd9fd81819f..869bdecff479f 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc @@ -149,7 +149,7 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, Success) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer1_uuid", - &ctx); + true, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -177,7 +177,37 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, RemoveSnapshot) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", - &ctx); + true, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, RemoveSnapshotNotAllowed) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + cls::rbd::MirrorSnapshotNamespace ns{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"peer_uuid"}, + "", CEPH_NOSNAP}; + auto snap_id = snap_create(mock_image_ctx, ns, "mirror_snap"); + snap_create(mock_image_ctx, ns, "mirror_snap2"); + + expect_get_snap_info(mock_image_ctx, snap_id); + + InSequence seq; + + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh_image(mock_image_ctx, 0); + expect_unlink_peer(mock_image_ctx, snap_id, "peer_uuid", 0); + expect_notify_update(mock_image_ctx, 0); + expect_refresh_image(mock_image_ctx, 0); + + C_SaferCond ctx; + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", + false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -206,7 +236,35 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, SnapshotRemoveEmptyPeers) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", - &ctx); + true, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, SnapshotRemoveEmptyPeersNotAllowed) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + cls::rbd::MirrorSnapshotNamespace ns{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {}, + "", CEPH_NOSNAP}; + auto snap_id = snap_create(mock_image_ctx, ns, "mirror_snap"); + ns.mirror_peer_uuids = {"peer_uuid"}; + snap_create(mock_image_ctx, ns, "mirror_snap2"); + + expect_get_snap_info(mock_image_ctx, snap_id); + + InSequence seq; + + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh_image(mock_image_ctx, 0); + + C_SaferCond ctx; + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", + false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -227,7 +285,8 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, SnapshotDNE) { expect_refresh_image(mock_image_ctx, 0); C_SaferCond ctx; - auto req = new MockUnlinkPeerRequest(&mock_image_ctx, 123, "peer_uuid", &ctx); + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, 123, "peer_uuid", + true, &ctx); req->send(); ASSERT_EQ(-ENOENT, ctx.wait()); } @@ -253,7 +312,33 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, PeerDNE) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "unknown_peer", - &ctx); + true, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, MultiPeerPeerDNE) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + cls::rbd::MirrorSnapshotNamespace ns{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"peer1_uuid", "peer2_uuid"}, + "", CEPH_NOSNAP}; + auto snap_id = snap_create(mock_image_ctx, ns, "mirror_snap"); + + expect_get_snap_info(mock_image_ctx, snap_id); + + InSequence seq; + + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh_image(mock_image_ctx, 0); + + C_SaferCond ctx; + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "unknown_peer", + true, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -276,7 +361,7 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, InvalidSnapshot) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", - &ctx); + true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -295,7 +380,8 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, RefreshError) { expect_refresh_image(mock_image_ctx, -EINVAL); C_SaferCond ctx; - auto req = new MockUnlinkPeerRequest(&mock_image_ctx, 123, "peer_uuid", &ctx); + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, 123, "peer_uuid", + true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -321,11 +407,41 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, UnlinkError) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer1_uuid", - &ctx); + true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } +TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, UnlinkErrorRestart) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + cls::rbd::MirrorSnapshotNamespace ns{ + cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"peer_uuid"}, + "", CEPH_NOSNAP}; + auto snap_id = snap_create(mock_image_ctx, ns, "mirror_snap"); + snap_create(mock_image_ctx, ns, "mirror_snap2"); + + expect_get_snap_info(mock_image_ctx, snap_id); + + InSequence seq; + + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh_image(mock_image_ctx, 0); + expect_unlink_peer(mock_image_ctx, snap_id, "peer_uuid", -ERESTART); + expect_refresh_image(mock_image_ctx, 0); + expect_remove_snapshot(mock_image_ctx, snap_id, 0); + + C_SaferCond ctx; + auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", + false, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, NotifyError) { REQUIRE_FORMAT_V2(); @@ -348,7 +464,7 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, NotifyError) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer1_uuid", - &ctx); + true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -375,7 +491,7 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, RemoveSnapshotError) { C_SaferCond ctx; auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid", - &ctx); + true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc index 480db94d2bf3f..19ba5277d3041 100644 --- a/src/test/librbd/test_mirroring.cc +++ b/src/test/librbd/test_mirroring.cc @@ -1269,7 +1269,7 @@ TEST_F(TestMirroring, SnapshotUnlinkPeer) C_SaferCond cond1; auto req = librbd::mirror::snapshot::UnlinkPeerRequest<>::create( - ictx, snap_id, peer1_uuid, &cond1); + ictx, snap_id, peer1_uuid, true, &cond1); req->send(); ASSERT_EQ(0, cond1.wait()); @@ -1314,7 +1314,7 @@ TEST_F(TestMirroring, SnapshotUnlinkPeer) C_SaferCond cond2; req = librbd::mirror::snapshot::UnlinkPeerRequest<>::create( - ictx, snap_id, peer2_uuid, &cond2); + ictx, snap_id, peer2_uuid, true, &cond2); req->send(); ASSERT_EQ(0, cond2.wait()); 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 26b4d32cc4ffd..75141e5a7be30 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 @@ -218,15 +218,18 @@ template <> struct UnlinkPeerRequest { uint64_t snap_id; std::string mirror_peer_uuid; + bool allow_remove; static UnlinkPeerRequest* s_instance; static UnlinkPeerRequest*create (MockTestImageCtx *image_ctx, uint64_t snap_id, const std::string &mirror_peer_uuid, + bool allow_remove, Context *on_finish) { ceph_assert(s_instance != nullptr); s_instance->snap_id = snap_id; s_instance->mirror_peer_uuid = mirror_peer_uuid; + s_instance->allow_remove = allow_remove; s_instance->on_finish = on_finish; return s_instance; } @@ -596,12 +599,13 @@ public: void expect_unlink_peer(MockUnlinkPeerRequest& mock_unlink_peer_request, uint64_t snap_id, const std::string& mirror_peer_uuid, - int r) { + bool allow_remove, int r) { EXPECT_CALL(mock_unlink_peer_request, send()) .WillOnce(Invoke([this, &req=mock_unlink_peer_request, snap_id, - mirror_peer_uuid, r]() { + mirror_peer_uuid, allow_remove, r]() { ASSERT_EQ(snap_id, req.snap_id); ASSERT_EQ(mirror_peer_uuid, req.mirror_peer_uuid); + ASSERT_EQ(allow_remove, req.allow_remove); m_threads->work_queue->queue(req.on_finish, r); })); } @@ -868,7 +872,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, SyncSnapshot) { 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); + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // prune non-primary snap1 @@ -1096,7 +1100,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedSyncDelta) { 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); + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // prune non-primary snap1 @@ -1218,7 +1222,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedSyncDeltaDemote) { 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); + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // idle @@ -1429,7 +1433,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedPendingSyncDelta) { 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); + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // prune non-primary snap1 @@ -1570,7 +1574,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedPendingSyncDeltaDemote) 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); + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // idle @@ -2650,7 +2654,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkPeerError) { expect_notify_update(mock_local_image_ctx); MockUnlinkPeerRequest mock_unlink_peer_request; expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid", - -EINVAL); + false, -EINVAL); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // wake-up replayer @@ -2876,7 +2880,8 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, RemoteFailover) { 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, 2, "remote mirror peer uuid", 0); + expect_unlink_peer(mock_unlink_peer_request, 2, "remote mirror peer uuid", + false, 0); expect_notify_sync_complete(mock_instance_watcher, mock_local_image_ctx.id); // idle @@ -2976,7 +2981,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkRemoteSnapshot) { expect_is_refresh_required(mock_remote_image_ctx, false); MockUnlinkPeerRequest mock_unlink_peer_request; expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid", - 0); + false, 0); // idle expect_load_image_meta(mock_image_meta, false, 0); diff --git a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc index b4da280bf1be8..3561f6fc69941 100644 --- a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc +++ b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc @@ -1293,7 +1293,7 @@ void Replayer::unlink_peer(uint64_t remote_snap_id) { Replayer, &Replayer::handle_unlink_peer>(this); auto req = librbd::mirror::snapshot::UnlinkPeerRequest::create( m_state_builder->remote_image_ctx, remote_snap_id, - m_remote_mirror_peer_uuid, ctx); + m_remote_mirror_peer_uuid, false, ctx); req->send(); } -- 2.39.5