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

fromani at redhat.com fromani at redhat.com
Tue Feb 18 11:16:51 UTC 2014


Francesco Romani has uploaded a new change for review.

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

clientIF: prepareVolumePath payload 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, used by cloud-init).

The code has been patched various times, and in the end the
code became more complex than is needed.

This patch refactors the existing code in order to simplify
it, without changes of behaviour.

Change-Id: I058206b7506ddbb5ec087c9ea0963a10ed57affb
Relates-To: http://gerrit.ovirt.org/22928/
Signed-off-by: Francesco Romani <fromani at redhat.com>
---
M vdsm/clientIF.py
1 file changed, 47 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/36/24636/1

diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index f8fe499..8ffc495 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -243,8 +243,9 @@
 
     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'])
@@ -277,37 +278,27 @@
             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):
+                # this is the easy case, and a sane default.
+                # Now things can get messy due to strange combinations.
+                volPath = drive.get('path', '')
+                # vmPayload is a special case for those devices
+                # which needs to be handled first.
+                # caution: this code is tricky. See for example:
+                # https://bugzilla.redhat.com/show_bug.cgi?id=1047356
+                params = drive['specParams']
+                if 'vmPayload' in params:
+                    payload = params['vmPayload']  # shortcut
+                    volPath = self._prepareVolumeFromPayload(vmId, drive,
+                                                             device, payload)
+                # better explicit than implicit, thus 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: %s" %
-                        (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']
@@ -329,6 +320,31 @@
         self.log.info("prepared volume path: %s", volPath)
         return volPath
 
+    def _prepareVolumeFromPayload(self, vmId, drive, device, payload):
+        """
+        vmPayload is a key in params
+        'vmPayload': {'volId': 'volume id',   # volId is optional
+                      'file': {'filename': 'content', ...}}
+        """
+        if 'file' not in payload:
+            raise vm.VolumeError("Missing 'file' attribute in 'payload'")
+
+        mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
+        if device not in mkFsNames:
+            raise vm.VolumeError("Unsupported 'device': %s" % device)
+
+        try:
+            mkFsFunction = getattr(supervdsm.getProxy(),
+                                   mkFsNames[device])
+        except RuntimeError:
+            # Here we're dealing with supervdsm proxy method failures.
+            # This is so generic because supervdsm.ProxyCaller wraps a
+            # multiprocessing' RemoteError into a RuntimeError.
+            raise vm.VolumeError("Supervdsm call failed for %s in "
+                                 "drive: %s" % (device, drive))
+
+        return mkFsFunction(vmId, payload['file'], payload.get('volId'))
+
     def teardownVolumePath(self, drive):
         res = {'status': doneCode}
         try:


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I058206b7506ddbb5ec087c9ea0963a10ed57affb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>


More information about the vdsm-patches mailing list