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

nsoffer at redhat.com nsoffer at redhat.com
Mon Mar 9 09:22:24 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 7:

(39 comments)

I think we should pack this in vdsm-tool:

    vdsm-tool dump-images

So most of the packaging code is not needed.

The script itself is very nice but needs some more work.

https://gerrit.ovirt.org/#/c/38281/7//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: This script queries VDSM about the existing structure of image
Line 10: volumes and prints them in an ordered fashion with optional
Line 11: additional info per volume
Line 12: 
Showing here example output can be nice
Line 13: Change-Id: I428c443bb7d6b2a504a6f77efcd4838f7ae6c404


https://gerrit.ovirt.org/#/c/38281/7/client/vdsm-dump-chains.in
File client/vdsm-dump-chains.in:

Line 3: CLIENT="@VDSMDIR@/vdsm_dump_chains.py"
Line 4: [ -e "$CLIENT" ] || CLIENT="$CLIENT"c
Line 5: 
Line 6: [ -n "$PYTHONPATH" ] && PYTHON_FORMATTED_PATH=":$PYTHONPATH"
Line 7: PYTHONPATH="@VDSMDIR@$PYTHON_FORMATTED_PATH" python "$CLIENT" "$@"
I don't know why we need to start additional shell process for running /usr/bin/python, or duplicate logic for handling .py and .pyc files.

Should be replaced with:

    #!/usr/bin/python
    import sys
    sys.path.insert(0, "@VDSMDIR@")
    from storage.dump_chains import main
    main()

Assuming that we move this script to the storage package, which is the natural place if it is not part of vdsm-tool.


https://gerrit.ovirt.org/#/c/38281/7/client/vdsm_dump_chains.py
File client/vdsm_dump_chains.py:

Line 19: 
Line 20: import socket
Line 21: import sys
Line 22: import optparse
Line 23: import errno
Should be sorted

And separate from vdsm imports with one empty line.
Line 24: from vdsm import vdscli
Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this
Line 26: # constant should be introduced under lib publically available
Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000'


Line 20: import socket
Line 21: import sys
Line 22: import optparse
Line 23: import errno
Line 24: from vdsm import vdscli
Add empty line
Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this
Line 26: # constant should be introduced under lib publically available
Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000'
Line 28: 


Line 23: import errno
Line 24: from vdsm import vdscli
Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this
Line 26: # constant should be introduced under lib publically available
Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000'
If _USAGE is private, so is BLANK_UUID. Either make both private or both public.
Line 28: 
Line 29: _USAGE = "usage: vdsm-dump-chains [options] <sd_UUID>"
Line 30: 
Line 31: 


Line 30: 
Line 31: 
Line 32: class ServerError(Exception):
Line 33:     def __init__(self, server_result):
Line 34:         super(ServerError, self).__init__()
Not needed
Line 35:         self.err_code = server_result['status']['code']
Line 36:         self.msg = server_result['status']['message']
Line 37: 
Line 38:     def __str__(self):


Line 32: class ServerError(Exception):
Line 33:     def __init__(self, server_result):
Line 34:         super(ServerError, self).__init__()
Line 35:         self.err_code = server_result['status']['code']
Line 36:         self.msg = server_result['status']['message']
I would be nicer to use the same terms used in the server status:

    self.code = ...
    self.message = ...
Line 37: 
Line 38:     def __str__(self):
Line 39:         return 'server error. code: %s msg: %s' % (self.err_code, self.msg)
Line 40: 


Line 35:         self.err_code = server_result['status']['code']
Line 36:         self.msg = server_result['status']['message']
Line 37: 
Line 38:     def __str__(self):
Line 39:         return 'server error. code: %s msg: %s' % (self.err_code, self.msg)
Same, use "code" and "message"
Line 40: 
Line 41: 
Line 42: class ChainError(Exception):
Line 43:     def __init__(self, volumes_children):


Line 40: 
Line 41: 
Line 42: class ChainError(Exception):
Line 43:     def __init__(self, volumes_children):
Line 44:         super(ChainError, self).__init__()
Not needed
Line 45:         self.volumes_children = volumes_children
Line 46: 
Line 47:     def __str__(self):
Line 48:         return 'volumes and parents: ' + ' '.join(


Line 41: 
Line 42: class ChainError(Exception):
Line 43:     def __init__(self, volumes_children):
Line 44:         super(ChainError, self).__init__()
Line 45:         self.volumes_children = volumes_children
I wonder if we should call this "relations" or some other name related to parent-child pairs.
Line 46: 
Line 47:     def __str__(self):
Line 48:         return 'volumes and parents: ' + ' '.join(
Line 49:             ['%s<-%s' % (p, c) for p, c in self.volumes_children])


Line 45:         self.volumes_children = volumes_children
Line 46: 
Line 47:     def __str__(self):
Line 48:         return 'volumes and parents: ' + ' '.join(
Line 49:             ['%s<-%s' % (p, c) for p, c in self.volumes_children])
Lets use "parent" and "child" to make this easier to parse for humans.
Line 50: 
Line 51: 
Line 52: class DuplicateParentError(ChainError):
Line 53:     pass


Line 49:             ['%s<-%s' % (p, c) for p, c in self.volumes_children])
Line 50: 
Line 51: 
Line 52: class DuplicateParentError(ChainError):
Line 53:     pass
It would be nicer to replace "pass" with a docstring explaining the nature of this error.

For example, for this error we can write something like:

    class DuplicateParentError(ChainError):
        """ More than volume pointing to the same parent volume """

If needed, you can also add example showing the bad chain. This can be great documentation for someone not familiar with the possible issues.
Line 54: 
Line 55: 
Line 56: class NoBaseVolume(ChainError):
Line 57:     pass


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:

- Put everything in main under a try-except block
- Have one Error in this module, which all errors inherit from
- All expected errors should raise Error subclass
- Handle Error by writing the error and exit
- Any other error is unexpected and should not be handled, causing a traceback
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
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.
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:  "vdsm-dump-chains: %s\n", so when using chain of programs, it is clear what is the source of the error.
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 we don't need any of its features.
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 99:     parser.add_option('-u', '--unsecured', action='store_false',
Line 100:                       dest='use_ssl', default=True,
Line 101:                       help="use unsecured connection")
Line 102:     parser.add_option('-H', '--host', default=vdscli._ADDRESS)
Line 103:     parser.add_option('-p', '--port', default=vdscli._PORT)
Lets make vdscli._ADDRESS and vdscli._PORT public in another patch
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


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 _parse_args, when implemented with OptionParser.parse_args.
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, and nicer too.
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" as you did in the other helpers.
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:

    return res["info"]

As you did in the other helpers
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"?
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 really want to avoid this in a debugging tool.

We should log a very detailed warning here and continue.

Even if we do want to abort here, the proper way would be not handle the error here and let it bubble up to main and handled by the main error handler of this program.
Line 142: 
Line 143:     return image_chains, volumes_info
Line 144: 
Line 145: 


Line 141:             _write_error_and_exit(str(e) + ' img_uuid:%s' % (img_uuid,))
Line 142: 
Line 143:     return image_chains, volumes_info
Line 144: 
Line 145: 
I like to break the source to section, with a comment for each section.

For the helpers calling rpc method, I would create a section:

# XMLRPC helpers

and put under it all the helpers.
Line 146: def _get_all_images(server, sd_uuid):
Line 147:     res = _call_server(server.getImagesList, sd_uuid)
Line 148:     return res['imageslist']
Line 149: 


Line 181: def _call_server(method, *args):
Line 182:     res = method(*args)
Line 183:     if res['status']['code']:
Line 184:         raise ServerError(res)
Line 185:     return res
This should be the last helper as it is used by all others - I like to see code from high-level to low level, not the other way around.
Line 186: 
Line 187: 
Line 188: def _get_sp_uuid(server):
Line 189:     """there can be only one storage pool in a single VDSM context"""


Line 191:     sp_uuid, = pools['poollist']
Line 192:     return sp_uuid
Line 193: 
Line 194: 
Line 195: def _print_volume_chains(volume_chains, dct_volumes_info, verbose=False):
Please no Hungarian notation.  volumes_info is used everywhere else nicely.
Line 196:     print '\nimages volume chains (base volume first)'
Line 197:     for img_uuid, vol_chain in volume_chains.iteritems():
Line 198:         print 'image: %s' % (img_uuid,)
Line 199: 


Line 192:     return sp_uuid
Line 193: 
Line 194: 
Line 195: def _print_volume_chains(volume_chains, dct_volumes_info, verbose=False):
Line 196:     print '\nimages volume chains (base volume first)'
Why not:

    print
    print "images ..."

More clear than embedded "\n"

Also, this is a title, so it should be "Images ..."
Line 197:     for img_uuid, vol_chain in volume_chains.iteritems():
Line 198:         print 'image: %s' % (img_uuid,)
Line 199: 
Line 200:         if not vol_chain:


Line 194: 
Line 195: def _print_volume_chains(volume_chains, dct_volumes_info, verbose=False):
Line 196:     print '\nimages volume chains (base volume first)'
Line 197:     for img_uuid, vol_chain in volume_chains.iteritems():
Line 198:         print 'image: %s' % (img_uuid,)
I think that the "image: " prefix is not needed - we just wrote "Images ..." in the title - right?
Line 199: 
Line 200:         if not vol_chain:
Line 201:             print '\t\t- no volumes'
Line 202:             continue


Line 198:         print 'image: %s' % (img_uuid,)
Line 199: 
Line 200:         if not vol_chain:
Line 201:             print '\t\t- no volumes'
Line 202:             continue
I don't think this is possible, so we can just remove this check, and have an image without any volumes in this impossible case happen.
Line 203:         for vol in vol_chain:
Line 204:             print '\t\t' + vol
Line 205:             if verbose:
Line 206:                 print "\t\tstatus: %(status)s, voltype: %(voltype)s, " \


Line 200:         if not vol_chain:
Line 201:             print '\t\t- no volumes'
Line 202:             continue
Line 203:         for vol in vol_chain:
Line 204:             print '\t\t' + vol
2 tabs looks be too much, but I need to see the output to tell.
Line 205:             if verbose:
Line 206:                 print "\t\tstatus: %(status)s, voltype: %(voltype)s, " \
Line 207:                       "format: %(format)s, legality: %(legality)s, " \
Line 208:                       "type: %(type)s" % (dct_volumes_info[vol])


Line 204:             print '\t\t' + vol
Line 205:             if verbose:
Line 206:                 print "\t\tstatus: %(status)s, voltype: %(voltype)s, " \
Line 207:                       "format: %(format)s, legality: %(legality)s, " \
Line 208:                       "type: %(type)s" % (dct_volumes_info[vol])
The "(" ")" around the dict is not needed - this is not a one-item-tuple and not needed at all when using % dict

We need an empty line separating output for each image:

    image uuid
            base volume uuid
            move volumes...

    image uuid
Line 209: 
Line 210: 
Line 211: if __name__ == '__main__':


Line 205:             if verbose:
Line 206:                 print "\t\tstatus: %(status)s, voltype: %(voltype)s, " \
Line 207:                       "format: %(format)s, legality: %(legality)s, " \
Line 208:                       "type: %(type)s" % (dct_volumes_info[vol])
Line 209: 
Please show example output of this - either in the commit message or in some pastebin, so your reviewers can decide if they like the output *without* checking out this and trying it.
Line 210: 
Line 211: if __name__ == '__main__':


https://gerrit.ovirt.org/#/c/38281/7/debian/vdsm-dump-chains.docs
File debian/vdsm-dump-chains.docs:

Line 1: COPYING
This does not look useful to anyone.


https://gerrit.ovirt.org/#/c/38281/7/debian/vdsm-dump-chains.install
File debian/vdsm-dump-chains.install:

Line 1: ./usr/bin/vdsm-dump-chains
Line 2: ./usr/share/vdsm/vdsm_dump_chains.py
We should move it to vdsm/storage/dump_chains.py


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

Line 18: #
Line 19: 
Line 20: from testlib import VdsmTestCase as TestCaseBase
Line 21: from vdsm_dump_chains import _get_volume_chain, ChainError, BLANK_UUID, \
Line 22:     OrphanVolumes, ChainLoopError, NoBaseVolume, DuplicateParentError
Lets use import (name, ...)
Line 23: 
Line 24: 
Line 25: class VdsmDumpChainsTest(TestCaseBase):
Line 26:     def test_empty(self):


Line 21: from vdsm_dump_chains import _get_volume_chain, ChainError, BLANK_UUID, \
Line 22:     OrphanVolumes, ChainLoopError, NoBaseVolume, DuplicateParentError
Line 23: 
Line 24: 
Line 25: class VdsmDumpChainsTest(TestCaseBase):
This tests only _get_volume_chain, so lets call it GetVolumeChainTests
Line 26:     def test_empty(self):
Line 27:         self.assertEqual(_get_volume_chain([]), [])
Line 28: 
Line 29:     def test_only_base_volume(self):


Line 29:     def test_only_base_volume(self):
Line 30:         self.assertEqual(_get_volume_chain([(BLANK_UUID, 'a')]), ['a'])
Line 31: 
Line 32:     def test_orphan_volumes(self):
Line 33:         volumes = [(BLANK_UUID, 'a'), ('a', 'b'), ('c', 'd')]
Lets use the same term you use for this data in the code: "volumes_children"
Line 34:         with self.assertRaises(OrphanVolumes) as cm:
Line 35:             _get_volume_chain(volumes)
Line 36:         self.assertEqual(cm.exception.volumes_children, volumes)
Line 37: 


Line 36:         self.assertEqual(cm.exception.volumes_children, volumes)
Line 37: 
Line 38:     def test_simple_chain(self):
Line 39:         self.assertEqual(_get_volume_chain(
Line 40:             [(BLANK_UUID, 'a'), ('a', 'b'), ('b', 'c')]), ['a', 'b', 'c'])
Use a temporary to make this test more clear:

    volumes_children = [(BLANK_UUID, 'a'), ('a', 'b'), ('b', 'c')]
    self.assertEqual(_get_volume_chain(volumes_children), ['a', 'b', 'c'])

Same for all tests
Line 41: 
Line 42:     def test_loop(self):
Line 43:         with self.assertRaises(ChainLoopError):
Line 44:             _get_volume_chain([


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