mgr: return the error if set_config() fails

in general, `ActivePyModules::set_config()` is called by mgr module when
serving user commands updating module, sometimes if the option is of the
wrong type or invalid value, monitor rejects this request sent by mgr,
but the error info is only logged in the logging message on mgr, but not
returned to user. in this change, `ceph_set_module_option()` and the
underlying methods are updated to return the error to the caller as an
python exception.

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

diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc
index e10546c20502c987553a3abec007b1f87678b445..eb7c81b47e22a3ea6717fb969d45cc9dc388510a 100644 (file)
--- a/src/mgr/ActivePyModules.cc
+++ b/src/mgr/ActivePyModules.cc
@@ -688,10 +688,12 @@ void ActivePyModules::set_store(const std::string &module_name,
   }
 }
 
-void ActivePyModules::set_config(const std::string &module_name,
-    const std::string &key, const boost::optional<std::string>& val)
+std::pair<int, std::string> ActivePyModules::set_config(
+  const std::string &module_name,
+  const std::string &key,
+  const boost::optional<std::string>& val)
 {
-  module_config.set_config(&monc, module_name, key, val);
+  return module_config.set_config(&monc, module_name, key, val);
 }
 
 std::map<std::string, std::string> ActivePyModules::get_services() const

diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h
index f4feec4307a92c10a1ae3e28e75e32f5dfc1641c..d1e90f8bd3c0d402c3c3a5faf39354dab3241332 100644 (file)
--- a/src/mgr/ActivePyModules.h
+++ b/src/mgr/ActivePyModules.h
@@ -131,7 +131,7 @@ public:
 
   bool get_config(const std::string &module_name,
       const std::string &key, std::string *val) const;
-  void set_config(const std::string &module_name,
+  std::pair<int, std::string> 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,

diff --git a/src/mgr/BaseMgrModule.cc b/src/mgr/BaseMgrModule.cc
index c1d42aa9b46d7c41749d080dcf1d930cde3bd8b9..00b3ebb1acfd9bdcc0e1d7b0935b665887e470a2 100644 (file)
--- a/src/mgr/BaseMgrModule.cc
+++ b/src/mgr/BaseMgrModule.cc
@@ -490,9 +490,13 @@ ceph_set_module_option(BaseMgrModule *self, PyObject *args)
   if (value) {
     val = value;
   }
-  without_gil([&] {
-    self->py_modules->set_config(module, key, val);
+  auto [ret, msg] = without_gil([&] {
+    return self->py_modules->set_config(module, key, val);
   });
+  if (ret) {
+    PyErr_SetString(PyExc_ValueError, msg.c_str());
+    return nullptr;
+  }
   Py_RETURN_NONE;
 }
 

diff --git a/src/mgr/PyModule.cc b/src/mgr/PyModule.cc
index 60c18eb3773a12950b148acb31c7b2802266f6d0..8756b77907eca372dfdc3c846285f2b4516fd769 100644 (file)
--- a/src/mgr/PyModule.cc
+++ b/src/mgr/PyModule.cc
@@ -145,7 +145,7 @@ PyModuleConfig::PyModuleConfig(PyModuleConfig &mconfig)
 PyModuleConfig::~PyModuleConfig() = default;
 
 
-void PyModuleConfig::set_config(
+std::pair<int, std::string> PyModuleConfig::set_config(
     MonClient *monc,
     const std::string &module_name,
     const std::string &key, const boost::optional<std::string>& val)
@@ -177,6 +177,7 @@ void PyModuleConfig::set_config(
     } else {
       config.erase(global_key);
     }
+    return {0, ""};
   } else {
     if (val) {
       dout(0) << "`config set mgr " << global_key << " " << val << "` failed: "
@@ -186,6 +187,7 @@ void PyModuleConfig::set_config(
         << cpp_strerror(set_cmd.r) << dendl;
     }
     dout(0) << "mon returned " << set_cmd.r << ": " << set_cmd.outs << dendl;
+    return {set_cmd.r, set_cmd.outs};
   }
 }
 

diff --git a/src/mgr/PyModule.h b/src/mgr/PyModule.h
index 6df3ee5860f4c380abb5f8dddc07735bcf3bd5cf..64fa373b8bfe4ad1987e4d1a1b3d07c66778c734 100644 (file)
--- a/src/mgr/PyModule.h
+++ b/src/mgr/PyModule.h
@@ -176,7 +176,7 @@ public:
   
   ~PyModuleConfig();
 
-  void set_config(
+  std::pair<int, std::string> set_config(
     MonClient *monc,
     const std::string &module_name,
     const std::string &key, const boost::optional<std::string>& val);

diff --git a/src/pybind/mgr/mgr_module.py b/src/pybind/mgr/mgr_module.py
index decbe0d061e5e7490d83ed6bd1ac54e2dca12d78..abfcfbd1bfb0319a487f25bc7b6ef6f5c1a634ed 100644 (file)
--- a/src/pybind/mgr/mgr_module.py
+++ b/src/pybind/mgr/mgr_module.py
@@ -1440,6 +1440,7 @@ class MgrModule(ceph_module.BaseMgrModule, MgrModuleLoggingMixin):
 
         :param str key:
         :type val: str | None
+        :raises ValueError: if `val` cannot be parsed or it is out of the specified range
         """
         self._validate_module_option(key)
         return self._set_module_option(key, val)

diff --git a/src/pybind/mgr/selftest/module.py b/src/pybind/mgr/selftest/module.py
index d63e4cc24ab69c84721fb9f3e149baa82c9d955b..cf3540000184011cf3d24324980a02ce699122f2 100644 (file)
--- a/src/pybind/mgr/selftest/module.py
+++ b/src/pybind/mgr/selftest/module.py
@@ -38,7 +38,8 @@ class Module(MgrModule):
         {'name': 'rwoption3', 'type': 'float'},
         {'name': 'rwoption4', 'type': 'str'},
         {'name': 'rwoption5', 'type': 'bool'},
-        {'name': 'rwoption6', 'type': 'bool', 'default': True}
+        {'name': 'rwoption6', 'type': 'bool', 'default': True},
+        {'name': 'rwoption7', 'type': 'int', 'min': 1, 'max': 42},
     ]
 
     COMMANDS = [
@@ -313,6 +314,15 @@ class Module(MgrModule):
         assert isinstance(value, bool)
         assert value is False
 
+        # Option value range is specified
+        try:
+            self.set_module_option("rwoption7", 43)
+        except Exception as e:
+            assert isinstance(e, ValueError)
+        else:
+            message = "should raise if value is not in specified range"
+            assert False, message
+
         # Specified module does not exist => return None.
         assert self.get_module_option_ex("foo", "bar") is None