On Wed, Jan 30, 2013 at 4:58 PM, Stratos Psomadakis <[email protected]> wrote:
> 'rbd showmapped' output formatting differs between older and newer versions of
> the ceph tools. Try to use json output formatting if available (currently
> available only in the ceph master branch). For bobtail, argonaut and older
> releases fallback to manually parsing the 'rbd showmapped' output, handling
> the
> differences in the output format for each rbd version correctly.
>
> Signed-off-by: Stratos Psomadakis <[email protected]>
> ---
> Hi,
>
> this patch addresses the comments made in the thread, regarding 'rbd
> showmapped' output handling. This patch makes Ganeti compatible with current
> (bobtail), older (<=argonaut) and future versions of the ceph tools. :)
>
> Apparently, pylint has some trouble inferring the type of the
> simplejson.loads() result, so, for the moment, I have disabled the check.
> I'm not sure if that's supposed to happen though.
>
> I also restructured the code a bit, to make it somewhat cleaner, after the
> changes in output handling.
>
> Thanks,
> Stratos
>
> lib/bdev.py | 90
> ++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 68 insertions(+), 22 deletions(-)
>
> diff --git a/lib/bdev.py b/lib/bdev.py
> index b221235..cbcc04e 100644
> --- a/lib/bdev.py
> +++ b/lib/bdev.py
> @@ -39,6 +39,7 @@ from ganeti import compat
> from ganeti import netutils
> from ganeti import pathutils
>
> +import simplejson
>
> # Size of reads in _CanReadDevice
> _DEVICE_READ_SIZE = 128 * 1024
> @@ -2726,13 +2727,8 @@ class RADOSBlockDevice(BlockDev):
> name = unique_id[1]
>
> # Check if the mapping already exists.
> - 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 = self._ParseRbdShowmappedOutput(result.output, name)
> + rbd_dev = self._VolumeToBlockdev(pool, name)
>
> if rbd_dev:
> # The mapping exists. Return it.
> @@ -2746,14 +2742,7 @@ class RADOSBlockDevice(BlockDev):
> result.fail_reason, result.output)
>
> # Find the corresponding rbd device.
> - showmap_cmd = [constants.RBD_CMD, "showmapped", "-p", pool]
> - result = utils.RunCmd(showmap_cmd)
> - if result.failed:
> - _ThrowError("rbd map succeeded, but showmapped failed (%s): %s",
> - result.fail_reason, result.output)
> -
> - rbd_dev = self._ParseRbdShowmappedOutput(result.output, name)
> -
> + rbd_dev = self._VolumeToBlockdev(pool, name)
> if not rbd_dev:
> _ThrowError("rbd map succeeded, but could not find the rbd block"
> " device in output of showmapped, for volume: %s", name)
> @@ -2762,6 +2751,59 @@ class RADOSBlockDevice(BlockDev):
> return rbd_dev
>
> @staticmethod
> + def _VolumeToBlockdev(pool, volume_name):
> + """Do the 'volume name'-to-'rbd block device' resolving.
> +
> + @type pool: string
> + @param pool: RADOS pool to use
> + @type volume_name: string
> + @param volume_name: the name of the volume whose device we search for
> +
Missing rtype and return
> + """
> + # 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(...)
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?
> +
> + @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?)
Thanks,
Guido
