From: John Spray Date: Thu, 23 Nov 2017 13:01:33 +0000 (-0500) Subject: mgr: load command definitions earlier X-Git-Tag: v13.0.2~448^2~6 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=834bc27940507737f064bca7362b4c7f4cd54c98;p=ceph.git mgr: load command definitions earlier ...and for all modules, not just the active ones. This enables us to give better feedback to the user when they try and use a command from a disabled module, and also fixes the race between enabling a module and trying to use its commands. Fixes: http://tracker.ceph.com/issues/21683 Signed-off-by: John Spray --- diff --git a/src/mgr/ActivePyModule.cc b/src/mgr/ActivePyModule.cc index 311144086abb..185ef4454288 100644 --- a/src/mgr/ActivePyModule.cc +++ b/src/mgr/ActivePyModule.cc @@ -46,7 +46,7 @@ int ActivePyModule::load(ActivePyModules *py_modules) dout(1) << "Constructed class from module: " << get_name() << dendl; } - return load_commands(); + return 0; } void ActivePyModule::notify(const std::string ¬ify_type, const std::string ¬ify_id) @@ -100,55 +100,7 @@ void ActivePyModule::notify_clog(const LogEntry &log_entry) } } -int ActivePyModule::load_commands() -{ - // Don't need a Gil here -- this is called from ActivePyModule::load(), - // which already has one. - PyObject *command_list = PyObject_GetAttrString(pClassInstance, "COMMANDS"); - if (command_list == nullptr) { - // Even modules that don't define command should still have the COMMANDS - // from the MgrModule definition. Something is wrong! - derr << "Module " << get_name() << " has missing COMMANDS member" << dendl; - return -EINVAL; - } - if (!PyObject_TypeCheck(command_list, &PyList_Type)) { - // Relatively easy mistake for human to make, e.g. defining COMMANDS - // as a {} instead of a [] - derr << "Module " << get_name() << " has COMMANDS member of wrong type (" - "should be a list)" << dendl; - return -EINVAL; - } - const size_t list_size = PyList_Size(command_list); - for (size_t i = 0; i < list_size; ++i) { - PyObject *command = PyList_GetItem(command_list, i); - assert(command != nullptr); - - ModuleCommand item; - - PyObject *pCmd = PyDict_GetItemString(command, "cmd"); - assert(pCmd != nullptr); - item.cmdstring = PyString_AsString(pCmd); - - dout(20) << "loaded command " << item.cmdstring << dendl; - PyObject *pDesc = PyDict_GetItemString(command, "desc"); - assert(pDesc != nullptr); - item.helpstring = PyString_AsString(pDesc); - - PyObject *pPerm = PyDict_GetItemString(command, "perm"); - assert(pPerm != nullptr); - item.perm = PyString_AsString(pPerm); - - item.handler = this; - - commands.push_back(item); - } - Py_DECREF(command_list); - - dout(10) << "loaded " << commands.size() << " commands" << dendl; - - return 0; -} int ActivePyModule::handle_command( const cmdmap_t &cmdmap, diff --git a/src/mgr/ActivePyModule.h b/src/mgr/ActivePyModule.h index b475610fbb1d..9279f1c8f5d8 100644 --- a/src/mgr/ActivePyModule.h +++ b/src/mgr/ActivePyModule.h @@ -34,26 +34,11 @@ class ActivePyModule; class ActivePyModules; -/** - * A Ceph CLI command description provided from a Python module - */ -class ModuleCommand { -public: - std::string cmdstring; - std::string helpstring; - std::string perm; - ActivePyModule *handler; -}; - class ActivePyModule : public PyModuleRunner { private: health_check_map_t health_checks; - std::vector commands; - - int load_commands(); - // Optional, URI exposed by plugins that implement serve() std::string uri; @@ -67,11 +52,6 @@ public: void notify(const std::string ¬ify_type, const std::string ¬ify_id); void notify_clog(const LogEntry &le); - const std::vector &get_commands() const - { - return commands; - } - int handle_command( const cmdmap_t &cmdmap, std::stringstream *ds, diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index 76b54203ded6..7b370800926e 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -491,34 +491,6 @@ void ActivePyModules::set_config(const std::string &module_name, } } -std::vector ActivePyModules::get_py_commands() const -{ - Mutex::Locker l(lock); - - std::vector result; - for (const auto& i : modules) { - auto module = i.second.get(); - auto mod_commands = module->get_commands(); - for (auto j : mod_commands) { - result.push_back(j); - } - } - - return result; -} - -std::vector ActivePyModules::get_commands() const -{ - std::vector commands = get_py_commands(); - std::vector result; - for (auto &pyc: commands) { - result.push_back({pyc.cmdstring, pyc.helpstring, "mgr", - pyc.perm, "cli", MonCommand::FLAG_MGR}); - } - return result; -} - - std::map ActivePyModules::get_services() const { std::map result; @@ -713,6 +685,18 @@ void ActivePyModules::set_health_checks(const std::string& module_name, } } +int ActivePyModules::handle_command( + std::string const &module_name, + const cmdmap_t &cmdmap, + std::stringstream *ds, + std::stringstream *ss) +{ + lock.Lock(); + auto mod = modules.at(module_name).get(); + lock.Unlock(); + return mod->handle_command(cmdmap, ds, ss); +} + void ActivePyModules::get_health_checks(health_check_map_t *checks) { Mutex::Locker l(lock); diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h index 6d0feedb4170..60572c25bfcb 100644 --- a/src/mgr/ActivePyModules.h +++ b/src/mgr/ActivePyModules.h @@ -90,11 +90,11 @@ public: void set_uri(const std::string& module_name, const std::string &uri); - // Python command definitions, including callback - std::vector get_py_commands() const; - - // Monitor command definitions, suitable for CLI - std::vector get_commands() const; + int handle_command( + const std::string &module_name, + const cmdmap_t &cmdmap, + std::stringstream *ds, + std::stringstream *ss); std::map get_services() const; diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index 409aa18bee15..5a18f27a947f 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -1332,37 +1332,80 @@ bool DaemonServer::handle_command(MCommand *m) } } - // None of the special native commands, - ActivePyModule *handler = nullptr; + // Resolve the command to the name of the module that will + // handle it (if the command exists) + std::string handler_name; auto py_commands = py_modules.get_py_commands(); for (const auto &pyc : py_commands) { auto pyc_prefix = cmddesc_get_prefix(pyc.cmdstring); dout(1) << "pyc_prefix: '" << pyc_prefix << "'" << dendl; if (pyc_prefix == prefix) { - handler = pyc.handler; + handler_name = pyc.module_name; break; } } - if (handler == nullptr) { + // Was the command unfound? + if (handler_name.empty()) { ss << "No handler found for '" << prefix << "'"; dout(4) << "No handler found for '" << prefix << "'" << dendl; cmdctx->reply(-EINVAL, ss); return true; - } else { - // Okay, now we have a handler to call, but we must not call it - // in this thread, because the python handlers can do anything, - // including blocking, and including calling back into mgr. - dout(4) << "passing through " << cmdctx->cmdmap.size() << dendl; - finisher.queue(new FunctionContext([cmdctx, handler](int r_) { - std::stringstream ds; - std::stringstream ss; - int r = handler->handle_command(cmdctx->cmdmap, &ds, &ss); - cmdctx->odata.append(ds); - cmdctx->reply(r, ss); - })); - return true; } + + dout(4) << "passing through " << cmdctx->cmdmap.size() << dendl; + finisher.queue(new FunctionContext([this, cmdctx, handler_name, prefix](int r_) { + std::stringstream ss; + + // Validate that the module is enabled + PyModuleRef module = py_modules.get_module(handler_name); + if (!module->is_enabled()) { + ss << "Module '" << handler_name << "' is not enabled (required by " + "command '" << prefix << "'): use `ceph mgr module enable " + << handler_name << "` to enable it"; + dout(4) << ss.str() << dendl; + cmdctx->reply(-EOPNOTSUPP, ss); + return; + } + + // Hack: allow the self-test method to run on unhealthy modules. + // Fix this in future by creating a special path for self test rather + // than having the hook be a normal module command. + std::string self_test_prefix = handler_name + " " + "self-test"; + + // Validate that the module is healthy + bool accept_command; + if (module->is_loaded()) { + if (module->get_can_run() && !module->is_failed()) { + // Healthy module + accept_command = true; + } else if (self_test_prefix == prefix) { + // Unhealthy, but allow because it's a self test command + accept_command = true; + } else { + accept_command = false; + ss << "Module '" << handler_name << "' has experienced an error and " + "cannot handle commands: " << module->get_error_string(); + } + } else { + // Module not loaded + accept_command = false; + ss << "Module '" << handler_name << "' failed to load and " + "cannot handle commands: " << module->get_error_string(); + } + + if (!accept_command) { + dout(4) << ss.str() << dendl; + cmdctx->reply(-EIO, ss); + return; + } + + std::stringstream ds; + int r = py_modules.handle_command(handler_name, cmdctx->cmdmap, &ds, &ss); + cmdctx->odata.append(ds); + cmdctx->reply(r, ss); + })); + return true; } void DaemonServer::_prune_pending_service_map() diff --git a/src/mgr/Mgr.cc b/src/mgr/Mgr.cc index eed2bae72007..6c92395bcfa5 100644 --- a/src/mgr/Mgr.cc +++ b/src/mgr/Mgr.cc @@ -22,9 +22,7 @@ #include "global/signal_handler.h" #include "mgr/MgrContext.h" -#include "mgr/mgr_commands.h" -//#include "MgrPyModule.h" #include "DaemonServer.h" #include "messages/MMgrBeacon.h" #include "messages/MMgrDigest.h" @@ -218,8 +216,8 @@ void Mgr::init() // assume finisher already initialized in background_init dout(4) << "starting python modules..." << dendl; - py_module_registry->active_start(loaded_config, daemon_state, cluster_state, *monc, - clog, *objecter, *client, finisher); + py_module_registry->active_start(loaded_config, daemon_state, cluster_state, + *monc, clog, *objecter, *client, finisher); dout(4) << "Complete." << dendl; initializing = false; @@ -624,16 +622,6 @@ void Mgr::tick() server.send_report(); } -std::vector Mgr::get_command_set() const -{ - Mutex::Locker l(lock); - - std::vector commands = mgr_commands; - std::vector py_commands = py_module_registry->get_commands(); - commands.insert(commands.end(), py_commands.begin(), py_commands.end()); - return commands; -} - std::map Mgr::get_services() const { Mutex::Locker l(lock); diff --git a/src/mgr/Mgr.h b/src/mgr/Mgr.h index a4d8aad39ff7..912114c380b9 100644 --- a/src/mgr/Mgr.h +++ b/src/mgr/Mgr.h @@ -101,7 +101,6 @@ public: void background_init(Context *completion); void shutdown(); - std::vector get_command_set() const; std::map get_services() const; }; diff --git a/src/mgr/MgrStandby.cc b/src/mgr/MgrStandby.cc index 91f7737813b6..cf491d8d72a9 100644 --- a/src/mgr/MgrStandby.cc +++ b/src/mgr/MgrStandby.cc @@ -22,6 +22,7 @@ #include "global/signal_handler.h" #include "mgr/MgrContext.h" +#include "mgr/mgr_commands.h" #include "messages/MMgrBeacon.h" #include "messages/MMgrMap.h" @@ -189,7 +190,10 @@ void MgrStandby::send_beacon() // We are informing the mon that we are done initializing: inform // it of our command set. This has to happen after init() because // it needs the python modules to have loaded. - m->set_command_descs(active_mgr->get_command_set()); + std::vector commands = mgr_commands; + std::vector py_commands = py_module_registry.get_commands(); + commands.insert(commands.end(), py_commands.begin(), py_commands.end()); + m->set_command_descs(commands); dout(4) << "going active, including " << m->get_command_descs().size() << " commands in beacon" << dendl; } diff --git a/src/mgr/PyModule.cc b/src/mgr/PyModule.cc index 3d4eb7e90fc4..d2d825ca0d5a 100644 --- a/src/mgr/PyModule.cc +++ b/src/mgr/PyModule.cc @@ -210,6 +210,7 @@ int PyModule::load(PyThreadState *pMainThreadState) // Environment is all good, import the external module { Gil gil(pMyThreadState); + int r; r = load_subclass_of("MgrModule", &pClass); if (r) { @@ -217,6 +218,15 @@ int PyModule::load(PyThreadState *pMainThreadState) return r; } + r = load_commands(); + if (r != 0) { + std::ostringstream oss; + oss << "Missing COMMAND attribute in module '" << module_name << "'"; + error_string = oss.str(); + derr << oss.str() << dendl; + return r; + } + // We've imported the module and found a MgrModule subclass, at this // point the module is considered loaded. It might still not be // runnable though, can_run populated later... @@ -268,6 +278,56 @@ int PyModule::load(PyThreadState *pMainThreadState) return 0; } +int PyModule::load_commands() +{ + // Don't need a Gil here -- this is called from load(), + // which already has one. + PyObject *command_list = PyObject_GetAttrString(pClass, "COMMANDS"); + if (command_list == nullptr) { + // Even modules that don't define command should still have the COMMANDS + // from the MgrModule definition. Something is wrong! + derr << "Module " << get_name() << " has missing COMMANDS member" << dendl; + return -EINVAL; + } + if (!PyObject_TypeCheck(command_list, &PyList_Type)) { + // Relatively easy mistake for human to make, e.g. defining COMMANDS + // as a {} instead of a [] + derr << "Module " << get_name() << " has COMMANDS member of wrong type (" + "should be a list)" << dendl; + return -EINVAL; + } + const size_t list_size = PyList_Size(command_list); + for (size_t i = 0; i < list_size; ++i) { + PyObject *command = PyList_GetItem(command_list, i); + assert(command != nullptr); + + ModuleCommand item; + + PyObject *pCmd = PyDict_GetItemString(command, "cmd"); + assert(pCmd != nullptr); + item.cmdstring = PyString_AsString(pCmd); + + dout(20) << "loaded command " << item.cmdstring << dendl; + + PyObject *pDesc = PyDict_GetItemString(command, "desc"); + assert(pDesc != nullptr); + item.helpstring = PyString_AsString(pDesc); + + PyObject *pPerm = PyDict_GetItemString(command, "perm"); + assert(pPerm != nullptr); + item.perm = PyString_AsString(pPerm); + + item.module_name = module_name; + + commands.push_back(item); + } + Py_DECREF(command_list); + + dout(10) << "loaded " << commands.size() << " commands" << dendl; + + return 0; +} + int PyModule::load_subclass_of(const char* base_class, PyObject** py_class) { // load the base class diff --git a/src/mgr/PyModule.h b/src/mgr/PyModule.h index d3c39f4abb33..14f418e446ea 100644 --- a/src/mgr/PyModule.h +++ b/src/mgr/PyModule.h @@ -22,6 +22,19 @@ std::string handle_pyerror(); +/** + * A Ceph CLI command description provided from a Python module + */ +class ModuleCommand { +public: + std::string cmdstring; + std::string helpstring; + std::string perm; + + // Call the ActivePyModule of this name to handle the command + std::string module_name; +}; + class PyModule { mutable Mutex lock{"PyModule::lock"}; @@ -48,6 +61,9 @@ private: // Populated if loaded, can_run or failed indicates a problem std::string error_string; + int load_commands(); + std::vector commands; + public: SafeThreadState pMyThreadState; PyObject *pClass = nullptr; @@ -62,6 +78,18 @@ public: int load(PyThreadState *pMainThreadState); + + /** + * Extend `out` with the contents of `this->commands` + */ + void get_commands(std::vector *out) const + { + Mutex::Locker l(lock); + assert(out != nullptr); + out->insert(out->end(), commands.begin(), commands.end()); + } + + /** * Mark the module as failed, recording the reason in the error * string. @@ -74,6 +102,9 @@ public: } bool is_enabled() const { Mutex::Locker l(lock) ; return enabled; } + bool is_failed() const { Mutex::Locker l(lock) ; return failed; } + bool is_loaded() const { Mutex::Locker l(lock) ; return loaded; } + const std::string &get_name() const { Mutex::Locker l(lock) ; return module_name; } diff --git a/src/mgr/PyModuleRegistry.cc b/src/mgr/PyModuleRegistry.cc index dcda1e72f84d..5c114b9e8b6f 100644 --- a/src/mgr/PyModuleRegistry.cc +++ b/src/mgr/PyModuleRegistry.cc @@ -251,3 +251,42 @@ void PyModuleRegistry::list_modules(std::set *modules) g_conf->with_val("mgr_module_path", &_list_modules, modules); } + +int PyModuleRegistry::handle_command( + std::string const &module_name, + const cmdmap_t &cmdmap, + std::stringstream *ds, + std::stringstream *ss) +{ + if (active_modules) { + return active_modules->handle_command(module_name, cmdmap, ds, ss); + } else { + // We do not expect to be called before active modules is up, but + // it's straightfoward to handle this case so let's do it. + return -EAGAIN; + } +} + +std::vector PyModuleRegistry::get_py_commands() const +{ + Mutex::Locker l(lock); + + std::vector result; + for (const auto& i : modules) { + i.second->get_commands(&result); + } + + return result; +} + +std::vector PyModuleRegistry::get_commands() const +{ + std::vector commands = get_py_commands(); + std::vector result; + for (auto &pyc: commands) { + result.push_back({pyc.cmdstring, pyc.helpstring, "mgr", + pyc.perm, "cli", MonCommand::FLAG_MGR}); + } + return result; +} + diff --git a/src/mgr/PyModuleRegistry.h b/src/mgr/PyModuleRegistry.h index 3eaa9f640926..397c46de720a 100644 --- a/src/mgr/PyModuleRegistry.h +++ b/src/mgr/PyModuleRegistry.h @@ -117,6 +117,25 @@ public: std::forward(cb)(*active_modules, std::forward(args)...); } + std::vector get_commands() const; + std::vector get_py_commands() const; + + /** + * module_name **must** exist, but does not have to be loaded + * or runnable. + */ + PyModuleRef get_module(const std::string &module_name) + { + Mutex::Locker l(lock); + return modules.at(module_name); + } + + int handle_command( + std::string const &module_name, + const cmdmap_t &cmdmap, + std::stringstream *ds, + std::stringstream *ss); + // FIXME: breaking interface so that I don't have to go rewrite all // the places that call into these (for now) // >>> @@ -135,16 +154,6 @@ public: } } - std::vector get_commands() const - { - assert(active_modules); - return active_modules->get_commands(); - } - std::vector get_py_commands() const - { - assert(active_modules); - return active_modules->get_py_commands(); - } void get_health_checks(health_check_map_t *checks) { assert(active_modules);