merged trunk

7 files changed, 189 insertions, 108 deletions

diff --git a/nova/db/api.py b/nova/db/api.py
index 507f70dd5..2db462bdd 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -139,12 +139,20 @@ def floating_ip_get_instance(context, address):
 ####################
 
 
-def fixed_ip_allocate(context, network_id):
-    """Allocate free fixed ip and return the address.
+def fixed_ip_associate(context, address, instance_id):
+    """Associate fixed ip to instance.
+
+    Raises if fixed ip is not available.
+    """
+    return IMPL.fixed_ip_associate(context, address, instance_id)
+
+
+def fixed_ip_associate_pool(context, network_id, instance_id):
+    """Find free ip in network and associate it to instance.
 
     Raises if one is not available.
     """
-    return IMPL.fixed_ip_allocate(context, network_id)
+    return IMPL.fixed_ip_associate_pool(context, network_id, instance_id)
 
 
 def fixed_ip_create(context, values):
@@ -152,9 +160,9 @@ def fixed_ip_create(context, values):
     return IMPL.fixed_ip_create(context, values)
 
 
-def fixed_ip_deallocate(context, address):
-    """Deallocate a fixed ip by address."""
-    return IMPL.fixed_ip_deallocate(context, address)
+def fixed_ip_disassociate(context, address):
+    """Disassociate a fixed ip from an instance by address."""
+    return IMPL.fixed_ip_disassociate(context, address)
 
 
 def fixed_ip_get_by_address(context, address):
@@ -172,16 +180,6 @@ def fixed_ip_get_network(context, address):
     return IMPL.fixed_ip_get_network(context, address)
 
 
-def fixed_ip_instance_associate(context, address, instance_id):
-    """Associate a fixed ip to an instance by address."""
-    return IMPL.fixed_ip_instance_associate(context, address, instance_id)
-
-
-def fixed_ip_instance_disassociate(context, address):
-    """Disassociate a fixed ip from an instance by address."""
-    return IMPL.fixed_ip_instance_disassociate(context, address)
-
-
 def fixed_ip_update(context, address, values):
     """Create a fixed ip from the values dictionary."""
     return IMPL.fixed_ip_update(context, address, values)
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index ce97f6710..be99a8eeb 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -174,7 +174,25 @@ def floating_ip_get_instance(_context, address):
 ###################
 
 
-def fixed_ip_allocate(_context, network_id):
+def fixed_ip_associate(_context, address, instance_id):
+    session = get_session()
+    with session.begin():
+        fixed_ip_ref = session.query(models.FixedIp
+                             ).filter_by(address=address
+                             ).filter_by(deleted=False
+                             ).filter_by(instance=None
+                             ).with_lockmode('update'
+                             ).first()
+        # NOTE(vish): if with_lockmode isn't supported, as in sqlite,
+        #             then this has concurrency issues
+        if not fixed_ip_ref:
+            raise db.NoMoreAddresses()
+        fixed_ip_ref.instance = models.Instance.find(instance_id,
+                                                     session=session)
+        session.add(fixed_ip_ref)
+
+
+def fixed_ip_associate_pool(_context, network_id, instance_id):
     session = get_session()
     with session.begin():
         network_or_none = or_(models.FixedIp.network_id == network_id,
@@ -182,9 +200,8 @@ def fixed_ip_allocate(_context, network_id):
         fixed_ip_ref = session.query(models.FixedIp
                              ).filter(network_or_none
                              ).filter_by(reserved=False
-                             ).filter_by(allocated=False
-                             ).filter_by(leased=False
                              ).filter_by(deleted=False
+                             ).filter_by(instance=None
                              ).with_lockmode('update'
                              ).first()
         # NOTE(vish): if with_lockmode isn't supported, as in sqlite,
@@ -194,7 +211,8 @@ def fixed_ip_allocate(_context, network_id):
         if not fixed_ip_ref.network:
             fixed_ip_ref.network = models.Network.find(network_id,
                                                        session=session)
-        fixed_ip_ref['allocated'] = True
+        fixed_ip_ref.instance = models.Instance.find(instance_id,
+                                                     session=session)
         session.add(fixed_ip_ref)
     return fixed_ip_ref['address']
 
@@ -207,6 +225,14 @@ def fixed_ip_create(_context, values):
     return fixed_ip_ref['address']
 
 
+def fixed_ip_disassociate(_context, address):
+    session = get_session()
+    with session.begin():
+        fixed_ip_ref = models.FixedIp.find_by_str(address, session=session)
+        fixed_ip_ref.instance = None
+        fixed_ip_ref.save(session=session)
+
+
 def fixed_ip_get_by_address(_context, address):
     return models.FixedIp.find_by_str(address)
 
@@ -223,27 +249,6 @@ def fixed_ip_get_network(_context, address):
         return models.FixedIp.find_by_str(address, session=session).network
 
 
-def fixed_ip_deallocate(context, address):
-    db.fixed_ip_update(context, address, {'allocated': False})
-
-
-def fixed_ip_instance_associate(_context, address, instance_id):
-    session = get_session()
-    with session.begin():
-        fixed_ip_ref = models.FixedIp.find_by_str(address, session=session)
-        instance_ref = models.Instance.find(instance_id, session=session)
-        fixed_ip_ref.instance = instance_ref
-        fixed_ip_ref.save(session=session)
-
-
-def fixed_ip_instance_disassociate(_context, address):
-    session = get_session()
-    with session.begin():
-        fixed_ip_ref = models.FixedIp.find_by_str(address, session=session)
-        fixed_ip_ref.instance = None
-        fixed_ip_ref.save(session=session)
-
-
 def fixed_ip_update(_context, address, values):
     session = get_session()
     with session.begin():
diff --git a/nova/endpoint/cloud.py b/nova/endpoint/cloud.py
index f30565aca..a88b1870e 100644
--- a/nova/endpoint/cloud.py
+++ b/nova/endpoint/cloud.py
@@ -632,7 +632,7 @@ class CloudController(object):
                 # NOTE(vish): Currently, nothing needs to be done on the
                 #             network node until release. If this changes,
                 #             we will need to cast here.
-                db.fixed_ip_deallocate(context, address)
+                self.network.deallocate_fixed_ip(context, address)
 
             host = instance_ref['host']
             if host:
diff --git a/nova/network/manager.py b/nova/network/manager.py
index 3212a7eab..7a3bcfc2f 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -61,6 +61,8 @@ flags.DEFINE_integer('cnt_vpn_clients', 5,
                      'Number of addresses reserved for vpn clients')
 flags.DEFINE_string('network_driver', 'nova.network.linux_net',
                     'Driver to use for network creation')
+flags.DEFINE_bool('update_dhcp_on_disassociate', False,
+                  'Whether to update dhcp when fixed_ip is disassocated')
 
 
 class AddressAlreadyAllocated(exception.Error):
@@ -96,6 +98,10 @@ class NetworkManager(manager.Manager):
         """Gets a fixed ip from the pool"""
         raise NotImplementedError()
 
+    def deallocate_fixed_ip(self, context, instance_id, *args, **kwargs):
+        """Returns a fixed ip to the pool"""
+        raise NotImplementedError()
+
     def setup_fixed_ip(self, context, address):
         """Sets up rules for fixed ip"""
         raise NotImplementedError()
@@ -170,10 +176,17 @@ class FlatManager(NetworkManager):
     def allocate_fixed_ip(self, context, instance_id, *args, **kwargs):
         """Gets a fixed ip from the pool"""
         network_ref = self.db.project_get_network(context, context.project.id)
-        address = self.db.fixed_ip_allocate(context, network_ref['id'])
-        self.db.fixed_ip_instance_associate(context, address, instance_id)
+        address = self.db.fixed_ip_associate_pool(context,
+                                                  network_ref['id'],
+                                                  instance_id)
+        self.db.fixed_ip_update(context, address, {'allocated': True})
         return address
 
+    def deallocate_fixed_ip(self, context, address, *args, **kwargs):
+        """Returns a fixed ip to the pool"""
+        self.db.fixed_ip_update(context, address, {'allocated': False})
+        self.db.fixed_ip_disassociate(context, address)
+
     def setup_compute_network(self, context, project_id):
         """Network is created manually"""
         pass
@@ -209,13 +222,29 @@ class VlanManager(NetworkManager):
         """Gets a fixed ip from the pool"""
         network_ref = self.db.project_get_network(context, context.project.id)
         if kwargs.get('vpn', None):
-            address = self._allocate_vpn_ip(context, network_ref['id'])
+            address = network_ref['vpn_private_address']
+            self.db.fixed_ip_associate(context, address, instance_id)
         else:
-            address = self.db.fixed_ip_allocate(context,
-                                                network_ref['id'])
-        self.db.fixed_ip_instance_associate(context, address, instance_id)
+            address = self.db.fixed_ip_associate_pool(context,
+                                                      network_ref['id'],
+                                                      instance_id)
+        self.db.fixed_ip_update(context, address, {'allocated': True})
         return address
 
+    def deallocate_fixed_ip(self, context, address, *args, **kwargs):
+        """Returns a fixed ip to the pool"""
+        self.db.fixed_ip_update(context, address, {'allocated': False})
+        fixed_ip_ref = self.db.fixed_ip_get_by_address(context, address)
+        if not fixed_ip_ref['leased']:
+            self.db.fixed_ip_disassociate(context, address)
+            # NOTE(vish): dhcp server isn't updated until next setup, this
+            #             means there will stale entries in the conf file
+            #             the code below will update the file if necessary
+            if FLAGS.update_dhcp_on_disassociate:
+                network_ref = self.db.fixed_ip_get_network(context, address)
+                self.driver.update_dhcp(context, network_ref['id'])
+
+
     def setup_fixed_ip(self, context, address):
         """Sets forwarding rules and dhcp for fixed ip"""
         fixed_ip_ref = self.db.fixed_ip_get_by_address(context, address)
@@ -226,22 +255,47 @@ class VlanManager(NetworkManager):
                                             network_ref['vpn_private_address'])
         self.driver.update_dhcp(context, network_ref['id'])
 
-    def lease_fixed_ip(self, context, address):
+    def lease_fixed_ip(self, context, mac, address):
         """Called by dhcp-bridge when ip is leased"""
         logging.debug("Leasing IP %s", address)
         fixed_ip_ref = self.db.fixed_ip_get_by_address(context, address)
         if not fixed_ip_ref['allocated']:
             logging.warn("IP %s leased that was already deallocated", address)
+            return
+        instance_ref = self.db.fixed_ip_get_instance(context, address)
+        if not instance_ref:
+            raise exception.Error("IP %s leased that isn't associated" %
+                                  address)
+        if instance_ref['mac_address'] != mac:
+            raise exception.Error("IP %s leased to bad mac %s vs %s" %
+                                  (address, instance_ref['mac_address'], mac))
         self.db.fixed_ip_update(context,
                                 fixed_ip_ref['str_id'],
                                 {'leased': True})
 
-    def release_fixed_ip(self, context, address):
+    def release_fixed_ip(self, context, mac, address):
         """Called by dhcp-bridge when ip is released"""
         logging.debug("Releasing IP %s", address)
-        self.db.fixed_ip_update(context, address, {'allocated': False,
-                                                   'leased': False})
-        self.db.fixed_ip_instance_disassociate(context, address)
+        fixed_ip_ref = self.db.fixed_ip_get_by_address(context, address)
+        if not fixed_ip_ref['leased']:
+            logging.warn("IP %s released that was not leased", address)
+            return
+        instance_ref = self.db.fixed_ip_get_instance(context, address)
+        if not instance_ref:
+            raise exception.Error("IP %s released that isn't associated" %
+                                  address)
+        if instance_ref['mac_address'] != mac:
+            raise exception.Error("IP %s released from bad mac %s vs %s" %
+                                  (address, instance_ref['mac_address'], mac))
+        self.db.fixed_ip_update(context, address, {'leased': False})
+        if not fixed_ip_ref['allocated']:
+            self.db.fixed_ip_disassociate(context, address)
+            # NOTE(vish): dhcp server isn't updated until next setup, this
+            #             means there will stale entries in the conf file
+            #             the code below will update the file if necessary
+            if FLAGS.update_dhcp_on_disassociate:
+                network_ref = self.db.fixed_ip_get_network(context, address)
+                self.driver.update_dhcp(context, network_ref['id'])
 
     def allocate_network(self, context, project_id):
         """Set up the network"""
@@ -283,19 +337,6 @@ class VlanManager(NetworkManager):
         # TODO(vish): Implement this
         pass
 
-    @staticmethod
-    def _allocate_vpn_ip(context, network_id):
-        """Allocate vpn ip for network"""
-        # TODO(vish): There is a possible concurrency issue here.
-        network_ref = db.network_get(context, network_id)
-        address = network_ref['vpn_private_address']
-        fixed_ip_ref = db.fixed_ip_get_by_address(context, address)
-        # TODO(vish): Should this be fixed_ip_is_allocated?
-        if fixed_ip_ref['allocated']:
-            raise AddressAlreadyAllocated()
-        db.fixed_ip_update(context, fixed_ip_ref['id'], {'allocated': True})
-        return fixed_ip_ref['str_id']
-
     def _ensure_indexes(self, context):
         """Ensure the indexes for the network exist
 
diff --git a/nova/tests/cloud_unittest.py b/nova/tests/cloud_unittest.py
index e56ea6ac2..317200e01 100644
--- a/nova/tests/cloud_unittest.py
+++ b/nova/tests/cloud_unittest.py
@@ -34,6 +34,7 @@ from nova import rpc
 from nova import test
 from nova import utils
 from nova.auth import manager
+from nova.compute import power_state
 from nova.endpoint import api
 from nova.endpoint import cloud
 
@@ -131,7 +132,7 @@ class CloudTestCase(test.BaseTestCase):
             rv = yield defer.succeed(time.sleep(1))
             info = self.cloud._get_instance(instance['instance_id'])
             logging.debug(info['state'])
-            if info['state'] == node.Instance.RUNNING:
+            if info['state'] == power_state.RUNNING:
                 break
         self.assert_(rv)
 
diff --git a/nova/tests/network_unittest.py b/nova/tests/network_unittest.py
index 9958600e0..dc5277f02 100644
--- a/nova/tests/network_unittest.py
+++ b/nova/tests/network_unittest.py
@@ -28,6 +28,7 @@ from nova import flags
 from nova import test
 from nova import utils
 from nova.auth import manager
+from nova.endpoint import api
 
 FLAGS = flags.FLAGS
 
@@ -48,7 +49,7 @@ class NetworkTestCase(test.TrialTestCase):
         self.user = self.manager.create_user('netuser', 'netuser', 'netuser')
         self.projects = []
         self.network = utils.import_object(FLAGS.network_manager)
-        self.context = None
+        self.context = api.APIRequestContext(None, project=None, user=self.user)
         for i in range(5):
             name = 'project%s' % i
             self.projects.append(self.manager.create_project(name,
@@ -75,12 +76,10 @@ class NetworkTestCase(test.TrialTestCase):
 
     def _create_address(self, project_num, instance_id=None):
         """Create an address in given project num"""
-        net = db.project_get_network(None, self.projects[project_num].id)
-        address = db.fixed_ip_allocate(None, net['id'])
         if instance_id is None:
             instance_id = self.instance_id
-        db.fixed_ip_instance_associate(None, address, instance_id)
-        return address
+        self.context.project = self.projects[project_num]
+        return self.network.allocate_fixed_ip(self.context, instance_id)
 
     def test_public_network_association(self):
         """Makes sure that we can allocaate a public ip"""
@@ -103,14 +102,14 @@ class NetworkTestCase(test.TrialTestCase):
         address = db.instance_get_floating_address(None, self.instance_id)
         self.assertEqual(address, None)
         self.network.deallocate_floating_ip(self.context, float_addr)
-        db.fixed_ip_deallocate(None, fix_addr)
+        self.network.deallocate_fixed_ip(self.context, fix_addr)
 
     def test_allocate_deallocate_fixed_ip(self):
         """Makes sure that we can allocate and deallocate a fixed ip"""
         address = self._create_address(0)
         self.assertTrue(is_allocated_in_project(address, self.projects[0].id))
         lease_ip(address)
-        db.fixed_ip_deallocate(None, address)
+        self.network.deallocate_fixed_ip(self.context, address)
 
         # Doesn't go away until it's dhcp released
         self.assertTrue(is_allocated_in_project(address, self.projects[0].id))
@@ -131,14 +130,14 @@ class NetworkTestCase(test.TrialTestCase):
         lease_ip(address)
         lease_ip(address2)
 
-        db.fixed_ip_deallocate(None, address)
+        self.network.deallocate_fixed_ip(self.context, address)
         release_ip(address)
         self.assertFalse(is_allocated_in_project(address, self.projects[0].id))
 
         # First address release shouldn't affect the second
         self.assertTrue(is_allocated_in_project(address2, self.projects[1].id))
 
-        db.fixed_ip_deallocate(None, address2)
+        self.network.deallocate_fixed_ip(self.context, address2)
         release_ip(address2)
         self.assertFalse(is_allocated_in_project(address2,
                                                  self.projects[1].id))
@@ -147,10 +146,23 @@ class NetworkTestCase(test.TrialTestCase):
         """Makes sure that private ips don't overlap"""
         first = self._create_address(0)
         lease_ip(first)
+        instance_ids = []
         for i in range(1, 5):
-            address = self._create_address(i)
-            address2 = self._create_address(i)
-            address3 = self._create_address(i)
+            mac = utils.generate_mac()
+            instance_ref = db.instance_create(None,
+                                              {'mac_address': mac})
+            instance_ids.append(instance_ref['id'])
+            address = self._create_address(i, instance_ref['id'])
+            mac = utils.generate_mac()
+            instance_ref = db.instance_create(None,
+                                              {'mac_address': mac})
+            instance_ids.append(instance_ref['id'])
+            address2 = self._create_address(i, instance_ref['id'])
+            mac = utils.generate_mac()
+            instance_ref = db.instance_create(None,
+                                              {'mac_address': mac})
+            instance_ids.append(instance_ref['id'])
+            address3 = self._create_address(i, instance_ref['id'])
             lease_ip(address)
             lease_ip(address2)
             lease_ip(address3)
@@ -160,14 +172,16 @@ class NetworkTestCase(test.TrialTestCase):
                                                      self.projects[0].id))
             self.assertFalse(is_allocated_in_project(address3,
                                                      self.projects[0].id))
-            db.fixed_ip_deallocate(None, address)
-            db.fixed_ip_deallocate(None, address2)
-            db.fixed_ip_deallocate(None, address3)
+            self.network.deallocate_fixed_ip(self.context, address)
+            self.network.deallocate_fixed_ip(self.context, address2)
+            self.network.deallocate_fixed_ip(self.context, address3)
             release_ip(address)
             release_ip(address2)
             release_ip(address3)
+        for instance_id in instance_ids:
+            db.instance_destroy(None, instance_id)
         release_ip(first)
-        db.fixed_ip_deallocate(None, first)
+        self.network.deallocate_fixed_ip(self.context, first)
 
     def test_vpn_ip_and_port_looks_valid(self):
         """Ensure the vpn ip and port are reasonable"""
@@ -194,12 +208,12 @@ class NetworkTestCase(test.TrialTestCase):
         """Makes sure that ip addresses that are deallocated get reused"""
         address = self._create_address(0)
         lease_ip(address)
-        db.fixed_ip_deallocate(None, address)
+        self.network.deallocate_fixed_ip(self.context, address)
         release_ip(address)
 
         address2 = self._create_address(0)
         self.assertEqual(address, address2)
-        db.fixed_ip_deallocate(None, address2)
+        self.network.deallocate_fixed_ip(self.context, address2)
 
     def test_available_ips(self):
         """Make sure the number of available ips for the network is correct
@@ -226,21 +240,27 @@ class NetworkTestCase(test.TrialTestCase):
         num_available_ips = db.network_count_available_ips(None,
                                                            network['id'])
         addresses = []
+        instance_ids = []
         for i in range(num_available_ips):
-            address = self._create_address(0)
+            mac = utils.generate_mac()
+            instance_ref = db.instance_create(None,
+                                              {'mac_address': mac})
+            instance_ids.append(instance_ref['id'])
+            address = self._create_address(0, instance_ref['id'])
             addresses.append(address)
             lease_ip(address)
 
         self.assertEqual(db.network_count_available_ips(None,
                                                         network['id']), 0)
         self.assertRaises(db.NoMoreAddresses,
-                          db.fixed_ip_allocate,
-                          None,
-                          network['id'])
+                          self.network.allocate_fixed_ip,
+                          self.context,
+                          'foo')
 
-        for i in range(len(addresses)):
-            db.fixed_ip_deallocate(None, addresses[i])
+        for i in range(num_available_ips):
+            self.network.deallocate_fixed_ip(self.context, addresses[i])
             release_ip(addresses[i])
+            db.instance_destroy(None, instance_ids[i])
         self.assertEqual(db.network_count_available_ips(None,
                                                         network['id']),
                          num_available_ips)
@@ -263,7 +283,10 @@ def binpath(script):
 def lease_ip(private_ip):
     """Run add command on dhcpbridge"""
     network_ref = db.fixed_ip_get_network(None, private_ip)
-    cmd = "%s add fake %s fake" % (binpath('nova-dhcpbridge'), private_ip)
+    instance_ref = db.fixed_ip_get_instance(None, private_ip)
+    cmd = "%s add %s %s fake" % (binpath('nova-dhcpbridge'),
+                                 instance_ref['mac_address'],
+                                 private_ip)
     env = {'DNSMASQ_INTERFACE': network_ref['bridge'],
            'TESTING': '1',
            'FLAGFILE': FLAGS.dhcpbridge_flagfile}
@@ -274,7 +297,10 @@ def lease_ip(private_ip):
 def release_ip(private_ip):
     """Run del command on dhcpbridge"""
     network_ref = db.fixed_ip_get_network(None, private_ip)
-    cmd = "%s del fake %s fake" % (binpath('nova-dhcpbridge'), private_ip)
+    instance_ref = db.fixed_ip_get_instance(None, private_ip)
+    cmd = "%s del %s %s fake" % (binpath('nova-dhcpbridge'),
+                                 instance_ref['mac_address'],
+                                 private_ip)
     env = {'DNSMASQ_INTERFACE': network_ref['bridge'],
            'TESTING': '1',
            'FLAGFILE': FLAGS.dhcpbridge_flagfile}
diff --git a/nova/virt/xenapi.py b/nova/virt/xenapi.py
index b44ac383a..1c6de4403 100644
--- a/nova/virt/xenapi.py
+++ b/nova/virt/xenapi.py
@@ -274,9 +274,19 @@ class XenAPIConnection(object):
     def destroy(self, instance):
         vm = yield self._lookup(instance.name)
         if vm is None:
-            raise Exception('instance not present %s' % instance.name)
-        task = yield self._call_xenapi('Async.VM.destroy', vm)
-        yield self._wait_for_task(task)
+            # Don't complain, just return.  This lets us clean up instances
+            # that have already disappeared from the underlying platform.
+            defer.returnValue(None)
+        try:
+            task = yield self._call_xenapi('Async.VM.hard_shutdown', vm)
+            yield self._wait_for_task(task)
+        except Exception, exc:
+            logging.warn(exc)
+        try:
+            task = yield self._call_xenapi('Async.VM.destroy', vm)
+            yield self._wait_for_task(task)
+        except Exception, exc:
+            logging.warn(exc)
 
     def get_info(self, instance_id):
         vm = self._lookup_blocking(instance_id)
@@ -330,9 +340,9 @@ class XenAPIConnection(object):
                              error_info)
                 deferred.errback(XenAPI.Failure(error_info))
             #logging.debug('Polling task %s done.', task)
-        except Exception, exn:
-            logging.warn(exn)
-            deferred.errback(exn)
+        except Exception, exc:
+            logging.warn(exc)
+            deferred.errback(exc)
 
     @utils.deferredToThread
     def _call_xenapi(self, method, *args):
@@ -358,21 +368,21 @@ class XenAPIConnection(object):
 def _unwrap_plugin_exceptions(func, *args, **kwargs):
     try:
         return func(*args, **kwargs)
-    except XenAPI.Failure, exn:
-        logging.debug("Got exception: %s", exn)
-        if (len(exn.details) == 4 and
-            exn.details[0] == 'XENAPI_PLUGIN_EXCEPTION' and
-            exn.details[2] == 'Failure'):
+    except XenAPI.Failure, exc:
+        logging.debug("Got exception: %s", exc)
+        if (len(exc.details) == 4 and
+            exc.details[0] == 'XENAPI_PLUGIN_EXCEPTION' and
+            exc.details[2] == 'Failure'):
             params = None
             try:
-                params = eval(exn.details[3])
+                params = eval(exc.details[3])
             except:
-                raise exn
+                raise exc
             raise XenAPI.Failure(params)
         else:
             raise
-    except xmlrpclib.ProtocolError, exn:
-        logging.debug("Got exception: %s", exn)
+    except xmlrpclib.ProtocolError, exc:
+        logging.debug("Got exception: %s", exc)
         raise