Change in vdsm[master]: storage: introducing vdsm-dump-chains script
ibarkan at redhat.com
ibarkan at redhat.com
Wed Mar 4 09:24:21 UTC 2015
Ido Barkan has posted comments on this change.
Change subject: storage: introducing vdsm-dump-chains script
......................................................................
Patch Set 5:
(23 comments)
https://gerrit.ovirt.org/#/c/38281/5/client/vdsm_dump_chains.py
File client/vdsm_dump_chains.py:
Line 28: class ServerError(Exception):
Line 29: pass
Line 30:
Line 31:
Line 32: def _volume_dict_to_volume_chains(dct):
> assuming we only have a single volume without a parent (base) we can simpli
Done
Line 33:
Line 34: def _tails_indices(chains_):
Line 35: return dict(
Line 36: (chain[-1], chain_index)
Line 38:
> How about having a single function and passing index as an argument ?
I completely rewrote _volume_dict_to_volume_chains
Line 82: return res
Line 83:
Line 84:
Line 85: def _get_volumes_chains(sd_uuid, server):
Line 86: pools = _call_server(server.getConnectedStoragePoolsList)
> factor out, and add a comment about the single pool
Done
Line 87: sp_uuid, = pools['poollist']
Line 88: images = _call_server(server.getImagesList, sd_uuid)
Line 89:
Line 90: dct_image_volume_chains = {} # {image_uuid -> vol_chains}
Line 84:
Line 85: def _get_volumes_chains(sd_uuid, server):
Line 86: pools = _call_server(server.getConnectedStoragePoolsList)
Line 87: sp_uuid, = pools['poollist']
Line 88: images = _call_server(server.getImagesList, sd_uuid)
> maybe create a small wrapper for each api call to clean up this code
Done
Line 89:
Line 90: dct_image_volume_chains = {} # {image_uuid -> vol_chains}
Line 91: dct_volumes_info = {} # {vol_uuid-> vol_info}
Line 92: for img_uuid in images['imageslist']:
Line 88: images = _call_server(server.getImagesList, sd_uuid)
Line 89:
Line 90: dct_image_volume_chains = {} # {image_uuid -> vol_chains}
Line 91: dct_volumes_info = {} # {vol_uuid-> vol_info}
Line 92: for img_uuid in images['imageslist']:
> factor put this get
Done
Line 93: volumes = _call_server(
Line 94: server.getVolumesList, sd_uuid, sp_uuid, img_uuid)
Line 95:
Line 96: dct_volumes = {} # {parent_vol_uuid-> child_vol_uuid}
Line 92: for img_uuid in images['imageslist']:
Line 93: volumes = _call_server(
Line 94: server.getVolumesList, sd_uuid, sp_uuid, img_uuid)
Line 95:
Line 96: dct_volumes = {} # {parent_vol_uuid-> child_vol_uuid}
> volumes_children
Done
Line 97:
Line 98: for vol_uuid in volumes['uuidlist']:
Line 99: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 100: sp_uuid, img_uuid, vol_uuid)
Line 97:
Line 98: for vol_uuid in volumes['uuidlist']:
Line 99: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 100: sp_uuid, img_uuid, vol_uuid)
Line 101: if 'info' not in vol_info_answer:
> can this really happen?
no
Line 102: print 'ERROR getting info for volume %s:%s' % (
Line 103: vol_uuid, vol_info_answer)
Line 104: continue
Line 105: vol_info = vol_info_answer['info']
Line 98: for vol_uuid in volumes['uuidlist']:
Line 99: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 100: sp_uuid, img_uuid, vol_uuid)
Line 101: if 'info' not in vol_info_answer:
Line 102: print 'ERROR getting info for volume %s:%s' % (
> write to stderr
Done
Line 103: vol_uuid, vol_info_answer)
Line 104: continue
Line 105: vol_info = vol_info_answer['info']
Line 106: dct_volumes_info[vol_uuid] = vol_info
Line 105: vol_info = vol_info_answer['info']
Line 106: dct_volumes_info[vol_uuid] = vol_info
Line 107:
Line 108: parent_uuid = vol_info['parent']
Line 109: if vdsClient.BLANK_UUID == parent_uuid:
> define here or try to import from vdsm
Done
Line 110: dct_volumes[None] = vol_uuid
Line 111: else:
Line 112: dct_volumes[parent_uuid] = vol_uuid
Line 113:
Line 106: dct_volumes_info[vol_uuid] = vol_info
Line 107:
Line 108: parent_uuid = vol_info['parent']
Line 109: if vdsClient.BLANK_UUID == parent_uuid:
Line 110: dct_volumes[None] = vol_uuid
> use BLANK_UUID?
Done
Line 111: else:
Line 112: dct_volumes[parent_uuid] = vol_uuid
Line 113:
Line 114: dct_image_volume_chains[img_uuid] = _volume_dict_to_volume_chains(
Line 139: :
> if vol: must be sufficient
not really needed anymore, since I changed the implementation of _volume_dict_to_volume_chains so None cannot be included in a volume chain
Line 145: relevant_info if verbose else ''
Line 146: )
Line 147:
Line 148:
Line 149: if __name__ == '__main__':
> just call main() here and implement it at the top of the module.
Done
Line 150:
Line 151: usage = "usage: vdsm-dump-chains [options] <ip|0> <storage_domain_UUID>"
Line 152: parser = optparse.OptionParser(usage)
Line 153: parser.add_option("-v", "--verbose", action="store_true", dest="verbose",
Line 148:
Line 149: if __name__ == '__main__':
Line 150:
Line 151: usage = "usage: vdsm-dump-chains [options] <ip|0> <storage_domain_UUID>"
Line 152: parser = optparse.OptionParser(usage)
> localhost + ssl should be the default. add options to override it.
Done
Line 153: parser.add_option("-v", "--verbose", action="store_true", dest="verbose",
Line 154: help="show additional volume info")
Line 155: parser.add_option("-s", "--SSL", action="store_true", dest="use_ssl",
Line 156: help="use SSL")
Line 152: )
> its better to use argparse instead of optparse as this module is deprecated
yea but shouldn't we support rhel6+Python2.6?
Line 154: help="show additional volume info")
Line 155: parser.add_option("-s", "--SSL", action="store_true", dest="use_ssl",
Line 156: help="use SSL")
Line 157:
Line 158: (options, args) = parser.parse_args()
> remove (,)
Done
Line 159: if len(args) != 2:
Line 160: parser.error('incorrect number of arguments')
Line 161:
Line 162: ip, sd_uuid = args
Line 156: help="use SSL")
Line 157:
Line 158: (options, args) = parser.parse_args()
Line 159: if len(args) != 2:
Line 160: parser.error('incorrect number of arguments')
> show a more verbose error message (what should have been entered)
Done
Line 161:
Line 162: ip, sd_uuid = args
Line 163:
Line 164: host_port = vdscli.cannonizeHostPort(ip)
Line 160: parser.error('incorrect number of arguments')
Line 161:
Line 162: ip, sd_uuid = args
Line 163:
Line 164: host_port = vdscli.cannonizeHostPort(ip)
> additional parameter for port (and use it directly without requiring cannon
Done
Line 165: service = vdsClient.service()
Line 166: service.useSSL = bool(options.use_ssl)
Line 167:
Line 168: try:
Line 161:
Line 162: ip, sd_uuid = args
Line 163:
Line 164: host_port = vdscli.cannonizeHostPort(ip)
Line 165: service = vdsClient.service()
> use vdsm/lib/vdsm/vdscli.py directly instead of vdsClient
Done
Line 166: service.useSSL = bool(options.use_ssl)
Line 167:
Line 168: try:
Line 169: service.do_connect(host_port)
Line 167:
Line 168: try:
Line 169: service.do_connect(host_port)
Line 170: except socket.error as e:
Line 171: if e[0] == 111:
> use errno
Done
Line 172: print "Connection to %s refused" % host_port
Line 173: else:
Line 174: traceback.print_exc()
Line 175: sys.exit(-1)
Line 168: try:
Line 169: service.do_connect(host_port)
Line 170: except socket.error as e:
Line 171: if e[0] == 111:
Line 172: print "Connection to %s refused" % host_port
> write to stderr (maybe use a proper func for this) and exit
Done
Line 173: else:
Line 174: traceback.print_exc()
Line 175: sys.exit(-1)
Line 176:
Line 170: except socket.error as e:
Line 171: if e[0] == 111:
Line 172: print "Connection to %s refused" % host_port
Line 173: else:
Line 174: traceback.print_exc()
> just raise
Done
Line 175: sys.exit(-1)
Line 176:
Line 177: volume_chains, dct_volumes_info = _get_volumes_chains(sd_uuid, service.s)
Line 171: if e[0] == 111:
Line 172: print "Connection to %s refused" % host_port
Line 173: else:
Line 174: traceback.print_exc()
Line 175: sys.exit(-1)
> 1
Done
Line 176:
Line 177: volume_chains, dct_volumes_info = _get_volumes_chains(sd_uuid, service.s)
Line 173: else:
Line 174: traceback.print_exc()
Line 175: sys.exit(-1)
Line 176:
Line 177: volume_chains, dct_volumes_info = _get_volumes_chains(sd_uuid, service.s)
> there could be one, and only one chain per image- need to reflect it in dat
Done
--
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: 5
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: 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