Merge lp:~soren/nova/avoid-zombie-chains into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Remove race condition when refreshing security groups and destroying instances at the same time.

See the linked bug report for the full explanation of the problem.

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Rick Harris (rconradharris)]

Just some really really minor nits:

> 10 + if self.instances.pop(instance['id'], False):

Since instance['id'] isn't a bool, the fallback should probably be None.

> 15 - 'filtered'), instance['id'])
> 6 + 'filtered'), instance['id'])

Any reason to include this in the final patch?

[Soren Hansen (soren)]

2011/3/11 Rick Harris <email address hidden>:
>> 10    +        if self.instances.pop(instance['id'], False):
> Since instance['id'] isn't a bool, the fallback should probably be None.

Fixed.

>> 15    -                       'filtered'), instance['id'])
>> 6     +                   'filtered'), instance['id'])
> Any reason to include this in the final patch?

Um, yes, but not like that :) I wanted to align it with the outer
paren, but somehow I managed to screw that up. Fixed.

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/

[lp:~soren/nova/avoid-zombie-chains]

734. By Soren Hansen on 2011-03-11

Indentation adjustment (cosmetical).

735. By Soren Hansen on 2011-03-11

Make the fallback value None instead of False

[Rick Harris (rconradharris)]

lgtm

review: Approve

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-03-10 20:40:19 +0000
+++ nova/virt/libvirt_conn.py	2011-03-11 22:43:43 +0000
@@ -1238,13 +1238,12 @@
         pass
 
     def unfilter_instance(self, instance):
-        if instance['id'] in self.instances:
-            del self.instances[instance['id']]
+        if self.instances.pop(instance['id'], None):
             self.remove_filters_for_instance(instance)
             self.iptables.apply()
         else:
             LOG.info(_('Attempted to unfilter instance %s which is not '
-                       'filtered'), instance['id'])
+                     'filtered'), instance['id'])
 
     def prepare_instance_filter(self, instance):
         self.instances[instance['id']] = instance
@@ -1387,11 +1386,11 @@
         pass
 
     def refresh_security_group_rules(self, security_group):
-        for instance in self.instances.values():
-            # We use the semaphore to make sure noone applies the rule set
-            # after we've yanked the existing rules but before we've put in
-            # the new ones.
-            with self.iptables.semaphore:
+        # We use the semaphore to make sure noone applies the rule set
+        # after we've yanked the existing rules but before we've put in
+        # the new ones.
+        with self.iptables.semaphore:
+            for instance in self.instances.values():
                 self.remove_filters_for_instance(instance)
                 self.add_filters_for_instance(instance)
         self.iptables.apply()