Bug #61186
openmgr/nfs: hitting incomplete command returns same suggestion twice
0%
Description
for example
sh-4.4$ ceph nfs export get
no valid command found; 2 closest matches:
nfs export get <cluster_id> <pseudo_path>
nfs export get <cluster_id> <pseudo_path>
Error EINVAL: invalid command
sh-4.4$ ceph nfs export delete
no valid command found; 2 closest matches:
nfs export delete <cluster_id> <pseudo_path>
nfs export delete <cluster_id> <pseudo_path>
Error EINVAL: invalid command
sh-4.4$ ceph nfs cluster config get
no valid command found; 2 closest matches:
nfs cluster config get <cluster_id>
nfs cluster config get <cluster_id>
Error EINVAL: invalid command
sh-4.4$ ceph nfs cluster config set
no valid command found; 2 closest matches:
nfs cluster config set <cluster_id>
nfs cluster config set <cluster_id>
Error EINVAL: invalid command
Haven't tested other operations, so these might not be the only ones.
Updated by Dhairya Parmar 11 months ago
Thanks to John Mulligan debugging the mgr code and finding the root cause.
NFS commands are decorated using CLICommand (and it is a global decorator), so any py module once loaded/imported that uses this decorator will create a commands list that will be read by the mgr cpp code. `rook_cluster.py` imports nfs.module (used for creating/updating nfs in rook), so the nfs commands gets into the suggestions dict at the time of loading the `nfs` module and the `rook` module. This is the reason for duplication.
Updated by Dhairya Parmar 11 months ago
- Status changed from New to Triaged
At first, we thought that NFS Command doubling is due to the import of NFS manager module into the Rook manager module (`rook_cluster.py`) where it is used to cast `mgr` to NFS to write a rados object at:
NFSRados(mgr_module.rados, service_id).write_obj('', f'conf-nfs.{spec.service_id}')
(here mgr_module is `mgr` casted to NFS module)
but this can be done without casting too because the `rados` is not specific to NFS module as it comes from the `MgrModule` that `nfs.module` and the `mgr` (which is of type `RookOrchestrator`) also inherits.
Therefore, John proposed some ideas to tackle this:
1 "quick fix" if we simply remove the `from nfs.module import Module` line and fix the lines that use(d) it.
2 Longer term it may be worth considering an approach where manager modules talk to each other using the {mgr,mon}_command functions.
Something calling a new command like `nfs cluster pool-init` or like.
3 Implment some way of blocking imports for unsafe modules? Maybe expose a python function from the C++ side indicating what module it is? Something like:
if mgr_module_realname() != "nfs":
raise ImportError("incorrect mgr module")
I tried out 1 but unfortunately it didn't work, reason being import of `nfs.module` in `nfs.__init__` [0] i.e. anything that imports any part of NFS will trigger the CLICommands, thus there seems to be no simple fix to get rid of this unless we try 2 or 3 but it would take some efforts and possibly some foundational changes to the way we treat imports. Therefore it was decided to keep this aside for sometime until we find some strong reason to pick this up.
Special shout-out to John Mulligan for all the debugging and helping find the root cause, most of this discussion took place in the Orchestration team weekly meeting on 6th June '23.
Updated by Dhairya Parmar 11 months ago
One more thing that we noticed - cross imports between mgr modules; we might need to address this before it turns into a big problem
Updated by Venky Shankar 10 months ago
- Status changed from Triaged to Fix Under Review
- Assignee changed from Dhairya Parmar to John Mulligan
- Target version set to v19.0.0
- Backport set to reef,quincy,pacific
- Pull request ID set to 52000