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