From 75bbe6863f5f68567b2011fdbdee1cb6ddf19b79 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 22 Feb 2021 12:38:13 +0800 Subject: [PATCH] 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 --- src/mgr/ActivePyModules.cc | 8 +++++--- src/mgr/ActivePyModules.h | 2 +- src/mgr/BaseMgrModule.cc | 8 ++++++-- src/mgr/PyModule.cc | 4 +++- src/mgr/PyModule.h | 2 +- src/pybind/mgr/mgr_module.py | 1 + src/pybind/mgr/selftest/module.py | 12 +++++++++++- 7 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index e10546c20502c..eb7c81b47e22a 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& val) +std::pair ActivePyModules::set_config( + const std::string &module_name, + const std::string &key, + const boost::optional& val) { - module_config.set_config(&monc, module_name, key, val); + return module_config.set_config(&monc, module_name, key, val); } std::map ActivePyModules::get_services() const diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h index f4feec4307a92..d1e90f8bd3c0d 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 set_config(const std::string &module_name, const std::string &key, const boost::optional &val); PyObject *get_typed_config(const std::string &module_name, diff --git a/src/mgr/BaseMgrModule.cc b/src/mgr/BaseMgrModule.cc index c1d42aa9b46d7..00b3ebb1acfd9 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 60c18eb3773a1..8756b77907eca 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 PyModuleConfig::set_config( MonClient *monc, const std::string &module_name, const std::string &key, const boost::optional& 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 6df3ee5860f4c..64fa373b8bfe4 100644 --- a/src/mgr/PyModule.h +++ b/src/mgr/PyModule.h @@ -176,7 +176,7 @@ public: ~PyModuleConfig(); - void set_config( + std::pair set_config( MonClient *monc, const std::string &module_name, const std::string &key, const boost::optional& val); diff --git a/src/pybind/mgr/mgr_module.py b/src/pybind/mgr/mgr_module.py index decbe0d061e5e..abfcfbd1bfb03 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 d63e4cc24ab69..cf35400001840 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 -- 2.39.5