mon/OSDMonitor.cc: fix expected_num_objects interpret error #19651
Conversation
src/mon/OSDMonitor.cc
Outdated
| @@ -10154,6 +10155,7 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, | |||
| } | |||
|
|
|||
| bool implicit_rule_creation = false; | |||
| int64_t expected_num_objects = int64_t(0); | |||
src/mon/OSDMonitor.cc
Outdated
| @@ -10193,6 +10195,10 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, | |||
| } | |||
| } else { | |||
| //NOTE:for replicated pool,cmd_map will put rule_name to erasure_code_profile field | |||
| // and put expected_num_objects to ruleset field | |||
| if (rule_name != "") { | |||
| expected_num_objects = std::atoll(rule_name.c_str()); | |||
There was a problem hiding this comment.
you should be able to get the value with cmd_getval().
There was a problem hiding this comment.
As 'rule' is stored as string, we can not get its value as a int64_t type with cmd_getval.
And rule already stored the string, so we only need to convert it to int64_t.
There was a problem hiding this comment.
good point. Please use strict_strtoll() as is used in other parts of OSDMonitor.cc.
|
@jecluis all done, please help to review |
|
ping @jecluis |
src/test/test_pool_create.sh
Outdated
| @@ -54,5 +54,27 @@ function TEST_pool_create() { | |||
| ceph osd crush rule rm $rulename | |||
| } | |||
|
|
|||
| function TEST_pool_create_rep_expected_num_objects() { | |||
There was a problem hiding this comment.
could you move this test to qa/standalone/mon/osd-pool-create.sh, so we can prevent "make check" from bloating over time.
|
@tchaikov done |
src/mon/OSDMonitor.cc
Outdated
| @@ -10207,8 +10218,10 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, | |||
| goto reply; | |||
| } | |||
|
|
|||
| int64_t expected_num_objects; | |||
| cmd_getval(cct, cmdmap, "expected_num_objects", expected_num_objects, int64_t(0)); | |||
| if (expected_num_objects == 0) { | |||
There was a problem hiding this comment.
do you think it would be better to move this block into the erasure branch, i.e. right after https://github.com/ceph/ceph/pull/19651/files#diff-0a5db46a44ae9900e226289a810f10e8L10193 ? it's more symmetric this way, IMHO.
|
@tchaikov done |
src/mon/OSDMonitor.cc
Outdated
| err = -EINVAL; | ||
| goto reply; | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't work. For example,
2018-01-12 03:30:18.208 7f08b542c700 1 -- 172.21.15.182:0/4125772450 --> 172.21.15.182:6789/0 -- mon_command({"format": "json", "rule": "by-host-smithi182", "pool_type": "replicated", "prefix": "osd pool create", "pg_num": "128", "erasure_code_profile": "by-host-smithi182", "pool": "by-host-smithi182"} v 0) v1 --
0x55b9e857ff80 con 0
2018-01-12 03:30:18.212 7f08bbf29700 1 -- 172.21.15.182:0/f5ea4aa2 <== mon.0 172.21.15.182:6789/0 11 ==== mon_command_ack([{"format": "json", "rule": "by-host-smithi182", "pool_type": "replicated", "prefix": "osd pool create", "pg_num": "128", "erasure_code_profile": "by-host-smithi182", "pool": "by-host-smithi182
"}]=ffffffea error parsing integer value 'by-host-smithi182': Expected option value to be integer, got 'by-host-smithi182' v10) v1 ==== 151+0+0 (883dec20 0 0) 0x55b9e857ff80 con 0x55b9e845a700
There was a problem hiding this comment.
@liewegas
I can reproduce your problem through 'ceph -c ceph.conf osd pool create by-host-smithi182 128 replicated by-host-smithi182 by-host-smithi182'.
The expected format of this command is 'ceph osd pool create {pool-name} {pg-num} [{pgp-num}] [replicated] [crush-rule-name] [expected-num-objects]'.
So the last 'by-host-smithi182' is parsed as 'expected-num-objects'.
All behaviours are expected.
There was a problem hiding this comment.
In this case the command isnt coming from the CLI, it's the localpool mgr module, and it's sending well-formed JSON. We need to make OSDMonitor handle a well-formed command.
(The weird positional CLI parsing is super-lame, but it is what we have right now. We should avoid breaking the JSON in order to fix the CLI...)
Fixes: http://tracker.ceph.com/issues/22530 Signed-off-by: Yang Honggang <joseph.yang@xtaotech.com>
|
@liewegas 'erasure_code_profile' can be used as a flag, to indicate whether cmd is well formed. |
|
@liewegas please help to review |
Fixes: http://tracker.ceph.com/issues/22530
Signed-off-by: Yang Honggang joseph.yang@xtaotech.com