Change in vdsm[master]: storage: introducing vdsm-dump-chains script

ibarkan at redhat.com ibarkan at redhat.com
Tue Mar 3 16:12:31 UTC 2015


Ido Barkan has posted comments on this change.

Change subject: storage: introducing vdsm-dump-chains script
......................................................................


Patch Set 5: Code-Review-1

(22 comments)

code review with nsoffer

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 simplify this by just probing for the next child, until the dictionary is exhausted. if there is a child that is not present in the dictionary- shout.
Line 33: 
Line 34:     def _tails_indices(chains_):
Line 35:         return dict(
Line 36:             (chain[-1], chain_index)


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
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
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
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
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?
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
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
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?
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 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.
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.
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 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 (,)
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)
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 cannonizeHostPort)
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
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
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
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
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
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 data structure.

also non need for hungarian notation


https://gerrit.ovirt.org/#/c/38281/5/tests/vdsmDumpChainsTests.py
File tests/vdsmDumpChainsTests.py:

Line 22: 
Line 23: from vdsm_dump_chains import _volume_dict_to_volume_chains
Line 24: 
Line 25: 
Line 26: class vdsmDumpChainsTest(TestCaseBase):
also test the base where there is no parent (parent is BLANK_UUID)
Line 27:     def test_single_chain(self):
Line 28:         d = {'a': 'b', 'b': None}
Line 29:         self.assertEqual(_volume_dict_to_volume_chains(d), [['a', 'b', None]])
Line 30: 


Line 28:         d = {'a': 'b', 'b': None}
Line 29:         self.assertEqual(_volume_dict_to_volume_chains(d), [['a', 'b', None]])
Line 30: 
Line 31:     def test_two_disconnected_chains(self):
Line 32:         d = {'a': 'b', 'c': 'd', 'b': None, 'd': None}
there is no such case (to chains)
Line 33:         self.assertEqual(
Line 34:             _volume_dict_to_volume_chains(d).sort(),
Line 35:             [['a', 'b', None], ['c', 'd', None]].sort())
Line 36: 


-- 
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 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