From dacb187fcd685c15fd084fdfce8c5e165130ff54 Mon Sep 17 00:00:00 2001
From: Melanie Witt <melwitt@yahoo-inc.com>
Date: Thu, 23 May 2013 00:23:16 +0000
Subject: [PATCH] Call virt.driver.destroy before deallocating network.

Call virt driver destroy before releasing the instance's
ip address, so that certain failures can be handled.
For instance, if destroy raises InstancePowerOffFailure,
do not deallocate network resources.

Fixes bug 1180178

Change-Id: I9bde984d2393bd73a73eae6065defb2ed7c01492
---
 nova/compute/manager.py            | 39 +++++++++++++++++++++---------
 nova/tests/compute/test_compute.py | 30 +++++++++++++++++++++++
 2 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 8a2ddf8770da..ac862ce4e409 100755
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -1279,6 +1279,16 @@ class ComputeManager(manager.SchedulerDependentManager):
                     admin_password, is_first_time, node, instance)
         do_run_instance()
 
+    def _try_deallocate_network(self, context, instance):
+        try:
+            # tear down allocated network structure
+            self._deallocate_network(context, instance)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                LOG.error(_('Failed to deallocate network for instance.'),
+                          instance=instance)
+                self._set_instance_error_state(context, instance['uuid'])
+
     def _shutdown_instance(self, context, instance, bdms):
         """Shutdown an instance on this host."""
         context = context.elevated()
@@ -1293,21 +1303,28 @@ class ComputeManager(manager.SchedulerDependentManager):
         except exception.NetworkNotFound:
             network_info = network_model.NetworkInfo()
 
-        try:
-            # tear down allocated network structure
-            self._deallocate_network(context, instance)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                LOG.error(_('Failed to deallocate network for instance.'),
-                          instance=instance)
-                self._set_instance_error_state(context, instance['uuid'])
-
         # NOTE(vish) get bdms before destroying the instance
         vol_bdms = self._get_volume_bdms(bdms)
         block_device_info = self._get_instance_volume_block_device_info(
             context, instance, bdms=bdms)
-        self.driver.destroy(instance, self._legacy_nw_info(network_info),
-                            block_device_info)
+
+        # NOTE(melwitt): attempt driver destroy before releasing ip, may
+        #                want to keep ip allocated for certain failures
+        try:
+            self.driver.destroy(instance, self._legacy_nw_info(network_info),
+                                block_device_info)
+        except exception.InstancePowerOffFailure:
+            # if the instance can't power off, don't release the ip
+            with excutils.save_and_reraise_exception():
+                pass
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                # deallocate ip and fail without proceeding to
+                # volume api calls, preserving current behavior
+                self._try_deallocate_network(context, instance)
+
+        self._try_deallocate_network(context, instance)
+
         for bdm in vol_bdms:
             try:
                 # NOTE(vish): actual driver detach done in driver.destroy, so
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 91a8c9c95a8e..71eb2818800d 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -2337,6 +2337,36 @@ class ComputeTestCase(BaseTestCase):
                 instance=jsonutils.to_primitive(instance),
                 bdms={})
 
+    def test_delete_instance_keeps_net_on_power_off_fail(self):
+        self.mox.StubOutWithMock(self.compute.driver, 'destroy')
+        self.mox.StubOutWithMock(self.compute, '_deallocate_network')
+        exp = exception.InstancePowerOffFailure(reason='')
+        self.compute.driver.destroy(mox.IgnoreArg(), mox.IgnoreArg(),
+                                    mox.IgnoreArg()).AndRaise(exp)
+        # mox will detect if _deallocate_network gets called unexpectedly
+        self.mox.ReplayAll()
+        instance = self._create_fake_instance()
+        self.assertRaises(exception.InstancePowerOffFailure,
+                          self.compute._delete_instance,
+                          self.context,
+                          instance=jsonutils.to_primitive(instance),
+                          bdms={})
+
+    def test_delete_instance_loses_net_on_other_fail(self):
+        self.mox.StubOutWithMock(self.compute.driver, 'destroy')
+        self.mox.StubOutWithMock(self.compute, '_deallocate_network')
+        exp = test.TestingException()
+        self.compute.driver.destroy(mox.IgnoreArg(), mox.IgnoreArg(),
+                                    mox.IgnoreArg()).AndRaise(exp)
+        self.compute._deallocate_network(mox.IgnoreArg(), mox.IgnoreArg())
+        self.mox.ReplayAll()
+        instance = self._create_fake_instance()
+        self.assertRaises(test.TestingException,
+                          self.compute._delete_instance,
+                          self.context,
+                          instance=jsonutils.to_primitive(instance),
+                          bdms={})
+
     def test_delete_instance_deletes_console_auth_tokens(self):
         instance = self._create_fake_instance()
         self.flags(vnc_enabled=True)