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

ibarkan at redhat.com ibarkan at redhat.com
Mon Mar 30 08:49:51 UTC 2015


Ido Barkan has posted comments on this change.

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


Patch Set 9:

(17 comments)

https://gerrit.ovirt.org/#/c/38281/9/lib/vdsm/tool/vdsm_dump_chains.py
File lib/vdsm/tool/vdsm_dump_chains.py:

> This should be named dump_volume_chains.py - same name exposed in vdsm-tool
Done
Line 1: # Copyright 2015 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


Line 27: from vdsm import vdscli
Line 28: 
Line 29: # BLANK_UUID is re-declared here since it cannot be imported properly. this
Line 30: # constant should be introduced under lib publically available
Line 31: _BLANK_UUID = '00000000-0000-0000-0000-000000000000'
> move it to constants
the fact that BLANK_UUID is not already there is a crime. moving it there and using it from the whole code in many many other places belongs to a separate commit.
Line 32: _NAME = 'dump-volume-chains'
Line 33: _USAGE = "usage: %s [options] <sd_UUID>" % (_NAME,)
Line 34: 
Line 35: 


Line 54:     pass
Line 55: 
Line 56: 
Line 57: class ChainError(DumpChainsError):
Line 58:     def __init__(self, volumes_children):
> Add img_uuid='' argument and set self.img_uuid
Done
Line 59:         self.volumes_children = volumes_children
Line 60:         self.img_uuid = ''  # can be optionally set
Line 61: 
Line 62:     def __str__(self):


Line 56: 
Line 57: class ChainError(DumpChainsError):
Line 58:     def __init__(self, volumes_children):
Line 59:         self.volumes_children = volumes_children
Line 60:         self.img_uuid = ''  # can be optionally set
> I don't see where we use it, but I think this must be required argument - c
right, too many changes at once.
Line 61: 
Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])


Line 61: 
Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])
Line 65:         return 'Error in volume chain %s: %s\n\t\tvolumes and parents: %s' % (
> It is hard to understand messages like this. Please use tripple string form
Done
Line 66:             self.img_uuid, self.__class__.__name__, volumes)
Line 67: 
Line 68: 
Line 69: class DuplicateParentError(ChainError):


Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])
Line 65:         return 'Error in volume chain %s: %s\n\t\tvolumes and parents: %s' % (
Line 66:             self.img_uuid, self.__class__.__name__, volumes)
> It will be easier to understand if the printing code will format the ChainE
I don't agree. It is good that every exception can represent itself, and the other printing code is more concerned with printing the output as a whole (separation of concerns).
Line 67: 
Line 68: 
Line 69: class DuplicateParentError(ChainError):
Line 70:     """ More than one volume pointing to the same parent volume


Line 88:      e.g: (BLANK_UUID<-a), (a<-b), (c<-d)"""
Line 89: 
Line 90: 
Line 91: @expose(_NAME)
Line 92: def main(*args):
> Do we get args from vdsm-tool in this argument? If not, remove args.
we do. see vdsm/vdsm-tool/vdsm-tool:main
Line 93:     """
Line 94:     dump-volume-chains
Line 95:     Query VDSM about the existing structure of image volumes and prints
Line 96:     them in an ordered fashion with optional additional info per volume.


Line 88:      e.g: (BLANK_UUID<-a), (a<-b), (c<-d)"""
Line 89: 
Line 90: 
Line 91: @expose(_NAME)
Line 92: def main(*args):
> function name can be dump_volume_chains
Done
Line 93:     """
Line 94:     dump-volume-chains
Line 95:     Query VDSM about the existing structure of image volumes and prints
Line 96:     them in an ordered fashion with optional additional info per volume.


Line 93:     """
Line 94:     dump-volume-chains
Line 95:     Query VDSM about the existing structure of image volumes and prints
Line 96:     them in an ordered fashion with optional additional info per volume.
Line 97:     """
> Better try to connect and fail, less racy (assuming that this fails fast).
this is already implemented in _connect_to_server
Line 98:     try:
Line 99:         options, args = _parse_args()
Line 100:         sd_uuid, = args
Line 101:         server = _connect_to_server(options)


Line 95:     Query VDSM about the existing structure of image volumes and prints
Line 96:     them in an ordered fashion with optional additional info per volume.
Line 97:     """
Line 98:     try:
Line 99:         options, args = _parse_args()
> If we get args using *args, you should use them to parse the arguments, ins
Done
Line 100:         sd_uuid, = args
Line 101:         server = _connect_to_server(options)
Line 102:         image_chains, volumes_info = _get_volumes_chains(server, sd_uuid)
Line 103: 


Line 118:         raise
Line 119: 
Line 120: 
Line 121: def _fail(error_msg):
Line 122:     sys.stderr.write("vdsm-dump-chains: %s\n" % (error_msg,))
> This is now vdsm-tool dump-volume-chains - but I'm not sure that this match
all other modules just raise and let vdsm-tool handle this. Since vdsm-tool scripts cannot really run without it, I'll drop this function. It's a residue from when this script was a stand-alone.
Line 123:     sys.exit(1)
Line 124: 
Line 125: 
Line 126: def _parse_args():


Line 119: 
Line 120: 
Line 121: def _fail(error_msg):
Line 122:     sys.stderr.write("vdsm-dump-chains: %s\n" % (error_msg,))
Line 123:     sys.exit(1)
> vdsm tool doesn't return exit code - it throws exception or return with 0
Done
Line 124: 
Line 125: 
Line 126: def _parse_args():
Line 127:     parser = optparse.OptionParser(_USAGE)


Line 136:     args = sys.argv[2:] if 'vdsm-tool' in sys.argv[0] else sys.argv[1:]
Line 137:     options, args = parser.parse_args(args=args)
Line 138:     if len(args) != 1:
Line 139:         parser.error('sd_UUID is the only argument required.')
Line 140:     return options, args
> please use argparse instead. we use it everywhere under tool/
Done
Line 141: 
Line 142: 
Line 143: def _get_volume_info(server, vol_uuid, img_uuid, sd_uuid, sp_uuid):
Line 144:     res = _call_server(server.getVolumeInfo, sd_uuid, sp_uuid, img_uuid,


Line 184:     res = _call_server(server.getVolumesList, sd_uuid, sp_uuid, img_uuid)
Line 185:     return res['uuidlist']
Line 186: 
Line 187: 
Line 188: def _get_volume_chain(volumes_children):
> Should be _build_volume_chain
Done
Line 189:     volumes_by_parents = dict(volumes_children)
Line 190:     if len(volumes_by_parents) < len(volumes_children):
Line 191:         raise DuplicateParentError(volumes_children)
Line 192: 


Line 198:             raise ChainLoopError(volumes_children)
Line 199:         elif child_vol is None:
Line 200:             break  # end of chain
Line 201:         else:
Line 202:             chain.append(child_vol)
> Can be simplified a little bit:
agreed on dropping the elif/else clause. do not agree on naming. this is an algorithm that naming helps to understand.
Line 203: 
Line 204:     if not chain and volumes_by_parents:
Line 205:         raise NoBaseVolume(volumes_children)
Line 206: 


Line 222:     else:
Line 223:         return sp_uuid
Line 224: 
Line 225: 
Line 226: def _print_volume_chains(volume_chains, volumes_info, verbose=False):
> First lets decide what should be the output - what do you think about this:
agreed on the verbosity.
formatting: done
about formatting: we have already discussed the output. let's not change our mind every round. as this is a debugging tool, the output is already 'clear enough' for the average technical user.
Line 227:     print
Line 228:     print('Images volume chains (base volume first)')
Line 229:     for img_uuid, vol_chain in volume_chains.iteritems():
Line 230:         print


Line 236:             for vol in vol_chain:
Line 237:                 print('\t\t' + vol)
Line 238:                 if verbose:
Line 239:                     print("\t\tstatus: %(status)s, voltype: %(voltype)s, "
Line 240:                           "format: %(format)s, legality: %(legality)s, "
> use sys.stdout.write instead of print
can you explain why?
Line 241:                           "type: %(type)s" % (volumes_info[vol]))
Line 242: 
Line 243: 
Line 244: def _call_server(method, *args):


-- 
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: 9
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