mgr: cut down duplication between active+standby

...by using PyModuleRunner class from ActivePyModule too.

Signed-off-by: John Spray <john.spray@redhat.com>

src/CMakeLists.txt

@@ -716,6 +716,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

src/mgr/ActivePyModule.cc

@@ -61,51 +61,9 @@ void *ServeThread::entry()
   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 != 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
@@ -130,46 +88,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<char*>("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<char*>("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 &notify_type, const std::string &notify_id)
 {
   assert(pClassInstance != nullptr);

src/mgr/ActivePyModule.h

@@ -24,6 +24,8 @@
 #include "common/Thread.h"
 #include "mon/health_check.h"
 
+#include "PyModuleRunner.h"
+
 #include <vector>
 #include <string>
 
@@ -53,21 +55,9 @@ class ServeThread : public Thread
   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
-  PyThreadState *pMyThreadState = nullptr;
-
-  // Populated when we construct our instance of pClass in load()
-  PyObject *pClassInstance = nullptr;
-
   health_check_map_t health_checks;
 
   std::vector<ModuleCommand> commands;
@@ -78,16 +68,13 @@ class ActivePyModule
   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 &notify_type, const std::string &notify_id);
   void notify_clog(const LogEntry &le);
 
@@ -96,11 +83,6 @@ class ActivePyModule
     return commands;
   }
 
-  const std::string &get_name() const
-  {
-    return module_name;
-  }
-
   int handle_command(
     const cmdmap_t &cmdmap,
     std::stringstream *ds,

src/mgr/ActivePyModules.cc

@@ -536,16 +536,6 @@ std::map<std::string, std::string> 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,

src/mgr/ActivePyModules.h

@@ -91,9 +91,6 @@ class ActivePyModules
 
   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<ModuleCommand> get_py_commands() const;
 

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;
 }

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);

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 <john.spray@redhat.com>
+ *
+ * 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<char*>("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<char*>("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;
+}