mds/quiesce: db: quiesce-await should EPERM if a set is past QS_QUIESCED

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

diff --git a/doc/cephfs/fs-volumes.rst b/doc/cephfs/fs-volumes.rst
index 5422795f0d821b9759afa4f0153ffdcc280b514a..8ad5258814ae6a56c88c42b69f41d090c855b941 100644 (file)
--- a/doc/cephfs/fs-volumes.rst
+++ b/doc/cephfs/fs-volumes.rst
@@ -1071,15 +1071,41 @@ Note that the commands above are all non-blocking. If we want to wait for the qu
 to reach the `QUIESCED` state, we should await it at some point. ``--await`` can be given
 along with other arguments to let the system know our intention.
 
-Technically, there are two types of await: `quiesce await` and `release await`. The former
-is the default, and the latter can only be achieved with ``--release`` present in the argument list.
-To avoid confision, it is not permitted to issue a `quiesce await` when the set is already `RELEASING`
-or `RELEASED`. Trying to ``--release`` a set that is not `QUIESCED` is an ``EPERM`` error as well, regardless
-of whether await is requested alongside. However, it's not an error to `release await` 
-an already released set, or to `quiesce await` a `QUIESCED` one.
-
-When awaiting, one may also specify a maximum duration that they would like this await request to block for,
-not affecting the two intrinsic timeouts discussed above. If the target awaited state isn't reached
+There are two types of await: `quiesce await` and `release await`. The former is the default,
+and the latter can only be achieved with ``--release`` present in the argument list.
+To avoid confision, it is not permitted to issue a `quiesce await` when the set is not `QUIESCING`.
+Trying to ``--release`` a set that is not `QUIESCED` is an ``EPERM`` error as well, regardless
+of whether await is requested alongside. However, it's not an error to `release await`
+an already released set, or to `quiesce await` a `QUIESCED` one - those are successful no-ops.
+
+Since a set is awaited after the application of the ``--await``-augmented command, the await operation
+may mask a successful result with its own error. A good example is trying to cancel-await a set:
+
+.. prompt:: bash $ auto
+
+  $ ceph fs quiesce fs1 --set-id set1 --cancel --await
+  {
+      // ...
+      "sets": {
+          "set1": {
+              // ...
+              "state": {
+                  "name": "CANCELED",
+                  "age": 0
+              },
+              // ...
+          }
+      }
+  }
+  Error EPERM: 
+
+Although ``--cancel`` will succeed syncrhonously for a set in an active state, awaiting a canceled
+set is not permitted, hence this call will result in an ``EPERM``. This is deliberately different from 
+returning a ``EINVAL`` error, denoting an error on the user's side, to simplify the system's behavior
+when ``--await`` is requested. As a result, it's also a simpler model for the user to work with.
+
+When awaiting, one may specify a maximum duration that they would like this await request to block for,
+orthogonally to the two intrinsic set timeouts discussed above. If the target awaited state isn't reached
 within the specified duration, then ``EINPROGRESS`` is returned. For that, one should use the argument
 ``--await-for=<seconds>``. One could think of ``--await`` as equivalent to ``--await-for=Infinity``.
 While it doesn't make sense to specify both arguments, it is not considered an error. If

diff --git a/src/mds/QuiesceDbManager.cc b/src/mds/QuiesceDbManager.cc
index d070434dbff3124c2c093b15ec3377e36a15d244..2c727d715260858ed5d3511cd9fe0aac39ec7a32 100644 (file)
--- a/src/mds/QuiesceDbManager.cc
+++ b/src/mds/QuiesceDbManager.cc
@@ -623,12 +623,13 @@ int QuiesceDbManager::leader_process_request(RequestContext* req_ctx)
     }
 
     if (request.await) {
+      // quiesce-await is only allowed for sets that are quiescing or quiesced.
       // this check may have a false negative for a quiesced set
       // that will be released in another request in the same batch
       // in that case, this await will be enqueued but then found and completed
       // with the same error in `leader_upkeep_awaits`
-      if ((set.is_releasing() || set.is_released()) && !request.is_release()) {
-        dout(2) << dset("can't quiesce-await a set that was released (") << set.rstate.state << ")" << dendl;
+      if (set.rstate.state > QS_QUIESCED && !request.is_release()) {
+        dout(2) << dset("can't quiesce-await a set in the state: ") << set.rstate.state << dendl;
         return EPERM;
       }
 
@@ -1055,7 +1056,7 @@ QuiesceTimeInterval QuiesceDbManager::leader_upkeep_awaits()
           break;
         case QS_EXPIRED:
         case QS_TIMEDOUT:
-          rc = ETIMEDOUT; 
+          rc = ETIMEDOUT;
           break;
         case QS_QUIESCED:
           rc = 0; // fallthrough

diff --git a/src/test/mds/TestQuiesceDb.cc b/src/test/mds/TestQuiesceDb.cc
index cf05aaa038d0ac7eff0520bb9c5b6455cfdd1e12..4f98e5dbab53517105f5159e752d1eba53f2e64c 100644 (file)
--- a/src/test/mds/TestQuiesceDb.cc
+++ b/src/test/mds/TestQuiesceDb.cc
@@ -1010,13 +1010,12 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait)
   EXPECT_EQ(ERR(ECANCELED), await3.wait_result());
   EXPECT_EQ(ERR(ECANCELED), await4.wait_result());
 
-  // awaiting a set in a terminal state should immediately
-  // complete with the corresponding error
-  ASSERT_EQ(ERR(ECANCELED), run_request([](auto& r) {
+  // awaiting a set in a terminal state should be illegal
+  EXPECT_EQ(ERR(EPERM), run_request([](auto& r) {
     r.set_id = "set1";
     r.await = sec(100);
   }));
-  ASSERT_EQ(ERR(ETIMEDOUT), run_request([](auto& r) {
+  EXPECT_EQ(ERR(EPERM), run_request([](auto& r) {
     r.set_id = "set2";
     r.await = sec(100);
   }));
@@ -1082,7 +1081,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) {
 
   EXPECT_EQ(QS_EXPIRED, last_request->response.sets.at("set1").rstate.state);
 
-  EXPECT_EQ(ERR(ETIMEDOUT), run_request([](auto& r) {
+  EXPECT_EQ(ERR(EPERM), run_request([](auto& r) {
     r.set_id = "set1";
     r.await = sec(0.1);
   }));