Merge pull request #21811 from tchaikov/wip-23627

librados: block MgrClient::start_command until mgrmap

Reviewed-by: John Spray <john.spray@redhat.com>
Reviewed-by: Sage Weil <sage@redhat.com>

qa/workunits/mon/caps.sh

@@ -8,6 +8,18 @@ exit_on_error=1
 
 [[ ! -z $TEST_EXIT_ON_ERROR ]] && exit_on_error=$TEST_EXIT_ON_ERROR
 
+if [ `uname` = FreeBSD ]; then
+    ETIMEDOUT=60
+else
+    ETIMEDOUT=110
+fi
+
+# monitor drops the subscribe message from client if it does not have enough caps
+# for read from mon. in that case, the client will be waiting for mgrmap in vain,
+# if it is instructed to send a command to mgr. "pg dump" is served by mgr. so,
+# we need to set a timeout for testing this scenario.
+export CEPH_ARGS='--rados-mon-op-timeout=5'
+
 expect()
 {
   cmd=$1
@@ -37,7 +49,7 @@ expect "ceph -k $tmp.foo.keyring --user foo auth ls" 0
 expect "ceph -k $tmp.foo.keyring --user foo auth export" 13
 expect "ceph -k $tmp.foo.keyring --user foo auth del client.bazar" 13
 expect "ceph -k $tmp.foo.keyring --user foo osd dump" 13
-expect "ceph -k $tmp.foo.keyring --user foo pg dump" 13
+expect "ceph -k $tmp.foo.keyring --user foo pg dump" $ETIMEDOUT
 expect "ceph -k $tmp.foo.keyring --user foo quorum_status" 13
 ceph auth del client.foo
 
@@ -48,7 +60,7 @@ expect "ceph -k $tmp.bar.keyring --user bar auth ls" 13
 expect "ceph -k $tmp.bar.keyring --user bar auth export" 13
 expect "ceph -k $tmp.bar.keyring --user bar auth del client.foo" 13
 expect "ceph -k $tmp.bar.keyring --user bar osd dump" 13
-expect "ceph -k $tmp.bar.keyring --user bar pg dump" 13
+expect "ceph -k $tmp.bar.keyring --user bar pg dump" $ETIMEDOUT
 expect "ceph -k $tmp.bar.keyring --user bar quorum_status" 13
 ceph auth del client.bar
 

src/ceph.in

@@ -134,7 +134,7 @@ import subprocess
 from ceph_argparse import \
     concise_sig, descsort_key, parse_json_funcsigs, \
     matchnum, validate_command, find_cmd_target, \
-    send_command, json_command, run_in_thread
+    json_command, run_in_thread
 
 from ceph_daemon import admin_socket, DaemonWatcher, Termsize
 
@@ -566,7 +566,11 @@ def do_command(parsed_args, target, cmdargs, sigdict, inbuf, verbose):
         except KeyboardInterrupt:
             print('Interrupted')
             return ret, '', ''
-
+    if ret == errno.ETIMEDOUT:
+        ret = -ret
+        if not outs:
+            outs = ("Connection timed out. Please check the client's " +
+                    "permission and connection.")
     return ret, outbuf, outs
 
 

src/common/Cond.h

@@ -195,6 +195,15 @@ public:
       cond.Wait(lock);
     return rval;
   }
+
+  /// Wait until the \c secs expires or \c complete() is called
+  int wait_for(double secs) {
+    utime_t interval;
+    interval.set_from_double(secs);
+    Mutex::Locker l{lock};
+    cond.WaitInterval(lock, interval);
+    return done ? rval : ETIMEDOUT;
+  }
 };
 
 #endif

src/librados/RadosClient.cc

@@ -311,6 +311,11 @@ int librados::RadosClient::connect()
   }
   messenger->set_myname(entity_name_t::CLIENT(monclient.get_global_id()));
 
+  // Detect older cluster, put mgrclient into compatible mode
+  mgrclient.set_mgr_optional(
+      !get_required_monitor_features().contains_all(
+        ceph::features::mon::FEATURE_LUMINOUS));
+
   // MgrClient needs this (it doesn't have MonClient reference itself)
   monclient.sub_want("mgrmap", 0, 0);
   monclient.renew_subs();
@@ -861,7 +866,11 @@ int librados::RadosClient::mgr_command(const vector<string>& cmd,
     return r;
 
   lock.Unlock();
-  r = cond.wait();
+  if (conf->rados_mon_op_timeout) {
+    r = cond.wait_for(conf->rados_mon_op_timeout);
+  } else {
+    r = cond.wait();
+  }
   lock.Lock();
 
   return r;
@@ -1096,5 +1105,6 @@ int librados::RadosClient::service_daemon_update_status(
 
 mon_feature_t librados::RadosClient::get_required_monitor_features() const
 {
-  return monclient.monmap.get_required_features();
+  return monclient.with_monmap([](const MonMap &monmap) {
+      return monmap.get_required_features(); } );
 }

src/mgr/MgrClient.cc

@@ -413,7 +413,7 @@ int MgrClient::start_command(const vector<string>& cmd, const bufferlist& inbl,
 
   ldout(cct, 20) << "cmd: " << cmd << dendl;
 
-  if (map.epoch == 0) {
+  if (map.epoch == 0 && mgr_optional) {
     ldout(cct,20) << " no MgrMap, assuming EACCES" << dendl;
     return -EACCES;
   }
@@ -429,6 +429,8 @@ int MgrClient::start_command(const vector<string>& cmd, const bufferlist& inbl,
     // Leaving fsid argument null because it isn't used.
     MCommand *m = op.get_message({});
     session->con->send_message(m);
+  } else {
+    ldout(cct, 4) << "start_command: no mgr session, waiting" << dendl;
   }
   return 0;
 }

src/mgr/MgrClient.h

@@ -88,6 +88,10 @@ protected:
   void reconnect();
   void _send_open();
 
+  // In pre-luminous clusters, the ceph-mgr service is absent or optional,
+  // so we must not block in start_command waiting for it.
+  bool mgr_optional = false;
+
 public:
   MgrClient(CephContext *cct_, Messenger *msgr_);
 
@@ -96,6 +100,8 @@ public:
   void init();
   void shutdown();
 
+  void set_mgr_optional(bool optional_) {mgr_optional = optional_;}
+
   bool ms_dispatch(Message *m) override;
   bool ms_handle_reset(Connection *con) override;
   void ms_handle_remote_reset(Connection *con) override {}