From: John Spray <john.spray@redhat.com>
Date: Sat, 23 Sep 2017 15:55:55 +0000 (-0400)
Subject: mgr: store declared_types in MgrSession
X-Git-Tag: v13.0.1~784^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=dc415f1ae09a308bd448614934a4c168eb9cf07b;p=ceph.git

mgr: store declared_types in MgrSession

Because we don't (yet) properly prevent multiple sessions
from daemons reporting the same name (e.g. rgws), storing
it in the DaemonPerfCounters meant that one daemon's report
was referring to another daemon's set of reported types.

This should always have been a property of the session.

The behaviour will still be ugly when multiple daemons
are using the same name (stomping on each other's stats/statsu)
but it shouldn't crash.

Fixes: http://tracker.ceph.com/issues/21197
Signed-off-by: John Spray <john.spray@redhat.com>
---

diff --git a/src/mgr/DaemonState.cc b/src/mgr/DaemonState.cc
index 8da321747357..a7b8f572e161 100644
--- a/src/mgr/DaemonState.cc
+++ b/src/mgr/DaemonState.cc
@@ -13,6 +13,8 @@
 
 #include "DaemonState.h"
 
+#include "MgrSession.h"
+
 #define dout_context g_ceph_context
 #define dout_subsys ceph_subsys_mgr
 #undef dout_prefix
@@ -123,14 +125,18 @@ void DaemonPerfCounters::update(MMgrReport *report)
 	   << types.size() << " types, got "
            << report->packed.length() << " bytes of data" << dendl;
 
+  // Retrieve session state
+  MgrSessionRef session(static_cast<MgrSession*>(
+        report->get_connection()->get_priv()));
+
   // Load any newly declared types
   for (const auto &t : report->declare_types) {
     types.insert(std::make_pair(t.path, t));
-    declared_types.insert(t.path);
+    session->declared_types.insert(t.path);
   }
   // Remove any old types
   for (const auto &t : report->undeclare_types) {
-    declared_types.erase(t);
+    session->declared_types.erase(t);
   }
 
   const auto now = ceph_clock_now();
@@ -138,7 +144,7 @@ void DaemonPerfCounters::update(MMgrReport *report)
   // Parse packed data according to declared set of types
   bufferlist::iterator p = report->packed.begin();
   DECODE_START(1, p);
-  for (const auto &t_path : declared_types) {
+  for (const auto &t_path : session->declared_types) {
     const auto &t = types.at(t_path);
     uint64_t val = 0;
     uint64_t avgcount = 0;
diff --git a/src/mgr/DaemonState.h b/src/mgr/DaemonState.h
index 7dbdeb657cda..846ce5dd8d9f 100644
--- a/src/mgr/DaemonState.h
+++ b/src/mgr/DaemonState.h
@@ -74,18 +74,11 @@ class DaemonPerfCounters
 
   std::map<std::string, PerfCounterInstance> instances;
 
-  // FIXME: this state is really local to DaemonServer, it's part
-  // of the protocol rather than being part of what other classes
-  // mgiht want to read.  Maybe have a separate session object
-  // inside DaemonServer instead of stashing session-ish state here?
-  std::set<std::string> declared_types;
-
   void update(MMgrReport *report);
 
   void clear()
   {
     instances.clear();
-    declared_types.clear();
   }
 };
 
diff --git a/src/mgr/MgrSession.h b/src/mgr/MgrSession.h
index 72afd7a67672..c52e2e177761 100644
--- a/src/mgr/MgrSession.h
+++ b/src/mgr/MgrSession.h
@@ -23,6 +23,8 @@ struct MgrSession : public RefCountedObject {
   // mon caps are suitably generic for mgr
   MonCap caps;
 
+  std::set<std::string> declared_types;
+
   MgrSession(CephContext *cct) : RefCountedObject(cct, 0) {}
   ~MgrSession() override {}
 };
diff --git a/src/mgr/PyModules.cc b/src/mgr/PyModules.cc
index 0fdf21654ab4..003a0b9bee53 100644
--- a/src/mgr/PyModules.cc
+++ b/src/mgr/PyModules.cc
@@ -761,7 +761,9 @@ PyObject* PyModules::get_perf_schema_python(
       f.open_object_section(daemon_name.str().c_str());
 
       Mutex::Locker l(state->lock);
-      for (auto typestr : state->perf_counters.declared_types) {
+      for (const auto &ctr_inst_iter : state->perf_counters.instances) {
+        const auto &typestr = ctr_inst_iter.first;
+
 	f.open_object_section(typestr.c_str());
 	auto type = state->perf_counters.types[typestr];
 	f.dump_string("description", type.description);