Reduce duplicated code in xenapi

Reduce three implementations of creating VBDs to one. Use helper
functions in other places.

Change-Id: I34ea03fc1fb3cb5d5d343cffa28da5bf3dff888b

nova/tests/test_xenapi.py

@@ -1201,10 +1201,10 @@ class XenAPIAutoDiskConfigTestCase(test.TestCase):
 
         @classmethod
         def fake_create_vbd(cls, session, vm_ref, vdi_ref, userdevice,
-                bootable=True):
+                            vbd_type='disk', read_only=False, bootable=True):
             pass
 
-        self.stubs.Set(volume_utils.VolumeHelper,
+        self.stubs.Set(vm_utils.VMHelper,
                        "create_vbd",
                        fake_create_vbd)
 
@@ -1294,10 +1294,10 @@ class XenAPIGenerateLocal(test.TestCase):
 
         @classmethod
         def fake_create_vbd(cls, session, vm_ref, vdi_ref, userdevice,
-                            bootable=True):
+                            vbd_type='disk', read_only=False, bootable=True):
             pass
 
-        self.stubs.Set(volume_utils.VolumeHelper,
+        self.stubs.Set(vm_utils.VMHelper,
                        "create_vbd",
                        fake_create_vbd)
 

nova/virt/xenapi/vm_utils.py

@@ -229,30 +229,6 @@ class VMHelper(xenapi.HelperBase):
                                                  host))
         return host_free_mem >= mem
 
-    @classmethod
-    def create_cd_vbd(cls, session, vm_ref, vdi_ref, userdevice, bootable):
-        """Create a VBD record.  Returns a Deferred that gives the new
-        VBD reference specific to CDRom devices."""
-        vbd_rec = {}
-        vbd_rec['VM'] = vm_ref
-        vbd_rec['VDI'] = vdi_ref
-        vbd_rec['userdevice'] = str(userdevice)
-        vbd_rec['bootable'] = bootable
-        vbd_rec['mode'] = 'RO'
-        vbd_rec['type'] = 'CD'
-        vbd_rec['unpluggable'] = True
-        vbd_rec['empty'] = False
-        vbd_rec['other_config'] = {}
-        vbd_rec['qos_algorithm_type'] = ''
-        vbd_rec['qos_algorithm_params'] = {}
-        vbd_rec['qos_supported_algorithms'] = []
-        LOG.debug(_('Creating a CDROM-specific VBD for VM %(vm_ref)s,'
-                ' VDI %(vdi_ref)s ... ') % locals())
-        vbd_ref = session.call_xenapi('VBD.create', vbd_rec)
-        LOG.debug(_('Created a CDROM-specific VBD %(vbd_ref)s '
-                ' for VM %(vm_ref)s, VDI %(vdi_ref)s.') % locals())
-        return vbd_ref
-
     @classmethod
     def find_vbd_by_number(cls, session, vm_ref, number):
         """Get the VBD reference from the device number"""
@@ -289,6 +265,30 @@ class VMHelper(xenapi.HelperBase):
             raise volume_utils.StorageError(
                     _('Unable to destroy VBD %s') % vbd_ref)
 
+    @classmethod
+    def create_vbd(cls, session, vm_ref, vdi_ref, userdevice,
+                   vbd_type='disk', read_only=False, bootable=False):
+        """Create a VBD record and returns its reference."""
+        vbd_rec = {}
+        vbd_rec['VM'] = vm_ref
+        vbd_rec['VDI'] = vdi_ref
+        vbd_rec['userdevice'] = str(userdevice)
+        vbd_rec['bootable'] = bootable
+        vbd_rec['mode'] = read_only and 'RO' or 'RW'
+        vbd_rec['type'] = vbd_type
+        vbd_rec['unpluggable'] = True
+        vbd_rec['empty'] = False
+        vbd_rec['other_config'] = {}
+        vbd_rec['qos_algorithm_type'] = ''
+        vbd_rec['qos_algorithm_params'] = {}
+        vbd_rec['qos_supported_algorithms'] = []
+        LOG.debug(_('Creating %(vbd_type)s-type VBD for VM %(vm_ref)s,'
+                ' VDI %(vdi_ref)s ... ') % locals())
+        vbd_ref = session.call_xenapi('VBD.create', vbd_rec)
+        LOG.debug(_('Created VBD %(vbd_ref)s for VM %(vm_ref)s,'
+                ' VDI %(vdi_ref)s.') % locals())
+        return vbd_ref
+
     @classmethod
     def destroy_vdi(cls, session, vdi_ref):
         try:
@@ -530,8 +530,8 @@ class VMHelper(xenapi.HelperBase):
                                   run_as_root=True)
 
             # 4. Create VBD between instance VM and swap VDI
-            volume_utils.VolumeHelper.create_vbd(
+            cls.create_vbd(session, vm_ref, vdi_ref, userdevice,
-                session, vm_ref, vdi_ref, userdevice, bootable=False)
+                           bootable=False)
         except Exception:
             with utils.save_and_reraise_exception():
                 cls.destroy_vdi(session, vdi_ref)
@@ -867,7 +867,7 @@ class VMHelper(xenapi.HelperBase):
                 filename = session.call_plugin('glance', fn, args)
 
                 # Remove the VDI as it is not needed anymore.
-                session.call_xenapi("VDI.destroy", vdi_ref)
+                cls.destroy_vdi(session, vdi_ref)
                 LOG.debug(_("Kernel/Ramdisk VDI %s destroyed"), vdi_ref)
                 return [dict(vdi_type=ImageType.to_string(image_type),
                              vdi_uuid=None,
@@ -1454,22 +1454,10 @@ def _wait_for_device(dev):
 @contextlib.contextmanager
 def vdi_attached_here(session, vdi_ref, read_only=False):
     this_vm_ref = get_this_vm_ref(session)
-    vbd_rec = {}
+
-    vbd_rec['VM'] = this_vm_ref
+    vbd_ref = VMHelper.create_vbd(session, this_vm_ref, vdi_ref,
-    vbd_rec['VDI'] = vdi_ref
+                                  'autodetect', read_only=read_only,
-    vbd_rec['userdevice'] = 'autodetect'
+                                  bootable=False)
-    vbd_rec['bootable'] = False
-    vbd_rec['mode'] = read_only and 'RO' or 'RW'
-    vbd_rec['type'] = 'disk'
-    vbd_rec['unpluggable'] = True
-    vbd_rec['empty'] = False
-    vbd_rec['other_config'] = {}
-    vbd_rec['qos_algorithm_type'] = ''
-    vbd_rec['qos_algorithm_params'] = {}
-    vbd_rec['qos_supported_algorithms'] = []
-    LOG.debug(_('Creating VBD for VDI %s ... '), vdi_ref)
-    vbd_ref = session.call_xenapi("VBD.create", vbd_rec)
-    LOG.debug(_('Creating VBD for VDI %s done.'), vdi_ref)
     try:
         LOG.debug(_('Plugging VBD %s ... '), vbd_ref)
         session.call_xenapi("VBD.plug", vbd_ref)
@@ -1491,7 +1479,11 @@ def vdi_attached_here(session, vdi_ref, read_only=False):
             LOG.debug(_('Destroying VBD for VDI %s ... '), vdi_ref)
             vbd_unplug_with_retry(session, vbd_ref)
     finally:
-        ignore_failure(session.call_xenapi, "VBD.destroy", vbd_ref)
+        try:
+            VMHelper.destroy_vbd(session, vbd_ref)
+        except volume_utils.StorageError:
+            # destroy_vbd() will log error
+            pass
         LOG.debug(_('Destroying VBD for VDI %s done.'), vdi_ref)
 
 
@@ -1502,7 +1494,7 @@ def vbd_unplug_with_retry(session, vbd_ref):
     should be dead."""
     while True:
         try:
-            session.call_xenapi("VBD.unplug", vbd_ref)
+            VMHelper.unplug_vbd(session, vbd_ref)
             LOG.debug(_('VBD.unplug successful first time.'))
             return
         except VMHelper.XenAPI.Failure, e:
@@ -1521,14 +1513,6 @@ def vbd_unplug_with_retry(session, vbd_ref):
                 return
 
 
-def ignore_failure(func, *args, **kwargs):
-    try:
-        return func(*args, **kwargs)
-    except VMHelper.XenAPI.Failure, e:
-        LOG.error(_('Ignoring XenAPI.Failure %s'), e)
-        return None
-
-
 def get_this_vm_uuid():
     with file('/sys/hypervisor/uuid') as f:
         return f.readline().strip()

nova/virt/xenapi/vmops.py

@@ -446,16 +446,16 @@ class VMOps(object):
                       "install")
 
             cd_vdi_ref = first_vdi_ref
-            first_vdi_ref = VMHelper.fetch_blank_disk(session=self._session,
+            first_vdi_ref = VMHelper.fetch_blank_disk(self._session,
-                        instance_type_id=instance.instance_type_id)
+                            instance.instance_type_id)
 
-            VolumeHelper.create_vbd(session=self._session, vm_ref=vm_ref,
+            VMHelper.create_vbd(self._session, vm_ref, first_vdi_ref,
-                vdi_ref=first_vdi_ref, userdevice=userdevice, bootable=False)
+                                userdevice, bootable=False)
 
             # device 1 reserved for rescue disk and we've used '0'
             userdevice = 2
-            VMHelper.create_cd_vbd(session=self._session, vm_ref=vm_ref,
+            VMHelper.create_vbd(self._session, vm_ref, cd_vdi_ref,
-                    vdi_ref=cd_vdi_ref, userdevice=userdevice, bootable=True)
+                                userdevice, vbd_type='CD', bootable=True)
 
             # set user device to next free value
             userdevice += 1
@@ -466,13 +466,12 @@ class VMOps(object):
                             " resize partition...") % locals())
                 instance_type = db.instance_type_get(ctx,
                         instance.instance_type_id)
-                VMHelper.auto_configure_disk(session=self._session,
+                VMHelper.auto_configure_disk(self._session,
-                                             vdi_ref=first_vdi_ref,
+                                             first_vdi_ref,
-                                             new_gb=instance_type['root_gb'])
+                                             instance_type['root_gb'])
 
-            VolumeHelper.create_vbd(session=self._session, vm_ref=vm_ref,
+            VMHelper.create_vbd(self._session, vm_ref, first_vdi_ref,
-                                    vdi_ref=first_vdi_ref,
+                                userdevice, bootable=True)
-                                    userdevice=userdevice, bootable=True)
 
             # set user device to next free value
             # userdevice 1 is reserved for rescue and we've used '0'
@@ -482,16 +481,14 @@ class VMOps(object):
         swap_mb = instance_type['swap']
         generate_swap = swap_mb and FLAGS.xenapi_generate_swap
         if generate_swap:
-            VMHelper.generate_swap(session=self._session, instance=instance,
+            VMHelper.generate_swap(self._session, instance,
-                                   vm_ref=vm_ref, userdevice=userdevice,
+                                   vm_ref, userdevice, swap_mb)
-                                   swap_mb=swap_mb)
             userdevice += 1
 
         ephemeral_gb = instance_type['ephemeral_gb']
         if ephemeral_gb:
             VMHelper.generate_ephemeral(self._session, instance,
-                                        vm_ref, userdevice,
+                                        vm_ref, userdevice, ephemeral_gb)
-                                        ephemeral_gb)
             userdevice += 1
 
         # Attach any other disks
@@ -504,9 +501,8 @@ class VMOps(object):
                 VMHelper.destroy_vdi(self._session, vdi_ref)
                 continue
 
-            VolumeHelper.create_vbd(session=self._session, vm_ref=vm_ref,
+            VMHelper.create_vbd(self._session, vm_ref, vdi_ref,
-                    vdi_ref=vdi_ref, userdevice=userdevice,
+                                userdevice, bootable=False)
-                    bootable=False)
             userdevice += 1
 
     def _configure_instance(self, ctx, instance, vm_ref,
@@ -1078,8 +1074,8 @@ class VMOps(object):
 
         vdi_ref = self._session.call_xenapi("VBD.get_record", vbd_ref)["VDI"]
 
-        return VolumeHelper.create_vbd(self._session, rescue_vm_ref, vdi_ref,
+        return VMHelper.create_vbd(self._session, rescue_vm_ref, vdi_ref,
-                1, False)
+                                   1, bootable=False)
 
     def _shutdown_rescue(self, rescue_vm_ref):
         """Shutdown a rescue instance."""

nova/virt/xenapi/volume_utils.py

@@ -186,30 +186,6 @@ class VolumeHelper(xenapi.HelperBase):
             raise StorageError(_('Unable to find SR from VBD %s') % vbd_ref)
         return sr_ref
 
-    @classmethod
-    def create_vbd(cls, session, vm_ref, vdi_ref, userdevice, bootable):
-        """Create a VBD record.  Returns a Deferred that gives the new
-        VBD reference."""
-        vbd_rec = {}
-        vbd_rec['VM'] = vm_ref
-        vbd_rec['VDI'] = vdi_ref
-        vbd_rec['userdevice'] = str(userdevice)
-        vbd_rec['bootable'] = bootable
-        vbd_rec['mode'] = 'RW'
-        vbd_rec['type'] = 'disk'
-        vbd_rec['unpluggable'] = True
-        vbd_rec['empty'] = False
-        vbd_rec['other_config'] = {}
-        vbd_rec['qos_algorithm_type'] = ''
-        vbd_rec['qos_algorithm_params'] = {}
-        vbd_rec['qos_supported_algorithms'] = []
-        LOG.debug(_('Creating VBD for VM %(vm_ref)s,'
-                ' VDI %(vdi_ref)s ... ') % locals())
-        vbd_ref = session.call_xenapi('VBD.create', vbd_rec)
-        LOG.debug(_('Created VBD %(vbd_ref)s for VM %(vm_ref)s,'
-                ' VDI %(vdi_ref)s.') % locals())
-        return vbd_ref
-
     @classmethod
     def create_pbd(cls, session, sr_ref, params):
         pbd_rec = {}

nova/virt/xenapi/volumeops.py

@@ -63,11 +63,7 @@ class VolumeOps(object):
         if vdi_ref is None:
             raise exception.Error(_('Could not find VDI ref'))
 
-        try:
+        vm_utils.VMHelper.destroy_vdi(vdi_ref)
-            self._session.call_xenapi("VDI.destroy", vdi_ref)
-        except self.XenAPI.Failure, exc:
-            LOG.exception(exc)
-            raise volume_utils.StorageError(_('Error destroying VDI'))
 
     def create_sr(self, label, params):
         LOG.debug(_("Creating SR %s") % label)
@@ -184,11 +180,9 @@ class VolumeOps(object):
 
         dev_number = volume_utils.VolumeHelper.mountpoint_to_number(mountpoint)
         try:
-            vbd_ref = volume_utils.VolumeHelper.create_vbd(self._session,
+            vbd_ref = vm_utils.VMHelper.create_vbd(self._session, vm_ref,
-                                                           vm_ref,
+                                                   vdi_ref, dev_number,
-                                                           vdi_ref,
+                                                   bootable=False)
-                                                           dev_number,
-                                                           False)
         except self.XenAPI.Failure, exc:
             LOG.exception(exc)
             self.forget_sr(uuid)