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