Change in vdsm[master]: storage: introducing vdsm-dump-chains script
ibarkan at redhat.com
ibarkan at redhat.com
Sun Mar 22 16:01:58 UTC 2015
Ido Barkan has posted comments on this change.
Change subject: storage: introducing vdsm-dump-chains script
......................................................................
Patch Set 7: -Verified
(11 comments)
https://gerrit.ovirt.org/#/c/38281/7/client/vdsm_dump_chains.py
File client/vdsm_dump_chains.py:
Line 72:
Line 73: try:
Line 74: image_chains, volumes_info = _get_volumes_chains(server, sd_uuid)
Line 75: _print_volume_chains(image_chains, volumes_info, options.verbose)
Line 76: except ServerError as e:
> Why only ServerError? We should:
Done
Line 77: _write_error_and_exit(str(e))
Line 78:
Line 79:
Line 80: def _connect_to_server(options):
Line 73: try:
Line 74: image_chains, volumes_info = _get_volumes_chains(server, sd_uuid)
Line 75: _print_volume_chains(image_chains, volumes_info, options.verbose)
Line 76: except ServerError as e:
Line 77: _write_error_and_exit(str(e))
> _fail() would be nicer name
Done
Line 78:
Line 79:
Line 80: def _connect_to_server(options):
Line 81: host_port = "%s:%s" % (options.host, options.port)
Line 82: try:
Line 83: return vdscli.connect(host_port, options.use_ssl)
Line 84: except socket.error as e:
Line 85: if e[0] == errno.ECONNREFUSED:
Line 86: _write_error_and_exit("Connection to %s refused" % (host_port,))
> Raise Error here, let main handle it.
Done
Line 87: raise
Line 88:
Line 89:
Line 90: def _write_error_and_exit(error_msg):
Line 87: raise
Line 88:
Line 89:
Line 90: def _write_error_and_exit(error_msg):
Line 91: sys.stderr.write("%s\n" % (error_msg,))
> Messages from command line programs should have a program name prefix: "vd
Done
Line 92: sys.exit(1)
Line 93:
Line 94:
Line 95: def _parse_args():
Line 92: sys.exit(1)
Line 93:
Line 94:
Line 95: def _parse_args():
Line 96: parser = optparse.OptionParser(_USAGE)
> This is deprecated, but I don't think using argparse is worth the time as w
ack
Line 97: parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
Line 98: default=False, help="show additional volume info")
Line 99: parser.add_option('-u', '--unsecured', action='store_false',
Line 100: dest='use_ssl', default=True,
Line 103: parser.add_option('-p', '--port', default=vdscli._PORT)
Line 104: options, args = parser.parse_args()
Line 105: if len(args) != 1:
Line 106: parser.error('sd_UUID is the only argument required.')
Line 107: return args, options
> This should return options, args, since this is what people expect from _pa
Done
Line 108:
Line 109:
Line 110: def _get_vol_info(server, vol_uuid, img_uuid, sd_uuid, sp_uuid):
Line 111: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 106: parser.error('sd_UUID is the only argument required.')
Line 107: return args, options
Line 108:
Line 109:
Line 110: def _get_vol_info(server, vol_uuid, img_uuid, sd_uuid, sp_uuid):
> Rename to _get_volume_info() - consistent with other functions in your code
Done
Line 111: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 112: sp_uuid, img_uuid, vol_uuid)
Line 113: vol_info = vol_info_answer['info']
Line 114: return vol_info
Line 107: return args, options
Line 108:
Line 109:
Line 110: def _get_vol_info(server, vol_uuid, img_uuid, sd_uuid, sp_uuid):
Line 111: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
> I don't think we need this context here - vol_info_answer can be just "res"
Done
Line 112: sp_uuid, img_uuid, vol_uuid)
Line 113: vol_info = vol_info_answer['info']
Line 114: return vol_info
Line 115:
Line 110: def _get_vol_info(server, vol_uuid, img_uuid, sd_uuid, sp_uuid):
Line 111: vol_info_answer = _call_server(server.getVolumeInfo, sd_uuid,
Line 112: sp_uuid, img_uuid, vol_uuid)
Line 113: vol_info = vol_info_answer['info']
Line 114: return vol_info
> We don't need to "vol_info" - just return "info" from the response:
Done
Line 115:
Line 116:
Line 117: def _get_volumes_chains(server, sd_uuid):
Line 118: sp_uuid = _get_sp_uuid(server)
Line 124: for img_uuid in images_uuids:
Line 125: volumes = _get_volumes_for_image(server, img_uuid, sd_uuid, sp_uuid)
Line 126:
Line 127: # to avoid 'double parent' bug here we don't use a dictionary
Line 128: volumes_children = [] # [(parent_vol_uuid, child_vol_uuid),]
> How about "descendants"?
too long to my taste (also, children are _direct_ descendants, which is more accurate here).
Line 129:
Line 130: for vol_uuid in volumes:
Line 131: vol_info = _get_vol_info(server, vol_uuid, img_uuid, sd_uuid,
Line 132: sp_uuid)
Line 137:
Line 138: try:
Line 139: image_chains[img_uuid] = _get_volume_chain(volumes_children)
Line 140: except ChainError as e:
Line 141: _write_error_and_exit(str(e) + ' img_uuid:%s' % (img_uuid,))
> This means that one invalid chain prevent display of all other chains, we r
Ok, _get_volume_chain will return a list of errors that will be printed in addition to a possibly partial image_chains
Line 142:
Line 143: return image_chains, volumes_info
Line 144:
Line 145:
--
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: 7
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