From cd5f7a18b547ff790bbc17566d1f9030e4b93aed Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Fri, 24 Oct 2025 03:00:45 +0000 Subject: [PATCH] mgr: serialize python objects sent between subinterpreters via remote Signed-off-by: Samuel Just (cherry picked from commit f69069e114ea8c785d6c27c57560a0b9bb8c16be) Conflicts: - src/mgr/PyModule.cc Missing commit 3366ef5 still didn't merge, just take the changes for the dtor --- src/mgr/ActivePyModule.cc | 72 ++++++++++++++++++++++++++++++-------- src/mgr/ActivePyModule.h | 6 ++-- src/mgr/ActivePyModules.cc | 10 +++--- src/mgr/ActivePyModules.h | 6 ++-- src/mgr/BaseMgrModule.cc | 69 +++++++++++++++++++++++++++++++++--- src/mgr/PyModule.cc | 50 ++++++++++++++++++++++++++ src/mgr/PyModule.h | 8 +++++ 7 files changed, 192 insertions(+), 29 deletions(-) diff --git a/src/mgr/ActivePyModule.cc b/src/mgr/ActivePyModule.cc index c244966e598d..240456146d16 100644 --- a/src/mgr/ActivePyModule.cc +++ b/src/mgr/ActivePyModule.cc @@ -128,23 +128,49 @@ bool ActivePyModule::method_exists(const std::string &method) const } } -PyObject *ActivePyModule::dispatch_remote( +std::optional> ActivePyModule::dispatch_remote( const std::string &method, - PyObject *args, - PyObject *kwargs, + std::span pickled_args, + std::span pickled_kwargs, std::string *err) { ceph_assert(err != nullptr); - // Rather than serializing arguments, pass the CPython objects. - // Works because we happen to know that the subinterpreter - // implementation shares a GIL, allocator, deallocator and GC state, so - // it's okay to pass the objects between subinterpreters. - // But in future this might involve serialization to support a CSP-aware - // future Python interpreter a la PEP554 + // deserialize arguments. Gil gil(py_module->pMyThreadState, true); + auto pmodule = py_module->pPickleModule; + auto pickled_args_bytes = py_bytes_from_span(pickled_args); + auto args = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("loads"), + pickled_args_bytes, + nullptr); + Py_DECREF(pickled_args_bytes); + if (args == nullptr) { + std::string caller = "ActivePyModule::dispatch_remote "s + method; + *err = handle_pyerror(true, get_name(), caller); + derr << "Failed to deserialize (pickle.loads) args: " << *err << dendl; + return std::nullopt; + } + + auto pickled_kwargs_bytes = py_bytes_from_span(pickled_kwargs); + auto kwargs = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("loads"), + pickled_kwargs_bytes, + nullptr); + Py_DECREF(pickled_kwargs_bytes); + if (kwargs == nullptr) { + std::string caller = "ActivePyModule::dispatch_remote "s + method; + *err = handle_pyerror(true, get_name(), caller); + derr << "Failed to deserialize (pickle.loads) kwargs: " << *err << dendl; + + Py_DECREF(args); + return std::nullopt; + } + // Fire the receiving method auto boundMethod = PyObject_GetAttrString(pClassInstance, method.c_str()); @@ -154,21 +180,37 @@ PyObject *ActivePyModule::dispatch_remote( dout(20) << "Calling " << py_module->get_name() << "." << method << "..." << dendl; - auto remoteResult = PyObject_Call(boundMethod, + auto ret = PyObject_Call(boundMethod, args, kwargs); Py_DECREF(boundMethod); - - if (remoteResult == nullptr) { + Py_DECREF(kwargs); + Py_DECREF(args); + if (ret == nullptr) { // Because the caller is in a different context, we can't let this // exception bubble up, need to re-raise it from the caller's // context later. std::string caller = "ActivePyModule::dispatch_remote "s + method; *err = handle_pyerror(true, get_name(), caller); - } else { - dout(20) << "Success calling '" << method << "'" << dendl; + return std::nullopt; + } + dout(20) << "Success calling '" << method << "'" << dendl; + + auto pickled_ret = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("dumps"), + ret, + nullptr); + Py_DECREF(ret); + if (pickled_ret == nullptr) { + std::string caller = "ActivePyModule::dispatch_remote "s + method; + *err = handle_pyerror(true, get_name(), caller); + derr << "Failed to serialize (pickle.dumps) ret: " << *err << dendl; + return std::nullopt; } - return remoteResult; + std::vector pickled_ret_str = py_bytes_as_vec(pickled_ret); + Py_DECREF(pickled_ret); + return pickled_ret_str; } void ActivePyModule::config_notify() diff --git a/src/mgr/ActivePyModule.h b/src/mgr/ActivePyModule.h index 8538f6e236a3..5b351cafbe50 100644 --- a/src/mgr/ActivePyModule.h +++ b/src/mgr/ActivePyModule.h @@ -66,10 +66,10 @@ public: bool method_exists(const std::string &method) const; - PyObject *dispatch_remote( + std::optional> dispatch_remote( const std::string &method, - PyObject *args, - PyObject *kwargs, + std::span pickled_args, + std::span pickled_kwargs, std::string *err); int handle_command( diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index adaa422754b8..5e0b385bf94a 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -636,19 +636,21 @@ bool ActivePyModules::get_store(const std::string &module_name, } } -PyObject *ActivePyModules::dispatch_remote( +std::optional> ActivePyModules::dispatch_remote( const std::string &other_module, const std::string &method, - PyObject *args, - PyObject *kwargs, + std::span pickled_args, + std::span pickled_kwargs, std::string *err) { auto mod_iter = modules.find(other_module); ceph_assert(mod_iter != modules.end()); - return mod_iter->second->dispatch_remote(method, args, kwargs, err); + return mod_iter->second->dispatch_remote( + method, pickled_args, pickled_kwargs, err); } + bool ActivePyModules::get_config(const std::string &module_name, const std::string &key, std::string *val) const { diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h index 62d0b18cf19e..662c3a2e3f2e 100644 --- a/src/mgr/ActivePyModules.h +++ b/src/mgr/ActivePyModules.h @@ -240,11 +240,11 @@ public: return modules.at(module_name)->method_exists(method_name); } - PyObject *dispatch_remote( + std::optional> dispatch_remote( const std::string &other_module, const std::string &method, - PyObject *args, - PyObject *kwargs, + std::span pickled_args, + std::span pickled_kwargs, std::string *err); int init(); diff --git a/src/mgr/BaseMgrModule.cc b/src/mgr/BaseMgrModule.cc index 5855c67f8281..eb302feffacc 100644 --- a/src/mgr/BaseMgrModule.cc +++ b/src/mgr/BaseMgrModule.cc @@ -40,6 +40,7 @@ using std::list; using std::string; +using namespace std::literals; typedef struct { PyObject_HEAD @@ -828,6 +829,7 @@ ceph_clear_all_progress_events(BaseMgrModule *self, PyObject *args) static PyObject * ceph_dispatch_remote(BaseMgrModule *self, PyObject *args) { + // PyArgs_ParseTuple doesn't give us refcounts here char *other_module = nullptr; char *method = nullptr; PyObject *remote_args = nullptr; @@ -846,6 +848,38 @@ ceph_dispatch_remote(BaseMgrModule *self, PyObject *args) return nullptr; } + auto pmodule = self->this_module->py_module->pPickleModule; + auto pickled_args = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("dumps"), + remote_args, + nullptr); + if (pickled_args == nullptr) { + std::string caller = "ceph_dispatch_remote "s + " " + method; + std::string err = handle_pyerror(true, other_module, caller); + PyErr_SetString(PyExc_RuntimeError, err.c_str()); + derr << err << dendl; + return nullptr; + } + std::span pickled_args_span = py_bytes_as_span(pickled_args); + + auto pickled_kwargs = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("dumps"), + remote_kwargs, + nullptr); + if (pickled_kwargs == nullptr) { + std::string caller = "ceph_dispatch_remote "s + " " + method; + std::string err = handle_pyerror(true, other_module, caller); + PyErr_SetString(PyExc_RuntimeError, err.c_str()); + derr << err << dendl; + + Py_DECREF(pickled_args); + return nullptr; + } + std::span pickled_kwargs_span = + py_bytes_as_span(pickled_kwargs); + // Drop GIL from calling python thread state, it will be taken // both for checking for method existence and for executing method. PyThreadState *tstate = PyEval_SaveThread(); @@ -853,23 +887,50 @@ ceph_dispatch_remote(BaseMgrModule *self, PyObject *args) if (!self->py_modules->method_exists(other_module, method)) { PyEval_RestoreThread(tstate); PyErr_SetString(PyExc_NameError, "Method not found"); + + Py_DECREF(pickled_args); + Py_DECREF(pickled_kwargs); return nullptr; } std::string err; - auto result = self->py_modules->dispatch_remote(other_module, method, - remote_args, remote_kwargs, &err); + std::optional> maybe_pickled_ret = + self->py_modules->dispatch_remote( + other_module, + method, + pickled_args_span, + pickled_kwargs_span, + &err); PyEval_RestoreThread(tstate); - if (result == nullptr) { + // we retain these references across the dispatch_remote call so that + // we can pass string_view and avoid the copy + Py_XDECREF(pickled_kwargs); + Py_XDECREF(pickled_args); + + if (!maybe_pickled_ret) { std::stringstream ss; ss << "Remote method threw exception: " << err; PyErr_SetString(PyExc_RuntimeError, ss.str().c_str()); derr << ss.str() << dendl; + return nullptr; } - return result; + auto pickled_ret_bytes = py_bytes_from_vec(*maybe_pickled_ret); + auto ret = PyObject_CallMethodObjArgs( + pmodule, + PyUnicode_FromString("loads"), + pickled_ret_bytes, + nullptr); + if (ret == nullptr) { + std::string caller = "ceph_dispatch_remote "s + " " + method; + std::string err = handle_pyerror(true, other_module, caller); + PyErr_SetString(PyExc_RuntimeError, err.c_str()); + derr << err << dendl; + } + Py_XDECREF(pickled_ret_bytes); + return ret; } static PyObject* diff --git a/src/mgr/PyModule.cc b/src/mgr/PyModule.cc index 4f996489ba08..44be043aeb1d 100644 --- a/src/mgr/PyModule.cc +++ b/src/mgr/PyModule.cc @@ -154,6 +154,48 @@ std::string peek_pyerror() return exc_msg; } +std::span py_bytes_as_span(PyObject *bytes) +{ + assert(bytes); + assert(PyBytes_CheckExact(bytes)); + Py_ssize_t length; + char *buf; + int r = PyBytes_AsStringAndSize( + bytes, &buf, &length); + assert(r == 0); + return std::span((const std::byte*)buf, size_t(length)); +} + +PyObject *py_bytes_from_span(std::span s) +{ + auto ret = PyBytes_FromStringAndSize( + reinterpret_cast(s.data()), s.size_bytes()); + assert(ret); + return ret; +} + +std::vector py_bytes_as_vec(PyObject *bytes) +{ + assert(bytes); + assert(PyBytes_CheckExact(bytes)); + Py_ssize_t length; + char *buf; + int r = PyBytes_AsStringAndSize( + bytes, &buf, &length); + assert(r == 0); + return std::vector{ + reinterpret_cast(buf), + reinterpret_cast(buf) + size_t(length)}; +} + +PyObject *py_bytes_from_vec(const std::vector &s) +{ + auto ret = PyBytes_FromStringAndSize( + reinterpret_cast(s.data()), s.size()); + assert(ret); + return ret; +} + namespace { PyObject* log_write(PyObject*, PyObject* args) { @@ -309,6 +351,13 @@ int PyModule::load(PyThreadState *pMainThreadState) Gil gil(pMyThreadState); int r; + + pPickleModule = PyImport_ImportModuleNoBlock("pickle"); + if (!pPickleModule) { + derr << "Unable to load pickle" << dendl; + return -EINVAL; + } + r = load_subclass_of("MgrModule", &pClass); if (r) { derr << "Class not found in module '" << module_name << "'" << dendl; @@ -707,6 +756,7 @@ PyModule::~PyModule() Gil gil(pMyThreadState, true); Py_XDECREF(pClass); Py_XDECREF(pStandbyClass); + Py_XDECREF(pPickleModule); } } diff --git a/src/mgr/PyModule.h b/src/mgr/PyModule.h index a47db3a47ef9..4cdb5e0b8e7f 100644 --- a/src/mgr/PyModule.h +++ b/src/mgr/PyModule.h @@ -32,6 +32,13 @@ std::string handle_pyerror(bool generate_crash_dump = false, std::string peek_pyerror(); +std::span py_bytes_as_span(PyObject*); +PyObject *py_bytes_from_span(std::span); + +std::vector py_bytes_as_vec(PyObject*); +PyObject *py_bytes_from_vec(const std::vector &); + + /** * A Ceph CLI command description provided from a Python module */ @@ -95,6 +102,7 @@ public: SafeThreadState pMyThreadState; PyObject *pClass = nullptr; PyObject *pStandbyClass = nullptr; + PyObject *pPickleModule = nullptr; explicit PyModule(const std::string &module_name_) : module_name(module_name_) -- 2.47.3