From: Kefu Chai <kchai@redhat.com>
Date: Mon, 22 Feb 2021 04:38:13 +0000 (+0800)
Subject: mgr: return the error if set_config() fails
X-Git-Tag: v17.1.0~2843^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=75bbe6863f5f68567b2011fdbdee1cb6ddf19b79;p=ceph.git

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 e10546c20502..eb7c81b47e22 100644
--- 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 f4feec4307a9..d1e90f8bd3c0 100644
--- 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 c1d42aa9b46d..00b3ebb1acfd 100644
--- 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 60c18eb3773a..8756b77907ec 100644
--- 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 6df3ee5860f4..64fa373b8bfe 100644
--- 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 decbe0d061e5..abfcfbd1bfb0 100644
--- 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 d63e4cc24ab6..cf3540000184 100644
--- 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