* merged with lp:~armando-migliaccio/nova/xenapi-refactoring

* fixed pylint score
* complied with HACKING guidelines

7 files changed, 281 insertions, 431 deletions

diff --git a/nova/virt/xenapi/__init__.py b/nova/virt/xenapi/__init__.py
index ece430407..3d598c463 100644
--- a/nova/virt/xenapi/__init__.py
+++ b/nova/virt/xenapi/__init__.py
@@ -13,14 +13,3 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-
-"""
-This is loaded late so that there's no need to install this library
-when not using XenAPI
-"""
-
-XenAPI = None
-global XenAPI
-
-if XenAPI is None:
-    XenAPI = __import__('XenAPI')
diff --git a/nova/virt/xenapi/novadeps.py b/nova/virt/xenapi/novadeps.py
deleted file mode 100644
index 3680a5dd2..000000000
--- a/nova/virt/xenapi/novadeps.py
+++ /dev/null
@@ -1,318 +0,0 @@
-# vim: tabstop=4 shiftwidth=4 softtabstop=4
-
-# Copyright (c) 2010 Citrix Systems, Inc.
-#
-#    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
-#    a copy of the License at
-#
-#         http://www.apache.org/licenses/LICENSE-2.0
-#
-#    Unless required by applicable law or agreed to in writing, software
-#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-#    License for the specific language governing permissions and limitations
-#    under the License.
-
-
-"""
-It captures all the inner details of Nova classes and avoid their exposure
-to the implementation of the XenAPI module. One benefit of this, is to avoid
-sprawl of code changes
-"""
-
-import re
-import string
-
-from nova import db
-from nova import flags
-from nova import context
-from nova import process
-
-from twisted.internet import defer
-
-from nova.compute import power_state
-from nova.auth.manager import AuthManager
-from nova.compute import instance_types
-from nova.virt import images
-
-XENAPI_POWER_STATE = {
-    'Halted': power_state.SHUTDOWN,
-    'Running': power_state.RUNNING,
-    'Paused': power_state.PAUSED,
-    'Suspended': power_state.SHUTDOWN,  # FIXME
-    'Crashed': power_state.CRASHED}
-
-flags.DEFINE_string('xenapi_connection_url',
-                    None,
-                    'URL for connection to XenServer/Xen Cloud Platform.'
-                    ' Required if connection_type=xenapi.')
-flags.DEFINE_string('xenapi_connection_username',
-                    'root',
-                    'Username for connection to XenServer/Xen Cloud Platform.'
-                    ' Used only if connection_type=xenapi.')
-flags.DEFINE_string('xenapi_connection_password',
-                    None,
-                    'Password for connection to XenServer/Xen Cloud Platform.'
-                    ' Used only if connection_type=xenapi.')
-flags.DEFINE_float('xenapi_task_poll_interval',
-                   0.5,
-                   'The interval used for polling of remote tasks '
-                   '(Async.VM.start, etc).  Used only if '
-                   'connection_type=xenapi.')
-flags.DEFINE_string('target_host',
-                    None,
-                    'iSCSI Target Host')
-flags.DEFINE_string('target_port',
-                    '3260',
-                    'iSCSI Target Port, 3260 Default')
-flags.DEFINE_string('iqn_prefix',
-                    'iqn.2010-10.org.openstack',
-                    'IQN Prefix')
-
-
-class Configuration(object):
-    """ Wraps Configuration details into common class """
-    def __init__(self):
-        self._flags = flags.FLAGS
-
-    @property
-    def xenapi_connection_url(self):
-        """ Return the connection url """
-        return self._flags.xenapi_connection_url
-
-    @property
-    def xenapi_connection_username(self):
-        """ Return the username used for the connection """
-        return self._flags.xenapi_connection_username
-
-    @property
-    def xenapi_connection_password(self):
-        """ Return the password used for the connection """
-        return self._flags.xenapi_connection_password
-
-    @property
-    def xenapi_task_poll_interval(self):
-        """ Return the poll interval for the connection """
-        return self._flags.xenapi_task_poll_interval
-
-    @property
-    def target_host(self):
-        return self._flags.target_host
-
-    @property
-    def target_port(self):
-        return self._flags.target_port
-
-    @property
-    def iqn_prefix(self):
-        return self._flags.iqn_prefix
-
-
-config = Configuration()
-
-
-class Instance(object):
-    """ Wraps up instance specifics """
-
-    @classmethod
-    def get_name(cls, instance):
-        """ The name of the instance """
-        return instance.name
-
-    @classmethod
-    def get_type(cls, instance):
-        """ The type of the instance """
-        return instance_types.INSTANCE_TYPES[instance.instance_type]
-
-    @classmethod
-    def get_project(cls, instance):
-        """ The project the instance belongs """
-        return AuthManager().get_project(instance.project_id)
-
-    @classmethod
-    def get_project_id(cls, instance):
-        """ The id of the project the instance belongs """
-        return instance.project_id
-
-    @classmethod
-    def get_image_id(cls, instance):
-        """ The instance's image id """
-        return instance.image_id
-
-    @classmethod
-    def get_kernel_id(cls, instance):
-        """ The instance's kernel id """
-        return instance.kernel_id
-
-    @classmethod
-    def get_ramdisk_id(cls, instance):
-        """ The instance's ramdisk id """
-        return instance.ramdisk_id
-
-    @classmethod
-    def get_network(cls, instance):
-        """ The network the instance is connected to """
-        # TODO: is ge_admin_context the right context to retrieve?
-        return db.project_get_network(context.get_admin_context(),
-                                      instance.project_id)
-
-    @classmethod
-    def get_mac(cls, instance):
-        """ The instance's MAC address """
-        return instance.mac_address
-
-    @classmethod
-    def get_user(cls, instance):
-        """ The owner of the instance """
-        return AuthManager().get_user(instance.user_id)
-
-
-class Network(object):
-    """ Wraps up network specifics """
-
-    @classmethod
-    def get_bridge(cls, network):
-        """ the bridge for the network """
-        return network.bridge
-
-
-class Image(object):
-    """ Wraps up image specifics """
-
-    @classmethod
-    def get_url(cls, image):
-        """ the url to get the image from """
-        return images.image_url(image)
-
-
-class User(object):
-    """ Wraps up user specifics """
-
-    @classmethod
-    def get_access(cls, user, project):
-        """ access key """
-        return AuthManager().get_access_key(user, project)
-
-    @classmethod
-    def get_secret(cls, user):
-        """ access secret """
-        return user.secret
-
-
-class Volume(object):
-    """ Wraps up volume specifics """
-
-    @classmethod
-    @defer.inlineCallbacks
-    def parse_volume_info(cls, device_path, mountpoint):
-        """
-        Parse device_path and mountpoint as they can be used by XenAPI.
-        In particular, the mountpoint (e.g. /dev/sdc) must be translated
-        into a numeric literal.
-        FIXME: As for device_path, currently cannot be used as it is,
-        because it does not contain target information. As for interim
-        solution, target details are passed either via Flags or obtained
-        by iscsiadm. Long-term solution is to add a few more fields to the
-        db in the iscsi_target table with the necessary info and modify
-        the iscsi driver to set them.
-        """
-        device_number = Volume.mountpoint_to_number(mountpoint)
-        volume_id = Volume.get_volume_id(device_path)
-        (iscsi_name, iscsi_portal) = yield Volume.get_target(volume_id)
-        target_host = Volume.get_target_host(iscsi_portal)
-        target_port = Volume.get_target_port(iscsi_portal)
-        target_iqn = Volume.get_iqn(iscsi_name, volume_id)
-
-        if (device_number < 0) or \
-            (volume_id is None) or \
-            (target_host is None) or \
-            (target_iqn is None):
-            raise Exception('Unable to obtain target information %s, %s' %
-                            (device_path, mountpoint))
-
-        volume_info = {}
-        volume_info['deviceNumber'] = device_number
-        volume_info['volumeId'] = volume_id
-        volume_info['targetHost'] = target_host
-        volume_info['targetPort'] = target_port
-        volume_info['targeIQN'] = target_iqn
-        defer.returnValue(volume_info)
-
-    @classmethod
-    def mountpoint_to_number(cls, mountpoint):
-        """ Translate a mountpoint like /dev/sdc into a numberic """
-        if mountpoint.startswith('/dev/'):
-            mountpoint = mountpoint[5:]
-        if re.match('^[hs]d[a-p]$', mountpoint):
-            return (ord(mountpoint[2:3]) - ord('a'))
-        elif re.match('^vd[a-p]$', mountpoint):
-            return (ord(mountpoint[2:3]) - ord('a'))
-        elif re.match('^[0-9]+$', mountpoint):
-            return string.atoi(mountpoint, 10)
-        else:
-            logging.warn('Mountpoint cannot be translated: %s', mountpoint)
-            return -1
-
-    @classmethod
-    def get_volume_id(cls, n):
-        """ Retrieve the volume id from device_path """
-        # n must contain at least the volume_id
-        # /vol- is for remote volumes
-        # -vol- is for local volumes
-        # see compute/manager->setup_compute_volume
-        volume_id = n[n.find('/vol-') + 1:]
-        if volume_id == n:
-            volume_id = n[n.find('-vol-') + 1:].replace('--', '-')
-        return volume_id
-
-    @classmethod
-    def get_target_host(cls, n):
-        """ Retrieve target host """
-        if n:
-            return n[0:n.find(':')]
-        elif n is None or config.target_host:
-            return config.target_host
-
-    @classmethod
-    def get_target_port(cls, n):
-        """ Retrieve target port """
-        if n:
-            return n[n.find(':') + 1:]
-        elif  n is None or config.target_port:
-            return config.target_port
-
-    @classmethod
-    def get_iqn(cls, n, id):
-        """ Retrieve target IQN """
-        if n:
-            return n
-        elif n is None or config.iqn_prefix:
-            volume_id = Volume.get_volume_id(id)
-            return '%s:%s' % (config.iqn_prefix, volume_id)
-
-    @classmethod
-    @defer.inlineCallbacks
-    def get_target(self, volume_id):
-        """
-        Gets iscsi name and portal from volume name and host.
-        For this method to work the following are needed:
-        1) volume_ref['host'] must resolve to something rather than loopback
-        2) ietd must bind only to the address as resolved above
-        If any of the two conditions are not met, fall back on Flags.
-        """
-        volume_ref = db.volume_get_by_ec2_id(context.get_admin_context(),
-                                             volume_id)
-
-        (r, _e) = yield process.simple_execute("sudo iscsiadm -m discovery -t "
-                                         "sendtargets -p %s" %
-                                         volume_ref['host'])
-        if len(_e) == 0:
-            for target in r.splitlines():
-                if volume_id in target:
-                    (location, _sep, iscsi_name) = target.partition(" ")
-                    break
-            iscsi_portal = location.split(",")[0]
-            defer.returnValue((iscsi_name, iscsi_portal))
-        else:
-            defer.returnValue((None, None))
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index e6d20f98b..762cbae83 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -25,10 +25,17 @@ import XenAPI
 from twisted.internet import defer
 
 from nova import utils
+from nova.auth.manager import AuthManager
+from nova.compute import instance_types
+from nova.virt import images
+from nova.compute import power_state
 
-from novadeps import Instance
-from novadeps import Image
-from novadeps import User
+XENAPI_POWER_STATE = {
+    'Halted': power_state.SHUTDOWN,
+    'Running': power_state.RUNNING,
+    'Paused': power_state.PAUSED,
+    'Suspended': power_state.SHUTDOWN,  # FIXME
+    'Crashed': power_state.CRASHED}
 
 
 class VMHelper():
@@ -44,7 +51,7 @@ class VMHelper():
         """Create a VM record.  Returns a Deferred that gives the new
         VM reference."""
 
-        instance_type = Instance.get_type(instance)
+        instance_type = instance_types.INSTANCE_TYPES[instance.instance_type]
         mem = str(long(instance_type['memory_mb']) * 1024 * 1024)
         vcpus = str(instance_type['vcpus'])
         rec = {
@@ -76,10 +83,9 @@ class VMHelper():
             'user_version': '0',
             'other_config': {},
             }
-        logging.debug('Created VM %s...', Instance.get_name(instance))
+        logging.debug('Created VM %s...', instance.name)
         vm_ref = yield session.call_xenapi('VM.create', rec)
-        logging.debug('Created VM %s as %s.',
-                      Instance.get_name(instance), vm_ref)
+        logging.debug('Created VM %s as %s.', instance.name, vm_ref)
         defer.returnValue(vm_ref)
 
     @classmethod
@@ -175,14 +181,14 @@ class VMHelper():
         its kernel and ramdisk (if external kernels are being used).
         Returns a Deferred that gives the new VDI UUID."""
 
-        url = Image.get_url(image)
-        access = User.get_access(user, project)
+        url = images.image_url(image)
+        access = AuthManager().get_access_key(user, project)
         logging.debug("Asking xapi to fetch %s as %s", url, access)
         fn = use_sr and 'get_vdi' or 'get_kernel'
         args = {}
         args['src_url'] = url
         args['username'] = access
-        args['password'] = User.get_secret(user)
+        args['password'] = user.secret
         if use_sr:
             args['add_partition'] = 'true'
         task = yield session.async_call_plugin('objectstore', fn, args)
@@ -217,7 +223,7 @@ class VMHelper():
     def lookup_vm_vdis_blocking(cls, session, vm):
         """ Synchronous lookup_vm_vdis """
         # Firstly we get the VBDs, then the VDIs.
-        # TODO: do we leave the read-only devices?
+        # TODO(Armando): do we leave the read-only devices?
         vbds = session.get_xenapi().VM.get_VBDs(vm)
         vdis = []
         if vbds:
@@ -235,3 +241,11 @@ class VMHelper():
                 return vdis
             else:
                 return None
+
+    @classmethod
+    def compile_info(cls, record):
+        return {'state': XENAPI_POWER_STATE[record['power_state']],
+                'max_mem': long(record['memory_static_max']) >> 10,
+                'mem': long(record['memory_dynamic_max']) >> 10,
+                'num_cpu': record['VCPUs_max'],
+                'cpu_time': 0}
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index 3db86f179..3696782b3 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -23,12 +23,11 @@ import XenAPI
 
 from twisted.internet import defer
 
-from novadeps import XENAPI_POWER_STATE
-from novadeps import Instance
-from novadeps import Network
-
-from vm_utils import VMHelper
-from network_utils import NetworkHelper
+from nova import db
+from nova import context
+from nova.auth.manager import AuthManager
+from nova.virt.xenapi.network_utils import NetworkHelper
+from nova.virt.xenapi.vm_utils import VMHelper
 
 
 class VMOps(object):
@@ -46,39 +45,40 @@ class VMOps(object):
     @defer.inlineCallbacks
     def spawn(self, instance):
         """ Create VM instance """
-        vm = yield VMHelper.lookup(self._session, Instance.get_name(instance))
+        vm = yield VMHelper.lookup(self._session, instance.name)
         if vm is not None:
             raise Exception('Attempted to create non-unique name %s' %
-                            Instance.get_name(instance))
+                            instance.name)
 
-        bridge = Network.get_bridge(Instance.get_network(instance))
+        bridge = db.project_get_network(context.get_admin_context(),
+                                      instance.project_id).bridge
         network_ref = \
             yield NetworkHelper.find_network_with_bridge(self._session, bridge)
 
-        user = Instance.get_user(instance)
-        project = Instance.get_project(instance)
+        user = AuthManager().get_user(instance.user_id)
+        project = AuthManager().get_project(instance.project_id)
         vdi_uuid = yield VMHelper.fetch_image(self._session,
-            Instance.get_image_id(instance), user, project, True)
+            instance.image_id, user, project, True)
         kernel = yield VMHelper.fetch_image(self._session,
-            Instance.get_kernel_id(instance), user, project, False)
+            instance.kernel_id, user, project, False)
         ramdisk = yield VMHelper.fetch_image(self._session,
-            Instance.get_ramdisk_id(instance), user, project, False)
+            instance.ramdisk_id, user, project, False)
         vdi_ref = yield self._session.call_xenapi('VDI.get_by_uuid', vdi_uuid)
         vm_ref = yield VMHelper.create_vm(self._session,
                                           instance, kernel, ramdisk)
         yield VMHelper.create_vbd(self._session, vm_ref, vdi_ref, 0, True)
         if network_ref:
             yield VMHelper.create_vif(self._session, vm_ref,
-                                      network_ref, Instance.get_mac(instance))
+                                      network_ref, instance.mac_address)
         logging.debug('Starting VM %s...', vm_ref)
         yield self._session.call_xenapi('VM.start', vm_ref, False, False)
-        logging.info('Spawning VM %s created %s.', Instance.get_name(instance),
+        logging.info('Spawning VM %s created %s.', instance.name,
                      vm_ref)
 
     @defer.inlineCallbacks
     def reboot(self, instance):
         """ Reboot VM instance """
-        instance_name = Instance.get_name(instance)
+        instance_name = instance.name
         vm = yield VMHelper.lookup(self._session, instance_name)
         if vm is None:
             raise Exception('instance not present %s' % instance_name)
@@ -88,7 +88,7 @@ class VMOps(object):
     @defer.inlineCallbacks
     def destroy(self, instance):
         """ Destroy VM instance """
-        vm = yield VMHelper.lookup(self._session, Instance.get_name(instance))
+        vm = yield VMHelper.lookup(self._session, instance.name)
         if vm is None:
             # Don't complain, just return.  This lets us clean up instances
             # that have already disappeared from the underlying platform.
@@ -122,11 +122,7 @@ class VMOps(object):
         if vm is None:
             raise Exception('instance not present %s' % instance_id)
         rec = self._session.get_xenapi().VM.get_record(vm)
-        return {'state': XENAPI_POWER_STATE[rec['power_state']],
-                'max_mem': long(rec['memory_static_max']) >> 10,
-                'mem': long(rec['memory_dynamic_max']) >> 10,
-                'num_cpu': rec['VCPUs_max'],
-                'cpu_time': 0}
+        return VMHelper.compile_info(rec)
 
     def get_console_output(self, instance):
         """ Return snapshot of console """
diff --git a/nova/virt/xenapi/volume_utils.py b/nova/virt/xenapi/volume_utils.py
index 3b3e8894c..48aff7ef5 100644
--- a/nova/virt/xenapi/volume_utils.py
+++ b/nova/virt/xenapi/volume_utils.py
@@ -19,105 +19,134 @@ Helper methods for operations related to the management of volumes,
 and storage repositories
 """
 
+import re
+import string
 import logging
+import XenAPI
 
 from twisted.internet import defer
 
+from nova import db
+from nova import context
+from nova import flags
+from nova import process
 from nova import utils
 
+FLAGS = flags.FLAGS
+
+
+class StorageError(Exception):
+    """ To raise errors related to SR, VDI, PBD, and VBD commands """
+    def __init__(self, message=None):
+        super(StorageError, self).__init__(message)
+
 
 class VolumeHelper():
+    """
+    The class that wraps the helper methods together.
+    """
     def __init__(self, session):
         return
 
     @classmethod
     @utils.deferredToThread
-    def create_iscsi_storage(self, session, target, port, target_iqn,
-                             username, password, label, description):
-
-        return VolumeHelper.create_iscsi_storage_blocking(session, target,
-                                                          port,
-                                                          target_iqn,
-                                                          username,
-                                                          password,
+    def create_iscsi_storage(cls, session, info, label, description):
+        """
+        Create an iSCSI storage repository that will be used to mount
+        the volume for the specified instance
+        """
+        return VolumeHelper.create_iscsi_storage_blocking(session, info,
                                                           label,
                                                           description)
 
     @classmethod
-    def create_iscsi_storage_blocking(self, session, target, port, target_iqn,
-                                      username, password, label, description):
-
+    def create_iscsi_storage_blocking(cls, session, info, label, description):
+        """ Synchronous create_iscsi_storage """
         sr_ref = session.get_xenapi().SR.get_by_name_label(label)
         if len(sr_ref) == 0:
-            logging.debug('Introducing %s...' % label)
+            logging.debug('Introducing %s...', label)
+            record = {}
+            if 'chapuser' in info and 'chappassword' in info:
+                record = {'target': info['targetHost'],
+                          'port': info['targetPort'],
+                          'targetIQN': info['targeIQN'],
+                          'chapuser': info['chapuser'],
+                          'chappassword': info['chappassword']
+                          }
+            else:
+                record = {'target': info['targetHost'],
+                          'port': info['targetPort'],
+                          'targetIQN': info['targeIQN']
+                          }
             try:
                 sr_ref = session.get_xenapi().SR.create(
                     session.get_xenapi_host(),
-                    {'target': target,
-                     'port': port,
-                     'targetIQN': target_iqn
-                     # TODO: when/if chap authentication is used
-                     #'chapuser': username,
-                     #'chappassword': password
-                     },
+                    record,
                     '0', label, description, 'iscsi', '', False, {})
-                logging.debug('Introduced %s as %s.' % (label, sr_ref))
+                logging.debug('Introduced %s as %s.', label, sr_ref)
                 return sr_ref
-            except Exception, exc:
+            except XenAPI.Failure, exc:
                 logging.warn(exc)
-                raise Exception('Unable to create Storage Repository')
+                raise StorageError('Unable to create Storage Repository')
         else:
             return sr_ref[0]
 
     @classmethod
     @defer.inlineCallbacks
-    def find_sr_from_vbd(self, session, vbd_ref):
+    def find_sr_from_vbd(cls, session, vbd_ref):
+        """ Find the SR reference from the VBD reference """
         vdi_ref = yield session.get_xenapi().VBD.get_VDI(vbd_ref)
         sr_ref = yield session.get_xenapi().VDI.get_SR(vdi_ref)
         defer.returnValue(sr_ref)
 
     @classmethod
     @utils.deferredToThread
-    def destroy_iscsi_storage(self, session, sr_ref):
+    def destroy_iscsi_storage(cls, session, sr_ref):
+        """ Forget the SR whilst preserving the state of the disk """
         VolumeHelper.destroy_iscsi_storage_blocking(session, sr_ref)
 
     @classmethod
-    def destroy_iscsi_storage_blocking(self, session, sr_ref):
+    def destroy_iscsi_storage_blocking(cls, session, sr_ref):
+        """ Synchronous destroy_iscsi_storage """
         logging.debug("Forgetting SR %s ... ", sr_ref)
         pbds = []
         try:
             pbds = session.get_xenapi().SR.get_PBDs(sr_ref)
-        except Exception, exc:
+        except XenAPI.Failure, exc:
             logging.warn('Ignoring exception %s when getting PBDs for %s',
                          exc, sr_ref)
         for pbd in pbds:
             try:
                 session.get_xenapi().PBD.unplug(pbd)
-            except Exception, exc:
+            except XenAPI.Failure, exc:
                 logging.warn('Ignoring exception %s when unplugging PBD %s',
                              exc, pbd)
         try:
             session.get_xenapi().SR.forget(sr_ref)
             logging.debug("Forgetting SR %s done.", sr_ref)
-        except Exception, exc:
+        except XenAPI.Failure, exc:
             logging.warn('Ignoring exception %s when forgetting SR %s',
                          exc, sr_ref)
 
     @classmethod
     @utils.deferredToThread
-    def introduce_vdi(self, session, sr_ref):
+    def introduce_vdi(cls, session, sr_ref):
+        """ Introduce VDI in the host """
         return VolumeHelper.introduce_vdi_blocking(session, sr_ref)
 
     @classmethod
-    def introduce_vdi_blocking(self, session, sr_ref):
+    def introduce_vdi_blocking(cls, session, sr_ref):
+        """ Synchronous introduce_vdi """
         try:
             vdis = session.get_xenapi().SR.get_VDIs(sr_ref)
-        except Exception, exc:
-            raise Exception('Unable to introduce VDI on SR %s' % sr_ref)
+        except XenAPI.Failure, exc:
+            logging.warn(exc)
+            raise StorageError('Unable to introduce VDI on SR %s' % sr_ref)
         try:
             vdi_rec = session.get_xenapi().VDI.get_record(vdis[0])
-        except Exception, exc:
-            raise Exception('Unable to get record of VDI %s on' % vdis[0])
+        except XenAPI.Failure, exc:
+            logging.warn(exc)
+            raise StorageError('Unable to get record of VDI %s on' % vdis[0])
         else:
             return session.get_xenapi().VDI.introduce(
                 vdi_rec['uuid'],
@@ -131,3 +160,121 @@ class VolumeHelper():
                 vdi_rec['location'],
                 vdi_rec['xenstore_data'],
                 vdi_rec['sm_config'])
+
+    @classmethod
+    @defer.inlineCallbacks
+    def parse_volume_info(cls, device_path, mountpoint):
+        """
+        Parse device_path and mountpoint as they can be used by XenAPI.
+        In particular, the mountpoint (e.g. /dev/sdc) must be translated
+        into a numeric literal.
+        FIXME(armando):
+        As for device_path, currently cannot be used as it is,
+        because it does not contain target information. As for interim
+        solution, target details are passed either via Flags or obtained
+        by iscsiadm. Long-term solution is to add a few more fields to the
+        db in the iscsi_target table with the necessary info and modify
+        the iscsi driver to set them.
+        """
+        device_number = VolumeHelper.mountpoint_to_number(mountpoint)
+        volume_id = _get_volume_id(device_path)
+        (iscsi_name, iscsi_portal) = yield _get_target(volume_id)
+        target_host = _get_target_host(iscsi_portal)
+        target_port = _get_target_port(iscsi_portal)
+        target_iqn = _get_iqn(iscsi_name, volume_id)
+        logging.debug('(vol_id,number,host,port,iqn): (%s,%s,%s,%s)',
+                      volume_id,
+                      target_host,
+                      target_port,
+                      target_iqn)
+        if (device_number < 0) or \
+            (volume_id is None) or \
+            (target_host is None) or \
+            (target_iqn is None):
+            raise StorageError('Unable to obtain target information %s, %s' %
+                            (device_path, mountpoint))
+        volume_info = {}
+        volume_info['deviceNumber'] = device_number
+        volume_info['volumeId'] = volume_id
+        volume_info['targetHost'] = target_host
+        volume_info['targetPort'] = target_port
+        volume_info['targeIQN'] = target_iqn
+        defer.returnValue(volume_info)
+
+    @classmethod
+    def mountpoint_to_number(cls, mountpoint):
+        """ Translate a mountpoint like /dev/sdc into a numberic """
+        if mountpoint.startswith('/dev/'):
+            mountpoint = mountpoint[5:]
+        if re.match('^[hs]d[a-p]$', mountpoint):
+            return (ord(mountpoint[2:3]) - ord('a'))
+        elif re.match('^vd[a-p]$', mountpoint):
+            return (ord(mountpoint[2:3]) - ord('a'))
+        elif re.match('^[0-9]+$', mountpoint):
+            return string.atoi(mountpoint, 10)
+        else:
+            logging.warn('Mountpoint cannot be translated: %s', mountpoint)
+            return -1
+
+
+def _get_volume_id(path):
+    """ Retrieve the volume id from device_path """
+    # n must contain at least the volume_id
+    # /vol- is for remote volumes
+    # -vol- is for local volumes
+    # see compute/manager->setup_compute_volume
+    volume_id = path[path.find('/vol-') + 1:]
+    if volume_id == path:
+        volume_id = path[path.find('-vol-') + 1:].replace('--', '-')
+    return volume_id
+
+
+def _get_target_host(iscsi_string):
+    """ Retrieve target host """
+    if iscsi_string:
+        return iscsi_string[0:iscsi_string.find(':')]
+    elif iscsi_string is None or FLAGS.target_host:
+        return FLAGS.target_host
+
+
+def _get_target_port(iscsi_string):
+    """ Retrieve target port """
+    if iscsi_string:
+        return iscsi_string[iscsi_string.find(':') + 1:]
+    elif  iscsi_string is None or FLAGS.target_port:
+        return FLAGS.target_port
+
+
+def _get_iqn(iscsi_string, id):
+    """ Retrieve target IQN """
+    if iscsi_string:
+        return iscsi_string
+    elif iscsi_string is None or FLAGS.iqn_prefix:
+        volume_id = _get_volume_id(id)
+        return '%s:%s' % (FLAGS.iqn_prefix, volume_id)
+
+
+@defer.inlineCallbacks
+def _get_target(volume_id):
+    """
+    Gets iscsi name and portal from volume name and host.
+    For this method to work the following are needed:
+    1) volume_ref['host'] must resolve to something rather than loopback
+    2) ietd must bind only to the address as resolved above
+    If any of the two conditions are not met, fall back on Flags.
+    """
+    volume_ref = db.volume_get_by_ec2_id(context.get_admin_context(),
+                                         volume_id)
+
+    (r, _e) = yield process.simple_execute("sudo iscsiadm -m discovery -t "
+                                     "sendtargets -p %s" %
+                                     volume_ref['host'])
+    if len(_e) == 0:
+        for target in r.splitlines():
+            if volume_id in target:
+                (location, _sep, iscsi_name) = target.partition(" ")
+                break
+        iscsi_portal = location.split(",")[0]
+        defer.returnValue((iscsi_name, iscsi_portal))
+    else:
+        defer.returnValue((None, None))
diff --git a/nova/virt/xenapi/volumeops.py b/nova/virt/xenapi/volumeops.py
index a052aaf95..4055688e3 100644
--- a/nova/virt/xenapi/volumeops.py
+++ b/nova/virt/xenapi/volumeops.py
@@ -18,14 +18,12 @@
 Management class for Storage-related functions (attach, detach, etc).
 """
 import logging
-import XenAPI
 
 from twisted.internet import defer
 
-from volume_utils import VolumeHelper
-from vm_utils import VMHelper
-
-from novadeps import Volume
+from nova.virt.xenapi.vm_utils import VMHelper
+from nova.virt.xenapi.volume_utils import VolumeHelper
+from nova.virt.xenapi.volume_utils import StorageError
 
 
 class VolumeOps(object):
@@ -47,22 +45,18 @@ class VolumeOps(object):
                       instance_name, device_path, mountpoint)
         # Create the iSCSI SR, and the PDB through which hosts access SRs.
         # But first, retrieve target info, like Host, IQN, LUN and SCSIID
-        vol_rec = yield Volume.parse_volume_info(device_path, mountpoint)
+        vol_rec = yield VolumeHelper.parse_volume_info(device_path, mountpoint)
         label = 'SR-%s' % vol_rec['volumeId']
         description = 'Disk-for:%s' % instance_name
         # Create SR
         sr_ref = yield VolumeHelper.create_iscsi_storage(self._session,
-                                                         vol_rec['targetHost'],
-                                                         vol_rec['targetPort'],
-                                                         vol_rec['targeIQN'],
-                                                         '',  # no CHAP auth
-                                                         '',
+                                                         vol_rec,
                                                          label,
                                                          description)
         # Introduce VDI  and attach VBD to VM
         try:
             vdi_ref = yield VolumeHelper.introduce_vdi(self._session, sr_ref)
-        except Exception, exc:
+        except StorageError, exc:
             logging.warn(exc)
             yield VolumeHelper.destroy_iscsi_storage(self._session, sr_ref)
             raise Exception('Unable to create VDI on SR %s for instance %s'
@@ -74,25 +68,26 @@ class VolumeOps(object):
                                                     vm_ref, vdi_ref,
                                                     vol_rec['deviceNumber'],
                                                     False)
-            except Exception, exc:
+            except StorageError, exc:
                 logging.warn(exc)
                 yield VolumeHelper.destroy_iscsi_storage(self._session, sr_ref)
-                raise Exception('Unable to create VBD on SR %s for instance %s'
+                raise StorageError('Unable to use SR %s for instance %s'
                             % (sr_ref,
                             instance_name))
             else:
                 try:
-                    #raise Exception('')
                     task = yield self._session.call_xenapi('Async.VBD.plug',
                                                            vbd_ref)
                     yield self._session.wait_for_task(task)
-                except Exception, exc:
+                except StorageError, exc:
                     logging.warn(exc)
                     yield VolumeHelper.destroy_iscsi_storage(self._session,
                                                              sr_ref)
                     raise Exception('Unable to attach volume to instance %s' %
                                     instance_name)
-        yield True
+        logging.info('Mountpoint %s attached to instance %s',
+                     mountpoint, instance_name)
+        yield
 
     @defer.inlineCallbacks
     def detach_volume(self, instance_name, mountpoint):
@@ -103,11 +98,11 @@ class VolumeOps(object):
             raise Exception('Instance %s does not exist' % instance_name)
         # Detach VBD from VM
         logging.debug("Detach_volume: %s, %s", instance_name, mountpoint)
-        device_number = Volume.mountpoint_to_number(mountpoint)
+        device_number = VolumeHelper.mountpoint_to_number(mountpoint)
         try:
             vbd_ref = yield VMHelper.find_vbd_by_number(self._session,
                                                         vm_ref, device_number)
-        except Exception, exc:
+        except StorageError, exc:
             logging.warn(exc)
             raise Exception('Unable to locate volume %s' % mountpoint)
         else:
@@ -115,13 +110,15 @@ class VolumeOps(object):
                 sr_ref = yield VolumeHelper.find_sr_from_vbd(self._session,
                                                              vbd_ref)
                 yield VMHelper.unplug_vbd(self._session, vbd_ref)
-            except Exception, exc:
+            except StorageError, exc:
                 logging.warn(exc)
                 raise Exception('Unable to detach volume %s' % mountpoint)
             try:
                 yield VMHelper.destroy_vbd(self._session, vbd_ref)
-            except Exception, exc:
-                    logging.warn(exc)
+            except StorageError, exc:
+                logging.warn(exc)
         # Forget SR
         yield VolumeHelper.destroy_iscsi_storage(self._session, sr_ref)
-        yield True
+        logging.info('Mountpoint %s detached from instance %s',
+                     mountpoint, instance_name)
+        yield
diff --git a/nova/virt/xenapi_conn.py b/nova/virt/xenapi_conn.py
index 58a505d89..f0819ec65 100644
--- a/nova/virt/xenapi_conn.py
+++ b/nova/virt/xenapi_conn.py
@@ -52,26 +52,51 @@ reactor thread if the VM.get_by_name_label or VM.get_record calls block.
 
 import logging
 import xmlrpclib
+import XenAPI
 
 from twisted.internet import defer
 from twisted.internet import reactor
 
 from nova import utils
-
-from xenapi.vmops import VMOps
-from xenapi.volumeops import VolumeOps
-from xenapi.novadeps import Configuration
-from xenapi import XenAPI
-
-Config = Configuration()
+from nova import flags
+from nova.virt.xenapi.vmops import VMOps
+from nova.virt.xenapi.volumeops import VolumeOps
+
+FLAGS = flags.FLAGS
+flags.DEFINE_string('xenapi_connection_url',
+                    None,
+                    'URL for connection to XenServer/Xen Cloud Platform.'
+                    ' Required if connection_type=xenapi.')
+flags.DEFINE_string('xenapi_connection_username',
+                    'root',
+                    'Username for connection to XenServer/Xen Cloud Platform.'
+                    ' Used only if connection_type=xenapi.')
+flags.DEFINE_string('xenapi_connection_password',
+                    None,
+                    'Password for connection to XenServer/Xen Cloud Platform.'
+                    ' Used only if connection_type=xenapi.')
+flags.DEFINE_float('xenapi_task_poll_interval',
+                   0.5,
+                   'The interval used for polling of remote tasks '
+                   '(Async.VM.start, etc).  Used only if '
+                   'connection_type=xenapi.')
+flags.DEFINE_string('target_host',
+                    None,
+                    'iSCSI Target Host')
+flags.DEFINE_string('target_port',
+                    '3260',
+                    'iSCSI Target Port, 3260 Default')
+flags.DEFINE_string('iqn_prefix',
+                    'iqn.2010-10.org.openstack',
+                    'IQN Prefix')
 
 
 def get_connection(_):
     """Note that XenAPI doesn't have a read-only connection mode, so
     the read_only parameter is ignored."""
-    url = Config.xenapi_connection_url
-    username = Config.xenapi_connection_username
-    password = Config.xenapi_connection_password
+    url = FLAGS.xenapi_connection_url
+    username = FLAGS.xenapi_connection_username
+    password = FLAGS.xenapi_connection_password
     if not url or password is None:
         raise Exception('Must specify xenapi_connection_url, '
                         'xenapi_connection_username (optionally), and '
@@ -168,7 +193,7 @@ class XenAPISession(object):
             #logging.debug('Polling task %s...', task)
             status = self._session.xenapi.task.get_status(task)
             if status == 'pending':
-                reactor.callLater(Config.xenapi_task_poll_interval,
+                reactor.callLater(FLAGS.xenapi_task_poll_interval,
                                   self._poll_task, task, deferred)
             elif status == 'success':
                 result = self._session.xenapi.task.get_result(task)