Francesco Romani has posted comments on this change.
Change subject: vdsm: prepareVolumePath payload detection cleanup
......................................................................
Patch Set 4:
(13 comments)
....................................................
Commit Message
Line 3: AuthorDate: 2014-01-02 17:09:04 +0100
Line 4: Commit: Francesco Romani <from...@redhat.com>
Line 5: CommitDate: 2014-01-03 17:24:31 +0100
Line 6:
Line 7: vdsm: prepareVolumePath payload detection cleanup
Done. Shortened to fit in 50 characters.
Line 8:
Line 9: the prepareVolumePath code path for cdrom/floppy
Line 10: images is complicated due the fact the code has to
Line 11: deal with both regular and payload-created
....................................................
File tests/clientifTests.py
Line 24: from vm import VolumeError
Line 25: import clientIF
Line 26:
Line 27:
Line 28: class supervdsmMock:
Right, thanks for pointing it out. Hopefully one day the term will eventually
stick in my head.
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):
Done
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):
Done
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.
Better than expected! so the comment is now gone :)
Line 56: clientIF.supervdsm = supervdsmMock(self.cif)
Line 57:
Line 58: def testThisModuleAsDrive(self):
Line 59: volPath = self.cif.prepareVolumePath(__file__)
Line 69: self.assertEquals(volPath, f)
Line 70:
Line 71: def testBadDrive(self):
Line 72: with self.assertRaises(VolumeError):
Line 73: self.cif.prepareVolumePath('/this/should/not/exist')
I think you have a point. I'll change the code accordingly
However, in order to explain where this came from: this usage is advertised
even in the official docs:
http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises
I'm admittedly a big fan of context managers, but I have to acknowledge they
are starting to be abused sometimes.
Line 74:
Line 75: def testCDRomFromPayload(self):
Line 76: # bz1047356
Line 77: drive = {
Line 131: 'type': 'disk'
Line 132: }
Line 133: volPath = self.cif.prepareVolumePath(drive)
Line 134: self.assertEquals(volPath, cdrom)
Line 135: self.assertFalse(self.cif.builtCdrom)
Done
....................................................
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):
Done
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'}
I Like exception to be specific, so I'll got the VolumeError route until
something better pops out
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])
Done
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))
supervdsm.ProxyCaller wraps a multiprocessing' RemoteError into a RuntimeError
and then retries to ensure the connection.
Then I handled RuntimeError here and changed the message in the exception.
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:
Done
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:
Will remove the temporaries.
Yes, 'the' file attribute must be present if we end up here. It is supposed to
hold the list of (path, base64_encoded_content) actually represeinting the
payload.
So, It will be validate above in the beginning of the method.
'volId' is indeed optional, None is fine here and handled explicitely down in
mkimage.mkIsoFs (or mkFloppyFs)
As a sidenote, I find the singular in 'file' misleading here, but that's
already part of the API and I think there is little value in changing it.
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 <from...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Eduardo <ewars...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Martin Polednik <mpole...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
