mgr: initialize PyModuleRegistry sooner

This was waiting on MgrMap to init, so that it would know whether
modules should be enabled.  However, you don't really need to know
that until someone calls standby_start or active_start, so we
can initialize during MgrStandby::init, and fill out the enabled
flags later.

This prevents the mgr from prematurely emitting a MMgrBeacon
that claims no modules are found.

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

diff --git a/src/mgr/MgrStandby.cc b/src/mgr/MgrStandby.cc
index 2172dbc5e5f1449f56266891bdbe163ddfadf9c8..f34580bde784355fb01bee3415aac578b36e8df1 100644 (file)
--- a/src/mgr/MgrStandby.cc
+++ b/src/mgr/MgrStandby.cc
@@ -144,6 +144,8 @@ int MgrStandby::init()
   client.init();
   timer.init();
 
+  py_module_registry.init();
+
   tick();
 
   dout(4) << "Complete." << dendl;
@@ -330,16 +332,11 @@ void MgrStandby::handle_mgr_map(MMgrMap* mmap)
   dout(4) << "active in map: " << active_in_map
           << " active is " << map.active_gid << dendl;
 
-  if (!py_module_registry.is_initialized()) {
-    int r = py_module_registry.init(map);
-
-    // FIXME: error handling
-    assert(r == 0);
-  } else {
-    bool need_respawn = py_module_registry.handle_mgr_map(map);
-    if (need_respawn) {
-      respawn();
-    }
+  // PyModuleRegistry may ask us to respawn if it sees that
+  // this MgrMap is changing its set of enabled modules
+  bool need_respawn = py_module_registry.handle_mgr_map(map);
+  if (need_respawn) {
+    respawn();
   }
 
   if (active_in_map) {

diff --git a/src/mgr/PyModule.h b/src/mgr/PyModule.h
index a25792f5a67e5c7d69d0a74997271cc55d25234e..bea92689e0a6550009e1bf166bae2dfe7da3d4c1 100644 (file)
--- a/src/mgr/PyModule.h
+++ b/src/mgr/PyModule.h
@@ -69,8 +69,8 @@ public:
   PyObject *pClass = nullptr;
   PyObject *pStandbyClass = nullptr;
 
-  PyModule(const std::string &module_name_, bool const enabled_)
-    : module_name(module_name_), enabled(enabled_)
+  PyModule(const std::string &module_name_)
+    : module_name(module_name_)
   {
   }
 
@@ -85,6 +85,11 @@ public:
   static void init_ceph_module();
 #endif
 
+  void set_enabled(const bool enabled_)
+  {
+    enabled = enabled_;
+  }
+
   /**
    * Extend `out` with the contents of `this->commands`
    */

diff --git a/src/mgr/PyModuleRegistry.cc b/src/mgr/PyModuleRegistry.cc
index 82060f483f7f0932850d6d43958c476bc0772f3c..7285d02d7d3dc782d16ed0957b899d215c689c28 100644 (file)
--- a/src/mgr/PyModuleRegistry.cc
+++ b/src/mgr/PyModuleRegistry.cc
@@ -37,15 +37,10 @@ std::string PyModuleRegistry::config_prefix;
 
 
 
-int PyModuleRegistry::init(const MgrMap &map)
+int PyModuleRegistry::init()
 {
   Mutex::Locker locker(lock);
 
-  // Don't try and init me if you don't really have a map
-  assert(map.epoch > 0);
-
-  mgr_map = map;
-
   // namespace in config-key prefixed by "mgr/"
   config_prefix = std::string(g_conf->name.get_type_str()) + "/";
 
@@ -82,9 +77,9 @@ int PyModuleRegistry::init(const MgrMap &map)
   for (const auto& module_name : module_names) {
     dout(1) << "Loading python module '" << module_name << "'" << dendl;
 
-    bool enabled = (mgr_map.modules.count(module_name) > 0);
-
-    auto mod = std::make_shared<PyModule>(module_name, enabled);
+    // Everything starts disabled, set enabled flag on module
+    // when we see first MgrMap
+    auto mod = std::make_shared<PyModule>(module_name);
     int r = mod->load(pMainThreadState);
     if (r != 0) {
       // Don't use handle_pyerror() here; we don't have the GIL
@@ -107,12 +102,43 @@ int PyModuleRegistry::init(const MgrMap &map)
   return 0;
 }
 
+bool PyModuleRegistry::handle_mgr_map(const MgrMap &mgr_map_)
+{
+  Mutex::Locker l(lock);
+
+  if (mgr_map.epoch == 0) {
+    mgr_map = mgr_map_;
+
+    // First time we see MgrMap, set the enabled flags on modules
+    // This should always happen before someone calls standby_start
+    // or active_start
+    for (const auto &[module_name, module] : modules) {
+      const bool enabled = (mgr_map.modules.count(module_name) > 0);
+      module->set_enabled(enabled);
+    }
+
+    return false;
+  } else {
+    bool modules_changed = mgr_map_.modules != mgr_map.modules;
+    mgr_map = mgr_map_;
+
+    if (standby_modules != nullptr) {
+      standby_modules->handle_mgr_map(mgr_map_);
+    }
+
+    return modules_changed;
+  }
+}
+
 void PyModuleRegistry::standby_start(MonClient *monc)
 {
   Mutex::Locker l(lock);
   assert(active_modules == nullptr);
   assert(standby_modules == nullptr);
-  assert(is_initialized());
+
+  // Must have seen a MgrMap by this point, in order to know
+  // which modules should be enabled
+  assert(mgr_map.epoch > 0);
 
   dout(4) << "Starting modules in standby mode" << dendl;
 
@@ -157,7 +183,10 @@ void PyModuleRegistry::active_start(
   dout(4) << "Starting modules in active mode" << dendl;
 
   assert(active_modules == nullptr);
-  assert(is_initialized());
+
+  // Must have seen a MgrMap by this point, in order to know
+  // which modules should be enabled
+  assert(mgr_map.epoch > 0);
 
   if (standby_modules != nullptr) {
     standby_modules->shutdown();

diff --git a/src/mgr/PyModuleRegistry.h b/src/mgr/PyModuleRegistry.h
index 1f5a910d046701785b30b678a36fb6eae8b298f6..16ec132bfbdff2bc0c1d88842d03fcf09c183035 100644 (file)
--- a/src/mgr/PyModuleRegistry.h
+++ b/src/mgr/PyModuleRegistry.h
@@ -81,26 +81,12 @@ public:
     : clog(clog_)
   {}
 
-  bool handle_mgr_map(const MgrMap &mgr_map_)
-  {
-    Mutex::Locker l(lock);
-
-    bool modules_changed = mgr_map_.modules != mgr_map.modules;
-    mgr_map = mgr_map_;
-
-    if (standby_modules != nullptr) {
-      standby_modules->handle_mgr_map(mgr_map_);
-    }
-
-    return modules_changed;
-  }
-
-  bool is_initialized() const
-  {
-    return mgr_map.epoch > 0;
-  }
+  /**
+   * @return true if the mgrmap has changed such that the service needs restart
+   */
+  bool handle_mgr_map(const MgrMap &mgr_map_);
 
-  int init(const MgrMap &map);
+  int init();
 
   void active_start(
                 PyModuleConfig &config_,