From: Jason Dillaman Date: Thu, 27 Feb 2020 13:53:37 +0000 (-0500) Subject: librbd: forced promotion of snapshot mirrored image needs to create orphan X-Git-Tag: v15.1.1~153^2~9 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=3e7642617a6d9b1f8b14f2e091919332a930671b;p=ceph.git librbd: forced promotion of snapshot mirrored image needs to create orphan Tweak the state machine to create an orphan record if the current state is not demoted. Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/mirror/PromoteRequest.cc b/src/librbd/mirror/PromoteRequest.cc index dc70e6af085..b7ae9366e20 100644 --- a/src/librbd/mirror/PromoteRequest.cc +++ b/src/librbd/mirror/PromoteRequest.cc @@ -78,7 +78,7 @@ void PromoteRequest::promote() { Journal::promote(&m_image_ctx, ctx); } else if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) { auto req = mirror::snapshot::PromoteRequest::create( - &m_image_ctx, m_mirror_image.global_image_id, m_force, ctx); + &m_image_ctx, m_mirror_image.global_image_id, ctx); req->send(); } else { lderr(cct) << "unknown image mirror mode: " << m_mirror_image.mode << dendl; diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc index 5e9c9637dc2..3d9481d5240 100644 --- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc +++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc @@ -40,7 +40,7 @@ void CreatePrimaryRequest::send() { if (!util::can_create_primary_snapshot( m_image_ctx, ((m_flags & CREATE_PRIMARY_FLAG_DEMOTED) != 0), - ((m_flags & CREATE_PRIMARY_FLAG_FORCE) != 0), nullptr)) { + ((m_flags & CREATE_PRIMARY_FLAG_FORCE) != 0), nullptr, nullptr)) { finish(-EINVAL); return; } diff --git a/src/librbd/mirror/snapshot/PromoteRequest.cc b/src/librbd/mirror/snapshot/PromoteRequest.cc index 418778ce038..91d4a25b116 100644 --- a/src/librbd/mirror/snapshot/PromoteRequest.cc +++ b/src/librbd/mirror/snapshot/PromoteRequest.cc @@ -31,22 +31,18 @@ using librbd::util::create_context_callback; template void PromoteRequest::send() { CephContext *cct = m_image_ctx->cct; + bool requires_orphan = false; if (!util::can_create_primary_snapshot(m_image_ctx, false, true, + &requires_orphan, &m_rollback_snap_id)) { lderr(cct) << "cannot promote" << dendl; finish(-EINVAL); return; - } else if (m_rollback_snap_id == CEPH_NOSNAP) { + } else if (m_rollback_snap_id == CEPH_NOSNAP && !requires_orphan) { create_promote_snapshot(); return; } - if (!m_force) { - lderr(cct) << "cannot promote: needs rollback and force not set" << dendl; - finish(-EINVAL); - return; - } - create_orphan_snapshot(); } diff --git a/src/librbd/mirror/snapshot/PromoteRequest.h b/src/librbd/mirror/snapshot/PromoteRequest.h index 7555842f2ce..1583551f1d5 100644 --- a/src/librbd/mirror/snapshot/PromoteRequest.h +++ b/src/librbd/mirror/snapshot/PromoteRequest.h @@ -26,15 +26,14 @@ class PromoteRequest { public: static PromoteRequest *create(ImageCtxT *image_ctx, const std::string& global_image_id, - bool force, Context *on_finish) { - return new PromoteRequest(image_ctx, global_image_id, force, on_finish); + Context *on_finish) { + return new PromoteRequest(image_ctx, global_image_id, on_finish); } - PromoteRequest(ImageCtxT *image_ctx, - const std::string& global_image_id, bool force, + PromoteRequest(ImageCtxT *image_ctx, const std::string& global_image_id, Context *on_finish) : m_image_ctx(image_ctx), m_global_image_id(global_image_id), - m_force(force), m_on_finish(on_finish) { + m_on_finish(on_finish) { } void send(); @@ -48,8 +47,8 @@ private: * | (can promote) * |\----------------------------------------\ * | | - * | (needs rollback) | - * v | + * | | + * v (skip if not needed) | * CREATE_ORPHAN_SNAPSHOT | * | | * | /-- UNREGISTER_UPDATE_WATCHER <-\ | @@ -77,7 +76,6 @@ private: ImageCtxT *m_image_ctx; std::string m_global_image_id; - bool m_force; Context *m_on_finish; uint64_t m_rollback_snap_id = CEPH_NOSNAP; diff --git a/src/librbd/mirror/snapshot/Utils.cc b/src/librbd/mirror/snapshot/Utils.cc index 41629beab5d..ecf884b54c4 100644 --- a/src/librbd/mirror/snapshot/Utils.cc +++ b/src/librbd/mirror/snapshot/Utils.cc @@ -54,9 +54,13 @@ std::string get_image_meta_key(const std::string& mirror_uuid) { template bool can_create_primary_snapshot(I *image_ctx, bool demoted, bool force, + bool* requires_orphan, uint64_t *rollback_snap_id) { CephContext *cct = image_ctx->cct; + if (requires_orphan != nullptr) { + *requires_orphan = false; + } if (rollback_snap_id) { *rollback_snap_id = CEPH_NOSNAP; } @@ -72,10 +76,7 @@ bool can_create_primary_snapshot(I *image_ctx, bool demoted, bool force, } ldout(cct, 20) << "previous snapshot snap_id=" << it->first << " " << *mirror_ns << dendl; - if ((mirror_ns->state == cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED || - mirror_ns->state == - cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY_DEMOTED) && - !force) { + if (mirror_ns->is_demoted() && !force) { lderr(cct) << "trying to create primary snapshot without force " << "when previous primary snapshot is demoted" << dendl; @@ -95,6 +96,10 @@ bool can_create_primary_snapshot(I *image_ctx, bool demoted, bool force, << dendl; return false; } + + if (requires_orphan != nullptr) { + *requires_orphan = !mirror_ns->is_demoted(); + } if (!mirror_ns->complete) { ldout(cct, 20) << "needs rollback" << dendl; if (!rollback_snap_id) { @@ -172,7 +177,7 @@ std::string image_state_object_name(I *image_ctx, uint64_t snap_id, template bool librbd::mirror::snapshot::util::can_create_primary_snapshot( librbd::ImageCtx *image_ctx, bool demoted, bool force, - uint64_t *rollback_snap_id); + bool* requires_orphan, uint64_t *rollback_snap_id); template bool librbd::mirror::snapshot::util::can_create_non_primary_snapshot( librbd::ImageCtx *image_ctx); diff --git a/src/librbd/mirror/snapshot/Utils.h b/src/librbd/mirror/snapshot/Utils.h index 86ecf04dfde..127ec58653a 100644 --- a/src/librbd/mirror/snapshot/Utils.h +++ b/src/librbd/mirror/snapshot/Utils.h @@ -20,6 +20,7 @@ std::string get_image_meta_key(const std::string& mirror_uuid); template bool can_create_primary_snapshot(ImageCtxT *image_ctx, bool demoted, bool force, + bool* requires_orphan, uint64_t *rollback_snap_id); template diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc index 5369fd46c8d..6e69e800256 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc @@ -46,6 +46,7 @@ Mock *Mock::s_instance = nullptr; template<> bool can_create_primary_snapshot(librbd::MockTestImageCtx *image_ctx, bool demoted, bool force, + bool* requires_orphan, uint64_t *rollback_snap_id) { return Mock::s_instance->can_create_primary_snapshot(image_ctx, demoted, force, rollback_snap_id); diff --git a/src/test/librbd/mirror/snapshot/test_mock_PromoteRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_PromoteRequest.cc index 2220d0e6424..3e1faf4873e 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_PromoteRequest.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_PromoteRequest.cc @@ -65,8 +65,8 @@ struct Mock { s_instance = this; } - MOCK_METHOD4(can_create_primary_snapshot, - bool(librbd::MockTestImageCtx *, bool, bool, uint64_t *)); + MOCK_METHOD5(can_create_primary_snapshot, + bool(librbd::MockTestImageCtx *, bool, bool, bool*, uint64_t *)); }; Mock *Mock::s_instance = nullptr; @@ -75,9 +75,11 @@ Mock *Mock::s_instance = nullptr; template<> bool can_create_primary_snapshot(librbd::MockTestImageCtx *image_ctx, bool demoted, bool force, + bool* requires_orphan, uint64_t *rollback_snap_id) { return Mock::s_instance->can_create_primary_snapshot(image_ctx, demoted, - force, rollback_snap_id); + force, requires_orphan, + rollback_snap_id); } } // namespace util @@ -157,6 +159,7 @@ using ::testing::Invoke; using ::testing::Return; using ::testing::StrEq; using ::testing::WithArg; +using ::testing::WithArgs; class TestMockMirrorSnapshotPromoteRequest : public TestMockFixture { public: @@ -167,14 +170,18 @@ public: typedef util::Mock MockUtils; void expect_can_create_primary_snapshot(MockUtils &mock_utils, bool force, + bool requires_orphan, uint64_t rollback_snap_id, bool result) { EXPECT_CALL(mock_utils, - can_create_primary_snapshot(_, false, force, _)) + can_create_primary_snapshot(_, false, force, _, _)) .WillOnce(DoAll( - WithArg<3>(Invoke([rollback_snap_id](uint64_t *snap_id) { - *snap_id = rollback_snap_id; - })), + WithArgs<3,4 >(Invoke( + [requires_orphan, rollback_snap_id] + (bool* orphan, uint64_t *snap_id) { + *orphan = requires_orphan; + *snap_id = rollback_snap_id; + })), Return(result))); } @@ -262,17 +269,54 @@ TEST_F(TestMockMirrorSnapshotPromoteRequest, Success) { MockTestImageCtx mock_image_ctx(*ictx); - const bool force = false; + InSequence seq; + + MockUtils mock_utils; + expect_can_create_primary_snapshot(mock_utils, true, false, CEPH_NOSNAP, + true); + MockCreatePrimaryRequest mock_create_primary_request; + expect_create_promote_snapshot(mock_image_ctx, mock_create_primary_request, + 0); + C_SaferCond ctx; + auto req = new MockPromoteRequest(&mock_image_ctx, "gid", &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockMirrorSnapshotPromoteRequest, SuccessForce) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + expect_op_work_queue(mock_image_ctx); + + MockExclusiveLock mock_exclusive_lock; + if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { + mock_image_ctx.exclusive_lock = &mock_exclusive_lock; + } InSequence seq; MockUtils mock_utils; - expect_can_create_primary_snapshot(mock_utils, true, CEPH_NOSNAP, true); + expect_can_create_primary_snapshot(mock_utils, true, true, CEPH_NOSNAP, true); + MockCreateNonPrimaryRequest mock_create_non_primary_request; + expect_create_orphan_snapshot(mock_image_ctx, mock_create_non_primary_request, + 0); + MockListWatchersRequest mock_list_watchers_request; + expect_list_watchers(mock_image_ctx, mock_list_watchers_request, {}, 0); + expect_acquire_lock(mock_image_ctx, 0); + + SnapInfo snap_info = {"snap", cls::rbd::MirrorSnapshotNamespace{}, 0, + {}, 0, 0, {}}; MockCreatePrimaryRequest mock_create_primary_request; expect_create_promote_snapshot(mock_image_ctx, mock_create_primary_request, 0); + expect_release_lock(mock_image_ctx, 0); + C_SaferCond ctx; - auto req = new MockPromoteRequest(&mock_image_ctx, "gid", force, &ctx); + auto req = new MockPromoteRequest(&mock_image_ctx, "gid", &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -291,12 +335,10 @@ TEST_F(TestMockMirrorSnapshotPromoteRequest, SuccessRollback) { mock_image_ctx.exclusive_lock = &mock_exclusive_lock; } - const bool force = true; - InSequence seq; MockUtils mock_utils; - expect_can_create_primary_snapshot(mock_utils, true, 123, true); + expect_can_create_primary_snapshot(mock_utils, true, false, 123, true); MockCreateNonPrimaryRequest mock_create_non_primary_request; expect_create_orphan_snapshot(mock_image_ctx, mock_create_non_primary_request, 0); @@ -313,7 +355,7 @@ TEST_F(TestMockMirrorSnapshotPromoteRequest, SuccessRollback) { expect_release_lock(mock_image_ctx, 0); C_SaferCond ctx; - auto req = new MockPromoteRequest(&mock_image_ctx, "gid", force, &ctx); + auto req = new MockPromoteRequest(&mock_image_ctx, "gid", &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -327,15 +369,14 @@ TEST_F(TestMockMirrorSnapshotPromoteRequest, ErrorCannotRollback) { MockTestImageCtx mock_image_ctx(*ictx); expect_op_work_queue(mock_image_ctx); - const bool force = false; - InSequence seq; MockUtils mock_utils; - expect_can_create_primary_snapshot(mock_utils, true, CEPH_NOSNAP, false); + expect_can_create_primary_snapshot(mock_utils, true, false, CEPH_NOSNAP, + false); C_SaferCond ctx; - auto req = new MockPromoteRequest(&mock_image_ctx, "gid", force, &ctx); + auto req = new MockPromoteRequest(&mock_image_ctx, "gid", &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } diff --git a/src/test/librbd/mirror/snapshot/test_mock_Utils.cc b/src/test/librbd/mirror/snapshot/test_mock_Utils.cc index 206f5f69f82..d2374332a10 100644 --- a/src/test/librbd/mirror/snapshot/test_mock_Utils.cc +++ b/src/test/librbd/mirror/snapshot/test_mock_Utils.cc @@ -28,7 +28,7 @@ struct MockTestImageCtx : public MockImageCtx { #include "librbd/mirror/snapshot/Utils.cc" template bool librbd::mirror::snapshot::util::can_create_primary_snapshot( librbd::MockTestImageCtx *image_ctx, bool demoted, bool force, - uint64_t *rollback_snap_id); + bool* requires_orphan, uint64_t *rollback_snap_id); template bool librbd::mirror::snapshot::util::can_create_non_primary_snapshot( librbd::MockTestImageCtx *image_ctx); @@ -67,9 +67,12 @@ TEST_F(TestMockMirrorSnapshotUtils, CanCreatePrimarySnapshot) { MockTestImageCtx mock_image_ctx(*ictx); // no previous mirror snapshots found + bool requires_orphan; uint64_t rollback_snap_id; ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, false, + &requires_orphan, &rollback_snap_id)); + ASSERT_FALSE(requires_orphan); ASSERT_EQ(rollback_snap_id, CEPH_NOSNAP); cls::rbd::MirrorSnapshotNamespace nns{ @@ -79,15 +82,17 @@ TEST_F(TestMockMirrorSnapshotUtils, CanCreatePrimarySnapshot) { // without force, previous snapshot is non-primary ASSERT_FALSE(util::can_create_primary_snapshot(&mock_image_ctx, false, false, - nullptr)); + nullptr, nullptr)); // demoted, previous snapshot is non-primary ASSERT_FALSE(util::can_create_primary_snapshot(&mock_image_ctx, true, true, - nullptr)); + nullptr, nullptr)); // previous non-primary snapshot is copied ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, true, + &requires_orphan, &rollback_snap_id)); + ASSERT_TRUE(requires_orphan); ASSERT_EQ(rollback_snap_id, CEPH_NOSNAP); nns.complete = false; @@ -95,11 +100,11 @@ TEST_F(TestMockMirrorSnapshotUtils, CanCreatePrimarySnapshot) { // previous non-primary snapshot is not copied yet ASSERT_FALSE(util::can_create_primary_snapshot(&mock_image_ctx, false, true, - nullptr)); + nullptr, nullptr)); // can rollback ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, true, - &rollback_snap_id)); + nullptr, &rollback_snap_id)); ASSERT_EQ(rollback_snap_id, copied_snap_id); nns.state = cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY_DEMOTED; @@ -107,7 +112,7 @@ TEST_F(TestMockMirrorSnapshotUtils, CanCreatePrimarySnapshot) { // previous non-primary snapshot is orphan ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, true, - nullptr)); + nullptr, nullptr)); cls::rbd::MirrorSnapshotNamespace pns{ cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY_DEMOTED, {"uuid"}, "", CEPH_NOSNAP}; @@ -115,18 +120,18 @@ TEST_F(TestMockMirrorSnapshotUtils, CanCreatePrimarySnapshot) { // previous primary snapshot is demoted, no force ASSERT_FALSE(util::can_create_primary_snapshot(&mock_image_ctx, false, false, - nullptr)); + nullptr, nullptr)); // previous primary snapshot is demoted, force ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, true, - nullptr)); + nullptr, nullptr)); pns.state = cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY; snap_create(mock_image_ctx, pns, "PS2"); // previous snapshot is not demoted primary ASSERT_TRUE(util::can_create_primary_snapshot(&mock_image_ctx, false, false, - nullptr)); + nullptr, nullptr)); } TEST_F(TestMockMirrorSnapshotUtils, CanCreateNonPrimarySnapshot) {