squid: mds: QuiesceDbRequest: update the internal encoding of ops

Excluding the last root from a set will automatically mark it as QS_CANCELED.
Hence, it makes more sense if `exclude` and `cancel` share the same op code,
rather than `exclude` and `release`.

Fixes: https://tracker.ceph.com/issues/66400
Signed-off-by: Leonid Usov <leonid.usov@ibm.com>
Fixes: https://tracker.ceph.com/issues/66383
(cherry picked from commit dad52497817c372fd7c61a88a210b5a3613cb807)

diff --git a/src/mds/MDSRankQuiesce.cc b/src/mds/MDSRankQuiesce.cc
index 0262cee63f44e6015841f6272730cd2732537b19..4727d137f628a2486779a6be31416c076fd1904b 100644 (file)
--- a/src/mds/MDSRankQuiesce.cc
+++ b/src/mds/MDSRankQuiesce.cc
@@ -197,9 +197,9 @@ void MDSRank::command_quiesce_db(const cmdmap_t& cmdmap, std::function<void(int,
     } else if (op_reset) {
       r.reset_roots(roots);
     } else if (op_release) {
-      r.release_roots();
+      r.release();
     } else if (op_cancel) {
-      r.cancel_roots();
+      r.cancel();
     }
 
     double timeout;
@@ -319,7 +319,7 @@ void MDSRank::quiesce_cluster_update() {
     struct CancelAll: public QuiesceDbManager::RequestContext {
       mds_rank_t whoami;
       CancelAll(mds_rank_t whoami) : whoami(whoami) {
-        request.cancel_roots();
+        request.cancel();
       }
       void finish(int rc) override {
         dout(rc == 0 ? 15 : 3) << "injected cancel all completed with rc: " << rc << dendl;

diff --git a/src/mds/QuiesceDb.h b/src/mds/QuiesceDb.h
index 244f342cc73eb79358f5d3503904dc0afa3ace59..8f0ed9729e445c28dc55ae1b07ff3e9095fe0e26 100644 (file)
--- a/src/mds/QuiesceDb.h
+++ b/src/mds/QuiesceDb.h
@@ -340,8 +340,8 @@ struct QuiesceDbRequest {
   ///        for when `roots` is empty
   enum RootsOp: uint8_t {
     INCLUDE_OR_QUERY,
-    EXCLUDE_OR_RELEASE,
-    RESET_OR_CANCEL,
+    EXCLUDE_OR_CANCEL,
+    RESET_OR_RELEASE,
     __INVALID
   };
 
@@ -427,15 +427,15 @@ struct QuiesceDbRequest {
 
   bool is_mutating() const { return (control.roots_op != INCLUDE_OR_QUERY) || !roots.empty() || timeout || expiration; }
   bool is_cancel_all() const { return !set_id && is_cancel(); }
-  bool excludes_roots() const { return control.roots_op == RESET_OR_CANCEL || (control.roots_op == EXCLUDE_OR_RELEASE && !roots.empty()); }
-  bool includes_roots() const { return (control.roots_op == RESET_OR_CANCEL || control.roots_op == INCLUDE_OR_QUERY) && !roots.empty(); }
+  bool excludes_roots() const { return is_exclude() || is_reset(); }
+  bool includes_roots() const { return is_include() || is_reset(); }
 
   bool is_include() const { return control.roots_op == INCLUDE_OR_QUERY && !roots.empty(); }
   bool is_query() const { return control.roots_op == INCLUDE_OR_QUERY && roots.empty(); }
-  bool is_exclude() const { return control.roots_op == EXCLUDE_OR_RELEASE && !roots.empty(); }
-  bool is_release() const { return control.roots_op == EXCLUDE_OR_RELEASE && roots.empty(); }
-  bool is_reset() const { return control.roots_op == RESET_OR_CANCEL && !roots.empty(); }
-  bool is_cancel() const { return control.roots_op == RESET_OR_CANCEL && roots.empty(); }
+  bool is_exclude() const { return control.roots_op == EXCLUDE_OR_CANCEL && !roots.empty(); }
+  bool is_release() const { return control.roots_op == RESET_OR_RELEASE && roots.empty(); }
+  bool is_reset() const { return control.roots_op == RESET_OR_RELEASE && !roots.empty(); }
+  bool is_cancel() const { return control.roots_op == EXCLUDE_OR_CANCEL && roots.empty(); }
 
   bool is_verbose() const { return control.flags & Flags::VERBOSE; }
   bool is_exclusive() const { return control.flags & Flags::EXCLUSIVE; }
@@ -444,11 +444,11 @@ struct QuiesceDbRequest {
     switch (control.roots_op) {
     case INCLUDE_OR_QUERY:
       return false;
-    case EXCLUDE_OR_RELEASE:
-      return roots.contains(root);
-    case RESET_OR_CANCEL:
-      return !roots.contains(root);
-      default: ceph_abort("unknown roots_op"); return false;
+    case EXCLUDE_OR_CANCEL:
+      return roots.empty() || roots.contains(root);
+    case RESET_OR_RELEASE:
+      return !roots.empty() && !roots.contains(root);
+    default: ceph_abort("unknown roots_op"); return false;
     }
   }
 
@@ -493,22 +493,22 @@ struct QuiesceDbRequest {
   template <typename R = Roots>
   void exclude_roots(R&& roots)
   {
-    set_roots(EXCLUDE_OR_RELEASE, std::forward<R>(roots));
+    set_roots(EXCLUDE_OR_CANCEL, std::forward<R>(roots));
   }
 
-  void release_roots() {
-    set_roots(EXCLUDE_OR_RELEASE, {});
+  void release() {
+    set_roots(RESET_OR_RELEASE, {});
   }
 
   template <typename R = Roots>
   void reset_roots(R&& roots)
   {
-    set_roots(RESET_OR_CANCEL, std::forward<R>(roots));
+    set_roots(RESET_OR_RELEASE, std::forward<R>(roots));
   }
 
-  void cancel_roots()
+  void cancel()
   {
-    set_roots(RESET_OR_CANCEL, {});
+    set_roots(EXCLUDE_OR_CANCEL, {});
   }
 
   template <typename S = std::string>
@@ -522,10 +522,10 @@ struct QuiesceDbRequest {
     switch (control.roots_op) {
     case INCLUDE_OR_QUERY:
       return roots.empty() ? "query" : "include";
-    case EXCLUDE_OR_RELEASE:
-      return roots.empty() ? "release" : "exclude";
-    case RESET_OR_CANCEL:
-      return roots.empty() ? "cancel" : "reset";
+    case EXCLUDE_OR_CANCEL:
+      return roots.empty() ? "cancel" : "exclude";
+    case RESET_OR_RELEASE:
+      return roots.empty() ? "release" : "reset";
     default:
       return "<unknown>";
     }
@@ -547,11 +547,11 @@ operator<<(std::basic_ostream<CharT, Traits>& os, const QuiesceDbRequest& req)
   os << "q-req[" << req.op_string();
 
   if (req.set_id) {
-    os << " \"" << req.set_id << "\"";
+    os << " \"" << *req.set_id << "\"";
   }
 
   if (req.if_version) {
-    os << " ?v:" << req.if_version;
+    os << " ?v:" << *req.if_version;
   }
 
   if (req.await) {

diff --git a/src/mds/QuiesceDbManager.cc b/src/mds/QuiesceDbManager.cc
index 88844f09c817e8400da1e71fac024ff7b50cd714..696386bea8e211b0b79841494580a0903cddf741 100644 (file)
--- a/src/mds/QuiesceDbManager.cc
+++ b/src/mds/QuiesceDbManager.cc
@@ -447,7 +447,7 @@ void QuiesceDbManager::complete_requests() {
     }
 
     // non-zero result codes are all errors
-    dout(10) << "completing request '" << req->request << " with rc: " << -res << dendl;
+    dout(10) << "completing " << req->request << " with rc: " << -res << dendl;
     req->complete(-res);
   }
   done_requests.clear();
@@ -589,6 +589,8 @@ int QuiesceDbManager::leader_process_request(RequestContext* req_ctx)
     return EINVAL;
   }
 
+  dout(20) << request << dendl;
+
   const auto db_age = db.get_age();
 
   if (request.is_cancel_all()) {

diff --git a/src/test/mds/TestQuiesceDb.cc b/src/test/mds/TestQuiesceDb.cc
index 19c8d9b6163d268d8f60989327b9a8045dddb95d..4a3f8ac597a5918045ead3e49e2f6e75120fcc91 100644 (file)
--- a/src/test/mds/TestQuiesceDb.cc
+++ b/src/test/mds/TestQuiesceDb.cc
@@ -562,7 +562,7 @@ TEST_F(QuiesceDbTest, QuiesceRequestValidation)
       return !testing::Test::HasFailure();
   };
 
-  const auto ops = std::array { QuiesceDbRequest::RootsOp::INCLUDE_OR_QUERY, QuiesceDbRequest::RootsOp::EXCLUDE_OR_RELEASE, QuiesceDbRequest::RootsOp::RESET_OR_CANCEL, QuiesceDbRequest::RootsOp::__INVALID };
+  const auto ops = std::array { QuiesceDbRequest::RootsOp::INCLUDE_OR_QUERY, QuiesceDbRequest::RootsOp::EXCLUDE_OR_CANCEL, QuiesceDbRequest::RootsOp::RESET_OR_RELEASE, QuiesceDbRequest::RootsOp::__INVALID };
   const auto strings = nullopt_and_default<std::string>();
   const auto versions = nullopt_and_default<QuiesceSetVersion>();
   const auto intervals = nullopt_and_default<QuiesceTimeInterval>();
@@ -766,7 +766,7 @@ TEST_F(QuiesceDbTest, SetModification)
 
   // cancel with no set_id should cancel all active sets
   ASSERT_EQ(OK(), run_request([](auto& r) {
-    r.control.roots_op = QuiesceDbRequest::RootsOp::RESET_OR_CANCEL;
+    r.cancel();
   }));
 
   ASSERT_TRUE(db(mds_gid_t(1)).sets.at("set1").members.at("file:/root4").excluded);
@@ -829,7 +829,7 @@ TEST_F(QuiesceDbTest, Timeouts) {
 
   ASSERT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set2";
-    r.release_roots();
+    r.release();
   }));
 
   ASSERT_EQ(QuiesceState::QS_RELEASING, last_request->response.sets.at("set2").rstate.state);
@@ -939,7 +939,7 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait)
   ASSERT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
     r.expiration = sec(100);
-    r.timeout = sec(10);
+    r.timeout = sec(100);
     r.roots.emplace("root1");
   }));
 
@@ -1006,7 +1006,7 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait)
 
   ASSERT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.reset_roots({});
+    r.cancel();
   }));
 
   EXPECT_EQ(ERR(ECANCELED), await3.wait_result());
@@ -1069,7 +1069,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) {
   for (int i = 0; i < 2; i++) {
     ASSERT_EQ(ERR(EINPROGRESS), run_request([=](auto& r) {
       r.set_id = "set1";
-      r.release_roots();
+      r.release();
       r.await = (expiration*2)/5;
     }));
   }
@@ -1077,7 +1077,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) {
   // NB: the ETIMEDOUT is the await result, while the set itself should be EXPIRED
   EXPECT_EQ(ERR(ETIMEDOUT), run_request([=](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
     r.await = expiration;
   }));
 
@@ -1090,7 +1090,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) {
 
   EXPECT_EQ(ERR(EPERM), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
   }));
 
 }
@@ -1115,7 +1115,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
   for (auto&& set_id : { "set1", "set2" }) {
     ASSERT_EQ(ERR(EPERM), run_request([set_id](auto& r) {
       r.set_id = set_id;
-      r.release_roots();
+      r.release();
       r.await = sec(1);
     })) << "bad release-await " << set_id;
   }
@@ -1134,13 +1134,13 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
 
   auto & release_await1 = start_request([](auto &r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
     r.await = sec(100);
   });
 
   auto& release_await2 = start_request([](auto& r) {
     r.set_id = "set2";
-    r.release_roots();
+    r.release();
     r.await = sec(100);
   });
 
@@ -1155,7 +1155,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
   // we can request release again without any version bump
   EXPECT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
   }));
 
   EXPECT_EQ(releasing_v1, last_request->response.sets.at("set1").version );
@@ -1163,7 +1163,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
   // we can release-await with a short await timeout
   EXPECT_EQ(ERR(EINPROGRESS), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
     r.await = sec(0.1);
   }));
 
@@ -1197,7 +1197,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
   // await again
   auto& release_await22 = start_request([](auto& r) {
     r.set_id = "set2";
-    r.release_roots();
+    r.release();
     r.await = sec(100);
   });
 
@@ -1235,18 +1235,18 @@ TEST_F(QuiesceDbTest, ReleaseAwait)
   // it should be OK to request release or release-await on a RELEASED set
   EXPECT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
   }));
 
   EXPECT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
     r.await = sec(0.1);
   }));
 
   // it's invalid to send a release without a set id
   EXPECT_EQ(ERR(EINVAL), run_request([](auto& r) {
-    r.release_roots();
+    r.release();
   }));
 }
 
@@ -1453,7 +1453,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease)
   // release roots
   ASSERT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
   }));
 
   EXPECT_EQ(QS_RELEASING, last_request->response.sets.at("set1").rstate.state);
@@ -1463,7 +1463,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease)
   auto &async_release = start_request([](auto& r) {
     r.set_id = "set2";
     r.await = sec(100);
-    r.release_roots();
+    r.release();
   });
 
   EXPECT_EQ(NA(), async_release.check_result());
@@ -1471,7 +1471,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease)
   // shouldn't hurt to run release twice for set 1
   ASSERT_EQ(OK(), run_request([](auto& r) {
     r.set_id = "set1";
-    r.release_roots();
+    r.release();
   }));
 
   EXPECT_EQ(releasing_v, last_request->response.sets.at("set1").version);
@@ -1522,7 +1522,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease)
     ASSERT_EQ(OK(), run_request([set_id](auto& r) {
       r.set_id = set_id;
       r.await = sec(100);
-      r.release_roots();
+      r.release();
     }));
     ASSERT_EQ(ERR(EPERM), run_request([set_id](auto& r) {
       r.set_id = set_id;