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