xenapi: Remove unecessary exception handling

The volume code was catching exceptions from the XenAPI only to re-raise
more-generic StorageError exceptions. This practice makes it much more
difficult to discover the root-cause of issues in production by making
the message in the instance-faults table less useful.

The solution is to have the original root-cause exception propogate all
the way up to the instance-faults table where the developer will be able
to make sense of it.

Other cleanups:

* Remove useless logging and stop logging opaque-refs in some places

* Remove dead code (create_iscsi_storage)

* Remove one-off functions (forget_sr_if_present,
  introduce_sr_unless_present); just use forget_sr and introduce_sr

* Removed unused `exception` import

* Added Openstack copyright lines

Future Work:

There are lots of other places where unecessary exception handling is
occuring; this patch just addresses this in the attach/detach code-path.

Fixes bug 1122733

Change-Id: I8c382fb505303e604ff2e86afcf302efe3d6851d

4 files changed, 99 insertions, 210 deletions

diff --git a/nova/tests/virt/xenapi/test_volumeops.py b/nova/tests/virt/xenapi/test_volumeops.py
index 844ae8459..3497babf2 100644
--- a/nova/tests/virt/xenapi/test_volumeops.py
+++ b/nova/tests/virt/xenapi/test_volumeops.py
@@ -14,6 +14,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import collections
+
 from nova import test
 from nova.tests.xenapi import stubs
 from nova.virt.xenapi import volumeops
@@ -125,31 +127,33 @@ class VolumeAttachTestCase(test.TestCase):
         vm_ref = 'vm_ref'
         dev_number = 1
 
-        called = {'xenapi': False}
+        called = collections.defaultdict(bool)
 
-        def fake_call_xenapi(self, *args, **kwargs):
-            # Only used for VBD.plug in this code path.
-            called['xenapi'] = True
-            raise Exception()
+        def fake_call_xenapi(self, method, *args, **kwargs):
+            called[method] = True
 
         self.stubs.Set(ops._session, 'call_xenapi', fake_call_xenapi)
 
         self.mox.StubOutWithMock(volumeops.volume_utils, 'parse_sr_info')
-        self.mox.StubOutWithMock(
-            volumeops.volume_utils, 'introduce_sr_unless_present')
-        self.mox.StubOutWithMock(volumeops.volume_utils, 'introduce_vdi')
-        self.mox.StubOutWithMock(volumeops.vm_utils, 'create_vbd')
-
         volumeops.volume_utils.parse_sr_info(
             connection_data, sr_label).AndReturn(
                 tuple([sr_uuid, sr_label, sr_params]))
 
-        volumeops.volume_utils.introduce_sr_unless_present(
+        self.mox.StubOutWithMock(
+            volumeops.volume_utils, 'find_sr_by_uuid')
+        volumeops.volume_utils.find_sr_by_uuid(session, sr_uuid).AndReturn(
+                None)
+
+        self.mox.StubOutWithMock(
+            volumeops.volume_utils, 'introduce_sr')
+        volumeops.volume_utils.introduce_sr(
             session, sr_uuid, sr_label, sr_params).AndReturn(sr_ref)
 
+        self.mox.StubOutWithMock(volumeops.volume_utils, 'introduce_vdi')
         volumeops.volume_utils.introduce_vdi(
-            session, sr_ref, vdi_uuid, None).AndReturn(vdi_ref)
+            session, sr_ref, vdi_uuid=vdi_uuid).AndReturn(vdi_ref)
 
+        self.mox.StubOutWithMock(volumeops.vm_utils, 'create_vbd')
         volumeops.vm_utils.create_vbd(
             session, vm_ref, vdi_ref, dev_number,
             bootable=False, osvol=True).AndReturn(vbd_ref)
@@ -157,6 +161,6 @@ class VolumeAttachTestCase(test.TestCase):
         self.mox.ReplayAll()
 
         ops._connect_volume(connection_data, dev_number, instance_name,
-                           vm_ref, hotplug=False)
+                            vm_ref, hotplug=False)
 
-        self.assertEquals(False, called['xenapi'])
+        self.assertEquals(False, called['VBD.plug'])
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index ec6450d9f..60613b5c9 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -483,7 +483,7 @@ def get_vdi_uuid_for_volume(session, connection_data):
             vdi_uuid = vdi_rec['uuid']
         except volume_utils.StorageError, exc:
             LOG.exception(exc)
-            volume_utils.forget_sr(session, sr_uuid)
+            volume_utils.forget_sr(session, sr_ref)
 
     return vdi_uuid
 
diff --git a/nova/virt/xenapi/volume_utils.py b/nova/virt/xenapi/volume_utils.py
index 7921e3e87..40451a48e 100644
--- a/nova/virt/xenapi/volume_utils.py
+++ b/nova/virt/xenapi/volume_utils.py
@@ -1,6 +1,7 @@
 # vim: tabstop=4 shiftwidth=4 softtabstop=4
 
 # Copyright (c) 2010 Citrix Systems, Inc.
+# Copyright (c) 2013 Openstack, LLC.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
 #    not use this file except in compliance with the License. You may obtain
@@ -22,7 +23,6 @@ and storage repositories
 import re
 import string
 
-from nova import exception
 from nova.openstack.common import cfg
 from nova.openstack.common import log as logging
 
@@ -38,94 +38,50 @@ class StorageError(Exception):
         super(StorageError, self).__init__(message)
 
 
-def create_sr(session, label, params):
-    LOG.debug(_("creating sr within volume_utils"))
-    type = params['sr_type']
-    del params['sr_type']
-    LOG.debug(_('type is = %s') % type)
-    if 'name_description' in params:
-        desc = params['name_description']
-        LOG.debug(_('name = %s') % desc)
-        del params['name_description']
-    else:
-        desc = ''
+def _handle_sr_params(params):
     if 'id' in params:
         del params['id']
-    LOG.debug(params)
 
-    try:
-        sr_ref = session.call_xenapi("SR.create",
-                    session.get_xenapi_host(),
-                    params,
-                    '0', label, desc, type, '', False, {})
-        LOG.debug(_('Created %(label)s as %(sr_ref)s.') % locals())
-        return sr_ref
+    sr_type = params.pop('sr_type', 'iscsi')
+    sr_desc = params.pop('name_description', '')
+    return sr_type, sr_desc
 
-    except session.XenAPI.Failure, exc:
-        LOG.exception(exc)
-        raise StorageError(_('Unable to create Storage Repository'))
+
+def create_sr(session, label, params):
+    LOG.debug(_('Creating SR %(label)s') % locals())
+    sr_type, sr_desc = _handle_sr_params(params)
+    sr_ref = session.call_xenapi("SR.create",
+                session.get_xenapi_host(),
+                params,
+                '0', label, sr_desc, sr_type, '', False, {})
+    return sr_ref
 
 
 def introduce_sr(session, sr_uuid, label, params):
-    LOG.debug(_("introducing sr within volume_utils"))
-    # If the sr_type is missing, we assume we are
-    # using the default iscsi back-end
-    type = params.pop('sr_type', 'iscsi')
-    LOG.debug(_('type is = %s') % type)
-    if 'name_description' in params:
-        desc = params['name_description']
-        LOG.debug(_('name = %s') % desc)
-        del params['name_description']
-    else:
-        desc = ''
-    if 'id' in params:
-        del params['id']
-    LOG.debug(params)
+    LOG.debug(_('Introducing SR %(label)s') % locals())
 
-    try:
-        sr_ref = session.call_xenapi("SR.introduce",
-                                      sr_uuid,
-                                      label,
-                                      desc,
-                                      type,
-                                      '',
-                                      False,
-                                      params,)
-        LOG.debug(_('Introduced %(label)s as %(sr_ref)s.') % locals())
-
-        #Create pbd
-        LOG.debug(_('Creating pbd for SR'))
-        pbd_ref = create_pbd(session, sr_ref, params)
-        LOG.debug(_('Plugging SR'))
-        #Plug pbd
-        session.call_xenapi("PBD.plug", pbd_ref)
-        session.call_xenapi("SR.scan", sr_ref)
-        return sr_ref
+    sr_type, sr_desc = _handle_sr_params(params)
 
-    except session.XenAPI.Failure, exc:
-        LOG.exception(exc)
-        raise StorageError(_('Unable to introduce Storage Repository'))
+    sr_ref = session.call_xenapi('SR.introduce', sr_uuid, label, sr_desc,
+            sr_type, '', False, params)
 
+    LOG.debug(_('Creating PBD for SR'))
+    pbd_ref = create_pbd(session, sr_ref, params)
 
-def forget_sr(session, sr_uuid):
-    """
-    Forgets the storage repository without destroying the VDIs within
-    """
-    try:
-        sr_ref = session.call_xenapi("SR.get_by_uuid", sr_uuid)
-    except session.XenAPI.Failure, exc:
-        LOG.exception(exc)
-        raise StorageError(_('Unable to get SR using uuid'))
+    LOG.debug(_('Plugging SR'))
+    session.call_xenapi("PBD.plug", pbd_ref)
 
-    LOG.debug(_('Forgetting SR %s...') % sr_ref)
+    session.call_xenapi("SR.scan", sr_ref)
+    return sr_ref
 
-    try:
-        unplug_pbds(session, sr_ref)
-        sr_ref = session.call_xenapi("SR.forget", sr_ref)
 
-    except session.XenAPI.Failure, exc:
-        LOG.exception(exc)
-        raise StorageError(_('Unable to forget Storage Repository'))
+def forget_sr(session, sr_ref):
+    """
+    Forgets the storage repository without destroying the VDIs within
+    """
+    LOG.debug(_('Forgetting SR...'))
+    unplug_pbds(session, sr_ref)
+    session.call_xenapi("SR.forget", sr_ref)
 
 
 def find_sr_by_uuid(session, sr_uuid):
@@ -138,35 +94,6 @@ def find_sr_by_uuid(session, sr_uuid):
     return None
 
 
-def create_iscsi_storage(session, info, label, description):
-    """
-    Create an iSCSI storage repository that will be used to mount
-    the volume for the specified instance
-    """
-    sr_ref = session.call_xenapi("SR.get_by_name_label", label)
-    if len(sr_ref) == 0:
-        LOG.debug(_('Introducing %s...'), label)
-        record = {}
-        if 'chapuser' in info and 'chappassword' in info:
-            record = {'target': info['targetHost'],
-                      'port': info['targetPort'],
-                      'targetIQN': info['targetIQN'],
-                      'chapuser': info['chapuser'],
-                      'chappassword': info['chappassword']}
-        else:
-            record = {'target': info['targetHost'],
-                      'port': info['targetPort'],
-                      'targetIQN': info['targetIQN']}
-        try:
-            LOG.debug(_('Introduced %(label)s as %(sr_ref)s.') % locals())
-            return sr_ref
-        except session.XenAPI.Failure, exc:
-            LOG.exception(exc)
-            raise StorageError(_('Unable to create Storage Repository'))
-    else:
-        return sr_ref[0]
-
-
 def find_sr_from_vbd(session, vbd_ref):
     """Find the SR reference from the VBD reference."""
     try:
@@ -188,18 +115,19 @@ def create_pbd(session, sr_ref, params):
 
 
 def unplug_pbds(session, sr_ref):
-    pbds = []
     try:
         pbds = session.call_xenapi("SR.get_PBDs", sr_ref)
     except session.XenAPI.Failure, exc:
         LOG.warn(_('Ignoring exception %(exc)s when getting PBDs'
-                ' for %(sr_ref)s') % locals())
+                   ' for %(sr_ref)s') % locals())
+        return
+
     for pbd in pbds:
         try:
             session.call_xenapi("PBD.unplug", pbd)
         except session.XenAPI.Failure, exc:
             LOG.warn(_('Ignoring exception %(exc)s when unplugging'
-                    ' PBD %(pbd)s') % locals())
+                       ' PBD %(pbd)s') % locals())
 
 
 def introduce_vdi(session, sr_ref, vdi_uuid=None, target_lun=None):
@@ -257,24 +185,15 @@ def introduce_vdi(session, sr_ref, vdi_uuid=None, target_lun=None):
 
 
 def purge_sr(session, sr_ref):
-    try:
-        sr_rec = session.call_xenapi("SR.get_record", sr_ref)
-        vdi_refs = session.call_xenapi("SR.get_VDIs", sr_ref)
-    except StorageError, ex:
-        LOG.exception(ex)
-        raise StorageError(_('Error finding vdis in SR %s') % sr_ref)
-
+    # Make sure no VBDs are referencing the SR VDIs
+    vdi_refs = session.call_xenapi("SR.get_VDIs", sr_ref)
     for vdi_ref in vdi_refs:
-        try:
-            vbd_refs = session.call_xenapi("VDI.get_VBDs", vdi_ref)
-        except StorageError, ex:
-            LOG.exception(ex)
-            raise StorageError(_('Unable to find vbd for vdi %s') %
-                               vdi_ref)
-        if len(vbd_refs) > 0:
+        vbd_refs = session.call_xenapi("VDI.get_VBDs", vdi_ref)
+        if vbd_refs:
+            LOG.warn(_('Cannot purge SR with referenced VDIs'))
             return
 
-    forget_sr(session, sr_rec['uuid'])
+    forget_sr(session, sr_ref)
 
 
 def get_device_number(mountpoint):
@@ -382,28 +301,3 @@ def _get_target_port(iscsi_string):
         return iscsi_string[iscsi_string.find(':') + 1:]
     elif iscsi_string is None or CONF.target_port:
         return CONF.target_port
-
-
-def introduce_sr_unless_present(session, sr_uuid, label, params):
-    LOG.debug(_("Introducing SR %s") % label)
-    sr_ref = find_sr_by_uuid(session, sr_uuid)
-    if sr_ref:
-        LOG.debug(_('SR found in xapi database. No need to introduce'))
-        return sr_ref
-    sr_ref = introduce_sr(session, sr_uuid, label, params)
-
-    if sr_ref is None:
-        raise exception.NovaException(_('Could not introduce SR'))
-    return sr_ref
-
-
-def forget_sr_if_present(session, sr_uuid):
-    sr_ref = find_sr_by_uuid(session, sr_uuid)
-    if sr_ref is None:
-        LOG.debug(_('SR %s not found in the xapi database') % sr_uuid)
-        return
-    try:
-        forget_sr(session, sr_uuid)
-    except StorageError, exc:
-        LOG.exception(exc)
-        raise exception.NovaException(_('Could not forget SR'))
diff --git a/nova/virt/xenapi/volumeops.py b/nova/virt/xenapi/volumeops.py
index 0c8a9e1c7..88119e10d 100644
--- a/nova/virt/xenapi/volumeops.py
+++ b/nova/virt/xenapi/volumeops.py
@@ -1,6 +1,7 @@
 # vim: tabstop=4 shiftwidth=4 softtabstop=4
 
 # Copyright (c) 2010 Citrix Systems, Inc.
+# Copyright (c) 2013 Openstack, LLC.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
 #    not use this file except in compliance with the License. You may obtain
@@ -59,59 +60,42 @@ class VolumeOps(object):
                 ' instance %(instance_name)s') % locals())
 
     def _connect_volume(self, connection_data, dev_number, instance_name,
-                       vm_ref, hotplug=True):
+                        vm_ref, hotplug=True):
+        sr_uuid, sr_label, sr_params = volume_utils.parse_sr_info(
+                connection_data, 'Disk-for:%s' % instance_name)
 
-        description = 'Disk-for:%s' % instance_name
-        uuid, label, sr_params = volume_utils.parse_sr_info(connection_data,
-                                                            description)
-
-        # Introduce SR
-        try:
-            sr_ref = volume_utils.introduce_sr_unless_present(
-                self._session, uuid, label, sr_params)
-            LOG.debug(_('Introduced %(label)s as %(sr_ref)s.') % locals())
-        except self._session.XenAPI.Failure, exc:
-            LOG.exception(exc)
-            raise volume_utils.StorageError(
-                                _('Unable to introduce Storage Repository'))
-
-        vdi_uuid = None
-        target_lun = None
-        if 'vdi_uuid' in connection_data:
-            vdi_uuid = connection_data['vdi_uuid']
-        elif 'target_lun' in connection_data:
-            target_lun = connection_data['target_lun']
-        else:
-            vdi_uuid = None
-
-        # Introduce VDI  and attach VBD to VM
-        try:
-            vdi_ref = volume_utils.introduce_vdi(self._session, sr_ref,
-                                                 vdi_uuid, target_lun)
-        except volume_utils.StorageError, exc:
-            LOG.exception(exc)
-            volume_utils.forget_sr_if_present(self._session, uuid)
-            raise Exception(_('Unable to create VDI on SR %(sr_ref)s for'
-                    ' instance %(instance_name)s') % locals())
+        # Introduce SR if not already present
+        sr_ref = volume_utils.find_sr_by_uuid(self._session, sr_uuid)
+        if not sr_ref:
+            sr_ref = volume_utils.introduce_sr(
+                    self._session, sr_uuid, sr_label, sr_params)
 
         try:
+            # Introduce VDI
+            if 'vdi_uuid' in connection_data:
+                vdi_ref = volume_utils.introduce_vdi(
+                        self._session, sr_ref,
+                        vdi_uuid=connection_data['vdi_uuid'])
+            elif 'target_lun' in connection_data:
+                vdi_ref = volume_utils.introduce_vdi(
+                        self._session, sr_ref,
+                        target_lun=connection_data['target_lun'])
+            else:
+                # NOTE(sirp): This will introduce the first VDI in the SR
+                vdi_ref = volume_utils.introduce_vdi(self._session, sr_ref)
+
+            # Attach
             vbd_ref = vm_utils.create_vbd(self._session, vm_ref, vdi_ref,
                                           dev_number, bootable=False,
                                           osvol=True)
-        except self._session.XenAPI.Failure, exc:
-            LOG.exception(exc)
-            volume_utils.forget_sr_if_present(self._session, uuid)
-            raise Exception(_('Unable to use SR %(sr_ref)s for'
-                              ' instance %(instance_name)s') % locals())
-
-        if hotplug:
-            try:
+
+            if hotplug:
                 self._session.call_xenapi("VBD.plug", vbd_ref)
-            except self._session.XenAPI.Failure, exc:
-                LOG.exception(exc)
-                volume_utils.forget_sr_if_present(self._session, uuid)
-                raise Exception(_('Unable to attach volume to instance %s')
-                                % instance_name)
+        except Exception:
+            # NOTE(sirp): Forgetting the SR will have the effect of cleaning up
+            # the VDI and VBD records, so no need to handle that explicitly.
+            volume_utils.forget_sr(self._session, sr_ref)
+            raise
 
     def detach_volume(self, connection_info, instance_name, mountpoint):
         """Detach volume storage to VM instance."""
@@ -121,8 +105,15 @@ class VolumeOps(object):
         device_number = volume_utils.get_device_number(mountpoint)
         vm_ref = vm_utils.vm_ref_or_raise(self._session, instance_name)
 
-        vbd_ref = vm_utils.find_vbd_by_number(
-                self._session, vm_ref, device_number)
+        try:
+            vbd_ref = vm_utils.find_vbd_by_number(
+                    self._session, vm_ref, device_number)
+        except volume_utils.StorageError:
+            # NOTE(sirp): If we don't find the VBD then it must have been
+            # detached previously.
+            LOG.warn(_('Skipping detach because VBD for %(instance_name)s was'
+                       ' not found') % locals())
+            return
 
         # Unplug VBD if we're NOT shutdown
         unplug = not vm_utils._is_vm_shutdown(self._session, vm_ref)