merge with lp:~armando-migliaccio/nova/xenapi-refactoring

7 files changed, 119 insertions, 42 deletions

diff --git a/nova/virt/xenapi/__init__.py b/nova/virt/xenapi/__init__.py
index 3d598c463..ece430407 100644
--- a/nova/virt/xenapi/__init__.py
+++ b/nova/virt/xenapi/__init__.py
@@ -13,3 +13,14 @@
 #    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/network_utils.py b/nova/virt/xenapi/network_utils.py
index b58b9159c..8cb4cce3a 100644
--- a/nova/virt/xenapi/network_utils.py
+++ b/nova/virt/xenapi/network_utils.py
@@ -15,20 +15,25 @@
 #    under the License.
 
 """
-Helper methods for operations related to the management of network records and
-their attributes like bridges, PIFs, QoS, as well as their lookup functions.
+Helper methods for operations related to the management of network
+records and their attributes like bridges, PIFs, QoS, as well as
+their lookup functions.
 """
 
 from twisted.internet import defer
 
 
 class NetworkHelper():
-    def __init__(self, session):
+    """
+    The class that wraps the helper methods together.
+    """
+    def __init__(self):
         return
 
     @classmethod
     @defer.inlineCallbacks
-    def find_network_with_bridge(self, session, bridge):
+    def find_network_with_bridge(cls, session, bridge):
+        """ Return the network on which the bridge is attached, if found """
         expr = 'field "bridge" = "%s"' % bridge
         networks = yield session.call_xenapi('network.get_all_records_where',
                                            expr)
diff --git a/nova/virt/xenapi/novadeps.py b/nova/virt/xenapi/novadeps.py
index aa3535162..65576019e 100644
--- a/nova/virt/xenapi/novadeps.py
+++ b/nova/virt/xenapi/novadeps.py
@@ -15,13 +15,17 @@
 #    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 process
-from nova import utils
 from nova import context
 
 from nova.compute import power_state
@@ -60,23 +64,28 @@ 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
@@ -96,72 +105,90 @@ config = Configuration()
 
 
 class Instance(object):
+    """ Wraps up instance specifics """
 
     @classmethod
-    def get_name(self, instance):
+    def get_name(cls, instance):
+        """ The name of the instance """
         return instance.name
 
     @classmethod
-    def get_type(self, instance):
+    def get_type(cls, instance):
+        """ The type of the instance """
         return instance_types.INSTANCE_TYPES[instance.instance_type]
 
     @classmethod
-    def get_project(self, instance):
+    def get_project(cls, instance):
+        """ The project the instance belongs """
         return AuthManager().get_project(instance.project_id)
 
     @classmethod
-    def get_project_id(self, instance):
+    def get_project_id(cls, instance):
+        """ The id of the project the instance belongs """
         return instance.project_id
 
     @classmethod
-    def get_image_id(self, instance):
+    def get_image_id(cls, instance):
+        """ The instance's image id """
         return instance.image_id
 
     @classmethod
-    def get_kernel_id(self, instance):
+    def get_kernel_id(cls, instance):
+        """ The instance's kernel id """
         return instance.kernel_id
 
     @classmethod
-    def get_ramdisk_id(self, instance):
+    def get_ramdisk_id(cls, instance):
+        """ The instance's ramdisk id """
         return instance.ramdisk_id
 
     @classmethod
-    def get_network(self, instance):
+    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(self, instance):
+    def get_mac(cls, instance):
+        """ The instance's MAC address """
         return instance.mac_address
 
     @classmethod
-    def get_user(self, instance):
+    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(self, network):
+    def get_bridge(cls, network):
+        """ the bridge for the network """
         return network.bridge
 
 
 class Image(object):
+    """ Wraps up image specifics """
 
     @classmethod
-    def get_url(self, image):
+    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(self, user, project):
+    def get_access(cls, user, project):
+        """ access key """
         return AuthManager().get_access_key(user, project)
 
     @classmethod
-    def get_secret(self, user):
+    def get_secret(cls, user):
+        """ access secret """
         return user.secret
 
 
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index 6966e7b7b..e6d20f98b 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -20,6 +20,7 @@ their attributes like VDIs, VIFs, as well as their lookup functions.
 """
 
 import logging
+import XenAPI
 
 from twisted.internet import defer
 
@@ -31,12 +32,15 @@ from novadeps import User
 
 
 class VMHelper():
-    def __init__(self, session):
+    """
+    The class that wraps the helper methods together.
+    """
+    def __init__(self):
         return
 
     @classmethod
     @defer.inlineCallbacks
-    def create_vm(self, session, instance, kernel, ramdisk):
+    def create_vm(cls, session, instance, kernel, ramdisk):
         """Create a VM record.  Returns a Deferred that gives the new
         VM reference."""
 
@@ -80,7 +84,7 @@ class VMHelper():
 
     @classmethod
     @defer.inlineCallbacks
-    def create_vbd(self, session, vm_ref, vdi_ref, userdevice, bootable):
+    def create_vbd(cls, session, vm_ref, vdi_ref, userdevice, bootable):
         """Create a VBD record.  Returns a Deferred that gives the new
         VBD reference."""
 
@@ -143,7 +147,7 @@ class VMHelper():
 
     @classmethod
     @defer.inlineCallbacks
-    def create_vif(self, session, vm_ref, network_ref, mac_address):
+    def create_vif(cls, session, vm_ref, network_ref, mac_address):
         """Create a VIF record.  Returns a Deferred that gives the new
         VIF reference."""
 
@@ -165,7 +169,7 @@ class VMHelper():
 
     @classmethod
     @defer.inlineCallbacks
-    def fetch_image(self, session, image, user, project, use_sr):
+    def fetch_image(cls, session, image, user, project, use_sr):
         """use_sr: True to put the image as a VDI in an SR, False to place
         it on dom0's filesystem.  The former is for VM disks, the latter for
         its kernel and ramdisk (if external kernels are being used).
@@ -173,7 +177,7 @@ class VMHelper():
 
         url = Image.get_url(image)
         access = User.get_access(user, project)
-        logging.debug("Asking xapi to fetch %s as %s" % (url, access))
+        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
@@ -187,11 +191,13 @@ class VMHelper():
 
     @classmethod
     @utils.deferredToThread
-    def lookup(self, session, i):
+    def lookup(cls, session, i):
+        """ Look the instance i up, and returns it if available """
         return VMHelper.lookup_blocking(session, i)
 
     @classmethod
-    def lookup_blocking(self, session, i):
+    def lookup_blocking(cls, session, i):
+        """ Synchronous lookup """
         vms = session.get_xenapi().VM.get_by_name_label(i)
         n = len(vms)
         if n == 0:
@@ -203,11 +209,13 @@ class VMHelper():
 
     @classmethod
     @utils.deferredToThread
-    def lookup_vm_vdis(self, session, vm):
+    def lookup_vm_vdis(cls, session, vm):
+        """ Look for the VDIs that are attached to the VM """
         return VMHelper.lookup_vm_vdis_blocking(session, vm)
 
     @classmethod
-    def lookup_vm_vdis_blocking(self, session, vm):
+    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?
         vbds = session.get_xenapi().VM.get_VBDs(vm)
@@ -218,7 +226,8 @@ class VMHelper():
                     vdi = session.get_xenapi().VBD.get_VDI(vbd)
                     # Test valid VDI
                     record = session.get_xenapi().VDI.get_record(vdi)
-                except Exception, exc:
+                    logging.debug('VDI %s is still available', record['uuid'])
+                except XenAPI.Failure, exc:
                     logging.warn(exc)
                 else:
                     vdis.append(vdi)
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index d6ea5e7db..3db86f179 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -19,6 +19,7 @@ Management class for VM-related functions (spawn, reboot, etc).
 """
 
 import logging
+import XenAPI
 
 from twisted.internet import defer
 
@@ -31,15 +32,20 @@ from network_utils import NetworkHelper
 
 
 class VMOps(object):
+    """
+    Management class for VM-related tasks
+    """
     def __init__(self, session):
         self._session = session
 
     def list_instances(self):
+        """ List VM instances """
         return [self._session.get_xenapi().VM.get_name_label(vm) \
                 for vm in self._session.get_xenapi().VM.get_all()]
 
     @defer.inlineCallbacks
     def spawn(self, instance):
+        """ Create VM instance """
         vm = yield VMHelper.lookup(self._session, Instance.get_name(instance))
         if vm is not None:
             raise Exception('Attempted to create non-unique name %s' %
@@ -71,6 +77,7 @@ class VMOps(object):
 
     @defer.inlineCallbacks
     def reboot(self, instance):
+        """ Reboot VM instance """
         instance_name = Instance.get_name(instance)
         vm = yield VMHelper.lookup(self._session, instance_name)
         if vm is None:
@@ -80,6 +87,7 @@ class VMOps(object):
 
     @defer.inlineCallbacks
     def destroy(self, instance):
+        """ Destroy VM instance """
         vm = yield VMHelper.lookup(self._session, Instance.get_name(instance))
         if vm is None:
             # Don't complain, just return.  This lets us clean up instances
@@ -91,7 +99,7 @@ class VMOps(object):
             task = yield self._session.call_xenapi('Async.VM.hard_shutdown',
                                                    vm)
             yield self._session.wait_for_task(task)
-        except Exception, exc:
+        except XenAPI.Failure, exc:
             logging.warn(exc)
         # Disk clean-up
         if vdis:
@@ -100,15 +108,16 @@ class VMOps(object):
                     task = yield self._session.call_xenapi('Async.VDI.destroy',
                                                            vdi)
                     yield self._session.wait_for_task(task)
-                except Exception, exc:
+                except XenAPI.Failure, exc:
                     logging.warn(exc)
         try:
             task = yield self._session.call_xenapi('Async.VM.destroy', vm)
             yield self._session.wait_for_task(task)
-        except Exception, exc:
+        except XenAPI.Failure, exc:
             logging.warn(exc)
 
     def get_info(self, instance_id):
+        """ Return data about VM instance """
         vm = VMHelper.lookup_blocking(self._session, instance_id)
         if vm is None:
             raise Exception('instance not present %s' % instance_id)
@@ -120,4 +129,6 @@ class VMOps(object):
                 'cpu_time': 0}
 
     def get_console_output(self, instance):
-        return 'FAKE CONSOLE OUTPUT'
+        """ Return snapshot of console """
+        # TODO: implement this to fix pylint!
+        return 'FAKE CONSOLE OUTPUT of instance'
diff --git a/nova/virt/xenapi/volumeops.py b/nova/virt/xenapi/volumeops.py
index ec4343329..6c48f6491 100644
--- a/nova/virt/xenapi/volumeops.py
+++ b/nova/virt/xenapi/volumeops.py
@@ -18,6 +18,7 @@
 Management class for Storage-related functions (attach, detach, etc).
 """
 import logging
+import XenAPI
 
 from twisted.internet import defer
 
@@ -28,11 +29,15 @@ from novadeps import Volume
 
 
 class VolumeOps(object):
+    """
+    Management class for Volume-related tasks
+    """
     def __init__(self, session):
         self._session = session
 
     @defer.inlineCallbacks
     def attach_volume(self, instance_name, device_path, mountpoint):
+        """ Attach volume storage to VM instance """
         # Before we start, check that the VM exists
         vm_ref = yield VMHelper.lookup(self._session, instance_name)
         if vm_ref is None:
@@ -91,6 +96,7 @@ class VolumeOps(object):
 
     @defer.inlineCallbacks
     def detach_volume(self, instance_name, mountpoint):
+        """ Detach volume storage to VM instance """
         # Before we start, check that the VM exists
         vm_ref = yield VMHelper.lookup(self._session, instance_name)
         if vm_ref is None:
diff --git a/nova/virt/xenapi_conn.py b/nova/virt/xenapi_conn.py
index d3f66b12c..58a505d89 100644
--- a/nova/virt/xenapi_conn.py
+++ b/nova/virt/xenapi_conn.py
@@ -61,19 +61,14 @@ from nova import utils
 from xenapi.vmops import VMOps
 from xenapi.volumeops import VolumeOps
 from xenapi.novadeps import Configuration
+from xenapi import XenAPI
 
-XenAPI = None
 Config = Configuration()
 
 
 def get_connection(_):
     """Note that XenAPI doesn't have a read-only connection mode, so
     the read_only parameter is ignored."""
-    # This is loaded late so that there's no need to install this
-    # library when not using XenAPI.
-    global XenAPI
-    if XenAPI is None:
-        XenAPI = __import__('XenAPI')
     url = Config.xenapi_connection_url
     username = Config.xenapi_connection_username
     password = Config.xenapi_connection_password
@@ -86,47 +81,59 @@ def get_connection(_):
 
 
 class XenAPIConnection(object):
+    """ A connection to XenServer or Xen Cloud Platform """
     def __init__(self, url, user, pw):
         session = XenAPISession(url, user, pw)
         self._vmops = VMOps(session)
         self._volumeops = VolumeOps(session)
 
     def list_instances(self):
+        """ List VM instances """
         return self._vmops.list_instances()
 
     def spawn(self, instance):
+        """ Create VM instance """
         self._vmops.spawn(instance)
 
     def reboot(self, instance):
+        """ Reboot VM instance """
         self._vmops.reboot(instance)
 
     def destroy(self, instance):
+        """ Destroy VM instance """
         self._vmops.destroy(instance)
 
     def get_info(self, instance_id):
+        """ Return data about VM instance """
         return self._vmops.get_info(instance_id)
 
     def get_console_output(self, instance):
+        """ Return snapshot of console """
         return self._vmops.get_console_output(instance)
 
     def attach_volume(self, instance_name, device_path, mountpoint):
+        """ Attach volume storage to VM instance """
         return self._volumeops.attach_volume(instance_name,
                                                device_path,
                                                mountpoint)
 
     def detach_volume(self, instance_name, mountpoint):
+        """ Detach volume storage to VM instance """
         return self._volumeops.detach_volume(instance_name, mountpoint)
 
 
 class XenAPISession(object):
+    """ The session to invoke XenAPI SDK calls """
     def __init__(self, url, user, pw):
         self._session = XenAPI.Session(url)
         self._session.login_with_password(user, pw)
 
     def get_xenapi(self):
+        """ Return the xenapi object """
         return self._session.xenapi
 
     def get_xenapi_host(self):
+        """ Return the xenapi host """
         return self._session.xenapi.session.get_this_host(self._session.handle)
 
     @utils.deferredToThread
@@ -173,12 +180,13 @@ class XenAPISession(object):
                              error_info)
                 deferred.errback(XenAPI.Failure(error_info))
             #logging.debug('Polling task %s done.', task)
-        except Exception, exc:
+        except XenAPI.Failure, exc:
             logging.warn(exc)
             deferred.errback(exc)
 
 
 def _unwrap_plugin_exceptions(func, *args, **kwargs):
+    """ Parse exception details """
     try:
         return func(*args, **kwargs)
     except XenAPI.Failure, exc: