Change in vdsm[master]: storage: introducing vdsm-dump-chains script (part of vdsm-t...

nsoffer at redhat.com nsoffer at redhat.com
Fri Mar 27 21:37:34 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storage: introducing vdsm-dump-chains script (part of vdsm-tool)
......................................................................


Patch Set 9:

(8 comments)

https://gerrit.ovirt.org/#/c/38281/9/lib/vdsm/tool/vdsm_dump_chains.py
File lib/vdsm/tool/vdsm_dump_chains.py:

This should be named dump_volume_chains.py - same name exposed in vdsm-tool.
Line 1: # Copyright 2015 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


Line 56: 
Line 57: class ChainError(DumpChainsError):
Line 58:     def __init__(self, volumes_children):
Line 59:         self.volumes_children = volumes_children
Line 60:         self.img_uuid = ''  # can be optionally set
I don't see where we use it, but I think this must be required argument - chain error belong to an image.
Line 61: 
Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])


Line 61: 
Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])
Line 65:         return 'Error in volume chain %s: %s\n\t\tvolumes and parents: %s' % (
It is hard to understand messages like this. Please use tripple string format so we can easily see how this will be formatted.
Line 66:             self.img_uuid, self.__class__.__name__, volumes)
Line 67: 
Line 68: 
Line 69: class DuplicateParentError(ChainError):


Line 62:     def __str__(self):
Line 63:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 64:                             self.volumes_children])
Line 65:         return 'Error in volume chain %s: %s\n\t\tvolumes and parents: %s' % (
Line 66:             self.img_uuid, self.__class__.__name__, volumes)
It will be easier to understand if the printing code will format the ChainError, since we have all printing related code in one place.
Line 67: 
Line 68: 
Line 69: class DuplicateParentError(ChainError):
Line 70:     """ More than one volume pointing to the same parent volume


Line 118:         raise
Line 119: 
Line 120: 
Line 121: def _fail(error_msg):
Line 122:     sys.stderr.write("vdsm-dump-chains: %s\n" % (error_msg,))
This is now vdsm-tool dump-volume-chains - but I'm not sure that this match other modules. Please check how other modules behave.
Line 123:     sys.exit(1)
Line 124: 
Line 125: 
Line 126: def _parse_args():


Line 184:     res = _call_server(server.getVolumesList, sd_uuid, sp_uuid, img_uuid)
Line 185:     return res['uuidlist']
Line 186: 
Line 187: 
Line 188: def _get_volume_chain(volumes_children):
Should be _build_volume_chain
Line 189:     volumes_by_parents = dict(volumes_children)
Line 190:     if len(volumes_by_parents) < len(volumes_children):
Line 191:         raise DuplicateParentError(volumes_children)
Line 192: 


Line 198:             raise ChainLoopError(volumes_children)
Line 199:         elif child_vol is None:
Line 200:             break  # end of chain
Line 201:         else:
Line 202:             chain.append(child_vol)
Can be simplified a little bit:

    while True:
        child_vol = volumes_by_parents.get(child_vol)
        if child_vol is None:
            break
        if child_vol in chain:
            raise ...
        chain.append(child_vol)

I would also use shorter names:

    while True:
        child = parents.get(child)
        if child is None:
            break
        if child in chain:
            raise ...
        chain.append(child)
Line 203: 
Line 204:     if not chain and volumes_by_parents:
Line 205:         raise NoBaseVolume(volumes_children)
Line 206: 


Line 222:     else:
Line 223:         return sp_uuid
Line 224: 
Line 225: 
Line 226: def _print_volume_chains(volume_chains, volumes_info, verbose=False):
First lets decide what should be the output - what do you think about this:

Normal output:

    image:   8e285114-448e-4924-9531-f4ac1f38b93b

             - fa6cb5b7-be42-45f5-994d-1129edb248b0

             - 06b512f6-ee6f-4c2c-b568-d40ff08fa482

             - b7b9479d-027f-4d88-be7f-8677e8dc27b6

Verbose output:

    image:   8e285114-448e-4924-9531-f4ac1f38b93b

             - b7b9479d-027f-4d88-be7f-8677e8dc27b6
               status: OK legality: LEGAL voltype: INTERNAL format: RAW type: SPARSE

             - 06b512f6-ee6f-4c2c-b568-d40ff08fa482
               status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE            

    image:   fa6cb5b7-be42-45f5-994d-1129edb248b0

             - fa6cb5b7-be42-45f5-994d-1129edb248b0
               status: OK legality: LEGAL voltype: INTERNAL format: RAW type: SPARSE

             - b7b9479d-027f-4d88-be7f-8677e8dc27b6
               status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE            


Error output:

    image:   8e285114-448e-4924-9531-f4ac1f38b93b

             Error description ...

             Volumes and children:

             - 00000000-0000-0000-0000-000000000000 <- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
               status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE            

             - b7b9479d-027f-4d88-be7f-8677e8dc27b6 <- 06b512f6-ee6f-4c2c-b568-d40ff08fa482
               status: OK legality: LEGAL voltype: INTERNAL format: COW type: SPARSE            

Looking at the output, I don't think we need the non-verbose mode. I would always like to see the attributes so I can detect erros such as incorrect voltype etc.
Line 227:     print
Line 228:     print('Images volume chains (base volume first)')
Line 229:     for img_uuid, vol_chain in volume_chains.iteritems():
Line 230:         print


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