2013/2/5 Stratos Psomadakis <[email protected]>:
> --- a/lib/bdev.py
> +++ b/lib/bdev.py
> @@ -38,11 +38,20 @@ from ganeti import objects
> from ganeti import compat
> from ganeti import netutils
> from ganeti import pathutils
> +from ganeti import serializer
>
> +from simplejson import JSONDecodeError
If you need to detect errors, please do so in serializer.py by raising
a custom exception from there (define it in errors.py). If
serializer.py were to change its implementation this code here
wouldn't work anymore as expected. You could name the exception class
“JsonDecodeError”.
> +class RbdShowmappedJsonError(Exception):
> + """`rbd showmmapped' json formatting error Exception class.
> +
> + """
> + def __init__(self):
> + Exception.__init__(self)
> + logging.warning("rbd json formatting failed, falling back to plain")
Don't log something like this from a constructor, please. Do it before
you raise the exception. Also, JSON is an acronym.
> + @classmethod
> + def _VolumeToBlockdev(cls, pool, volume_name):
> + try:
> + return cls._ParseRbdShowmappedJson(pool, volume_name)
> + except RbdShowmappedJsonError:
> + return cls._ParseRbdShowmappedPlain(pool, volume_name)
This is somewhat ugly. You're first trying to run the JSON version and
if that doesn't work the plain-text one. Can't you detect beforehand
if JSON is available?
> + @staticmethod
> + def _ParseRbdShowmappedJson(pool, volume_name):
> + # 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:
> + logging.error(result.output)
Please make the log entry more verbose. You can look at others for
logging failed commands.
> + raise RbdShowmappedJsonError
> […]
> + # Try parsing the new output format (ceph >= 0.55).
> + splitted_lines = map(lambda l: l.split(), lines)
> +
> + # Check for emptmy output.
s/emptmy/empty/
> 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 expected %s fields,"
> + " found %s", allfields, field_cnt)
Okay, can we please have unittests? There are just too many variations
and version dependencies.
Michael
