Change in vdsm[master]: storage: introducing vdsm-dump-chains script (part of vdsm-t...
nsoffer at redhat.com
nsoffer at redhat.com
Fri Mar 27 21:37:34 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: storage: introducing vdsm-dump-chains script (part of vdsm-tool)
......................................................................
Patch Set 9:
(8 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.
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 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 - chain error belong to an image.
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 format so we can easily see how this will be formatted.
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 ChainError, since we have all printing related code in one place.
Line 67:
Line 68:
Line 69: class DuplicateParentError(ChainError):
Line 70: """ More than one volume pointing to the same parent volume
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 other modules. Please check how other modules behave.
Line 123: sys.exit(1)
Line 124:
Line 125:
Line 126: def _parse_args():
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
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:
while True:
child_vol = volumes_by_parents.get(child_vol)
if child_vol is None:
break
if child_vol in chain:
raise ...
chain.append(child_vol)
I would also use shorter names:
while True:
child = parents.get(child)
if child is None:
break
if child in chain:
raise ...
chain.append(child)
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:
Normal output:
image: 8e285114-448e-4924-9531-f4ac1f38b93b
- fa6cb5b7-be42-45f5-994d-1129edb248b0
- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
- b7b9479d-027f-4d88-be7f-8677e8dc27b6
Verbose output:
image: 8e285114-448e-4924-9531-f4ac1f38b93b
- b7b9479d-027f-4d88-be7f-8677e8dc27b6
status: OK legality: LEGAL voltype: INTERNAL format: RAW type: SPARSE
- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE
image: fa6cb5b7-be42-45f5-994d-1129edb248b0
- fa6cb5b7-be42-45f5-994d-1129edb248b0
status: OK legality: LEGAL voltype: INTERNAL format: RAW type: SPARSE
- b7b9479d-027f-4d88-be7f-8677e8dc27b6
status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE
Error output:
image: 8e285114-448e-4924-9531-f4ac1f38b93b
Error description ...
Volumes and children:
- 00000000-0000-0000-0000-000000000000 <- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE
- b7b9479d-027f-4d88-be7f-8677e8dc27b6 <- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE
Looking at the output, I don't think we need the non-verbose mode. I would always like to see the attributes so I can detect erros such as incorrect voltype etc.
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
--
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: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan 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