Change in vdsm[master]: storage: introducing vdsm-dump-chains script (part of vdsm-t...

nsoffer at redhat.com nsoffer at redhat.com
Wed Apr 1 20:35:49 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storage: introducing vdsm-dump-chains script (part of vdsm-tool)
......................................................................


Patch Set 13:

(5 comments)

https://gerrit.ovirt.org/#/c/38281/13/lib/vdsm/tool/dump_volume_chains.py
File lib/vdsm/tool/dump_volume_chains.py:

Line 57:         self.volumes_children = volumes_children
Line 58: 
Line 59: 
Line 60: class DuplicateParentError(ChainError):
Line 61:     desc = "More than one volume pointing to the same parent volume e.g: " \
Nice idea, but we can keep the docstring, and use err.__doc__ when printing.

If you want to make it clear that this is a description of the error and is meant for the user and not the programmer, why not call it "description"?
Line 62:            "(_BLANK_UUID<-a), (a<-b), (a<-c)"
Line 63: 
Line 64: 
Line 65: class NoBaseVolume(ChainError):


Line 63: 
Line 64: 
Line 65: class NoBaseVolume(ChainError):
Line 66:     desc = "no volume with a parent volume Id _BLANK_UUID found e.g: " \
Line 67:            "(a<-b), (b<-c)"""
You left """ at the end - also in rest of the errors.

Please avoid "\" and use triple quote for multiline strings like these.
Line 68: 
Line 69: 
Line 70: class ChainLoopError(ChainError):
Line 71:     desc = "A loop found in the volume chain. This happens if a volume " \


Line 207:                "legality: {legality}, type: {type}"
Line 208:     try:
Line 209:         formatted_info = info_fmt.format(**volumes_info[child])
Line 210:     except KeyError:
Line 211:         formatted_info = 'no volume data found'
It will be sad to see this instead of partial volume info. A better way would be show existing values, and a default for missing value.

We can wrap volume info dict with defaultdict:

    >>> d = defaultdict(lambda: "(missing)", {"status": "OK"})
    >>> "status: {d[status]}, volType: {d[volType]}".format(d=d)
    'status: OK, volType: (missing)'
Line 212:     print(_LINE_FORMAT.format('', '  ' + formatted_info))
Line 213: 
Line 214: 
Line 215: def _print_volume_chains(volume_chains, volumes_info):


Line 222:         if isinstance(vol_chain, ChainError):
Line 223:             chain_err = vol_chain
Line 224:             print(_LINE_FORMAT.format('', chain_err.desc))
Line 225:             print(_LINE_FORMAT.format('', 'Volumes and children:'))
Line 226:             print
You are mixing print() and print, which is not python 3 friendly. Lets have only print().

Or avoid this issue by using sys.stdout.write as Yaniv suggested.
Line 227:             for parent, child in chain_err.volumes_children:
Line 228:                 print(_LINE_FORMAT.format('', '- %s <- %s' % (parent, child)))
Line 229:                 _print_vol_info(child, volumes_info)
Line 230:                 print


Line 226:             print
Line 227:             for parent, child in chain_err.volumes_children:
Line 228:                 print(_LINE_FORMAT.format('', '- %s <- %s' % (parent, child)))
Line 229:                 _print_vol_info(child, volumes_info)
Line 230:                 print
We repeat this too much:

   print(_LINE_FORMT.format("", something))

There is only one line that actually use the first argument to format().

We need a small helper that print a line, with optional label:

    def print_line(body, title=''):
        print(_LINE_FORMT.format(title, body))

So code can look like this:

    print_line(img_uuid, label="image:")
    print_line(chain_err.desc)
    print_line('Volumes and children:')
    for parent, child in chain_err.voliumes_children:
        print_line("%s <- %s".format(parrent, child))
Line 231:         else:
Line 232:             for vol in vol_chain:
Line 233:                 print(_LINE_FORMAT.format('', '- ' + vol))
Line 234:                 _print_vol_info(vol, volumes_info)


-- 
To view, visit https://gerrit.ovirt.org/38281
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I428c443bb7d6b2a504a6f77efcd4838f7ae6c404
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vladik Romanovsky <vladik.romanovsky at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yaniv Dary <ydary at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list