Skip to content

mgr: add config to load modules in main interpreter instead of subinterpreter#66176

Closed
athanatos wants to merge 3 commits intoceph:mainfrom
athanatos:sjust/wip-mgr-subinterpreter-config
Closed

mgr: add config to load modules in main interpreter instead of subinterpreter#66176
athanatos wants to merge 3 commits intoceph:mainfrom
athanatos:sjust/wip-mgr-subinterpreter-config

Conversation

@athanatos
Copy link
Contributor

See commit and config doc for details.

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

Signed-off-by: Samuel Just <sjust@redhat.com>
…to allow unpickle to work correctly

Signed-off-by: Samuel Just <sjust@redhat.com>
This commit adds a mgr_main_interpreter_modules config to cause
specified modules (or all if * is specified) to be loaded in the main
interpreter rather than in a subinterpreter.

By default, cephadm, k8sevents, and rook are set to be loaded in the
main interpreter to work around
https://bugzilla.redhat.com/show_bug.cgi?id=2255688.

Signed-off-by: Samuel Just <sjust@redhat.com>
@github-actions
Copy link

github-actions bot commented Nov 8, 2025

Config Diff Tool Output

+ added: mgr_main_interpreter_modules (mgr.yaml.in)

The above configuration changes are found in the PR. Please update the relevant release documentation if necessary.
Ignore this comment if docs are already updated. To make the "Check ceph config changes" CI check pass, please comment /config check ok and re-run the test.

@athanatos
Copy link
Contributor Author

Depends on #66071

Copy link
Contributor

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to learn more about how running modules in the main interpreter is expected to work. Are they still running in seperate threads? Is it expected that if all of them are being loaded that they can operate concurrently?

Thanks!

@athanatos
Copy link
Contributor Author

Still in separate threads. The subinterpreter thing really didn't buy us much. All of the sub interpreters still shared the same GIL and a ton of other state. We got away with using subinterpreters as we did for so long because there was so little actual isolation between the subinterpreters. The changes in 3.12 to allow you to give them different GIL (which, yes, we are not yet using) is most likely the reason why this all broke.

@batrick
Copy link
Member

batrick commented Nov 11, 2025

I'd like to learn more about how running modules in the main interpreter is expected to work. Are they still running in seperate threads? Is it expected that if all of them are being loaded that they can operate concurrently?

Thanks!

So importing them in the main interpreter means they're also imported in the sub-interpreters? This is confusing.

@athanatos athanatos marked this pull request as ready for review November 12, 2025 17:10
@athanatos athanatos requested review from a team as code owners November 12, 2025 17:10
@athanatos
Copy link
Contributor Author

jenkins test make check arm64

@athanatos
Copy link
Contributor Author

jenkins test make check

@athanatos
Copy link
Contributor Author

I'd like to learn more about how running modules in the main interpreter is expected to work. Are they still running in seperate threads? Is it expected that if all of them are being loaded that they can operate concurrently?
Thanks!

So importing them in the main interpreter means they're also imported in the sub-interpreters? This is confusing.

I'm not sure I understand the question. The problem seems to be that when pyo3 is imported anywhere, it sets up process-global state. Prior to 3.12, I suspect the sub interpreters shared library import state, so it only really got imported once. Now, it really gets imported multiple times so we get an import failure after the first one. Moving these modules to the same interpreter (the main one) means that the import once more only actually happens once avoiding the problem.

@athanatos
Copy link
Contributor Author

Replaced by #66244 to avoid / in branch name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants