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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Darshan N <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yaniv Dary <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
