Makes sure instance deletion ok with deleted data

Commit 5ad1dea4 added changed the network deallocation code to
work with deleted instances. This was done by setting the context
to read deleted records. Unfortunately this was done a little too
broadly, leading to a new bug where a deleted floating_ip will
cause an instance to not be able to be deleted.

This fixes the issue by limiting the use of read_deleted context
to only the places it is trying to access the instance record. It
adds a test  to verify that the code works with a duplicate
deleted floating_ip and updates the existing test for a deleted
instance to exercise the entire code path.

Fixes bug 1038266

Change-Id: I1aef94369e5bcf951e78e89b1eded5305cf36b53

3 files changed, 58 insertions, 21 deletions

diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index d15b7b353..8867d3dc2 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -1170,7 +1170,8 @@ def fixed_ip_get(context, id, session=None):
     # FIXME(sirp): shouldn't we just use project_only here to restrict the
     # results?
     if is_user_context(context) and result['instance_uuid'] is not None:
-        instance = instance_get_by_uuid(context, result['instance_uuid'],
+        instance = instance_get_by_uuid(context.elevated(read_deleted='yes'),
+                                        result['instance_uuid'],
                                         session)
         authorize_project_context(context, instance.project_id)
 
@@ -1199,7 +1200,8 @@ def fixed_ip_get_by_address(context, address, session=None):
     # NOTE(sirp): shouldn't we just use project_only here to restrict the
     # results?
     if is_user_context(context) and result['instance_uuid'] is not None:
-        instance = instance_get_by_uuid(context, result['instance_uuid'],
+        instance = instance_get_by_uuid(context.elevated(read_deleted='yes'),
+                                        result['instance_uuid'],
                                         session)
         authorize_project_context(context, instance.project_id)
 
diff --git a/nova/network/manager.py b/nova/network/manager.py
index 470a186ee..c1f3a6418 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -353,14 +353,11 @@ class FloatingIP(object):
         # NOTE(francois.charlier): in some cases the instance might be
         # deleted before the IPs are released, so we need to get deleted
         # instances too
-        read_deleted_context = context.elevated(read_deleted='yes')
-        instance = self.db.instance_get(read_deleted_context, instance_id)
-
-        LOG.debug(_("floating IP deallocation for instance |%s|"),
-                  instance=instance, context=read_deleted_context)
+        instance = self.db.instance_get(
+                context.elevated(read_deleted='yes'), instance_id)
 
         try:
-            fixed_ips = self.db.fixed_ip_get_by_instance(read_deleted_context,
+            fixed_ips = self.db.fixed_ip_get_by_instance(context,
                                                          instance['uuid'])
         except exception.FixedIpNotFoundForInstance:
             fixed_ips = []
@@ -374,14 +371,14 @@ class FloatingIP(object):
             for floating_ip in floating_ips:
                 address = floating_ip['address']
                 try:
-                    self.disassociate_floating_ip(read_deleted_context,
+                    self.disassociate_floating_ip(context,
                                                   address,
                                                   affect_auto_assigned=True)
                 except exception.FloatingIpNotAssociated:
                     LOG.exception(_("Floating IP is not associated. Ignore."))
                 # deallocate if auto_assigned
                 if floating_ip['auto_assigned']:
-                    self.deallocate_floating_ip(read_deleted_context, address,
+                    self.deallocate_floating_ip(context, address,
                                                 affect_auto_assigned=True)
 
         # call the next inherited class's deallocate_for_instance()
@@ -1276,8 +1273,9 @@ class NetworkManager(manager.SchedulerDependentManager):
         """Returns a fixed ip to the pool."""
         fixed_ip_ref = self.db.fixed_ip_get_by_address(context, address)
         vif_id = fixed_ip_ref['virtual_interface_id']
-        instance = self.db.instance_get_by_uuid(context,
-                                                fixed_ip_ref['instance_uuid'])
+        instance = self.db.instance_get_by_uuid(
+                context.elevated(read_deleted='yes'),
+                fixed_ip_ref['instance_uuid'])
 
         self._do_trigger_security_group_members_refresh_for_instance(
             instance['uuid'])
diff --git a/nova/tests/network/test_manager.py b/nova/tests/network/test_manager.py
index 9c53525f4..5ae8cf81c 100644
--- a/nova/tests/network/test_manager.py
+++ b/nova/tests/network/test_manager.py
@@ -1042,12 +1042,14 @@ class VlanNetworkTestCase(test.TestCase):
         fixed = db.fixed_ip_get_by_address(elevated, fix_addr)
         network = db.network_get(elevated, fixed['network_id'])
 
-        db.instance_destroy(self.context.elevated(), instance['uuid'])
-        self.assertRaises(exception.InstanceNotFound,
-                                  self.network.deallocate_fixed_ip,
-                                  context1,
-                                  fix_addr,
-                                  'fake')
+        def fake_refresh(instance_uuid):
+            raise test.TestingException()
+        self.stubs.Set(self.network,
+                '_do_trigger_security_group_members_refresh_for_instance',
+                fake_refresh)
+        self.assertRaises(test.TestingException,
+                          self.network.deallocate_fixed_ip,
+                          context1, fix_addr, 'fake')
         fixed = db.fixed_ip_get_by_address(elevated, fix_addr)
         self.assertTrue(fixed['allocated'])
 
@@ -1541,10 +1543,45 @@ class FloatingIPTestCase(test.TestCase):
                 instance_id=instance_ref['id'])
 
     def test_deallocation_deleted_instance(self):
-        instance_ref = db.api.instance_create(self.context,
-                {"project_id": self.project_id, "deleted": True})
+        self.stubs.Set(self.network, '_teardown_network_on_host',
+                       lambda *args, **kwargs: None)
+        instance = db.api.instance_create(self.context, {
+                'project_id': self.project_id, 'deleted': True})
+        network = db.api.network_create_safe(self.context.elevated(), {
+                'project_id': self.project_id})
+        addr = db.fixed_ip_create(self.context, {'allocated': True,
+                'instance_uuid': instance['uuid'], 'address': '10.1.1.1',
+                'network_id': network['id']})
+        fixed = db.fixed_ip_get_by_address(
+                self.context.elevated(read_deleted='yes'), addr)
+        db.api.floating_ip_create(self.context, {
+                'address': '10.10.10.10', 'instance_uuid': instance['uuid'],
+                'fixed_ip_id': fixed['id'],
+                'project_id': self.project_id})
         self.network.deallocate_for_instance(self.context,
-                instance_id=instance_ref['id'])
+                instance_id=instance['id'])
+
+    def test_deallocation_duplicate_floating_ip(self):
+        self.stubs.Set(self.network, '_teardown_network_on_host',
+                       lambda *args, **kwargs: None)
+        instance = db.api.instance_create(self.context, {
+                'project_id': self.project_id})
+        network = db.api.network_create_safe(self.context.elevated(), {
+                'project_id': self.project_id})
+        addr = db.fixed_ip_create(self.context, {'allocated': True,
+                'instance_uuid': instance['uuid'], 'address': '10.1.1.1',
+                'network_id': network['id']})
+        fixed = db.fixed_ip_get_by_address(
+                self.context.elevated(read_deleted='yes'), addr)
+        db.api.floating_ip_create(self.context, {
+                'address': '10.10.10.10',
+                'deleted': True})
+        db.api.floating_ip_create(self.context, {
+                'address': '10.10.10.10', 'instance_uuid': instance['uuid'],
+                'fixed_ip_id': fixed['id'],
+                'project_id': self.project_id})
+        self.network.deallocate_for_instance(self.context,
+                instance_id=instance['id'])
 
     def test_floating_dns_create_conflict(self):
         zone = "example.org"