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: introduce "tell x help" subcommand #15111

Merged
merged 6 commits into from May 27, 2017

Conversation

Projects
None yet
5 participants
@Liuchang0812
Contributor

Liuchang0812 commented May 16, 2017

Fixes: http://tracker.ceph.com/issues/19885

@tchaikov @jcsp please take a look. I enable get_command_descriptions command in MGR, and update relevant documentations.

Note that it is not necessary to address a particular mgr instance,
simply ``mgr`` will pick the current active daemon.
Use the ``help`` command to get a list of available commands from all
Use the ``descriptions`` command to get a list of available commands from all

This comment has been minimized.

@tchaikov

tchaikov May 16, 2017

Contributor

s/description/get_command_descriptions/

@@ -44,12 +44,12 @@ Calling module commands
Where a module implements command line hooks, using the Ceph CLI's
``tell`` command to call them like this::
ceph tell mgr <command | help>
ceph tell mgr <command | get_command_descriptions>

This comment has been minimized.

@tchaikov

tchaikov May 16, 2017

Contributor

i am not very sure if the json output of get_command_description is supposed to be consumed by human being...

This comment has been minimized.

@jcsp

jcsp May 16, 2017

Contributor

get_command_descriptions is indeed not meant for humans. It's what the CLI uses to resolve a series of positional arguments into a {"prefix":...} dict for use in commands.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 16, 2017

Contributor

yes, get_command_descriptions is not meant for humans. I spent a lot of time to learn it. I have a simple idea. how about adding a new command help and just print all available commands?

This comment has been minimized.

@jcsp

jcsp May 16, 2017

Contributor

That would be nice. The "normal" help command (for mon commands) is implemented in the CLI client (see function do_extended_help), we could have a similar thing for tell -- the client already has the command descriptions for tell, as we can see when it does the "no valid command found; 10 closest matches:" output, so implementing help should be simple.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 16, 2017

Contributor

I will try to do it. Here is the first step that mgr support get_command_description command. I prefer to create another PR, support ceph tell osd/mgr help via modify CLI client. I will revert last commit 2b816fa soon

@tchaikov tchaikov added the mgr label May 16, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 17, 2017

Hi, @tchaikov @jcsp @liewegas . As we discussed, I make a new command to CLI client that we could use ceph tell mon.1/osd.2/mds.a/mgr help to get command list. how about this? I will create a PR if it's ok.

➜  build git:(wip-19885-mgr-help) ✗ ./bin/ceph tell mgr help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
fs status {<fs>}                         Show the status of a CephFS filesystem
get_command_descriptions                 list commands
osd blocked-by                           print histogram of which OSDs are
                                          blocking their peers
osd perf                                 print dump of OSD perf summary stats
osd perf {<bucket>}                      Show the status of OSDs within a
                                          bucket, or all
osd pool stats {<name>}                  obtain stats from all pools, or from
                                          specified pool
osd reweight-by-pg {<int>} {<float>}     reweight OSDs by PG distribution
 {<int>} {<poolname> [<poolname>...]}     [overload-percentage-for-
                                          consideration, default 120]
osd reweight-by-utilization {<int>}      reweight OSDs by utilization [overload-
 {<float>} {<int>} {--no-increasing}      percentage-for-consideration, default
                                          120]

➜  build git:(wip-19885-mgr-help) ✗ ./bin/ceph tell osd.2 help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-05-17 13:23:54.293521 7f491b162700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-17 13:23:54.300451 7f491b162700 -1 WARNING: all dangerous and experimental features are enabled.
bench {<int>} {<int>} {<int>} {<int>}    OSD benchmark: write <count> <size>-
                                          byte objects, (default 1G size 4MB). 
                                          Results in log.
cluster_log error <message> [<message>.. log a message to the cluster log
 .]                                      
cpu_profiler status|flush                run cpu profiling on daemon
debug dump_missing <outfilename>         dump missing objects to a named file
debug kick_recovery_wq <int[0-]>         set osd_recovery_delay_start to <val>
dump_pg_recovery_stats                   dump pg recovery statistics
flush_pg_stats                           flush pg stats
get_command_descriptions                 list commands descriptions
heap dump|start_profiler|stop_profiler|  show heap usage info (available only 
 release|stats                            if compiled with tcmalloc)
injectargs <injected_args> [<injected_   inject configuration arguments into 
 args>...]                                running OSD
list_missing {<offset>}                  list missing objects on this pg, 
                                          perhaps starting at an offset given 
                                          in JSON
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
pg <pgid> list_missing {<offset>}        list missing objects on this pg, 
                                          perhaps starting at an offset given 
                                          in JSON
pg <pgid> mark_unfound_lost revert|      mark all unfound objects in this pg as 
 delete                                   lost, either removing or reverting to 
                                          a prior version if one is available
pg <pgid> query                          show details of a specific pg
query                                    show details of a specific pg
reset_pg_recovery_stats                  reset pg recovery statistics
version                                  report version of OSD

➜  build git:(wip-19885-mgr-help) ✗ ./bin/ceph tell mds.a help
cpu_profiler status|flush                run cpu profiling on daemon
damage ls                                List detected metadata damage
damage rm <int>                          Remove a damage table entry
exit                                     Terminate this MDS
heap dump|start_profiler|stop_profiler|  show heap usage info (available only 
 release|stats                            if compiled with tcmalloc)
injectargs <injected_args> [<injected_   inject configuration arguments into 
 args>...]                                running MDS
respawn                                  Restart this MDS
session evict {<filters> [<filters>...]} Evict client session(s)
session kill <int>                       End a client session
session ls {<filters> [<filters>...]}    List client sessions

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 17, 2017

@Liuchang0812 i think this looks great!

for (MgrCommand *cp = mgr_commands;
cp < &mgr_commands[ARRAY_SIZE(mgr_commands)]; cp++) {
ostringstream secname;
secname << "cmd" << setfill('0') << std::setw(3) << cmdnum;
dump_cmddesc_to_json(f, secname.str(), cp->cmdstring, cp->helpstring,
dump_cmddesc_to_json(&f, secname.str(), cp->cmdstring, cp->helpstring,

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

could you take this opportunity to use the range-based loop here? like

--- a/src/mgr/DaemonServer.cc
+++ b/src/mgr/DaemonServer.cc
@@ -492,17 +492,13 @@ bool DaemonServer::handle_command(MCommand *m)
                           "mgr", pyc.perm, "cli", 0);
       cmdnum++;
     }
-#if 0
-    for (MgrCommand *cp = mgr_commands;
-        cp < &mgr_commands[ARRAY_SIZE(mgr_commands)]; cp++) {
-
+    for (const auto &cp : mgr_commands) {
       ostringstream secname;
       secname << "cmd" << setfill('0') << std::setw(3) << cmdnum;
-      dump_cmddesc_to_json(f, secname.str(), cp->cmdstring, cp->helpstring,
-                          cp->module, cp->perm, cp->availability, 0);
+      dump_cmddesc_to_json(&f, secname.str(), cp.cmdstring, cp.helpstring,
+                          cp.module, cp.perm, cp.availability, 0);
       cmdnum++;
     }
-#endif
     f.close_section(); // command_descriptions
     f.flush(cmdctx->odata);
     cmdctx->reply(0, ss);

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

and can we have a simple test in qa/workunits/cephtool/test.sh?

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 17, 2017

Contributor

yes, of course. I prefer range-based looop.

@@ -5,6 +5,8 @@
// see MonCommands.h
COMMAND("get_command_descriptions", "list commands descriptions", "mgr", "r", "cli,rest")

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

why do we need this line?

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 17, 2017

Contributor

well, I saw that OSD supports this command. and, I want to let CLI client could run ceph tell mgr get_command_descriptions at first, CLI client will return -INVAL error if we don't add this line.

now, the question is whether we should support tell mgr get_command_descriptions in CLI client. IMO, we should support it so that senior user could get detail information about MGR commands.

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

yes, get_command_descriptions is not meant for humans.

thought we were on the same page?

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 17, 2017

Contributor

emm...keep it simple since we will have tell module help. we could add it if user need it.

@tchaikov tchaikov added the needs-test label May 17, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 17, 2017

@Liuchang0812 Liuchang0812 changed the title from mgr: support get_command_descriptions command to tool: support "tell help" in CLI client May 17, 2017

@Liuchang0812 Liuchang0812 changed the title from tool: support "tell help" in CLI client to tool: introduce "tell x help" command to CLI client May 17, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 17, 2017

Pass Jenkins test now. Any suggestion? thanks

ceph osd pool create fs_data 10
ceph osd pool create fs_metadata 10
ceph fs new $FS_NAME fs_metadata fs_data
wait_mds_active $FS_NAME

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

why would you need these pools for testing ceph tell?

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 17, 2017

Contributor

seens that i need to create a file system. i will try it without this

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 18, 2017

Contributor

hi, @tchaikov , I test it today. result shows that we need these pools. here is error log.

/home/jenkins-build/build/workspace/ceph-pull-requests/qa/workunits/cephtool/test.sh:2035: test_mds_tell_help_command:  ceph tell mds.a help
2017-05-18 06:49:38.874232 7efd58dd3700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-18 06:49:38.887392 7efd58dd3700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-18 06:49:38.922283 7efd6706e700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-18 06:49:38.922596 7efd6706e700 -1 asok(0x23b87d0) AdminSocketConfigObs::init: failed: AdminSocket::bind_and_listen: failed to bind the UNIX domain socket to '/home/jenkins-build/build/workspace/ceph-pull-requests/build/src/test/td/t-7200/out/client.admin.24768.asok': (17) File exists
2017-05-18 06:49:38.927826 7efd6706e700 -1 client.4313 MDS ID 'a' not found
2017-05-18 06:49:38.927829 7efd6706e700 -1 client.4313 FSMap: e91:
couldn't get command descriptions for [u'mds', u'a']: 
/home/jenkins-build/build/workspace/ceph-pull-requests/qa/workunits/cephtool/test.sh:1: test_mds_tell_help_command:  rm -fr /tmp/cephtool.yN1
src/ceph.in Outdated
return do_extended_help(parser, childargs, filter=True)
# implement "tell module help"
if childargs and len(childargs) >= 3 and childargs[0] == "tell" and childargs[2] == "help":

This comment has been minimized.

@tchaikov

tchaikov May 17, 2017

Contributor

if len(childargs) >= 3 implies if childargs

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 18, 2017

Contributor

it will breaks when childargs is None. It's better to do this check.

In [4]: a = None

In [5]: len(a)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-af8c77e09569> in <module>()
----> 1 len(a)

TypeError: object of type 'NoneType' has no len()

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

oic.

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

i don't think childargs can be None here.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 19, 2017

Contributor

Python's doc says that childargs will be [], not None.

I'll test it and update. but i prefer to take None check, it's more safe.

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

it's not "safe". IMO it's wrong.

src/ceph.in Outdated
if target == None:
target = ('mon', '')
elif isinstance(target, str) or isinstance(target, unicode):

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

should use basestring instead.

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

but i wonder under what circumstances, target is neither None nor str?

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 18, 2017

Contributor

it must be None or str now. but we may use it in future and pass a tuple/array. It will be more safe and make this function more generic.

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

we can always make it more generic when it is necessary.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 18, 2017

changelog:

  1. address @tchaikov 's comments
  2. update man document
  3. add test case in qa/workunits/testtool/test.sh
  4. update commit message
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 18, 2017

i will review the changes in ceph.in tomorrow.

src/ceph.in Outdated
@@ -287,7 +287,7 @@ daemonperf {type.id | path} list|ls [stat-pats] [priority]
""", file=sys.stdout)
def do_extended_help(parser, args):
def do_extended_help(parser, args, target=None, filter=True):

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

what does filter mean here? BTW, filter is a builtin method in python, might want to use some name less general.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 18, 2017

Contributor

do_extended_help was used in ceph --help originally, and ceph foo --help only shows commands which startwith foo. for example:

[root@node44 /data/seal/deploy]# ceph pg --help
/* some help messages */
pg debug unfound_objects_exist|degraded_ show debug info about pgs
 pgs_exist                               
pg deep-scrub <pgid>                     start deep-scrub on <pgid>
pg dump {all|summary|sum|delta|pools|    show human-readable versions of pg map 
 osds|pgs|pgs_brief [all|summary|sum|     (only 'all' valid with plain)
 delta|pools|osds|pgs|pgs_brief...]}     
pg dump_json {all|summary|sum|pools|     show human-readable version of pg map 
 osds|pgs [all|summary|sum|pools|osds|    in json only
 pgs...]}    

[root@node44 /data/seal/deploy]# ceph osd --help

osd blacklist add|rm <EntityAddr>        add (optionally until <expire> seconds 
 {<float[0.0-]>}                          from now) or remove <addr> from 
                                          blacklist
osd blacklist clear                      clear all blacklisted clients
osd blacklist ls                         show blacklisted clients
osd blocked-by                           print histogram of which OSDs are 
                                          blocking their peers
osd create {<uuid>} {<osdname (id|osd.   create new osd (with optional UUID and 
 id)>}                                    ID)
osd crush add <osdname (id|osd.id)>      add or update crushmap position and 
 <float[0.0-]> <args> [<args>...]         weight for <name> with <weight> and 
                                          location <args>
osd crush add-bucket <name> <type>       add no-parent (probably root) crush 
                                          bucket <name> of type <type>
osd crush create-or-move <osdname (id|   create entry or move existing entry 
 osd.id)> <float[0.0-]> <args> [<args>..  for <name> <weight> at/to location 
 .]                                       <args>
osd crush dump                           dump crush map                            

but we don't want this feature. filter is used to close this feature.

BTW, ceph osd --help's output is messy. you could try it.

src/ceph.in Outdated
help_for_sigs(outbuf.decode('utf-8'), partial)
return help_for_sigs(outbuf.decode('utf-8'), partial)
if filter is True:

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

why we want to check for True instead of:

if filter:

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 19, 2017

Contributor

done

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 19, 2017

i will review the changes in ceph.in tomorrow.

Hi, Kefu, have you reviewed it? any suggestion?

src/ceph.in Outdated
@@ -287,7 +287,7 @@ daemonperf {type.id | path} list|ls [stat-pats] [priority]
""", file=sys.stdout)
def do_extended_help(parser, args):
def do_extended_help(parser, args, target=None, filter=True):

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

instead of using the default parameters, i'd suggest expose the partial parameter to the caller. in other words, to define

def do_extended_help(parser, args, target, partial):
  # ...
  return help_for_target(target, partial)
# ... 

return do_extended_help(parser, childargs, ('mon', ''), ' '.join(childargs))
#...
return do_extended_help(parser, childargs, childargs[1].split('.', 1), None)

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 19, 2017

Contributor

OK, agree with you

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 19, 2017

@tchaikov updated, and I tested in my env. thank you very much.

src/ceph.in Outdated
@@ -849,7 +849,11 @@ def main():
return 1
if parsed_args.help:
return do_extended_help(parser, childargs)
return do_extended_help(parser, childargs, ("mon", ""), ' '.join(childargs))

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

please use single quote, to be more consistent with the rest of this source file.

src/ceph.in Outdated
if (cluster_handle.state == "connected"):
help_for_target(target=('mon', ''), partial=partial)
return help_for_target(target=target, partial=partial)

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

no need to use kwargs, the names of parameters are good enough.

src/ceph.in Outdated
return do_extended_help(parser, childargs, ("mon", ""), ' '.join(childargs))
# implement "tell module help"
if len(childargs) >= 3 and childargs[0] == "tell" and childargs[2] == "help":

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

ditto.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 19, 2017

updated. thanks.

function test_mds_tell_help_command()
{
FS_NAME=cephfs

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

nit, make it a local var. and move it down after this if block.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 22, 2017

Contributor

i have converted FS_NAME to local var

@tchaikov

modulo the nit, lgtm.

@tchaikov tchaikov added needs-qa and removed needs-test labels May 19, 2017

@tchaikov tchaikov changed the title from tool: introduce "tell x help" command to CLI client to ceph: introduce "tell x help" subcommand May 19, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 24, 2017

This failed a test in qa:

2017-05-23T21:18:57.180 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2197: :  test_mds_tell_help_command
2017-05-23T21:18:57.180 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2032: test_mds_tell_help_commandlocal FS_NAME=cephfs
2017-05-23T21:18:57.180 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2033: test_mds_tell_help_commandmds_exists
2017-05-23T21:18:57.181 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:749: mds_exists:  ceph auth list
2017-05-23T21:18:57.181 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:749: mds_exists:  grep '^mds'
2017-05-23T21:18:57.291 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:57.288853 7f3471188700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:57.338 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:57.336087 7f3471188700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:57.531 INFO:tasks.workunit.client.0.smithi032.stderr:installed auth entries:
2017-05-23T21:18:57.532 INFO:tasks.workunit.client.0.smithi032.stderr:
2017-05-23T21:18:57.543 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2034: test_mds_tell_help_commandecho 'Skipping test, no MDS found'
2017-05-23T21:18:57.543 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2035: test_mds_tell_help_commandreturn
2017-05-23T21:18:57.543 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2198: :  set +x
2017-05-23T21:18:57.544 INFO:tasks.workunit.client.0.smithi032.stdout:Skipping test, no MDS found
2017-05-23T21:18:57.626 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:57.624395 7fba1da8a700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:57.670 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:57.667728 7fba1da8a700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:57.937 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2197: :  test_mgr_tell_help_command
2017-05-23T21:18:57.937 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2055: test_mgr_tell_help_commandceph tell mgr help
2017-05-23T21:18:58.066 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:58.064264 7f1df7f3f700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:58.111 INFO:tasks.workunit.client.0.smithi032.stderr:2017-05-23 21:18:58.108686 7f1df7f3f700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-23T21:18:58.194 INFO:tasks.workunit.client.0.smithi032.stderr:couldn't get command descriptions for [u'mgr']:
2017-05-23T21:18:58.200 INFO:tasks.workunit.client.0.smithi032.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:1: test_mgr_tell_help_command:  rm -fr /tmp/cephtool.xsw
2017-05-23T21:18:58.205 INFO:tasks.workunit:Stopping ['cephtool'] on client.0...
2017-05-23T21:18:58.205 INFO:teuthology.orchestra.run.smithi032:Running: 'rm -rf -- /home/ubuntu/cephtest/workunits.list.client.0 /home/ubuntu/cephtest/clone.client.0'
2017-05-23T21:18:58.627 ERROR:teuthology.run_tasks:Saw exception from tasks.
Traceback (most recent call last):
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/run_tasks.py", line 86, in run_tasks
    manager = run_one_task(taskname, ctx=ctx, config=config)
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/run_tasks.py", line 65, in run_one_task
    return task(**kwargs)
  File "/home/teuthworker/src/github.com_ceph_ceph-c_wip-sage-testing2/qa/tasks/workunit.py", line 186, in task
    config.get('subdir'), timeout=timeout)
  File "/home/teuthworker/src/github.com_ceph_ceph-c_wip-sage-testing2/qa/tasks/workunit.py", line 334, in _spawn_on_all_clients
    timeout=timeout)
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/parallel.py", line 85, in __exit__
    for result in self:
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/parallel.py", line 99, in next
    resurrect_traceback(result)
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/parallel.py", line 22, in capture_traceback
    return func(*args, **kwargs)
  File "/home/teuthworker/src/github.com_ceph_ceph-c_wip-sage-testing2/qa/tasks/workunit.py", line 450, in _run_tests
    label="workunit test {workunit}".format(workunit=workunit)
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/orchestra/remote.py", line 193, in run
    r = self._runner(client=self.ssh, name=self.shortname, **kwargs)
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/orchestra/run.py", line 414, in run
    r.wait()
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/orchestra/run.py", line 149, in wait
    self._raise_for_status()
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/orchestra/run.py", line 171, in _raise_for_status
    node=self.hostname, label=self.label
CommandFailedError: Command failed (workunit test cephtool/test.sh) on smithi032 with status 1: 'mkdir -p -- /home/ubuntu/cephtest/mnt.0/client.0/tmp && cd -- /home/ubuntu/cephtest/mnt.0/client.0/tmp && CEPH_CLI_TEST_DUP_COMMAND=1 CEPH_REF=wip-sage-testing2 TESTDIR="/home/ubuntu/cephtest" CEPH_ARGS="--cluster ceph" CEPH_ID="0" PATH=$PATH:/usr/sbin CEPH_BASE=/home/ubuntu/cephtest/clone.client.0 adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 3h /home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh'

http://pulpito.ceph.com/sage-2017-05-23_19:52:01-rados-wip-sage-testing2---basic-smithi/1221487

@@ -2013,6 +2013,48 @@ function test_mon_cephdf_commands()
expect_false test $cal_raw_used_size != $raw_used_size
}
function test_mon_tell_help_command()
{
ceph tell mon.1 help

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 25, 2017

Contributor

ceph tell mon.a help here

@hequan

This comment has been minimized.

hequan commented May 25, 2017

@Liuchang0812

i have

$ ./bin/ceph tell osd help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-05-25 12:12:05.047464 7fd20e445700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-25 12:12:05.086759 7fd20e445700 -1 WARNING: all dangerous and experimental features are enabled.
Traceback (most recent call last):
  File "./bin/ceph", line 1062, in <module>
    retval = main()
  File "./bin/ceph", line 856, in main
    return do_extended_help(parser, childargs, childargs[1].split('.'), None)
  File "./bin/ceph", line 307, in do_extended_help
    return help_for_target(target, partial)
  File "./bin/ceph", line 298, in help_for_target
    timeout=10)
  File "/home/hequan/gsoc2017/wip-19885-mgr-help/ceph/src/pybind/ceph_argparse.py", line 1342, in json_command
    raise RuntimeError('"{0}": exception {1}'.format(argdict, e))
RuntimeError: "None": exception tuple index out of range

in my test (wip-19885-mgr-help ) just now.
something wrong?

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 25, 2017

@hequan could you print your ./bin/ceph line 305~310 here. I think your code isn't up to date. try to git fetch again?

ceph tell osd help                
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-05-25 12:44:22.391979 7f0ca1292700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-25 12:44:22.419799 7f0ca1292700 -1 WARNING: all dangerous and experimental features are enabled.
target osd doesn't exists, please pass correct target to tell command, such as mon.a/osd.1/mds.a/mgr.

another problem, you need to cmake .. after fetching code.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 25, 2017

jenkens test this please

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 25, 2017

Jenkins is down? I tested it in local.

The following tests passed:
        cephtool-test-mds.sh
        cephtool-test-mon.sh
        cephtool-test-osd.sh
        cephtool-test-rados.sh

100% tests passed, 0 tests failed out of 4
@hequan

This comment has been minimized.

hequan commented May 25, 2017

@Liuchang0812
yes, you are right, my code is not the latest.
the new test is

$ ./bin/ceph tell osd  help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-05-25 16:21:34.770810 7f746f702700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-25 16:21:34.809822 7f746f702700 -1 WARNING: all dangerous and experimental features are enabled.
target osd doesn't exists, please pass correct target to tell command, such as mon.a/osd.1/mds.a/mgr

it looks nice.

src/ceph.in Outdated
argdict={'format': 'json'})
if ret == -errno.EINVAL:
# try old mon
ret, outbuf, outs = send_command(cluster_handle,

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

i don't think we need to support server not supporting json commands, we use json to represent command parameters back to hammer.

src/ceph.in Outdated
print('WARN: the service id you provided does not exist. service id should '
'be one of {0}.'.format('/'.join(exist_ids)), file=sys.stderr)
elif len(_target) == 1 and _target[0] == "mgr":

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

['mon'] is also a valid target. please use single quote here also.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 25, 2017

Contributor
➜  build git:(wip-19885-mgr-help) ✗ ceph tell mon.a status

    cluster 0f638af9-bf50-414d-85d0-3f72a8664d91
     health HEALTH_OK
     monmap e2: 3 mons at {a=127.0.0.1:40000/0,b=127.0.0.1:40001/0,c=127.0.0.1:40002/0}
            election epoch 10, quorum 0,1,2 a,b,c
      fsmap e7: 1/1/1 up {0=a=up:active}, 2 up:standby
        mgr active: x 
     osdmap e11: 3 osds: 3 up, 3 in
      pgmap v22733: 24 pgs, 3 pools, 2246 bytes data, 21 objects
            170 GB used, 242 GB / 413 GB avail
                  24 active+clean
➜  build git:(wip-19885-mgr-help) ✗ ceph tell mon status  
error handling command target: CephName: no . in mon

I tested it in my PC, it seems that mon is not allowed in tell command. maybe we should only support mgr here. I'm pleasure to add mon when we have fixed the problem error handling command target: CephName: no . in mon. how about?

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

that's why i pasted the code snippet in #15111 (comment) for your consideration so we can support it.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 25, 2017

Contributor

thanks, appending empty string when target is mon is a simple solution, not a clean solution. IMO, it would be great if ceph pybind is compatible with all these target formats. (in fact, mgr has been supported. mon should not be difficult.)

I will modify my PR based on your comments tomorrow.

src/ceph.in Outdated
if service_id in exist_ids:
ret = True
if not ret and verbose:

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

we should always print out error messages when it's an error. following message is not for debugging purpose.

src/ceph.in Outdated
return False
if service_id in exist_ids:
ret = True

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

nit, please move the print('WARN ...) into the else clause.

Liuchang0812 added some commits May 16, 2017

tool/ceph: create "tell x help" command
Fixes: http://tracker.ceph.com/issues/19885

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
doc/tool: introduce "ceph tell x help" command in man
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
test/cephtool: convert FS_NAME to local variable
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
tool/ceph: validate target in tell subcommand
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
tool/ceph: remove old mon support and verbose support
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 26, 2017

➜  build git:(wip-19885-mgr-help) ✗ ceph tell mon help
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-05-26 14:41:13.734780 7f0f3bee2700 -1 WARNING: all dangerous and experimental features are enabled.
2017-05-26 14:41:13.764413 7f0f3bee2700 -1 WARNING: all dangerous and experimental features are enabled.
auth add <entity> {<caps> [<caps>...]}                                                                add auth info for <entity> from input file, or random key if no input is given, and/or any caps
                                                                                                       specified in the command
auth caps <entity> <caps> [<caps>...]                                                                 update caps for <name> from caps specified in the command
auth del <entity>                                                                                     delete all caps for <name>
auth export {<entity>}                                                                                write keyring for requested entity, or master keyring if none given
auth get <entity>                                                                                     write keyring file with requested key
auth get-key <entity>                                                                                 display requested key
auth get-or-create <entity> {<caps> [<caps>...]}                                                      add auth info for <entity> from input file, or random key if no input given, and/or any caps
                                                                                                       specified in the command

@tchaikov I have updated code and fixed your comments.

src/ceph.in Outdated
# implement "tell service.id help"
if len(childargs) >= 3 and childargs[0] == 'tell' and childargs[2] == 'help':
if validate_target(childargs[1]):
target = childargs[1].split('.')

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

please move target = childargs[1].split('.') out of the if block, and pass target to validate_target(), this would simplify the code a little bit.

src/ceph.in Outdated
return False
if service_id in exist_ids:
ret = True

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

could just do early return.

src/ceph.in Outdated
'be one of {0}.'.format('/'.join(exist_ids)), file=sys.stderr)
elif len(_target) == 1 and _target[0] in ['mgr', 'mon']:
ret = True

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

ditto.

tool/ceph: support target mon in tell help subcommand
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 26, 2017

fixed!

if len(target) == 1:
# TODO(Chang Liu): for "mon" target only, we should remove this
# patch after pybind supports this target format.
target.append("")

This comment has been minimized.

@tchaikov

tchaikov May 27, 2017

Contributor

you can also hide this fix in validate_target(), target is passed in by reference.

This comment has been minimized.

@Liuchang0812

Liuchang0812 May 27, 2017

Contributor

I considered this question, the function should be simple, and only one function: to determine whether target is legal. so i put this fix out of the function.

I will try to fix it via pybind later. how about?

This comment has been minimized.

@tchaikov

tchaikov May 27, 2017

Contributor

fair enough.

@tchaikov

aside from the nit, lgtm. but i am fine either way. feel free to merge it once you are ready.

@tchaikov tchaikov merged commit af97f16 into ceph:master May 27, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 27, 2017

\0/ thank you all for helping me.

@Liuchang0812 Liuchang0812 deleted the Liuchang0812:wip-19885-mgr-help branch May 27, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 31, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 31, 2017

I will try to fix this as soon as possible. I thought it may be an environment problem.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented May 31, 2017

That ceph.in prints this error message(couldn't get command descriptions) means that json_command is failed. We print out (target, outs) now, without ret code. I am not sure what the problem is with MGR.

2017-05-30T19:19:08.916 INFO:tasks.workunit.client.0.smithi180.stderr:couldn't get command descriptions for [u'mgr', '']:

## u'mgr' is target
## '' is `outs`

could I create a new PR that prints out json_command's return code? I could resolve this problem next time.

@tchaikov json_command's timeout is 10 seconds now. is this configuration appropriate?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 31, 2017

please go ahead.

@tchaikov json_command's timeout is 10 seconds now. is this configuration appropriate?

i would need more context on why it does not work for you. if this is what you meant.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Jun 1, 2017

I created a PR which prints more information of json_command #15378

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