Merge pull request #21607 from jcsp/wip-mgr-selftest-fixup

mgr: reconcile can_run checks and selftest

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

debian/control

@@ -185,6 +185,7 @@ Depends: ceph-base (= ${binary:Version}),
          ${misc:Depends},
          ${python:Depends},
          ${shlibs:Depends},
+Suggests: python-influxdb
 Replaces: ceph (<< 0.93-417),
 Breaks: ceph (<< 0.93-417),
 Description: manager for the ceph distributed storage system

qa/tasks/mgr/mgr_test_case.py

@@ -118,7 +118,7 @@ def _load_module(cls, module_name):
 
         initial_gid = cls.mgr_cluster.get_mgr_map()['active_gid']
         cls.mgr_cluster.mon_manager.raw_cluster_cmd("mgr", "module", "enable",
-                                         module_name)
+                                                    module_name, "--force")
 
         # Wait for the module to load
         def has_restarted():

src/mgr/ActivePyModule.cc

@@ -110,6 +110,13 @@ int ActivePyModule::handle_command(
   assert(ss != nullptr);
   assert(ds != nullptr);
 
+  if (pClassInstance == nullptr) {
+    // Not the friendliest error string, but we could only
+    // hit this in quite niche cases, if at all.
+    *ss << "Module not instantiated";
+    return -EINVAL;
+  }
+
   Gil gil(py_module->pMyThreadState, true);
 
   PyFormatter f;

src/mgr/ActivePyModules.cc

@@ -756,9 +756,14 @@ int ActivePyModules::handle_command(
   std::stringstream *ss)
 {
   lock.Lock();
-  auto mod = modules.at(module_name).get();
+  auto mod_iter = modules.find(module_name);
+  if (mod_iter == modules.end()) {
+    *ss << "Module '" << module_name << "' is not available";
+    return -ENOENT;
+  }
+
   lock.Unlock();
-  return mod->handle_command(cmdmap, ds, ss);
+  return mod_iter->second->handle_command(cmdmap, ds, ss);
 }
 
 void ActivePyModules::get_health_checks(health_check_map_t *checks)

src/mgr/PyModuleRegistry.cc

@@ -195,7 +195,7 @@ void PyModuleRegistry::active_start(
   for (const auto &i : modules) {
     // Anything we're skipping because of !can_run will be flagged
     // to the user separately via get_health_checks
-    if (!(i.second->is_enabled() && i.second->get_can_run())) {
+    if (!(i.second->is_enabled() && i.second->is_loaded())) {
       continue;
     }
 
@@ -353,7 +353,13 @@ void PyModuleRegistry::get_health_checks(health_check_map_t *checks)
       if (module->is_enabled() && !module->get_can_run()) {
         dependency_modules[module->get_name()] = module->get_error_string();
       } else if ((module->is_enabled() && !module->is_loaded())
-              || module->is_failed()) {
+              || (module->is_failed() && module->get_can_run())) {
+        // - Unloadable modules are only reported if they're enabled,
+        //   to avoid spamming users about modules they don't have the
+        //   dependencies installed for because they don't use it.
+        // - Failed modules are only reported if they passed the can_run
+        //   checks (to avoid outputting two health messages about a
+        //   module that said can_run=false but we tried running it anyway)
         failed_modules[module->get_name()] = module->get_error_string();
       }
     }