pybind/mgr: update modules to use independent CLICommand subtypes with distinct COMMAND attributes by athanatos · Pull Request #66467 · ceph/ceph

athanatos · 2025-12-02T01:12:14Z

Fixes: https://tracker.ceph.com/issues/74042

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

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

batrick

Also, maybe a separate PR for pybind/.../dashboard: misc automatic linter fixes?

batrick · 2025-12-02T18:47:12Z

src/pybind/mgr/mgr_module.py

+    def make_registry_subtype(cls, name) -> 'CLICommandBase':
+        return type(name, (cls,), {
+            'COMMANDS': {},
+        })


You could avoid this construction (and all of the associated cli.py files and CLICommand = class members):

MirroringCLICommand = CLICommandBase.make_registry_subtype("MirroringCLICommand")

by doing something like:

class CLICommand: ... def __init__(self, mgrm_cls, prefix, "w", poll): COMMANDS = mgrm_cls.set_default('COMMANDS', {}) ... @classmethod def Write(cls,mgrm_cls,prefix,poll): return cls(mgrm_cls, prefix, "w", poll)

@CLICommand.Write(locals(), 'fs snapshot mirror enable') def snapshot_mirror_enable(self, fs_name: str):

Obviously, the key bit there is the slightly onerous passing of locals() to the CLICommand so that it can put the COMMANDS dict in the module class definition.

Anyway, I also think what you have done is clear so I'm good with this either way.

Passing in locals() doesn't really seem cleaner than this.

My original draft (#66415) was able to hide all of this (except for the more special cases like dashboard and cephadm/orchestrator) by using a metaclass to inject a magic CLICommand (along with the other globals) into the class namespace prior to executing class body of the modules derived from MgrModule. @zmc wasn't fond of the level of magic. Tracking down and untangling the knot of magic that already existed between CLICommand and its users cost me a ton of time, so I ended up generating the explicit boilerplate here instead to avoid passing something confusing like that along.

The linter commit is because I needed it to make tox happy. I don't actually understand how those files merged like that unless making tox happy isn't actually a merge requirement.

phlogistonjohn · 2025-12-03T16:16:15Z

src/pybind/mgr/smb/module.py

        return all_results

-    @cli.SMBCommand('apply', perm='rw')
+    @SMBCLICommand('apply', perm='rw')


Maybe I don't fully understand something, but these lines seem like unnecessary churn to me? Is there a reason not to CLICommand = cli.SMBCLICommand above and leave these lines alone? It's the same object after all (AFAICT).

This way it matches every other cli command decorator in pybind/mgr.

OK, fair enough for now.

phlogistonjohn

most of these patches look generally OK to me. I do have a few q's about stuff that are more like 'preference or necessary change'? :-)

phlogistonjohn · 2025-12-03T16:27:44Z

src/pybind/mgr/mgr_module.py

-    def dump_cmd_list(cls) -> List[Dict[str, Union[str, bool]]]:
-        return [cmd.dump_cmd() for cmd in cls.COMMANDS.values()]
-
+    def Read(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':


Is there a compatibility reason for the capital-R? It's not pep8 compliant. If it's necessary, can we add a comment noting the reason?

No, just chose to match the capitalization of CLIReadCommand and CLIWriteCommand. tox is ok with it. I'm not really inclined to change it unless it really bothers you.

OK, I will ignore it for now but I do have plans to enable more linters on the python code in ceph (a lot of stuff for mgr is old and exempt right now) eventually and if the linters start complaining in the future I may end up changing it, but if they don't complain I will continue to ignore it.

Python is snake_case by default. All-caps/CamelCase is just reserved for GLOBALS and ClassNames. This should be easy to fix, you know s/\.Read.../....

phlogistonjohn · 2025-12-03T16:36:04Z

src/pybind/mgr/tests/test_object_format.py



-class DecoDemo:
+class DecoDemo(object):


I'm so curious.... did not having object as a base actually have visible behavhior impacts on python 3?

shrug matches other code now.

Well, that's probably because that code is older and either supported python 2 or carried over that pattern from python 2 out of habit. I won't ask you to change it but I may change it back the next time I need to work on that module. :-)

NitzanMordhai · 2025-12-08T11:26:22Z

I ran rados suite on the branch-of-the-day (wip-rocky10-branch-of-the-day-2025-12-01-1764638786) that includes that PR, the results are in https://tracker.ceph.com/issues/74070 and look good (accept a few segfaults that we already saw)

batrick · 2025-12-09T14:11:14Z

This PR is under test in https://tracker.ceph.com/issues/74161.

github-actions · 2025-12-14T13:19:15Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

epuertat

First of all, kudos for tackling such an unpleasant duty 😅 The COMMANDS part and all the many layers built on top of it (the decorators, global registry, etc.) definitely requires a refactoring/clean-up.

I'm not sure if I'm missing anything with the original issue, but if it had to do with the base class ClassVars shared across subclasses, Python provides a way to deal with that situation, the __init_subclass__ hook, explicitly designed to avoid having to deal with metaclasses:

class MgrModule(ceph_module.BaseMgrModule, MgrModuleLoggingMixin):
    def __init_subclass__(cls, **kwargs):
        super().__init_subclass__(**kwargs)
        cls.COMMANDS = []
        ...

epuertat · 2026-01-27T09:38:34Z

src/pybind/mgr/mgr_module.py

-    def dump_cmd_list(cls) -> List[Dict[str, Union[str, bool]]]:
-        return [cmd.dump_cmd() for cmd in cls.COMMANDS.values()]
-
+    def Read(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':


Python is snake_case by default. All-caps/CamelCase is just reserved for GLOBALS and ClassNames. This should be easy to fix, you know s/\.Read.../....

epuertat · 2026-01-27T09:39:58Z

src/pybind/mgr/mgr_module.py

+        return cls(prefix, "r", poll)

+    @classmethod
+    def Write(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':


Same here:

Suggested change

def Write(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':

def write(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':

Write and Read are simply wrappers around the CLICommand constructor, so I figured caps would be better.

Given the length of time that this has been tested and up for review and the urgency of merging it for rocky10 support, I'm not inclined to change it at this point unless you feel really strongly.

epuertat · 2026-01-27T10:52:32Z

src/pybind/mgr/alerts/module.py

                    self.get_ceph_option(opt))
            self.log.debug(' native option %s = %s', opt, getattr(self, opt))

-    @CLIReadCommand('alerts send')


If possible I'd suggest that we keep the original interface as much as possible (@CLI*Command) and no Read/Write methods, in order to reduce conflicts with backports.

My original solution used some subtle metaclass trickery to avoid changing a bunch of code, but @zmc asked me to do it more explicitly. Given the difficulty of untangling the magic around the old mechanism (and mechanisms for subverting it in some modules), I agreed that this would probably be a lot more maintainable.

epuertat · 2026-01-27T10:53:53Z

src/pybind/mgr/alerts/module.py

+

 class Alerts(MgrModule):
+    CLICommand = AlertsCLICommand


Maybe this alias was here to reduce interface changes and keep the @CLICommand decorator?

MgrModule uses it for command registration. I'm reluctant to use CLICommand in the decorators in the body as I wanted it to be obvious which CLICommand subclass was actually being used. Doing it this way allows one to actually grep for uses of any particular CLICommand subclass and allows decorators in MgrModule bodies to match those not in MgrModule bodies.

athanatos · 2026-02-02T20:30:55Z

@ljflores Can you arrange for this PR to be tested on its own (no need for rocky10, suite changes, or any of the other PRs)? It should work in isolation, and that would let us merge it ahead of other fixes.

ljflores · 2026-02-02T20:57:54Z

@ljflores Can you arrange for this PR to be tested on its own (no need for rocky10, suite changes, or any of the other PRs)? It should work in isolation, and that would let us merge it ahead of other fixes.

@athanatos ACK

athanatos · 2026-02-04T21:50:14Z

jenkins test all

athanatos · 2026-02-04T21:50:35Z

jenkins test make check

athanatos · 2026-02-04T21:50:40Z

jenkins test make check arm64

athanatos · 2026-02-04T21:50:45Z

jenkins test dashboard

athanatos · 2026-02-04T21:50:49Z

jenkins test dashboard cephadm

athanatos · 2026-02-04T21:50:54Z

jenkins test api

ljflores · 2026-02-05T22:38:59Z

jenkins retest this please

ljflores · 2026-02-05T22:39:08Z

@athanatos RADOS approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues74716

yuriw · 2026-02-06T18:15:00Z

jenkins retest this please

ljflores · 2026-02-06T20:51:45Z

This PR is blocked by these bugs:

https://tracker.ceph.com/issues/70691 - FAILED ceph_assert(r == 0) in TestLibRBD.ConcurrentOperations - (rbd)
https://tracker.ceph.com/issues/74808 - run-rbd-unit-tests-109.sh fails on "make check arm64" and "make check" - (rbd)
https://tracker.ceph.com/issues/74809 - make check run-tox-qa failure: workunits/rgw/test_rgw_versioning.py:78: error: Assertion is always true, perhaps remove parentheses? - (rgw)

I have asked Casey and Ilya to triage them.

idryomov · 2026-02-06T21:00:23Z

The only blocker here is https://tracker.ceph.com/issues/74809. The two RBD issues aren't new, they are intermittent and typically go away on a retrigger. This is evidenced by the current make check (arm64) failure:

The following tests FAILED:
	300 - run-tox-qa (Failed)

github-actions · 2026-02-06T21:25:47Z

This is an automated message by src/script/redmine-upkeep.py.

I have resolved the following tracker ticket due to the merge of this PR:

https://tracker.ceph.com/issues/74042

No backports are pending for the ticket. If this is incorrect, please update the tracker
ticket and reset to Pending Backport state.

Update Log: https://github.com/ceph/ceph/actions/runs/21766408041

Update documentation to reflect the new per-module command registry pattern introduced in PR ceph#66467. The old global CLICommand decorators have been replaced with module-specific registries. Changes: - doc/mgr/modules.rst: Rewrite CLICommand section with setup guide, update all examples to use AntigravityCLICommand pattern - src/pybind/mgr/object_format.py: Add note explaining per-module registries and update all decorator examples - doc/dev/developer_guide/dash-devel.rst: Update dashboard plugin examples to use DBCLICommand All examples now correctly show: - Creating registry with CLICommandBase.make_registry_subtype() - Using module-specific decorator names (e.g., @StatusCLICommand.Read) - Setting CLICommand class attribute for framework registration Signed-off-by: Kefu Chai <k.chai@proxmox.com>

github-actions bot added cephadm cephfs Ceph File System dashboard mgr monitoring nfs orchestrator pybind rbd rook telemetry labels Dec 2, 2025

github-project-automation bot added this to Ceph-Dashboard Dec 2, 2025

github-project-automation bot moved this to New in Ceph-Dashboard Dec 2, 2025

batrick approved these changes Dec 2, 2025

View reviewed changes

github-project-automation bot moved this from New to Reviewer approved in Ceph-Dashboard Dec 2, 2025

phlogistonjohn reviewed Dec 3, 2025

View reviewed changes

NitzanMordhai approved these changes Dec 8, 2025

View reviewed changes

batrick added the wip-pdonnell-testing2 label Dec 9, 2025

github-actions bot added the needs-rebase label Dec 14, 2025

athanatos force-pushed the wip-sjust-mgr-cli-command-74042 branch from 9daaea2 to 8231b8e Compare December 16, 2025 16:45

athanatos marked this pull request as ready for review December 16, 2025 16:46

athanatos requested review from a team as code owners December 16, 2025 16:46

athanatos requested a review from cloudbehl December 16, 2025 16:46

athanatos force-pushed the wip-sjust-mgr-cli-command-74042 branch from 8231b8e to 4aa9e24 Compare December 17, 2025 17:51

epuertat reviewed Jan 27, 2026

View reviewed changes

yuriw added wip-yuri4-testing and removed wip-yuri4-testing labels Feb 2, 2026

dmick merged commit e60d9dc into ceph:main Feb 6, 2026
8 of 16 checks passed

github-project-automation bot moved this from Reviewer approved to Done in Ceph-Dashboard Feb 6, 2026

tchaikov mentioned this pull request Feb 8, 2026

doc/_ext: fix ceph_commands.py for new decorator-based command system #67254

Merged

14 tasks

tchaikov mentioned this pull request Feb 9, 2026

doc: update mgr module command documentation for per-module registries #67257

Merged

14 tasks

NitzanMordhai added the rocky10 label Feb 19, 2026

NitzanMordhai mentioned this pull request Feb 25, 2026

tentacle: pybind/mgr: update modules to use independent CLICommand subtypes with distinct COMMAND attributes #67511

Draft

	def Write(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':
	def write(cls, prefix: str, poll: bool = False) -> 'CLICommandBase':

Conversation

athanatos commented Dec 2, 2025

Uh oh!

batrick left a comment

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!

athanatos Dec 3, 2025 • 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!

phlogistonjohn left a comment

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!

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!

NitzanMordhai commented Dec 8, 2025

Uh oh!

batrick commented Dec 9, 2025

Uh oh!

github-actions bot commented Dec 14, 2025

Uh oh!

epuertat left a comment

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!

Choose a reason for hiding this comment

Uh oh!

athanatos Jan 27, 2026 • 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!

athanatos commented Feb 2, 2026

Uh oh!

ljflores commented Feb 2, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

athanatos commented Feb 4, 2026

Uh oh!

ljflores commented Feb 5, 2026

Uh oh!

athanatos Dec 3, 2025 •

edited

Loading

athanatos Jan 27, 2026 •

edited

Loading