mgr: load modules in separate python sub-interpreters #14971

tserong · 2017-05-05T12:09:16Z

This provides a reasonable amount of isolation between mgr
modules. Notably, with this change, it's possible to have more
than one mgr module use cherrypy.

I consider this implementation to be a bit messy; I think it'd
be neater if the sub-interpreter was created inside MgrPyModule,
rather than in PyModules::init(), but see the comment in that
function for more details.

Also, due to the lack of documentation on python sub-interpreters,
I'm not completely confident that I'm doing everything correctly.
Notably, there's still calls to PyGILState_Ensure() and
PyEval_ReleaseThread() in PyState.cc, and apparently those two
are either not-compatible with sub-interpreters, or at least need
to be handled delicately.

TL;DR: Everything seems to work for me so far, but should be
considered experimental until further notice.

Signed-off-by: Tim Serong [email protected]

tserong · 2017-05-05T12:13:00Z

Ping @jcsp, @tchaikov, @dmick. I tested this by applying it on top of #14946, and both 'rest' and 'dashboard' modules were able to load and run without stomping on each other.

tserong · 2017-05-05T23:07:21Z

One notable known problem: if a module fails to load because it can't be found (e.g. sys.path is wrong, which shouldn't happen in real life, or a module name is mis-typed in ceph.conf, which might), the python bit of mgr locks up somehow. The loop in PyModules::init() just kinda stops somehow.

tserong · 2017-05-05T23:11:03Z

(That last was just a rebase)

tserong · 2017-05-10T08:53:29Z

OK, I'm confident in this one now.

Each MgrPyModule gets its own python sub-interpreter. The logger, the ceph_state module and sys.path need to be set up separately for each sub-interpreter, so all that happens in MgrPyModule's constructor (previously this was done on the main interpreter in PyModules::init()).

Each sub-interpreter has its own python thread state. The main interpreter also has a python thread state, but that is almost unused except for during setup and teardown of the whole beast.

Some care needs to be taken to ensure that the right thread state is active at the right time; note how the call to handle_pyerror() in PyModules::init() had to be moved inside MgrPyModle::load(). It's not too bad though, just remember that PyModules should be using the main thread state, and MgrPyModule should always be using its sub-interpreter thread state, except for very early in its constructor and very late in its destructor.

The ceph_state module (PyState.cc) naturally has no idea what context it's being run in, so uses PyThreadState_Get() when it needs to know the python thread state.

I've also added a little Gil class to make it easy to acquire and release the GIL for whichever thread state you're in.

tserong · 2017-05-11T06:49:58Z

Dammit, I just tried using mgr built with this changeset to load the restful module from #14457. It loads fine, but segfaults on shutdown...

jcsp · 2017-05-11T14:14:50Z

This is a tough one to review because the subinterpreter functionality in python itself is so obscure. It would be nice to spin the Gil class out into a separate commit.

Since PyFinalize allegedly (https://docs.python.org/2.7/c-api/init.html#c.Py_EndInterpreter) will clean up all un-terminated sub interpreters, I wonder if the stuff in ~MgrPyModule could just go away -- it doesn't seem that useful to maybe terminate sub interpreters (obviously not your fault, quirk of the framework). When taking http://tracker.ceph.com/issues/19549 into account, we never truly need to do completely clean teardown, it just needs to be torn down far enough for the process to end cleanly.

tserong · 2017-05-11T23:14:20Z

Ok, I'll split this up into separate commits. And yeah, I wondered the same about getting rid of the attempted termination in ~MgrPyModule, given the quirks. Now that there's two of us, I'll do that as well (but leave some sort of comment).

tserong · 2017-05-12T13:40:34Z

Splitting out the Gil change was a really good idea. Turns out that change by itself exhibited the segfault on shutdown, even without the sub-interpreters, and now I've learned even more than I ever wanted to know about embedding Python :-)

Here's what I was missing: not only do we need separate sub-interpreters, we need to manually create an additional python thread state for each OS thread that runs the module's serve function. The PyGILState_*() APIs did that bit automatically, but because I removed those (as they're not supported with sub-interpreters), that extra thread state creation wasn't happening, which resulted in that weird segfault.

tserong · 2017-05-12T14:52:18Z

OK, that's better. No weird segfaults. I think I'll avoid using the word "confident" this time and instead go with cautious optimism (and flashbacks to when I drew this thing when I first started working on HA).

My one outstanding question now, I think, is: are there any other OS threads that need another manually created python thread state?

tserong · 2017-05-15T10:59:51Z

I've moved the additional thread state creation inside the Gil class, but the caller has to pass new_thread == true when we know we're in a new OS thread . IMO it'd be better to do this automatically, which we can do, if we don't mind using something that is theoretically a private member of PyThreadState (see the comments in Gil.h for details). What do you think @jcsp?

chardan · 2017-05-16T12:59:27Z

src/mgr/Gil.h

+  : pThreadState(ts), pNewThreadState(nullptr)
+  {
+    // Acquire the GIL, set the current thread state
+    PyEval_RestoreThread(pThreadState);


What happens if pThreadState (ts) is nullptr? The documentation for PyEvalRestoreThread() says that it must not be called on a NULL pointer. Similarly, as I'm reading it, before we do this we have to be sure that we haven't already been called: what happens if this ctor is invoked twice?
https://docs.python.org/2/c-api/init.html#c.PyEval_RestoreThread

If pThreadState is null it probably dies horribly. I've added an assert (so it'll still die horribly, but on our terms).

As for ensuring we haven't already been called, that's (sadly) up to the caller to be aware of. I've updated the comment to hopefully clarify this a bit more. If you invoke this ctor twice in the same thread it'll deadlock, e.g. do not do this:

void func1() { Gil gil(pMyThreadState); // Do some stuff that needs the GIL here func2(); // this will deadlock } void func2() { Gil gil(pMyThreadState); // Do some stuff that needs the GIL }

You can avoid this deadlock like so:

void func1() { { // extra scope Gil gil(pMyThreadState); // Do some stuff that needs the GIL here } func2(); // this won't deadlock }

In summary, you have to be just as careful using this class as you need to be when using PyEvalRestoreThread(). AFAICT there's no public API for figuring out whether or not the GIL is already held.

chardan · 2017-05-16T13:01:52Z

src/mgr/Gil.h

+    // something that's meant to be a black box.
+    //
+    if (new_thread) {
+      pNewThreadState = PyThreadState_New(pThreadState->interp);


Does the old state need to be released in some way before re-assigning pNewThreadState?

No. At this point, pNewThreadState is NULL, and PyThreadState_New creates a new PyThreadState attached to the same interpreter that the existing pThreadState is attached to. It's fine for the two PyThreadState objects to exist at the same time; only one is ever active (the PyThreadState_Swap() calls activate one or the other).

Given that we're now sure pNewThreadState is nullptr, this sounds good. Thanks!

chardan · 2017-05-17T08:28:01Z

src/mgr/Gil.h

+  Gil(PyThreadState *ts, bool new_thread = false)
+  : pThreadState(ts), pNewThreadState(nullptr)
+  {
+    assert(pThreadState != nullptr);


Consider-also throwing an exception instead.

chardan · 2017-05-17T08:29:46Z

src/mgr/Gil.h

+
+private:
+  PyThreadState *pThreadState;
+  PyThreadState *pNewThreadState;


I think initializing these in the ctor's initializer-list is fine, but since we're using C++ it's also possible to write:
PyThreadState *pNewThreadState = nullptr;
(And/or use std::unique_ptr<>.)
...I'm fine either way, but figured I should point it out.

chardan · 2017-05-17T08:33:14Z

src/mgr/MgrPyModule.cc

+	m[len-1] = '\0';
+      }
+      dout(4) << m << dendl;
+    }


If PyArg_ParseTuple() has returned false, we're swallowing the returned error here. Certainly, fine if we didn't want to take any action, but is this intentional?

chardan · 2017-05-17T08:36:33Z

src/mgr/MgrPyModule.cc

+      auto len = strlen(m);
+      if (len && m[len-1] == '\n') {
+	m[len-1] = '\0';
+      }


If len is one, this assumes not-an-error (m[len-1] won't explode because len > 0 on account of the first clause). I don't see in the Python documentation whether or not that's possible, but if it is it might be worth checking for.

chardan · 2017-05-17T08:37:45Z

src/mgr/MgrPyModule.cc

 {
-  PyGILState_STATE gstate;
-  gstate = PyGILState_Ensure();
+  assert(pMainThreadState != nullptr);


Consider throwing an exception.

chardan · 2017-05-17T08:39:00Z

src/mgr/MgrPyModule.cc

+  pMyThreadState = Py_NewInterpreter();
+  if (pMyThreadState == nullptr) {
+    derr << "Failed to create python sub-interpreter for '" << module_name << '"' << dendl;
+  } else {


If pMyThreadState == nullptr, the program continues, leaving the object constructed. This seems like a critical failure to me? If so, I think it would be better to throw an exception than to leave the object in an invalid state.

I wondered about that, but couldn't see any other exceptions being thrown anywhere in the mgr codebase, so decided not to go there.

The way these things get constructed (in PyModules::init()), the next thing that's called is MgrPyModule::load() which will return with an error if the object is in that invalid state. So it's handled, if not exactly clean.

chardan · 2017-05-17T08:40:45Z

src/mgr/MgrPyModule.cc

+      PySys_SetObject("stderr", py_logger);
+      PySys_SetObject("stdout", py_logger);
+#else
+      PySys_SetObject(const_cast<char*>("stderr"), py_logger);


Is the first #ifdef needed? (eg. will the version here with the cast always work anyway?)

chardan · 2017-05-17T08:42:57Z

src/mgr/MgrPyModule.cc

+#endif
+    }
+    // Populate python namespace with callable hooks
+    Py_InitModule("ceph_state", CephStateMethods);


Should the return value of this function be checked?

chardan · 2017-05-17T08:43:17Z

src/mgr/MgrPyModule.cc


-  Py_XDECREF(pClassInstance);
+    PySys_SetPath((char*)(sys_path.c_str()));


Prefer C++ style casts. Even though they're annoying to type. :-)

chardan · 2017-05-17T08:45:08Z

src/mgr/MgrPyModule.cc

+    return -EINVAL;
+  }
+
+  Gil gil(pMyThreadState);


I'm starting to wonder if it's worth calling type Gil something like "Gil_scope_guard"?

That's probably a good idea - it'd make it more obvious to the reader WTF it was for.

chardan · 2017-05-17T08:46:37Z

src/mgr/MgrPyModule.cc

+  }
+
+  Gil gil(pMyThreadState);
+
  // Load the module
  PyObject *pName = PyString_FromString(module_name.c_str());


We're guaranteed that module_name.c_str() will not be NULL, but PyString_FromString() is going to make a copy, which could fail. We may want to check pName for nullptr.

liewegas added the mgr label May 5, 2017

tchaikov self-requested a review May 5, 2017 15:25

tserong force-pushed the wip-mgr-py-sub-interpreter branch from c4cd997 to 8b5ceca Compare May 5, 2017 23:10

tserong force-pushed the wip-mgr-py-sub-interpreter branch from 8b5ceca to f6a135a Compare May 10, 2017 08:49

tserong changed the title ~~[DNM] mgr: load modules in separate python sub-interpreters~~ mgr: load modules in separate python sub-interpreters May 10, 2017

tserong changed the title ~~mgr: load modules in separate python sub-interpreters~~ [DNM] mgr: load modules in separate python sub-interpreters May 11, 2017

tserong force-pushed the wip-mgr-py-sub-interpreter branch 2 times, most recently from f3b82bc to 1f9b93d Compare May 12, 2017 14:43

tserong changed the title ~~[DNM] mgr: load modules in separate python sub-interpreters~~ mgr: load modules in separate python sub-interpreters May 12, 2017

tserong force-pushed the wip-mgr-py-sub-interpreter branch from 1f9b93d to f21ba89 Compare May 15, 2017 10:57

chardan reviewed May 16, 2017

View reviewed changes

chardan reviewed May 17, 2017

View reviewed changes


		Py_XDECREF(pClassInstance);
		PySys_SetPath((char*)(sys_path.c_str()));

mgr: load modules in separate python sub-interpreters #14971

mgr: load modules in separate python sub-interpreters #14971

Uh oh!

Conversation

tserong commented May 5, 2017

Uh oh!

tserong commented May 5, 2017

Uh oh!

tserong commented May 5, 2017

Uh oh!

tserong commented May 5, 2017

Uh oh!

tserong commented May 10, 2017

Uh oh!

tserong commented May 11, 2017

Uh oh!

jcsp commented May 11, 2017

Uh oh!

tserong commented May 11, 2017

Uh oh!

tserong commented May 12, 2017

Uh oh!

tserong commented May 12, 2017

Uh oh!

tserong commented May 15, 2017

Uh oh!

chardan May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tserong May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chardan May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tserong May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chardan May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chardan May 16, 2017 •

edited

Loading

tserong May 17, 2017 •

edited

Loading

chardan May 17, 2017 •

edited

Loading

tserong May 18, 2017 •

edited

Loading

chardan May 17, 2017 •

edited

Loading