Change in vdsm[master]: vdsm: prepareVolumePath payload detection cleanup

nsoffer at redhat.com nsoffer at redhat.com
Fri Jan 3 17:20:56 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: vdsm: prepareVolumePath payload detection cleanup
......................................................................


Patch Set 4:

(10 comments)

The new method is clean and not tricky, and the tests are  simple and clean. I looked only very briefly at the tests and I'll do another pass later.

See the comments for possible cleanups and simplification.

....................................................
File tests/clientifTests.py
Line 24: from vm import VolumeError
Line 25: import clientIF
Line 26: 
Line 27: 
Line 28: class supervdsmMock:
This is not a mock but a fake - supervdsmFake.

See http://martinfowler.com/articles/mocksArentStubs.html
Line 29:     def __init__(self, cifMock=None):
Line 30:         self.cifMock = cifMock
Line 31: 
Line 32:     def getProxy(self):


Line 40:         setattr(self.cifMock, 'builtFloppy', True)
Line 41:         return 'floppy'
Line 42: 
Line 43: 
Line 44: class clientIFMock(clientIF.clientIF):
Again, this is a fake, not a mock.
Line 45:     def __init__(self):
Line 46:         self.irs = None  # just to make sure nothing ever happens
Line 47:         self.log = logging.getLogger('clientIFTest')
Line 48:         self.builtFloppy = False


Line 48:         self.builtFloppy = False
Line 49:         self.builtCdrom = False
Line 50: 
Line 51: 
Line 52: class clientIFTests(TestCaseBase):
Whitspace between class and next method make it more readable.
Line 53:     def setUp(self):
Line 54:         self.cif = clientIFMock()
Line 55:         # this is a necessary evil.
Line 56:         clientIF.supervdsm = supervdsmMock(self.cif)


Line 51: 
Line 52: class clientIFTests(TestCaseBase):
Line 53:     def setUp(self):
Line 54:         self.cif = clientIFMock()
Line 55:         # this is a necessary evil.
Nothing evil here :-)
Line 56:         clientIF.supervdsm = supervdsmMock(self.cif)
Line 57: 
Line 58:     def testThisModuleAsDrive(self):
Line 59:         volPath = self.cif.prepareVolumePath(__file__)


....................................................
File vdsm/clientIF.py
Line 240:         except blkid.BlockIdException:
Line 241:             self.log.info('Error finding path for device', exc_info=True)
Line 242:             raise vm.VolumeError(uuid)
Line 243: 
Line 244:     def _prepareVolumeFromPayload(self, vmId, drive, device, params):
Lets accept payload instead of params, since the caller already know it exists.
Line 245:         '''
Line 246:         vmPayload is a key in params
Line 247:         'vmPayload': {'volId': 'volume id',   # volId is optional
Line 248:                       'file': {'filename': 'content', ...}}


Line 246:         vmPayload is a key in params
Line 247:         'vmPayload': {'volId': 'volume id',   # volId is optional
Line 248:                       'file': {'filename': 'content', ...}}
Line 249:         '''
Line 250:         mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
It is little ugly that we have to dispatch 'cdrom' and 'floppy' here - it would be much nicer if supervdsmServer was doing this dispatching.

Lets first make sure that device is valid - if not, lets raise ValueError, or better a more specific VolumeError:

    if 'device' not in mkFsNames:
        raise ValueError(device)  # VolumeError?

This fix the unhandled KeyError in line 253.
Line 251:         try:
Line 252:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 253:                                    mkFsNames[device])
Line 254:         except AttributeError:


Line 249:         '''
Line 250:         mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
Line 251:         try:
Line 252:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 253:                                    mkFsNames[device])
Can raise KeyError. We don't want to handle it here since this is a rather expensive call to another process, and we dont want to mix KeyError raised from supervdsm.getProxy() with KeyError from mkFsNames[device].
Line 254:         except AttributeError:
Line 255:             raise vm.VolumeError("Unsupported 'device': %s in "
Line 256:                                  "drive: %s" % (device, drive))
Line 257:         else:


Line 252:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 253:                                    mkFsNames[device])
Line 254:         except AttributeError:
Line 255:             raise vm.VolumeError("Unsupported 'device': %s in "
Line 256:                                  "drive: %s" % (device, drive))
Since now we use one of the function names defined by supervdsmServer, checking AttributeError is not needed now - if this code fails, the error is not unsupported device but some bug in supervdsmServer.

Check what errors other code raise when trying to get supervdsm proxy method fails.
Line 257:         else:
Line 258:             payload = params['vmPayload']
Line 259:             files = payload['file']
Line 260:             volId = payload.get('volId')


Line 253:                                    mkFsNames[device])
Line 254:         except AttributeError:
Line 255:             raise vm.VolumeError("Unsupported 'device': %s in "
Line 256:                                  "drive: %s" % (device, drive))
Line 257:         else:
The else is not needed, as we just raised. This pattern is needed when there are two code paths, that merge after the else.
Line 258:             payload = params['vmPayload']
Line 259:             files = payload['file']
Line 260:             volId = payload.get('volId')
Line 261:             return mkFsFunction(vmId, files, volId)


Line 258:             payload = params['vmPayload']
Line 259:             files = payload['file']
Line 260:             volId = payload.get('volId')
Line 261:             return mkFsFunction(vmId, files, volId)
Line 262: 
The temporary variables are not needed (assuming we got a payload parameter):

    return mkFsFunction(vmId, payload['file'], payload.get('volId'))

I assume from your code that 'file' is required parameter and you want to raise a KeyError here if missing, and 'volId' is optional parameter, and you want to silently ignore it if missing. Is this right?

Maybe payload should be validated at the top before doing anything else.
Line 263:     def prepareVolumePath(self, drive, vmId=None):
Line 264:         if type(drive) is dict:
Line 265:             device = drive['device']
Line 266:             # PDIV drive format


-- 
To view, visit http://gerrit.ovirt.org/22928
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a630d74ec0910c669e0326ad343c5dbea25357e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list