Merge pull request #25651 from votdev/improve_basemgrmodule

mgr: Allow modules to get/set other module options

Reviewed-by: Sage Weil <sage@redhat.com>
Reviewed-by: Sebastian Wagner <swagner@suse.com>
Reviewed-by: Ernesto Puerta <epuertat@redhat.com>

src/mgr/ActivePyModules.cc

@@ -26,6 +26,7 @@
 
 // For ::config_prefix
 #include "PyModule.h"
+#include "PyModuleRegistry.h"
 
 #include "ActivePyModules.h"
 #include "DaemonServer.h"
@@ -40,11 +41,12 @@ ActivePyModules::ActivePyModules(PyModuleConfig &module_config_,
           DaemonStateIndex &ds, ClusterState &cs,
           MonClient &mc, LogChannelRef clog_,
           LogChannelRef audit_clog_, Objecter &objecter_,
-          Client &client_, Finisher &f, DaemonServer &server)
+          Client &client_, Finisher &f, DaemonServer &server,
+          PyModuleRegistry &pmr)
   : module_config(module_config_), daemon_state(ds), cluster_state(cs),
     monc(mc), clog(clog_), audit_clog(audit_clog_), objecter(objecter_),
     client(client_), finisher(f),
-    server(server), lock("ActivePyModules")
+    server(server), py_module_registry(pmr), lock("ActivePyModules")
 {
   store_cache = std::move(store_data);
 }
@@ -519,6 +521,27 @@ bool ActivePyModules::get_config(const std::string &module_name,
   }
 }
 
+PyObject *ActivePyModules::get_typed_config(
+  const std::string &module_name,
+  const std::string &key) const
+{
+  if (!py_module_registry.module_exists(module_name)) {
+    derr << "Module '" << module_name << "' is not available" << dendl;
+    Py_RETURN_NONE;
+  }
+
+  std::string value;
+  bool found = get_config(module_name, key, &value);
+  if (found) {
+    PyModuleRef module = py_module_registry.get_module(module_name);
+    dout(10) << __func__ << " " << key << " found: " << value << dendl;
+    return module->get_typed_option_value(key, value);
+  }
+
+  dout(4) << __func__ << " " << key << " not found " << dendl;
+  Py_RETURN_NONE;
+}
+
 PyObject *ActivePyModules::get_store_prefix(const std::string &module_name,
     const std::string &prefix) const
 {

src/mgr/ActivePyModules.h

@@ -32,6 +32,7 @@
 
 class health_check_map_t;
 class DaemonServer;
+class PyModuleRegistry;
 
 class ActivePyModules
 {
@@ -46,6 +47,7 @@ class ActivePyModules
   Client   &client;
   Finisher &finisher;
   DaemonServer &server;
+  PyModuleRegistry &py_module_registry;
 
 
   mutable Mutex lock{"ActivePyModules::lock"};
@@ -55,7 +57,7 @@ public:
             std::map<std::string, std::string> store_data,
             DaemonStateIndex &ds, ClusterState &cs, MonClient &mc,
             LogChannelRef clog_, LogChannelRef audit_clog_, Objecter &objecter_, Client &client_,
-            Finisher &f, DaemonServer &server);
+            Finisher &f, DaemonServer &server, PyModuleRegistry &pmr);
 
   ~ActivePyModules();
 
@@ -110,6 +112,9 @@ public:
   void set_config(const std::string &module_name,
       const std::string &key, const boost::optional<std::string> &val);
 
+  PyObject *get_typed_config(const std::string &module_name,
+    const std::string &key) const;
+
   void set_health_checks(const std::string& module_name,
 			 health_check_map_t&& checks);
   void get_health_checks(health_check_map_t *checks);
@@ -165,4 +170,3 @@ public:
   void cluster_log(const std::string &channel, clog_type prio,
     const std::string &message);
 };
-

src/mgr/BaseMgrModule.cc

@@ -386,28 +386,36 @@ ceph_option_get(BaseMgrModule *self, PyObject *args)
 }
 
 static PyObject*
-ceph_get_module_option(BaseMgrModule *self, PyObject *args)
+ceph_get_module_option_ex(BaseMgrModule *self, PyObject *args)
 {
+  char *module = nullptr;
   char *what = nullptr;
-  if (!PyArg_ParseTuple(args, "s:ceph_get_module_option", &what)) {
+  if (!PyArg_ParseTuple(args, "ss:ceph_get_module_option_ex", &module, &what)) {
     derr << "Invalid args!" << dendl;
     return nullptr;
   }
-
   PyThreadState *tstate = PyEval_SaveThread();
-  std::string value;
-  bool found = self->py_modules->get_config(self->this_module->get_name(),
-      what, &value);
-
+  auto pResult = self->py_modules->get_typed_config(module, what);
   PyEval_RestoreThread(tstate);
+  return pResult;
+}
 
-  if (found) {
-    dout(10) << __func__ << " " << what << " found: " << value.c_str() << dendl;
-    return self->this_module->py_module->get_typed_option_value(what, value);
-  } else {
-    dout(4) << __func__ << " " << what << " not found " << dendl;
-    Py_RETURN_NONE;
+static PyObject*
+ceph_get_module_option(BaseMgrModule *self, PyObject *args)
+{
+  char *key = nullptr;
+  if (!PyArg_ParseTuple(args, "s:ceph_get_module_option", &key)) {
+    derr << "Invalid args!" << dendl;
+    return nullptr;
   }
+  auto pKey = PyString_FromString(key);
+  auto pModule = PyString_FromString(self->this_module->get_name().c_str());
+  auto pArgs = PyTuple_Pack(2, pModule, pKey);
+  Py_DECREF(pKey);
+  Py_DECREF(pModule);
+  auto pResult = ceph_get_module_option_ex(self, pArgs);
+  Py_DECREF(pArgs);
+  return pResult;
 }
 
 static PyObject*
@@ -424,22 +432,45 @@ ceph_store_get_prefix(BaseMgrModule *self, PyObject *args)
 }
 
 static PyObject*
-ceph_set_module_option(BaseMgrModule *self, PyObject *args)
+ceph_set_module_option_ex(BaseMgrModule *self, PyObject *args)
 {
+  char *module = nullptr;
   char *key = nullptr;
   char *value = nullptr;
-  if (!PyArg_ParseTuple(args, "sz:ceph_set_module_option", &key, &value)) {
+  if (!PyArg_ParseTuple(args, "ssz:ceph_set_module_option_ex",
+        &module, &key, &value)) {
+    derr << "Invalid args!" << dendl;
     return nullptr;
   }
   boost::optional<string> val;
   if (value) {
     val = value;
   }
-  self->py_modules->set_config(self->this_module->get_name(), key, val);
+  self->py_modules->set_config(module, key, val);
 
   Py_RETURN_NONE;
 }
 
+static PyObject*
+ceph_set_module_option(BaseMgrModule *self, PyObject *args)
+{
+  char *key = nullptr;
+  char *value = nullptr;
+  if (!PyArg_ParseTuple(args, "sz:ceph_set_module_option", &key, &value)) {
+    derr << "Invalid args!" << dendl;
+    return nullptr;
+  }
+  auto pModule = PyString_FromString(self->this_module->get_name().c_str());
+  auto pKey = PyString_FromString(key);
+  auto pValue = PyString_FromString(value);
+  auto pArgs = PyTuple_Pack(3, pModule, pKey, pValue);
+  Py_DECREF(pValue);
+  Py_DECREF(pKey);
+  Py_DECREF(pModule);
+  auto pResult = ceph_set_module_option_ex(self, pArgs);
+  Py_DECREF(pArgs);
+  return pResult;
+}
 
 static PyObject*
 ceph_store_get(BaseMgrModule *self, PyObject *args)
@@ -974,12 +1005,18 @@ PyMethodDef BaseMgrModule_methods[] = {
   {"_ceph_get_module_option", (PyCFunction)ceph_get_module_option, METH_VARARGS,
    "Get a module configuration option value"},
 
+  {"_ceph_get_module_option_ex", (PyCFunction)ceph_get_module_option_ex, METH_VARARGS,
+   "Get a module configuration option value from the specified module"},
+
   {"_ceph_get_store_prefix", (PyCFunction)ceph_store_get_prefix, METH_VARARGS,
    "Get all KV store values with a given prefix"},
 
   {"_ceph_set_module_option", (PyCFunction)ceph_set_module_option, METH_VARARGS,
    "Set a module configuration option value"},
 
+  {"_ceph_set_module_option_ex", (PyCFunction)ceph_set_module_option_ex, METH_VARARGS,
+   "Set a module configuration option value for the specified module"},
+
   {"_ceph_get_store", (PyCFunction)ceph_store_get, METH_VARARGS,
    "Get a stored field"},
 

src/mgr/PyModuleRegistry.cc

@@ -199,7 +199,8 @@ void PyModuleRegistry::active_start(
 
   active_modules.reset(new ActivePyModules(
               module_config, kv_store, ds, cs, mc,
-              clog_, audit_clog_, objecter_, client_, f, server));
+              clog_, audit_clog_, objecter_, client_, f, server,
+              *this));
 
   for (const auto &i : modules) {
     // Anything we're skipping because of !can_run will be flagged

src/mgr/PyModuleRegistry.h

@@ -122,6 +122,13 @@ public:
     return modules.at(module_name);
   }
 
+  bool module_exists(const std::string &module_name) const
+  {
+    std::lock_guard l(lock);
+    auto mod_iter = modules.find(module_name);
+    return mod_iter != modules.end();
+  }
+
   /**
    * Pass through command to the named module for execution.
    *

src/pybind/mgr/mgr_module.py

@@ -890,6 +890,23 @@ class MgrModule(ceph_module.BaseMgrModule):
         self._validate_module_option(key)
         return self._get_module_option(key, default)
 
+    def get_module_option_ex(self, module, key, default=None):
+        """
+        Retrieve the value of a persistent configuration setting
+        for the specified module.
+
+        :param str module: The name of the module, e.g. 'dashboard'
+            or 'telemetry'.
+        :param str key: The configuration key, e.g. 'server_addr'.
+        :param str,None default: The default value to use when the
+            returned value is ``None``. Defaults to ``None``.
+        :return: str,int,bool,float,None
+        """
+        if module == self.module_name:
+            self._validate_module_option(key)
+        r = self._ceph_get_module_option_ex(module, key)
+        return default if r is None else r
+
     def get_store_prefix(self, key_prefix):
         """
         Retrieve a dict of KV store keys to values, where the keys
@@ -933,6 +950,19 @@ class MgrModule(ceph_module.BaseMgrModule):
         self._validate_module_option(key)
         return self._set_module_option(key, val)
 
+    def set_module_option_ex(self, module, key, val):
+        """
+        Set the value of a persistent configuration setting
+        for the specified module.
+
+        :param str module:
+        :param str key:
+        :param str val:
+        """
+        if module == self.module_name:
+            self._validate_module_option(key)
+        return self._ceph_set_module_option_ex(module, key, str(val))
+
     def set_localized_module_option(self, key, val):
         """
         Set localized configuration for this ceph-mgr instance

src/pybind/mgr/selftest/module.py

@@ -29,9 +29,16 @@ class Module(MgrModule):
     # The test code in qa/ relies on these options existing -- they
     # are of course not really used for anything in the module
     MODULE_OPTIONS = [
-            {'name': 'testkey'},
-            {'name': 'testlkey'},
-            {'name': 'testnewline'}
+        {'name': 'testkey'},
+        {'name': 'testlkey'},
+        {'name': 'testnewline'},
+        {'name': 'roption1'},
+        {'name': 'roption2', 'type': 'str', 'default': 'xyz'},
+        {'name': 'rwoption1'},
+        {'name': 'rwoption2', 'type': 'int'},
+        {'name': 'rwoption3', 'type': 'float'},
+        {'name': 'rwoption4', 'type': 'str'},
+        {'name': 'rwoption5', 'type': 'bool'}
     ]
 
     COMMANDS = [
@@ -260,6 +267,73 @@ class Module(MgrModule):
         self.set_localized_module_option("testkey", "testvalue")
         assert self.get_localized_module_option("testkey") == "testvalue"
 
+        # Use default value.
+        assert self.get_module_option("roption1") is None
+        assert self.get_module_option("roption1", "foobar") == "foobar"
+        assert self.get_module_option("roption2") == "xyz"
+        assert self.get_module_option("roption2", "foobar") == "xyz"
+
+        # Option type is not defined => return as string.
+        self.set_module_option("rwoption1", 8080)
+        value = self.get_module_option("rwoption1")
+        assert isinstance(value, str)
+        assert value == "8080"
+
+        # Option type is defined => return as integer.
+        self.set_module_option("rwoption2", 10)
+        value = self.get_module_option("rwoption2")
+        assert isinstance(value, int)
+        assert value == 10
+
+        # Option type is defined => return as float.
+        self.set_module_option("rwoption3", 1.5)
+        value = self.get_module_option("rwoption3")
+        assert isinstance(value, float)
+        assert value == 1.5
+
+        # Option type is defined => return as string.
+        self.set_module_option("rwoption4", "foo")
+        value = self.get_module_option("rwoption4")
+        assert isinstance(value, str)
+        assert value == "foo"
+
+        # Option type is defined => return as bool.
+        self.set_module_option("rwoption5", False)
+        value = self.get_module_option("rwoption5")
+        assert isinstance(value, bool)
+        assert value is False
+
+        # Specified module does not exist => return None.
+        assert self.get_module_option_ex("foo", "bar") is None
+
+        # Specified key does not exist => return None.
+        assert self.get_module_option_ex("dashboard", "bar") is None
+
+        self.set_module_option_ex("telemetry", "contact", "test@test.com")
+        assert self.get_module_option_ex("telemetry", "contact") == "test@test.com"
+
+        # No option default value, so use the specified one.
+        assert self.get_module_option_ex("dashboard", "password") is None
+        assert self.get_module_option_ex("dashboard", "password", "foobar") == "foobar"
+
+        # Option type is not defined => return as string.
+        self.set_module_option_ex("dashboard", "server_port", 8080)
+        value = self.get_module_option_ex("dashboard", "server_port")
+        assert isinstance(value, str)
+        assert value == "8080"
+
+        # Option type is defined => return as integer.
+        self.set_module_option_ex("telemetry", "interval", 60)
+        value = self.get_module_option_ex("telemetry", "interval")
+        assert isinstance(value, int)
+        assert value == 60
+
+        # Option type is defined => return as bool.
+        self.set_module_option_ex("telemetry", "leaderboard", True)
+        value = self.get_module_option_ex("telemetry", "leaderboard")
+        assert isinstance(value, bool)
+        assert value is True
+
     def _self_test_store(self):
         existing_keys = set(self.get_store_prefix("test").keys())
         self.set_store("testkey", "testvalue")