From df8797320bed7ad9f121477e35d7e3862efd89bd Mon Sep 17 00:00:00 2001 From: John Spray Date: Fri, 6 Oct 2017 11:02:44 -0400 Subject: [PATCH] mgr: cut down duplication between active+standby ...by using PyModuleRunner class from ActivePyModule too. Signed-off-by: John Spray --- src/CMakeLists.txt | 1 + src/mgr/ActivePyModule.cc | 93 +---------------------------------- src/mgr/ActivePyModule.h | 43 +++------------- src/mgr/ActivePyModules.cc | 12 ----- src/mgr/ActivePyModules.h | 4 -- src/mgr/BaseMgrModule.cc | 3 +- src/mgr/PyModuleRegistry.cc | 27 ++++++++++- src/mgr/PyModuleRunner.cc | 97 +++++++++++++++++++++++++++++++++++++ src/mgr/PyModuleRunner.h | 76 +++++++++++++++++++++++++++++ src/mgr/StandbyPyModules.cc | 69 -------------------------- src/mgr/StandbyPyModules.h | 56 +-------------------- 11 files changed, 211 insertions(+), 270 deletions(-) create mode 100644 src/mgr/PyModuleRunner.cc create mode 100644 src/mgr/PyModuleRunner.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 997137111d052..9c9fa76e2e2db 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -715,6 +715,7 @@ if (WITH_MGR) mgr/ActivePyModules.cc mgr/StandbyPyModules.cc mgr/PyModuleRegistry.cc + mgr/PyModuleRunner.cc mgr/PyFormatter.cc mgr/PyOSDMap.cc mgr/BaseMgrModule.cc diff --git a/src/mgr/ActivePyModule.cc b/src/mgr/ActivePyModule.cc index 6048dc707cf81..3a6fe6a9eca24 100644 --- a/src/mgr/ActivePyModule.cc +++ b/src/mgr/ActivePyModule.cc @@ -51,60 +51,9 @@ std::string handle_pyerror() return extract(formatted); } - -void *ServeThread::entry() -{ - // No need to acquire the GIL here; the module does it. - dout(4) << "Entering thread for " << mod->get_name() << dendl; - mod->serve(); - return nullptr; -} - -ActivePyModule::ActivePyModule(const std::string &module_name_, - PyObject *pClass_, - PyThreadState *my_ts_) - : module_name(module_name_), - pClass(pClass_), - pMyThreadState(my_ts_), - thread(this) -{ -} - -ActivePyModule::~ActivePyModule() -{ - if (pMyThreadState.ts != nullptr) { - Gil gil(pMyThreadState); - - Py_XDECREF(pClassInstance); - - // - // Ideally, now, we'd be able to do this: - // - // Py_EndInterpreter(pMyThreadState); - // PyThreadState_Swap(pMainThreadState); - // - // Unfortunately, if the module has any other *python* threads active - // at this point, Py_EndInterpreter() will abort with: - // - // Fatal Python error: Py_EndInterpreter: not the last thread - // - // This can happen when using CherryPy in a module, becuase CherryPy - // runs an extra thread as a timeout monitor, which spends most of its - // life inside a time.sleep(60). Unless you are very, very lucky with - // the timing calling this destructor, that thread will still be stuck - // in a sleep, and Py_EndInterpreter() will abort. - // - // This could of course also happen with a poorly written module which - // made no attempt to clean up any additional threads it created. - // - // The safest thing to do is just not call Py_EndInterpreter(), and - // let Py_Finalize() kill everything after all modules are shut down. - // - } -} - int ActivePyModule::load(ActivePyModules *py_modules) { + assert(py_modules); Gil gil(pMyThreadState); // We tell the module how we name it, so that it can be consistent @@ -129,46 +78,6 @@ int ActivePyModule::load(ActivePyModules *py_modules) return load_commands(); } -int ActivePyModule::serve() -{ - assert(pClassInstance != nullptr); - - // This method is called from a separate OS thread (i.e. a thread not - // created by Python), so tell Gil to wrap this in a new thread state. - Gil gil(pMyThreadState, true); - - auto pValue = PyObject_CallMethod(pClassInstance, - const_cast("serve"), nullptr); - - int r = 0; - if (pValue != NULL) { - Py_DECREF(pValue); - } else { - derr << module_name << ".serve:" << dendl; - derr << handle_pyerror() << dendl; - return -EINVAL; - } - - return r; -} - -void ActivePyModule::shutdown() -{ - assert(pClassInstance != nullptr); - - Gil gil(pMyThreadState); - - auto pValue = PyObject_CallMethod(pClassInstance, - const_cast("shutdown"), nullptr); - - if (pValue != NULL) { - Py_DECREF(pValue); - } else { - derr << "Failed to invoke shutdown() on " << module_name << dendl; - derr << handle_pyerror() << dendl; - } -} - void ActivePyModule::notify(const std::string ¬ify_type, const std::string ¬ify_id) { assert(pClassInstance != nullptr); diff --git a/src/mgr/ActivePyModule.h b/src/mgr/ActivePyModule.h index 839f678f75297..2a3e0c8bfe092 100644 --- a/src/mgr/ActivePyModule.h +++ b/src/mgr/ActivePyModule.h @@ -25,6 +25,8 @@ #include "mon/health_check.h" #include "mgr/Gil.h" +#include "PyModuleRunner.h" + #include #include @@ -43,32 +45,9 @@ public: ActivePyModule *handler; }; -class ServeThread : public Thread -{ - ActivePyModule *mod; - -public: - ServeThread(ActivePyModule *mod_) - : mod(mod_) {} - - void *entry() override; -}; - -class ActivePyModule +class ActivePyModule : public PyModuleRunner { private: - const std::string module_name; - - // Passed in by whoever loaded our python module and looked up - // the symbols in it. - PyObject *pClass = nullptr; - - // Passed in by whoever created our subinterpreter for us - SafeThreadState pMyThreadState = nullptr; - - // Populated when we construct our instance of pClass in load() - PyObject *pClassInstance = nullptr; - health_check_map_t health_checks; std::vector commands; @@ -79,16 +58,13 @@ private: std::string uri; public: - ActivePyModule(const std::string &module_name, + ActivePyModule(const std::string &module_name_, PyObject *pClass_, - PyThreadState *my_ts); - ~ActivePyModule(); - - ServeThread thread; + PyThreadState *my_ts_) + : PyModuleRunner(module_name_, pClass_, my_ts_) + {} int load(ActivePyModules *py_modules); - int serve(); - void shutdown(); void notify(const std::string ¬ify_type, const std::string ¬ify_id); void notify_clog(const LogEntry &le); @@ -97,11 +73,6 @@ public: return commands; } - const std::string &get_name() const - { - return module_name; - } - int handle_command( const cmdmap_t &cmdmap, std::stringstream *ds, diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index 3dcf65dffd161..e1cde4fd7d31d 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -35,8 +35,6 @@ #undef dout_prefix #define dout_prefix *_dout << "mgr " << __func__ << " " -// constructor/destructor implementations cannot be in .h, -// because ServeThread is still an "incomplete" type there ActivePyModules::ActivePyModules(PyModuleConfig const &config_, DaemonStateIndex &ds, ClusterState &cs, @@ -536,16 +534,6 @@ std::map ActivePyModules::get_services() const return result; } -void ActivePyModules::log(const std::string &module_name, - int level, const std::string &record) -{ -#undef dout_prefix -#define dout_prefix *_dout << "mgr[" << module_name << "] " - dout(level) << record << dendl; -#undef dout_prefix -#define dout_prefix *_dout << "mgr " << __func__ << " " -} - PyObject* ActivePyModules::get_counter_python( const std::string &svc_name, const std::string &svc_id, diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h index 6e97e244c1fcc..688469f8c135e 100644 --- a/src/mgr/ActivePyModules.h +++ b/src/mgr/ActivePyModules.h @@ -27,7 +27,6 @@ #include "DaemonState.h" #include "ClusterState.h" -class ServeThread; class health_check_map_t; typedef std::map PyModuleConfig; @@ -91,9 +90,6 @@ public: void set_uri(const std::string& module_name, const std::string &uri); - void log(const std::string &module_name, - int level, const std::string &record); - // Python command definitions, including callback std::vector get_py_commands() const; diff --git a/src/mgr/BaseMgrModule.cc b/src/mgr/BaseMgrModule.cc index b16cc29650277..6f7252e86b159 100644 --- a/src/mgr/BaseMgrModule.cc +++ b/src/mgr/BaseMgrModule.cc @@ -410,6 +410,7 @@ get_daemon_status(BaseMgrModule *self, PyObject *args) static PyObject* ceph_log(BaseMgrModule *self, PyObject *args) { + int level = 0; char *record = nullptr; if (!PyArg_ParseTuple(args, "is:log", &level, &record)) { @@ -418,7 +419,7 @@ ceph_log(BaseMgrModule *self, PyObject *args) assert(self->this_module); - self->py_modules->log(self->this_module->get_name(), level, record); + self->this_module->log(level, record); Py_RETURN_NONE; } diff --git a/src/mgr/PyModuleRegistry.cc b/src/mgr/PyModuleRegistry.cc index 117ee4bed8aef..b0e9ef493279c 100644 --- a/src/mgr/PyModuleRegistry.cc +++ b/src/mgr/PyModuleRegistry.cc @@ -358,7 +358,10 @@ void PyModuleRegistry::active_start( int r = active_modules->start_one(i.first, i.second->pClass, i.second->pMyThreadState); - assert(r == 0); // TODO + if (r != 0) { + derr << "Failed to run module in active mode ('" << i.first << "')" + << dendl; + } } } @@ -381,6 +384,28 @@ void PyModuleRegistry::shutdown() standby_modules.reset(); } + // Ideally, now, we'd be able to do this for all modules: + // + // Py_EndInterpreter(pMyThreadState); + // PyThreadState_Swap(pMainThreadState); + // + // Unfortunately, if the module has any other *python* threads active + // at this point, Py_EndInterpreter() will abort with: + // + // Fatal Python error: Py_EndInterpreter: not the last thread + // + // This can happen when using CherryPy in a module, becuase CherryPy + // runs an extra thread as a timeout monitor, which spends most of its + // life inside a time.sleep(60). Unless you are very, very lucky with + // the timing calling this destructor, that thread will still be stuck + // in a sleep, and Py_EndInterpreter() will abort. + // + // This could of course also happen with a poorly written module which + // made no attempt to clean up any additional threads it created. + // + // The safest thing to do is just not call Py_EndInterpreter(), and + // let Py_Finalize() kill everything after all modules are shut down. + modules.clear(); PyEval_RestoreThread(pMainThreadState); diff --git a/src/mgr/PyModuleRunner.cc b/src/mgr/PyModuleRunner.cc new file mode 100644 index 0000000000000..c20199e9c6662 --- /dev/null +++ b/src/mgr/PyModuleRunner.cc @@ -0,0 +1,97 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab +/* + * Ceph - scalable distributed file system + * + * Copyright (C) 2016 John Spray + * + * This is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2.1, as published by the Free Software + * Foundation. See file COPYING. + */ + + +// Python.h comes first because otherwise it clobbers ceph's assert +#include "Python.h" + +#include "common/debug.h" +#include "mgr/Gil.h" + +#include "PyModuleRunner.h" + +#define dout_context g_ceph_context +#define dout_subsys ceph_subsys_mgr + + +std::string handle_pyerror(); + +PyModuleRunner::~PyModuleRunner() +{ + Gil gil(pMyThreadState); + + if (pClassInstance) { + Py_XDECREF(pClassInstance); + pClassInstance = nullptr; + } + + Py_DECREF(pClass); + pClass = nullptr; +} + +int PyModuleRunner::serve() +{ + assert(pClassInstance != nullptr); + + // This method is called from a separate OS thread (i.e. a thread not + // created by Python), so tell Gil to wrap this in a new thread state. + Gil gil(pMyThreadState, true); + + auto pValue = PyObject_CallMethod(pClassInstance, + const_cast("serve"), nullptr); + + int r = 0; + if (pValue != NULL) { + Py_DECREF(pValue); + } else { + derr << module_name << ".serve:" << dendl; + derr << handle_pyerror() << dendl; + return -EINVAL; + } + + return r; +} + +void PyModuleRunner::shutdown() +{ + assert(pClassInstance != nullptr); + + Gil gil(pMyThreadState); + + auto pValue = PyObject_CallMethod(pClassInstance, + const_cast("shutdown"), nullptr); + + if (pValue != NULL) { + Py_DECREF(pValue); + } else { + derr << "Failed to invoke shutdown() on " << module_name << dendl; + derr << handle_pyerror() << dendl; + } +} + +void PyModuleRunner::log(int level, const std::string &record) +{ +#undef dout_prefix +#define dout_prefix *_dout << "mgr[" << module_name << "] " + dout(level) << record << dendl; +#undef dout_prefix +#define dout_prefix *_dout << "mgr " << __func__ << " " +} + +void* PyModuleRunner::PyModuleRunnerThread::entry() +{ + // No need to acquire the GIL here; the module does it. + dout(4) << "Entering thread for " << mod->get_name() << dendl; + mod->serve(); + return nullptr; +} diff --git a/src/mgr/PyModuleRunner.h b/src/mgr/PyModuleRunner.h new file mode 100644 index 0000000000000..1cb0506054ec7 --- /dev/null +++ b/src/mgr/PyModuleRunner.h @@ -0,0 +1,76 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab +/* + * Ceph - scalable distributed file system + * + * Copyright (C) 2016 John Spray + * + * This is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2.1, as published by the Free Software + * Foundation. See file COPYING. + */ + + +#pragma once + +#include "common/Thread.h" +#include "mgr/Gil.h" + +/** + * Implement the pattern of calling serve() on a module in a thread, + * until shutdown() is called. + */ +class PyModuleRunner +{ +protected: + const std::string module_name; + + // Passed in by whoever loaded our python module and looked up + // the symbols in it. + PyObject *pClass = nullptr; + + // Passed in by whoever created our subinterpreter for us + SafeThreadState pMyThreadState = nullptr; + + // Populated when we construct our instance of pClass in load() + PyObject *pClassInstance = nullptr; + + class PyModuleRunnerThread : public Thread + { + PyModuleRunner *mod; + + public: + PyModuleRunnerThread(PyModuleRunner *mod_) + : mod(mod_) {} + + void *entry() override; + }; + +public: + int serve(); + void shutdown(); + void log(int level, const std::string &record); + + PyModuleRunner( + const std::string &module_name_, + PyObject *pClass_, + PyThreadState *pMyThreadState_) + : + module_name(module_name_), + pClass(pClass_), pMyThreadState(pMyThreadState_), + thread(this) + { + assert(pClass != nullptr); + assert(pMyThreadState != nullptr); + assert(!module_name.empty()); + } + + ~PyModuleRunner(); + + PyModuleRunnerThread thread; + + std::string const &get_name() const { return module_name; } +}; + + diff --git a/src/mgr/StandbyPyModules.cc b/src/mgr/StandbyPyModules.cc index ef17dbbeb9c89..3992553bd4408 100644 --- a/src/mgr/StandbyPyModules.cc +++ b/src/mgr/StandbyPyModules.cc @@ -33,13 +33,6 @@ // Declaration fulfilled by ActivePyModules std::string handle_pyerror(); -void* PyModuleRunner::PyModuleRunnerThread::entry() -{ - // No need to acquire the GIL here; the module does it. - dout(4) << "Entering thread for " << mod->get_name() << dendl; - mod->serve(); - return nullptr; -} StandbyPyModules::StandbyPyModules(MonClient *monc_, const MgrMap &mgr_map_) : monc(monc_), load_config_thread(monc, &state) @@ -138,19 +131,6 @@ int StandbyPyModule::load() } } -StandbyPyModule::~StandbyPyModule() -{ - Gil gil(pMyThreadState); - - if (pClassInstance) { - Py_XDECREF(pClassInstance); - pClassInstance = nullptr; - } - - Py_DECREF(pClass); - pClass = nullptr; -} - void *StandbyPyModules::LoadConfigThread::entry() { dout(10) << "listing keys" << dendl; @@ -217,52 +197,3 @@ std::string StandbyPyModule::get_active_uri() const return result; } -int PyModuleRunner::serve() -{ - assert(pClassInstance != nullptr); - - // This method is called from a separate OS thread (i.e. a thread not - // created by Python), so tell Gil to wrap this in a new thread state. - Gil gil(pMyThreadState, true); - - auto pValue = PyObject_CallMethod(pClassInstance, - const_cast("serve"), nullptr); - - int r = 0; - if (pValue != NULL) { - Py_DECREF(pValue); - } else { - derr << module_name << ".serve:" << dendl; - derr << handle_pyerror() << dendl; - return -EINVAL; - } - - return r; -} - -void PyModuleRunner::shutdown() -{ - assert(pClassInstance != nullptr); - - Gil gil(pMyThreadState); - - auto pValue = PyObject_CallMethod(pClassInstance, - const_cast("shutdown"), nullptr); - - if (pValue != NULL) { - Py_DECREF(pValue); - } else { - derr << "Failed to invoke shutdown() on " << module_name << dendl; - derr << handle_pyerror() << dendl; - } -} - -void PyModuleRunner::log(int level, const std::string &record) -{ -#undef dout_prefix -#define dout_prefix *_dout << "mgr[" << module_name << "] " - dout(level) << record << dendl; -#undef dout_prefix -#define dout_prefix *_dout << "mgr " << __func__ << " " -} - diff --git a/src/mgr/StandbyPyModules.h b/src/mgr/StandbyPyModules.h index 4b2e47a6127c4..e5896e05ce291 100644 --- a/src/mgr/StandbyPyModules.h +++ b/src/mgr/StandbyPyModules.h @@ -23,6 +23,7 @@ #include "mon/MonClient.h" #include "mon/MgrMap.h" +#include "mgr/PyModuleRunner.h" typedef std::map PyModuleConfig; @@ -78,59 +79,6 @@ public: } }; -/** - * Implement the pattern of calling serve() on a module in a thread, - * until shutdown() is called. - */ -class PyModuleRunner -{ -protected: - const std::string module_name; - - // Passed in by whoever loaded our python module and looked up - // the symbols in it. - PyObject *pClass = nullptr; - - // Passed in by whoever created our subinterpreter for us - PyThreadState *pMyThreadState = nullptr; - - // Populated when we construct our instance of pClass in load() - PyObject *pClassInstance = nullptr; - - class PyModuleRunnerThread : public Thread - { - PyModuleRunner *mod; - - public: - PyModuleRunnerThread(PyModuleRunner *mod_) - : mod(mod_) {} - - void *entry() override; - }; - -public: - int serve(); - void shutdown(); - void log(int level, const std::string &record); - - PyModuleRunner( - const std::string &module_name_, - PyObject *pClass_, - PyThreadState *pMyThreadState_) - : - module_name(module_name_), - pClass(pClass_), pMyThreadState(pMyThreadState_), - thread(this) - { - assert(pClass != nullptr); - assert(pMyThreadState != nullptr); - assert(!module_name.empty()); - } - - PyModuleRunnerThread thread; - - std::string const &get_name() { return module_name; } -}; class StandbyPyModule : public PyModuleRunner { @@ -149,8 +97,6 @@ class StandbyPyModule : public PyModuleRunner { } - ~StandbyPyModule(); - bool get_config(const std::string &key, std::string *value) const; std::string get_active_uri() const; -- 2.39.5