Merge pull request ceph#57099 from ceph/wip-lusov-quiesce-await-eperm

mds/quiesce: db: quiesce-await should EPERM if a set is past QS_QUIESCED
Reviewed-by: Patrick Donnelly <[email protected]>

Author: leonid-s-usov

doc/cephfs/fs-volumes.rst

@@ -1024,15 +1024,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

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

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);
   }));