Merge pull request #20321 from jcsp/wip-22918

mgr: initialize PyModuleRegistry sooner

Reviewed-by: Kefu Chai <kchai@redhat.com>

src/mgr/MgrStandby.cc

@@ -154,6 +154,8 @@ int MgrStandby::init()
   client.init();
   timer.init();
 
+  py_module_registry.init();
+
   tick();
 
   dout(4) << "Complete." << dendl;
@@ -342,16 +344,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) {

src/mgr/PyModule.h

@@ -69,8 +69,8 @@ class PyModule
   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 @@ class PyModule
   static void init_ceph_module();
 #endif
 
+  void set_enabled(const bool enabled_)
+  {
+    enabled = enabled_;
+  }
+
   /**
    * Extend `out` with the contents of `this->commands`
    */

src/mgr/PyModuleRegistry.cc

@@ -37,15 +37,10 @@ std::string PyModuleRegistry::config_prefix;
 
 
 
-int PyModuleRegistry::init(const MgrMap &map)
+void 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
@@ -103,16 +98,45 @@ int PyModuleRegistry::init(const MgrMap &map)
     clog->error() << "Failed to load ceph-mgr modules: " << joinify(
         failed_modules.begin(), failed_modules.end(), std::string(", "));
   }
+}
 
-  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 +181,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();

src/mgr/PyModuleRegistry.h

@@ -81,26 +81,12 @@ class PyModuleRegistry
     : 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);
+  void init();
 
   void active_start(
                 PyModuleConfig &config_,