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

Francesco Romani has uploaded a new change for review.

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

vdsm: prepareVolumePath payload detection cleanup

the prepareVolumePath code path for cdrom/floppy
images is complicated due the fact the code has to
deal with both regular and payload-created
images (vmPayload).

This lead to complicate and error-prone code paths,
which subsequent fixes made even more cumbersome.

This patch is a more aggressive and deep cleanup of
the code aiming to properly fix bz1047356.

The idea behind the patch is to simplify the detection
by factoring the conditions, changing the order
of the checks and factoring out the image creation
in the payload case.

This code proven to be tricky, so deep and extensive
verification is due.

Change-Id: I3a630d74ec0910c669e0326ad343c5dbea25357e
Bug-Url: https://bugzilla.redhat.com/1047356
Signed-off-by: Francesco Romani 
---
M vdsm/clientIF.py
1 file changed, 37 insertions(+), 30 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/28/22928/1

diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index 8e7f5f4..134b805 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -241,10 +241,30 @@
             self.log.info('Error finding path for device', exc_info=True)
             raise vm.VolumeError(uuid)

+    def _prepareVolumeFromPayload(self, vmId, drive, device, params):
+        '''
+        vmPayload is a key in params
+        'vmPayload': {'volId': 'volume id',   # volId is optional
+                      'file': {'filename': 'content', ...}}
+        '''
+        mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
+        try:
+            mkFsFunction = getattr(supervdsm.getProxy(),
+                                   mkFsNames[device])
+        except AttributeError:
+            raise vm.VolumeError("Unsupported 'device': %s in "
+                                 "drive: %s" % (device, drive))
+        else:
+            payload = params['vmPayload']
+            files = payload['file']
+            volId = payload.get('volId')
+            return mkFsFunction(vmId, files, volId)
+
     def prepareVolumePath(self, drive, vmId=None):
         if type(drive) is dict:
+            device = drive['device']
             # PDIV drive format
-            if drive['device'] == 'disk' and vm.isVdsmImage(drive):
+            if device == 'disk' and vm.isVdsmImage(drive):
                 res = self.irs.prepareImage(
                     drive['domainID'], drive['poolID'],
                     drive['imageID'], drive['volumeID'])
@@ -272,36 +292,23 @@
             elif "UUID" in drive:
                 volPath = self._getUUIDSpecPath(drive["UUID"])

-            # leave path == '' for empty cdrom and floppy drives ...
-            elif (drive['device'] in ('cdrom', 'floppy') and
-                  'specParams' in drive and
-                  # next line can be removed in future, when < 3.3 engine
-                  # is not supported
-                  drive['specParams'].get('path', '') == '' and
-                  drive.get('path', '') == '' and
-                  'vmPayload' not in drive['specParams']):
+            # cdrom and floppy drives
+            elif (device in ('cdrom', 'floppy') and 'specParams' in drive):
+                # vmPayload is a special case for those devices
+                # which needs to be handled first.
+                # caution: this code is trickier than it seems.
+                # see for example:
+                # https://bugzilla.redhat.com/shoaw_bug.cgi?id=1047356
+                params = drive['specParams']
+                if 'vmPayload' in params:
+                    volPath = self._prepareVolumeFromPayload(vmId, drive,
+                                                             device, params)
+                # leave path == '' for empty cdrom and floppy drives ...
+                elif (params.get('path', '') == '' and
+                      # line above can be removed in future, when < 3.3 engine
+                      # is not supported
+                      drive.get('path', '') == ''):
                     volPath = ''
-
-            # ... or load the drive from vmPayload:
-            elif drive['device'] in ('cdrom', 'floppy') and \
-                    'specParams' in drive and \
-                    'vmPayload' in drive['specParams']:
-                '''
-                vmPayload is a key in specParams
-                'vmPayload': {'volId': 'volume id',   # volId is optional
-                              'file': {'filename': 'content', ...}}
-                '''
-                mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
-                try:
-                    mkFsFunction = getattr(supervdsm.getProxy(),
-                                           mkFsNames[drive['device']])
-                except AttributeError:
-                    raise vm.VolumeError("Unsupported 'device': %s in "
-                                         "drive: %" % (drive['device'], drive))
-                else:
-                    files = drive['specParams']['vmPayload']['file']
-                    volId = drive['specParams']['vmPayload'].get('volId')
-                    volPath = mkFsFunction(vmId, files, volId)

elif "path" in drive:
                 volPath = drive['path']

Francesco Romani has posted comments on this change.

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

Patch Set 1:

better, long term fix of the same issue of http://gerrit.ovirt.org/#/c/22925/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6521/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5628/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6434/ : SUCCESS

Francesco Romani has posted comments on this change.

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

Patch Set 1: Code-Review-1

-1 because proper unit testing is still missing.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6531/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5638/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6444/ : SUCCESS

Francesco Romani has posted comments on this change.

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

Patch Set 3:

PatchSet 2: added unit tests, using just a bit of black magic.
PatchSet 3: removed redundant field in tests

now proceeding to the verification step.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6532/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5639/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6445/ : SUCCESS

Francesco Romani has posted comments on this change.

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

Patch Set 4:

PatchSet 4: there is no real need for globals.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6533/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5640/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6446/ : SUCCESS

[Nir Soffer]

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

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

Patch Set 4:

(3 comments)

Some more comments about the tests.

....................................................
Commit Message
Line 3: AuthorDate: 2014-01-02 17:09:04 +0100
Line 4: Commit:     Francesco Romani 
Line 5: CommitDate: 2014-01-03 17:24:31 +0100
Line 6: 
Line 7: vdsm: prepareVolumePath payload detection cleanup
Everything is vdsm here, use "clientIF:" prefix.
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 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')
Why not:

self.assertRaises(VolumeError, self.cif.prepareVolumePath,
                      '/this/should/not/exist')

This syntax may be supported, but it does not make sense. This is abusing of the context manager concept.
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)
Missing tests:

- drive without device key
- drive with unsupported device key
- no payload[file] key
- no palyad[volId] key

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 
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.assertRaise...

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

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 5:

Changes: addressed Nir comments.

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6579/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5686/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6492/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 5:

(5 comments)

Partial review, I'll get to it later.

....................................................
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, payload):
It would be nice to put private methods after the public method they serve. Puting methods/functions before code calling them is not required even in C (with proper prototypes). This makes it easier to read the code.
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 251:             raise vm.VolumeError("Missing 'file' attribute in 'payload'")
Line 252: 
Line 253:         mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
Line 254:         if device not in mkFsNames:
Line 255:             raise vm.VolumeError("Unsupported 'device': %s" % device)
An empty line after we raise makes the flow more clear - see raise in line 250.
Line 256:         try:
Line 257:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 258:                                    mkFsNames[device])
Line 259:         except RuntimeError:

Line 257:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 258:                                    mkFsNames[device])
Line 259:         except RuntimeError:
Line 260:             raise vm.VolumeError("Supervdsm call failed for %s in "
Line 261:                                  "drive: %s" % (device, drive))
Same
Line 262:         return mkFsFunction(vmId, payload['file'], payload.get('volId'))
Line 263: 
Line 264:     def prepareVolumePath(self, drive, vmId=None):
Line 265:         if type(drive) is dict:

Line 308:                 elif (params.get('path', '') == '' and
Line 309:                       # line above can be removed in future, when < 3.3 engine
Line 310:                       # is not supported
Line 311:                       drive.get('path', '') == ''):
Line 312:                     volPath = ''
Do you mean if path is empty or missing on both params and drive, set volPath to empty?
Line 313:                 else:
Line 314:                     volPath = drive.get('path', '')
Line 315: 
Line 316:             elif "path" in drive:

Line 310:                       # is not supported
Line 311:                       drive.get('path', '') == ''):
Line 312:                     volPath = ''
Line 313:                 else:
Line 314:                     volPath = drive.get('path', '')
It would be nice to get a nice comment about each condition, like the one about vmPayload. I don't have any idea why we have so many special cases here.
Line 315: 
Line 316:             elif "path" in drive:
Line 317:                 volPath = drive['path']
Line 318:

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 5:

(5 comments)

http://gerrit.ovirt.org/#/c/22928/5/vdsm/clientIF.py
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, payload):
> It would be nice to put private methods after the public method they serve.

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 251:             raise vm.VolumeError("Missing 'file' attribute in 'payload'")
Line 252: 
Line 253:         mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
Line 254:         if device not in mkFsNames:
Line 255:             raise vm.VolumeError("Unsupported 'device': %s" % device)
> An empty line after we raise makes the flow more clear - see raise in line

Done
Line 256:         try:
Line 257:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 258:                                    mkFsNames[device])
Line 259:         except RuntimeError:

Line 257:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 258:                                    mkFsNames[device])
Line 259:         except RuntimeError:
Line 260:             raise vm.VolumeError("Supervdsm call failed for %s in "
Line 261:                                  "drive: %s" % (device, drive))
> Same

Done
Line 262:         return mkFsFunction(vmId, payload['file'], payload.get('volId'))
Line 263: 
Line 264:     def prepareVolumePath(self, drive, vmId=None):
Line 265:         if type(drive) is dict:

Line 308:                 elif (params.get('path', '') == '' and
Line 309:                       # line above can be removed in future, when < 3.3 engine
Line 310:                       # is not supported
Line 311:                       drive.get('path', '') == ''):
Line 312:                     volPath = ''
> Do you mean if path is empty or missing on both params and drive, set volPa

Yes. This is a nasty case we need to support due to strange combination of parameters and multiple engine support. It's one of the dark corners I mentioned;
Line 313:                 else:
Line 314:                     volPath = drive.get('path', '')
Line 315: 
Line 316:             elif "path" in drive:

Line 310:                       # is not supported
Line 311:                       drive.get('path', '') == ''):
Line 312:                     volPath = ''
Line 313:                 else:
Line 314:                     volPath = drive.get('path', '')
> It would be nice to get a nice comment about each condition, like the one a

Good point. I think the code becomes more robust and cleaner if I just move above this line, and we use this value as default.
Line 315: 
Line 316:             elif "path" in drive:
Line 317:                 volPath = drive['path']
Line 318:

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

Patch set 6: address comments.

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6707/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6620/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5814/ : FAILURE

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

Jenkins failure is unrelated (and quite strange to me)

Fetching upstream changes from origin
Commencing build of Revision 6d7f196cdba3b76a6005b44fc710d55f9b7bd6d5 (master)
Checking out Revision 6d7f196cdba3b76a6005b44fc710d55f9b7bd6d5 (master)
FATAL: Could not checkout null with start point 6d7f196cdba3b76a6005b44fc710d55f9b7bd6d5
hudson.plugins.git.GitException: Could not checkout null with start point 6d7f196cdba3b76a6005b44fc710d55f9b7bd6d5
[...]
stderr: error: git checkout-index: unable to write file tests/functional/storageTests.py
error: git checkout-index: unable to write file tests/netinfoTests.py
error: git checkout-index: unable to write file tests/vmTestsData.py
error: git checkout-index: unable to write file vdsm/API.py
Note: checking out '6d7f196cdba3b76a6005b44fc710d55f9b7bd6d5'.

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6: -Verified

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6731/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6644/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5839/ : SUCCESS

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6: Verified+1

Tested using the same tests of: http://gerrit.ovirt.org/#/c/22925/
Marking as verified; I'm of course open to suggestions for any further test/verification.

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

Michal Skrivanek has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

(1 comment)

http://gerrit.ovirt.org/#/c/22928/6/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 284: shoaw_bug
typo "shoaw"

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

Antoni Segura Puimedon has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/22928/6/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 315:         self.log.info("prepared volume path: %s", volPath)
Line 316:         return volPath
Line 317: 
Line 318:     def _prepareVolumeFromPayload(self, vmId, drive, device, payload):
Line 319:         '''
s/'''/"""/
Line 320:         vmPayload is a key in params
Line 321:         'vmPayload': {'volId': 'volume id',   # volId is optional
Line 322:                       'file': {'filename': 'content', ...}}
Line 323:         '''

Line 330: 
Line 331:         try:
Line 332:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 333:                                    mkFsNames[device])
Line 334:         except RuntimeError:
Can you elaborate on the change of checking for RuntimeError (very generic) instead of AttributeError
Line 335:             raise vm.VolumeError("Supervdsm call failed for %s in "
Line 336:                                  "drive: %s" % (device, drive))
Line 337: 
Line 338:         return mkFsFunction(vmId, payload['file'], payload.get('volId'))

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

Michal Skrivanek has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

(1 comment)

http://gerrit.ovirt.org/#/c/22928/6/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 334: RuntimeError
can you please add this to commit msg that we're do extra check for the supervdsm call

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 7: Verified+1

Patch Set 7:
addressed reviewers comment. No code changes.

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 6:

(3 comments)

http://gerrit.ovirt.org/#/c/22928/6/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 284: shoaw_bug
> typo "shoaw"

Done (tnx for spotting this!)

Line 315:         self.log.info("prepared volume path: %s", volPath)
Line 316:         return volPath
Line 317: 
Line 318:     def _prepareVolumeFromPayload(self, vmId, drive, device, payload):
Line 319:         '''
> s/'''/"""/

Done
Line 320:         vmPayload is a key in params
Line 321:         'vmPayload': {'volId': 'volume id',   # volId is optional
Line 322:                       'file': {'filename': 'content', ...}}
Line 323:         '''

Line 330: 
Line 331:         try:
Line 332:             mkFsFunction = getattr(supervdsm.getProxy(),
Line 333:                                    mkFsNames[device])
Line 334:         except RuntimeError:
> Can you elaborate on the change of checking for RuntimeError (very generic)

Here I was trying to comply to Nir's comment in revision 4 (http://gerrit.ovirt.org/#/c/22928/4/vdsm/clientIF.py,cm)

For the sake of review, I'm quoting myself here:
"""
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.
"""

I'll add (an amended version of) the above in the code as comment to explain why I catch a such generic error.
Line 335:             raise vm.VolumeError("Supervdsm call failed for %s in "
Line 336:                                  "drive: %s" % (device, drive))
Line 337: 
Line 338:         return mkFsFunction(vmId, payload['file'], payload.get('volId'))

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6277/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7167/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7056/ : SUCCESS

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

Antoni Segura Puimedon has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 7: Code-Review+1

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

Martin Polednik has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 7: Code-Review+1

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 7:

- The new code looks clean and good
- The commit message is confusing, as this is a refactoring and it does not fix any bug - we can replace the Bug-Url with Relates-To, since the bug explains why the refactoring is needed.
- The test module is a great addition, but has many minor issues which make it harder to understand and use for adding more tests. I talked with Francesco about all the issues and hopefully he will address them in the next patch.
- An important test is missing, checking that we do reach the old code when a drive does not have a specParams key (the tests test only an empty specParams dict).

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8: Verified+1

Patch set 8:

* improved the commit message, which was indeed confused as Nir pointed out
* fixed minor style issues in tests
* made evident that tests do not need state (avoid references to self)
* added tests without specParams

no changes in the actual clientIF code, so marking as Verified again

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7285/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6386/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7170/ : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8:

(5 comments)

Tests are better now, but the fake objects can be more generic and easier to use when we add more tests, and the FakeClientIF class can be more robust.

http://gerrit.ovirt.org/#/c/22928/8/tests/clientifTests.py
File tests/clientifTests.py:

Line 34: 
Line 35: 
Line 36: class FakeSuperVdsm:
Line 37:     def __init__(self, FakeCIF=None):
Line 38:         self.FakeCIF = FakeCIF
We don't need clientIF here.

Instead we can keep a calls list:
    self.calls = []
Line 39: 
Line 40:     def getProxy(self):
Line 41:         return self
Line 42:

Line 40:     def getProxy(self):
Line 41:         return self
Line 42: 
Line 43:     def mkIsoFs(self, *args, **kwargs):
Line 44:         self.FakeCIF.builtCdrom = True
Instead of setting a flag on the clientIF, we can keep the call in the log:
    self.calls.append(('mkIsofs', args, kw))
Line 45:         return FAKE_ISOFS_PATH
Line 46: 
Line 47:     def mkFloppyFs(self, *args, **kwargs):
Line 48:         self.FakeCIF.builtFloppy = True

Line 49:         return FAKE_FLOPPY_PATH
Line 50: 
Line 51: 
Line 52: class FakeClientIF(clientIF.clientIF):
Line 53:     def __init__(self):
We can accept here the supervdsm:
    def __init__(self, supervdsm):
        self.supervdsm = supervdsm

And we don't need the builtFloppy and builtCdrom flags.

We do not want to modify clientIF, because it is the class under test - anything you add may break it and make the tests invalid. So it is much safer to put the test logic into the fake supervdsm, which is our tiny fake object.
Line 54:         self.irs = None  # just to make sure nothing ever happens
Line 55:         self.log = logging.getLogger('ClientIFTests')
Line 56:         self.builtFloppy = False
Line 57:         self.builtCdrom = False

Line 94: 
Line 95: 
Line 96: class ClientIFTests(TestCaseBase):
Line 97: 
Line 98:     def setUp(self):
We can create the fake super vdsm here:
    self.supervdsm = FakeSuperVdsm()

And create the clientif with the fake supervdsm:
    self.cif = FakeClientIF(self.supervdsm)
Line 99:         self.cif = FakeClientIF()
Line 100:         clientIF.supervdsm = FakeSuperVdsm(self.cif)
Line 101: 
Line 102:     def testNoneDrive(self):

Line 116:     def testCDRomFromPayload(self):
Line 117:         # bz1047356
Line 118:         volPath = self.cif.prepareVolumePath(fakePayloadDrive())
Line 119:         self.assertEquals(volPath, FAKE_ISOFS_PATH)
Line 120:         self.assertTrue(self.cif.builtCdrom)
We can check how we called supervdsm:

self.assertEquals(1, len(self.supervdsm.calls))

We can check that we called the correct function:

funcname, args, kwargs = self.supervdsm.calls[0]
       self.assertEquals(funcname, "mkIsofs")

And if needed, we can check the arguments:

self.assertEquals(args, ('foo', 'bar', ...))
Line 121: 
Line 122:     def testNoPayloadFileKey(self):
Line 123:         payloadDrive = fakePayloadDrive()
Line 124:         del payloadDrive['specParams']['vmPayload']['file']

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8:

(1 comment)

Another test that would be nice to have.

http://gerrit.ovirt.org/#/c/22928/8/tests/clientifTests.py
File tests/clientifTests.py:

Line 175:         drive = fakeDrive()
Line 176:         drive['device'] = 'tape'
Line 177:         # fallback case: we must use the top-level key 'path'
Line 178:         volPath = self.cif.prepareVolumePath(drive)
Line 179:         self.assertEquals(volPath, ISOFS_PATH)
Another interesting test would be to an error where supervdsm fails with a RuntimeError (see http://gerrit.ovirt.org/#/c/22928/8/vdsm/clientIF.py line 339).

To simulate that we can replace the fake supervdsm method that should be called with one that raises an exception:

def fail():
        raise RuntimeError(...)
    self.cif.mkIsoFs = fail

# Now try input that should call mkIsoFs and ensure that 
    # it raise VolumeError.

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8:

Finally, I think it would be best to separate this to two patches. First a patch that add the tests, working with the old code. Then, a patch that refactor the code. This will make sure that the code changes that look nice and clean are actually are.

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8:

(6 comments)

Followed Nir's advice and split the tests in a separate change here:

http://gerrit.ovirt.org/#/c/24614/

in order to have easier review, cleaner history and less risk of unwanted changes/regressions.

I'll amend this change to make it dependent from the above one, and to let it include just the refactoring.

http://gerrit.ovirt.org/#/c/22928/8/tests/clientifTests.py
File tests/clientifTests.py:

Line 34: 
Line 35: 
Line 36: class FakeSuperVdsm:
Line 37:     def __init__(self, FakeCIF=None):
Line 38:         self.FakeCIF = FakeCIF
> We don't need clientIF here.

Done
Line 39: 
Line 40:     def getProxy(self):
Line 41:         return self
Line 42:

Line 40:     def getProxy(self):
Line 41:         return self
Line 42: 
Line 43:     def mkIsoFs(self, *args, **kwargs):
Line 44:         self.FakeCIF.builtCdrom = True
> Instead of setting a flag on the clientIF, we can keep the call in the log:

Yes, this way is better. Done.
Line 45:         return FAKE_ISOFS_PATH
Line 46: 
Line 47:     def mkFloppyFs(self, *args, **kwargs):
Line 48:         self.FakeCIF.builtFloppy = True

Line 49:         return FAKE_FLOPPY_PATH
Line 50: 
Line 51: 
Line 52: class FakeClientIF(clientIF.clientIF):
Line 53:     def __init__(self):
> We can accept here the supervdsm:

I 100% agree about the idea, however in this particular case there are some complications which led me to write the code as it is; please see the comments for line 98 below.
Line 54:         self.irs = None  # just to make sure nothing ever happens
Line 55:         self.log = logging.getLogger('ClientIFTests')
Line 56:         self.builtFloppy = False
Line 57:         self.builtCdrom = False

Line 94: 
Line 95: 
Line 96: class ClientIFTests(TestCaseBase):
Line 97: 
Line 98:     def setUp(self):
> We can create the fake super vdsm here:

The problem here is supervdsm uses a global variable to implement a singleton; so we need to override the supervdsm.getProxy() function.
Unfortunately I don't see a clean way to do it without touching clientIF, even though we all know it is better to avoid that. In the new revision I further reduced the changes. Let's continue from there, please.
Line 99:         self.cif = FakeClientIF()
Line 100:         clientIF.supervdsm = FakeSuperVdsm(self.cif)
Line 101: 
Line 102:     def testNoneDrive(self):

Line 116:     def testCDRomFromPayload(self):
Line 117:         # bz1047356
Line 118:         volPath = self.cif.prepareVolumePath(fakePayloadDrive())
Line 119:         self.assertEquals(volPath, FAKE_ISOFS_PATH)
Line 120:         self.assertTrue(self.cif.builtCdrom)
> We can check how we called supervdsm:

Good idea, done; Also extracted a couple of helper methods.
Line 121: 
Line 122:     def testNoPayloadFileKey(self):
Line 123:         payloadDrive = fakePayloadDrive()
Line 124:         del payloadDrive['specParams']['vmPayload']['file']

Line 175:         drive = fakeDrive()
Line 176:         drive['device'] = 'tape'
Line 177:         # fallback case: we must use the top-level key 'path'
Line 178:         volPath = self.cif.prepareVolumePath(drive)
Line 179:         self.assertEquals(volPath, ISOFS_PATH)
> Another interesting test would be to an error where supervdsm fails with a

Good catch. Added.

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

Francesco Romani has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Patch Set 8: -Verified

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

Francesco Romani has abandoned this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................

Abandoned

Patch fully splitted. refactoring: http://gerrit.ovirt.org/24636
tests: http://gerrit.ovirt.org/#/c/24614/