Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ceph.in: print all matched commands if arg missing #19547

Merged
merged 1 commit into from Jan 10, 2018

Conversation

Projects
None yet
5 participants
@tchaikov
Copy link
Contributor

commented Dec 15, 2017

so if only a single command matches, the "Invalid command:" error message is
printed, otherwise, the top 10 matched candidate commands are printed.

Fixes: http://tracker.ceph.com/issues/22344
Signed-off-by: Kefu Chai kchai@redhat.com

@joscollin
Copy link
Member

left a comment

@tchaikov

See:

$ ./bin/ceph tell osd.0 pg jospool.0 query
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-15 17:05:15.560 7ff8e7351700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-15 17:05:15.592 7ff8e7351700 -1 WARNING: all dangerous and experimental features are enabled.
no valid command found; 3 closest matches:
pg <pgid> list_missing {<offset>}
pg <pgid> query
pg <pgid> mark_unfound_lost revert|delete
Error EINVAL: invalid command

This fix messes with the above command. Here it should complain about jospool, which was happening before. But instead it displays the closest matches now, which is not accurate in this particular situation.

I have tried this fix initially and found this issue during testing and created a new except ArgumentMissing in #19399, so that this fix won't interfere with other outputs.

@joscollin

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@tchaikov @liewegas

Another issue that I found with this fix is: It is not printing missing required parameter now. Please see the below output. It prints no valid command found instead. But session is a valid command !

[jcollin@cephbuild build]$ ./bin/ceph tell mds.b session
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-19 11:38:44.777 7f25e508c700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-19 11:38:44.795 7f25e508c700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-19 11:38:44.829 7f25a27fc700  0 client.4166 ms_handle_reset on 127.0.0.1:6814/1100233223
no valid command found; 3 closest matches:
session evict {<filters> [<filters>...]}
session ls {<filters> [<filters>...]}
session kill <int>
Error EINVAL: invalid command
[jcollin@cephbuild build]$ cp ../../fix-by-joscollin/src/pybind/ceph_argparse.py ../src/pybind/
[jcollin@cephbuild build]$ ./bin/ceph tell mds.b session
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-19 11:40:07.730 7fca59370700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-19 11:40:07.755 7fca59370700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-19 11:40:07.804 7fca167fc700  0 client.4172 ms_handle_reset on 127.0.0.1:6814/1100233223
Invalid command: missing required parameter
3 closest matches:
session evict {<filters> [<filters>...]} :  Evict client session(s)
session ls {<filters> [<filters>...]} :  List client sessions
session kill <int> :  End a client session
Error EINVAL: invalid command
[jcollin@cephbuild build]$ 

@liewegas

This comment has been minimized.

Copy link
Member

commented Dec 29, 2017

@tchaikov ping

@tchaikov tchaikov force-pushed the tchaikov:wip-22344 branch from 7c2b97e to 24f9d43 Jan 2, 2018

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

But session is a valid command !

I don't think session is a valid command. session kill <int> is.

fixed and repushed

@tchaikov tchaikov force-pushed the tchaikov:wip-22344 branch from 24f9d43 to d61df34 Jan 2, 2018

@joscollin joscollin self-requested a review Jan 2, 2018

@tchaikov tchaikov force-pushed the tchaikov:wip-22344 branch from d61df34 to 0386c94 Jan 3, 2018

ceph.in: print all matched commands if arg missing
so if only a single command matches, the "Invalid command:" error
message is
printed, otherwise, the top 10 matched candidate commands are printed.

also, validate_command() now always returns `{}` if the command is not
valid. because we just the retval of it using
```py
if validate_command(...):
  # ...
```
and we do not differentiate `None` from `{}` here. and it'd be more
consistent and easier from the implementation perspective this way.

Fixes: http://tracker.ceph.com/issues/22344
Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-22344 branch from 0386c94 to ab076ed Jan 3, 2018

@tchaikov tchaikov requested a review from liewegas Jan 8, 2018

@liewegas
Copy link
Member

left a comment

looks ok to me, but i'm not very familiar with this code. @dmick?

@dmick

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Seems OK. I haven't tested myself.

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

@joscollin
Copy link
Member

left a comment

Many previous missing required parameter cases are showing no valid command found now. Is it okay ?

[jcollin@cephbuild build]$ ./bin/ceph tell osd.0 pg 1.0
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-01-10 17:15:44.188 7fd85c106700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:15:44.207 7fd85c106700 -1 WARNING: all dangerous and experimental features are enabled.
Invalid command: missing required parameter
3 closest matches:
pg <pgid> list_missing {<offset>} :  list missing objects on this pg, perhaps starting at an offset given in JSON
pg <pgid> query :  show details of a specific pg
pg <pgid> mark_unfound_lost revert|delete :  mark all unfound objects in this pg as lost, either removing or reverting to a prior version if one is available
Error EINVAL: invalid command

[jcollin@cephbuild build]$ cp ~/ceph_argparse.py.tchaikov ~/testbuild/src/pybind/ceph_argparse.py

[jcollin@cephbuild build]$ ./bin/ceph tell osd.0 pg
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-01-10 17:03:13.439 7fbd39b1b700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:03:13.456 7fbd39b1b700 -1 WARNING: all dangerous and experimental features are enabled.
no valid command found; 3 closest matches:
pg <pgid> list_missing {<offset>}
pg <pgid> query
pg <pgid> mark_unfound_lost revert|delete
Error EINVAL: invalid command

[jcollin@cephbuild build]$ ./bin/ceph tell mds.a session
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-01-10 17:04:02.026 7f2dd1330700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:04:02.038 7f2dd1330700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:04:02.065 7f2d8e7fc700  0 client.4270 ms_handle_reset on 127.0.0.1:6813/1585110491
no valid command found; 3 closest matches:
session evict {<filters> [<filters>...]}
session ls {<filters> [<filters>...]}
session kill <int>
Error EINVAL: invalid command

[jcollin@cephbuild build]$ ./bin/ceph tell osd.0 pg 1.0
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-01-10 17:04:26.337 7facc99f6700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:04:26.347 7facc99f6700 -1 WARNING: all dangerous and experimental features are enabled.
no valid command found; 3 closest matches:
pg <pgid> list_missing {<offset>}
pg <pgid> query
pg <pgid> mark_unfound_lost revert|delete
Error EINVAL: invalid command

This is a non related issue found during my testing. It doesn't like mds.* help. Any other text after mds.* is okay. What's wrong with help ?

[jcollin@cephbuild build]$ ./bin/ceph tell mds.a help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-01-10 17:13:10.702 7f2adfd92700 -1 WARNING: all dangerous and experimental features are enabled.
2018-01-10 17:13:10.717 7f2adfd92700 -1 WARNING: all dangerous and experimental features are enabled.
Traceback (most recent call last):
  File "./bin/ceph", line 1125, in <module>
    retval = main()
  File "./bin/ceph", line 931, in main
    if validate_target(target):
  File "./bin/ceph", line 224, in validate_target
    exist_ids = ids_by_service(service_name)
  File "./bin/ceph", line 208, in ids_by_service
    return ids[service]()
  File "./bin/ceph", line 184, in mdsids
    for info in fs['mdsmap']['info'].values():
AttributeError: 'list' object has no attribute 'values'

@yuriw yuriw merged commit 5197633 into ceph:master Jan 10, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

@joscollin i think it's fine. IMHO, it'd be hard for a user to understand the missing parameter is actually "list_missing {}" when "pg 1.0" is rejected. the new error message is better, because it explains that none of the commands is identified to be a match.

w.r.t. the "tell mds.a help", could you file a ticket?

@tchaikov tchaikov deleted the tchaikov:wip-22344 branch Jan 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.