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 <dillaman@redhat.com>

diff --git a/src/librbd/mirror/PromoteRequest.cc b/src/librbd/mirror/PromoteRequest.cc
index dc70e6af085791c7e94e85c28f537f30af4099c0..b7ae9366e20e4f073fdea0f6ce3505c4705eced1 100644 (file)
--- a/src/librbd/mirror/PromoteRequest.cc
+++ b/src/librbd/mirror/PromoteRequest.cc
@@ -78,7 +78,7 @@ void PromoteRequest<I>::promote() {
     Journal<I>::promote(&m_image_ctx, ctx);
   } else if (m_mirror_image.mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) {
     auto req = mirror::snapshot::PromoteRequest<I>::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 5e9c9637dc27e3efd29f25c176f6699545584960..3d9481d5240ee82cdf7d7be8b4f07c802d9ffc75 100644 (file)
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -40,7 +40,7 @@ void CreatePrimaryRequest<I>::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 418778ce0380f7b883e611b3ab6f10214b15720e..91d4a25b1161a0537bb5039696d6eb44bd76b090 100644 (file)
--- 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 <typename I>
 void PromoteRequest<I>::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 7555842f2ced5fae551cd81732d51ffc31737208..1583551f1d53483100c9604f14e3a7089aaa0903 100644 (file)
--- 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 41629beab5de6c22889a0d2be7611e005f482f40..ecf884b54c41cd7a6080ffacfac0cc15b481231a 100644 (file)
--- 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 <typename I>
 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 86ecf04dfde558c4ff5bbe34a81bc7a093b16eb9..127ec58653a178df05885c70584958e5effd6403 100644 (file)
--- 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 <typename ImageCtxT = librbd::ImageCtx>
 bool can_create_primary_snapshot(ImageCtxT *image_ctx, bool demoted, bool force,
+                                 bool* requires_orphan,
                                  uint64_t *rollback_snap_id);
 
 template <typename ImageCtxT = librbd::ImageCtx>

diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
index 5369fd46c8d75f3c41a562194846c3678323df4a..6e69e800256398a0470dd9ed4780a11ae17f71a9 100644 (file)
--- 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 2220d0e64244ec8ec5454cef4e9284124d4358ac..3e1faf4873ecf1d31ea2ab74c8acd69b03cb5cd5 100644 (file)
--- 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 206f5f69f827bf5481cb670c8beaaaf7abe42926..d2374332a10ce790fc1e6807d69c2a9e6e777890 100644 (file)
--- 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) {