Hi, On 01/30/2013 07:07 PM, Guido Trotter wrote: <snip> > On Wed, Jan 30, 2013 at 4:58 PM, Stratos Psomadakis <[email protected]> wrote: >> + """ >> + # Newer versions of the rbd tool support json output formatting. Use it >> if >> + # available. >> + showmap_cmd = [ >> + constants.RBD_CMD, >> + "showmapped", "-p", pool, "--format=json" >> + ] >> + result = utils.RunCmd(showmap_cmd) >> + if result.failed: >> + # For older versions of rbd, we have to parse the output manually. >> + showmap_cmd = [constants.RBD_CMD, "showmapped", "-p", pool] >> + result = utils.RunCmd(showmap_cmd) >> + if result.failed: >> + _ThrowError("rbd showmapped failed (%s): %s", >> + result.fail_reason, result.output) >> + >> + rbd_dev = RADOSBlockDevice._ParseRbdShowmappedOutput(result.output, >> + volume_name) >> + return rbd_dev >> + >> + >> + try: >> + devices = simplejson.loads(result.output) >> + except simplejson.JSONDecodeError: >> + _ThrowError("Invalid json input %s!\n", result.output) >> + >> + rbd_dev = None >> + # pylint: disable=E1103 >> + for d in devices.itervalues(): >> + if "name" not in d: >> + _ThrowError("'name' key missing from json object %s!\n", >> + devices) >> + >> + if d["name"] == volume_name: >> + if rbd_dev is not None: >> + _ThrowError("The rbd volume %s is mapped more than once." >> + " This shouldn't happen, try to unmap the extra" >> + " devices manually.", volume_name) >> + >> + rbd_dev = d["device"] >> + >> + return rbd_dev >> + > This is actually quite confusing. Could you change it so that it's clearer? > Perhaps split it into two functions and then do something like: > > try: > return_ShowMapJson(...) > except Unsupported...: > return _ShowMapBare(...)
ok, I'll do that.
> Also, would there be a way to check for json support without calling it?
> (eg. in --help?). This is because if json is supported but errors out,
> falling back is not the right thing to do, right?
Probably yes. Although, if showmapped errors out, and it's not due to
not supporting the json formatting, falling back isn't really an issue
(we retry and it will either succeed, or error out again and finally
throw an error). If that's not acceptable, there are 2 alternatives:
1) Parse the output of --help, and 'grep' for "json" or "image-format"
("format" exists in previous ceph releases to specify the image
format, and got renamed after the json formatting patch). However, this
can easily break, if something changes in the --help output (unlikely, I
guess).
2) Parse the output of --version, and compare it with the known ceph
releases when the output format changed. The problem could be a problem
with manual ceph builds. Besides, we don't know yet in which release the
json formatting patch is going to be included.
What do you think?
>> +
>> + @staticmethod
>> def _ParseRbdShowmappedOutput(output, volume_name):
>> """Parse the output of `rbd showmapped'.
>>
>> @@ -2781,21 +2823,25 @@ class RADOSBlockDevice(BlockDev):
>> volumefield = 2
>> devicefield = 4
>>
>> - field_sep = "\t"
>> -
>> lines = output.splitlines()
>> - splitted_lines = map(lambda l: l.split(field_sep), lines)
>>
>> - # Check empty output.
>> + # Try parsing the new output format (ceph >= 0.55).
>> + splitted_lines = map(lambda l: l.split(), lines)
>> +
>> + # Check for empty output.
>> if not splitted_lines:
>> - _ThrowError("rbd showmapped returned empty output")
>> + return None
>>
>> # Check showmapped header line, to determine number of fields.
>> field_cnt = len(splitted_lines[0])
>> if field_cnt != allfields:
>> - _ThrowError("Cannot parse rbd showmapped output because its format"
>> - " seems to have changed; expected %s fields, found %s",
>> - allfields, field_cnt)
>> + # Parsing the new format failed. Fallback to parsing the old output
>> + # format (< 0.55).
>> + splitted_lines = map(lambda l: l.split('\t'), lines)
>> + if field_cnt != allfields:
>> + _ThrowError("Cannot parse rbd showmapped output because its format"
>> + " seems to have changed; expected %s fields, found %s",
>> + allfields, field_cnt)
> How about we submit just this part as a separate patch for devel-2.7?
> (or should it be both for that version?)
If possible, both for devel-2.7.
> Thanks,
>
> Guido
Thanks,
Stratos
--
Stratos Psomadakis
<[email protected]>
signature.asc
Description: OpenPGP digital signature
