Merge pull request #18509 from tchaikov/wip-osd-cleanups

osd,mgrclient: pass daemon_status by rvalue ref and other cleanups

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

src/include/rados/librados.hpp

@@ -1318,7 +1318,7 @@ namespace librados
       const std::string& name,     ///< daemon name (e.g., 'gwfoo')
       const std::map<std::string,std::string>& metadata); ///< static metadata about daemon
     int service_daemon_update_status(
-      const std::map<std::string,std::string>& status);
+      std::map<std::string,std::string>&& status);
 
     int pool_create(const char *name);
     int pool_create(const char *name, uint64_t auid);

src/include/str_list.h

@@ -77,7 +77,7 @@ extern void get_str_set(const std::string& str,
  * @param [in] sep String used to join each element from **v**
  * @return empty string if **v** is empty or concatenated string
 **/
-inline std::string str_join(const std::vector<std::string>& v, std::string sep)
+inline std::string str_join(const std::vector<std::string>& v, const std::string& sep)
 {
   if (v.empty())
     return std::string();

src/librados/RadosClient.cc

@@ -1062,12 +1062,12 @@ int librados::RadosClient::service_daemon_register(
 }
 
 int librados::RadosClient::service_daemon_update_status(
-  const std::map<std::string,std::string>& status)
+  std::map<std::string,std::string>&& status)
 {
   if (state != CONNECTED) {
     return -ENOTCONN;
   }
-  return mgrclient.service_daemon_update_status(status);
+  return mgrclient.service_daemon_update_status(std::move(status));
 }
 
 mon_feature_t librados::RadosClient::get_required_monitor_features() const

src/librados/RadosClient.h

@@ -164,7 +164,7 @@ class librados::RadosClient : public Dispatcher
     const std::string& name,     ///< daemon name (e.g., 'gwfoo')
     const std::map<std::string,std::string>& metadata); ///< static metadata about daemon
   int service_daemon_update_status(
-    const std::map<std::string,std::string>& status);
+    std::map<std::string,std::string>&& status);
 
   mon_feature_t get_required_monitor_features() const;
 };

src/librados/librados.cc

@@ -2355,9 +2355,9 @@ int librados::Rados::service_daemon_register(
 }
 
 int librados::Rados::service_daemon_update_status(
-  const std::map<std::string,std::string>& status)
+  std::map<std::string,std::string>&& status)
 {
-  return client->service_daemon_update_status(status);
+  return client->service_daemon_update_status(std::move(status));
 }
 
 int librados::Rados::pool_create(const char *name)
@@ -3338,7 +3338,7 @@ CEPH_RADOS_API int rados_service_update_status(rados_t cluster,
   std::map<std::string, std::string> status;
   dict_to_map(status_dict, &status);
 
-  return client->service_daemon_update_status(status);
+  return client->service_daemon_update_status(std::move(status));
 }
 
 static void do_out_buffer(bufferlist& outbl, char **outbuf, size_t *outbuflen)

src/mgr/MgrClient.cc

@@ -216,6 +216,19 @@ bool MgrClient::ms_handle_refused(Connection *con)
   return false;
 }
 
+void MgrClient::send_stats()
+{
+  send_report();
+  send_pgstats();
+  if (stats_period != 0) {
+    report_callback = timer.add_event_after(
+      stats_period,
+      new FunctionContext([this](int) {
+	  send_stats();
+	}));
+  }
+}
+
 void MgrClient::send_report()
 {
   assert(lock.is_locked_by_me());
@@ -314,13 +327,6 @@ void MgrClient::send_report()
   }
 
   session->con->send_message(report);
-
-  if (stats_period != 0) {
-    report_callback = new FunctionContext([this](int r){send_report();});
-    timer.add_event_after(stats_period, report_callback);
-  }
-
-  send_pgstats();
 }
 
 void MgrClient::send_pgstats()
@@ -352,7 +358,7 @@ bool MgrClient::handle_mgr_configure(MMgrConfigure *m)
   bool starting = (stats_period == 0) && (m->stats_period != 0);
   stats_period = m->stats_period;
   if (starting) {
-    send_report();
+    send_stats();
   }
 
   m->put();
@@ -453,11 +459,11 @@ int MgrClient::service_daemon_register(
 }
 
 int MgrClient::service_daemon_update_status(
-  const std::map<std::string,std::string>& status)
+  std::map<std::string,std::string>&& status)
 {
   Mutex::Locker l(lock);
   ldout(cct,10) << status << dendl;
-  daemon_status = status;
+  daemon_status = std::move(status);
   daemon_dirty_status = true;
   return 0;
 }

src/mgr/MgrClient.h

@@ -100,9 +100,7 @@ class MgrClient : public Dispatcher
   bool handle_mgr_configure(MMgrConfigure *m);
   bool handle_command_reply(MCommandReply *m);
 
-  void send_report();
   void send_pgstats();
-
   void set_pgstats_cb(std::function<MPGStats*()> cb_)
   {
     Mutex::Locker l(lock);
@@ -118,8 +116,11 @@ class MgrClient : public Dispatcher
     const std::string& name,
     const std::map<std::string,std::string>& metadata);
   int service_daemon_update_status(
-    const std::map<std::string,std::string>& status);
+    std::map<std::string,std::string>&& status);
+
+private:
+  void send_stats();
+  void send_report();
 };
 
 #endif
-

src/osd/OSD.cc

@@ -2594,40 +2594,7 @@ int OSD::init()
   if (r < 0)
     goto out;
 
-  // This implementation unconditionally sends every is_primary PG's
-  // stats every time we're called.  This has equivalent cost to the
-  // previous implementation's worst case where all PGs are busy and
-  // their stats are always enqueued for sending.
-  mgrc.set_pgstats_cb([this](){
-      RWLock::RLocker l(map_lock);
-
-      utime_t had_for = ceph_clock_now() - had_map_since;
-      osd_stat_t cur_stat = service.get_osd_stat();
-      cur_stat.os_perf_stat = store->get_cur_stats();
-
-      MPGStats *m = new MPGStats(monc->get_fsid(), osdmap->get_epoch(), had_for);
-      m->osd_stat = cur_stat;
-
-      Mutex::Locker lec{min_last_epoch_clean_lock};
-      min_last_epoch_clean = osdmap->get_epoch();
-      min_last_epoch_clean_pgs.clear();
-      RWLock::RLocker lpg(pg_map_lock);
-      for (const auto &i : pg_map) {
-        PG *pg = i.second;
-        if (!pg->is_primary()) {
-          continue;
-        }
-
-	pg->get_pg_stats([&](const pg_stat_t& s, epoch_t lec) {
-	    m->pg_stat[pg->pg_id.pgid] = s;
-	    min_last_epoch_clean = min(min_last_epoch_clean, lec);
-	    min_last_epoch_clean_pgs.push_back(pg->pg_id.pgid);
-	  });
-      }
-
-      return m;
-  });
-
+  mgrc.set_pgstats_cb([this](){ return collect_pg_stats(); });
   mgrc.init();
   client_messenger->add_dispatcher_head(&mgrc);
 
@@ -7059,7 +7026,40 @@ void OSD::sched_scrub()
   dout(20) << "sched_scrub done" << dendl;
 }
 
+MPGStats* OSD::collect_pg_stats()
+{
+  // This implementation unconditionally sends every is_primary PG's
+  // stats every time we're called.  This has equivalent cost to the
+  // previous implementation's worst case where all PGs are busy and
+  // their stats are always enqueued for sending.
+  RWLock::RLocker l(map_lock);
+
+  utime_t had_for = ceph_clock_now() - had_map_since;
+  osd_stat_t cur_stat = service.get_osd_stat();
+  cur_stat.os_perf_stat = store->get_cur_stats();
+
+  auto m = new MPGStats(monc->get_fsid(), osdmap->get_epoch(), had_for);
+  m->osd_stat = cur_stat;
+
+  Mutex::Locker lec{min_last_epoch_clean_lock};
+  min_last_epoch_clean = osdmap->get_epoch();
+  min_last_epoch_clean_pgs.clear();
+  RWLock::RLocker lpg(pg_map_lock);
+  for (const auto &i : pg_map) {
+    PG *pg = i.second;
+    if (!pg->is_primary()) {
+      continue;
+    }
 
+    pg->get_pg_stats([&](const pg_stat_t& s, epoch_t lec) {
+	m->pg_stat[pg->pg_id.pgid] = s;
+	min_last_epoch_clean = min(min_last_epoch_clean, lec);
+	min_last_epoch_clean_pgs.push_back(pg->pg_id.pgid);
+      });
+  }
+
+  return m;
+}
 
 // =====================================================
 // MAP

src/osd/OSD.h

@@ -2217,6 +2217,9 @@ class OSD : public Dispatcher,
     }
   } remove_wq;
 
+  // -- status reporting --
+  MPGStats *collect_pg_stats();
+
 private:
   bool ms_can_fast_dispatch_any() const override { return true; }
   bool ms_can_fast_dispatch(const Message *m) const override {

src/rgw/rgw_rados.cc

@@ -3761,9 +3761,9 @@ int RGWRados::register_to_service_map(const string& daemon_type, const map<strin
   return 0;
 }
 
-int RGWRados::update_service_map(const std::map<std::string, std::string>& status)
+int RGWRados::update_service_map(std::map<std::string, std::string>&& status)
 {
-  int ret = rados[0].service_daemon_update_status(status);
+  int ret = rados[0].service_daemon_update_status(move(status));
   if (ret < 0) {
     ldout(cct, 0) << "ERROR: service_daemon_update_status() returned ret=" << ret << ": " << cpp_strerror(-ret) << dendl;
     return ret;

src/rgw/rgw_rados.h

@@ -2580,7 +2580,7 @@ class RGWRados
   void finalize();
 
   int register_to_service_map(const string& daemon_type, const map<string, string>& meta);
-  int update_service_map(const std::map<std::string, std::string>& status);
+  int update_service_map(std::map<std::string, std::string>&& status);
 
   void schedule_context(Context *c);
 

src/rgw/rgw_sync_trace.cc

@@ -72,7 +72,7 @@ int RGWSyncTraceServiceMapThread::process()
 {
   map<string, string> status;
   status["current_sync"] = manager->get_active_names();
-  int ret = store->update_service_map(status);
+  int ret = store->update_service_map(std::move(status));
   if (ret < 0) {
     ldout(store->ctx(), 0) << "ERROR: update_service_map() returned ret=" << ret << dendl;
   }

src/test/librados_test_stub/LibradosTestStub.cc

@@ -1052,9 +1052,9 @@ int Rados::service_daemon_register(const std::string& service,
   return impl->service_daemon_register(service, name, metadata);
 }
 
-int Rados::service_daemon_update_status(const std::map<std::string,std::string>& status) {
+int Rados::service_daemon_update_status(std::map<std::string,std::string>&& status) {
   TestRadosClient *impl = reinterpret_cast<TestRadosClient*>(client);
-  return impl->service_daemon_update_status(status);
+  return impl->service_daemon_update_status(std::move(status));
 }
 
 int Rados::pool_create(const char *name) {

src/test/librados_test_stub/MockTestMemRadosClient.h

@@ -45,10 +45,12 @@ class MockTestMemRadosClient : public TestMemRadosClient {
     return TestMemRadosClient::service_daemon_register(service, name, metadata);
   }
 
-  MOCK_METHOD1(service_daemon_update_status,
+  // workaround of https://github.com/google/googletest/issues/1155
+  MOCK_METHOD1(service_daemon_update_status_r,
                int(const std::map<std::string,std::string>&));
-  int do_service_daemon_update_status(const std::map<std::string,std::string>& status) {
-    return TestMemRadosClient::service_daemon_update_status(status);
+  int do_service_daemon_update_status_r(const std::map<std::string,std::string>& status) {
+    auto s = status;
+    return TestMemRadosClient::service_daemon_update_status(std::move(s));
   }
 
   void default_to_dispatch() {
@@ -57,7 +59,7 @@ class MockTestMemRadosClient : public TestMemRadosClient {
     ON_CALL(*this, create_ioctx(_, _)).WillByDefault(Invoke(this, &MockTestMemRadosClient::do_create_ioctx));
     ON_CALL(*this, blacklist_add(_, _)).WillByDefault(Invoke(this, &MockTestMemRadosClient::do_blacklist_add));
     ON_CALL(*this, service_daemon_register(_, _, _)).WillByDefault(Invoke(this, &MockTestMemRadosClient::do_service_daemon_register));
-    ON_CALL(*this, service_daemon_update_status(_)).WillByDefault(Invoke(this, &MockTestMemRadosClient::do_service_daemon_update_status));
+    ON_CALL(*this, service_daemon_update_status_r(_)).WillByDefault(Invoke(this, &MockTestMemRadosClient::do_service_daemon_update_status_r));
   }
 };
 

src/test/librados_test_stub/TestMemRadosClient.h

@@ -37,7 +37,7 @@ class TestMemRadosClient : public TestRadosClient {
                               const std::map<std::string,std::string>& metadata) override {
     return 0;
   }
-  int service_daemon_update_status(const std::map<std::string,std::string>& status) override {
+  int service_daemon_update_status(std::map<std::string,std::string>&& status) override {
     return 0;
   }
 

src/test/librados_test_stub/TestRadosClient.h

@@ -81,7 +81,7 @@ class TestRadosClient {
   virtual int service_daemon_register(const std::string& service,
                                       const std::string& name,
                                       const std::map<std::string,std::string>& metadata) = 0;
-  virtual int service_daemon_update_status(const std::map<std::string,std::string>& status) = 0;
+  virtual int service_daemon_update_status(std::map<std::string,std::string>&& status) = 0;
 
   virtual int pool_create(const std::string &pool_name) = 0;
   virtual int pool_delete(const std::string &pool_name) = 0;