Avoid referencing stale instance/network_info dicts in firewall

This makes the virt.firewall code cleaner in terms of referencing
the cached instance and network_info code it stores. Before this
patch, concurrent instance operations could modify these two dicts
so that while we're iterating instances, the network_info dict
is suddenly missing information we need.

The right fix for this is to use instance objects and their
associated info_cache objects, but that's a larger fix and one
not as well-suited to backporting to previous releases which
suffer from this as well.

The approach taken here is that we store the instance and
network_info cache together in the same dict that we can pop()
from atomically (this is not really necessary, but helps to
prevent introducing more of these cases). When we iterate over
the contents, we iterate over a copy of the keys, being careful
not to let a suddenly-missing key break us, and passing the
details all the way down the stack instead of having deeper calls
hit the cache dicts again.

Change-Id: I33366f50024a82451842d045b830ab19b59879c3
Closes-bug: #1182131

nova/tests/virt/libvirt/test_driver.py

@@ -7732,15 +7732,15 @@ class IptablesFirewallTestCase(test.TestCase):
         self.fw.instance_rules(instance_ref,
                                mox.IgnoreArg()).AndReturn((None, None))
         self.fw.add_filters_for_instance(instance_ref, mox.IgnoreArg(),
-                                         mox.IgnoreArg())
+                                         mox.IgnoreArg(), mox.IgnoreArg())
         self.fw.instance_rules(instance_ref,
                                mox.IgnoreArg()).AndReturn((None, None))
         self.fw.add_filters_for_instance(instance_ref, mox.IgnoreArg(),
-                                         mox.IgnoreArg())
+                                         mox.IgnoreArg(), mox.IgnoreArg())
         self.mox.ReplayAll()
 
         self.fw.prepare_instance_filter(instance_ref, mox.IgnoreArg())
-        self.fw.instances[instance_ref['id']] = instance_ref
+        self.fw.instance_info[instance_ref['id']] = (instance_ref, None)
         self.fw.do_refresh_security_group_rules("fake")
 
     def test_unfilter_instance_undefines_nwfilter(self):

nova/tests/virt/xenapi/test_xenapi.py

@@ -2680,7 +2680,8 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase):
         db.instance_add_security_group(admin_ctxt, instance_ref['uuid'],
                                        secgroup['id'])
         self.fw.prepare_instance_filter(instance_ref, network_info)
-        self.fw.instances[instance_ref['id']] = instance_ref
+        self.fw.instance_info[instance_ref['id']] = (instance_ref,
+                                                     network_info)
         self._validate_security_group()
         # add a rule to the security group
         db.security_group_rule_create(admin_ctxt,

nova/virt/firewall.py

@@ -142,8 +142,7 @@ class IptablesFirewallDriver(FirewallDriver):
     def __init__(self, virtapi, **kwargs):
         super(IptablesFirewallDriver, self).__init__(virtapi)
         self.iptables = linux_net.iptables_manager
-        self.instances = {}
+        self.instance_info = {}
-        self.network_infos = {}
         self.basically_filtered = False
 
         # Flags for DHCP request rule
@@ -169,9 +168,7 @@ class IptablesFirewallDriver(FirewallDriver):
         self.iptables.defer_apply_off()
 
     def unfilter_instance(self, instance, network_info):
-        if self.instances.pop(instance['id'], None):
+        if self.instance_info.pop(instance['id'], None):
-            # NOTE(vish): use the passed info instead of the stored info
-            self.network_infos.pop(instance['id'])
             self.remove_filters_for_instance(instance)
             self.iptables.apply()
         else:
@@ -179,10 +176,10 @@ class IptablesFirewallDriver(FirewallDriver):
                      'filtered'), instance=instance)
 
     def prepare_instance_filter(self, instance, network_info):
-        self.instances[instance['id']] = instance
+        self.instance_info[instance['id']] = (instance, network_info)
-        self.network_infos[instance['id']] = network_info
         ipv4_rules, ipv6_rules = self.instance_rules(instance, network_info)
-        self.add_filters_for_instance(instance, ipv4_rules, ipv6_rules)
+        self.add_filters_for_instance(instance, network_info, ipv4_rules,
+                                      ipv6_rules)
         LOG.debug('Filters added to instance', instance=instance)
         self.refresh_provider_fw_rules()
         LOG.debug('Provider Firewall Rules refreshed', instance=instance)
@@ -239,9 +236,8 @@ class IptablesFirewallDriver(FirewallDriver):
             for rule in ipv6_rules:
                 self.iptables.ipv6['filter'].add_rule(chain_name, rule)
 
-    def add_filters_for_instance(self, instance, inst_ipv4_rules,
+    def add_filters_for_instance(self, instance, network_info, inst_ipv4_rules,
                                  inst_ipv6_rules):
-        network_info = self.network_infos[instance['id']]
         chain_name = self._instance_chain_name(instance)
         if CONF.use_ipv6:
             self.iptables.ipv6['filter'].add_chain(chain_name)
@@ -444,22 +440,31 @@ class IptablesFirewallDriver(FirewallDriver):
         self.iptables.apply()
 
     @utils.synchronized('iptables', external=True)
-    def _inner_do_refresh_rules(self, instance, ipv4_rules,
+    def _inner_do_refresh_rules(self, instance, network_info, ipv4_rules,
                                 ipv6_rules):
         self.remove_filters_for_instance(instance)
-        self.add_filters_for_instance(instance, ipv4_rules, ipv6_rules)
+        self.add_filters_for_instance(instance, network_info, ipv4_rules,
+                                      ipv6_rules)
 
     def do_refresh_security_group_rules(self, security_group):
-        for instance in self.instances.values():
+        id_list = self.instance_info.keys()
-            network_info = self.network_infos[instance['id']]
+        for instance_id in id_list:
+            try:
+                instance, network_info = self.instance_info[instance_id]
+            except KeyError:
+                # NOTE(danms): instance cache must have been modified,
+                # ignore this deleted instance and move on
+                continue
             ipv4_rules, ipv6_rules = self.instance_rules(instance,
                                                          network_info)
-            self._inner_do_refresh_rules(instance, ipv4_rules, ipv6_rules)
+            self._inner_do_refresh_rules(instance, network_info, ipv4_rules,
+                                         ipv6_rules)
 
     def do_refresh_instance_rules(self, instance):
-        network_info = self.network_infos[instance['id']]
+        _instance, network_info = self.instance_info[instance['id']]
         ipv4_rules, ipv6_rules = self.instance_rules(instance, network_info)
-        self._inner_do_refresh_rules(instance, ipv4_rules, ipv6_rules)
+        self._inner_do_refresh_rules(instance, network_info, ipv4_rules,
+                                     ipv6_rules)
 
     def refresh_provider_fw_rules(self):
         """See :class:`FirewallDriver` docs."""

nova/virt/libvirt/firewall.py

@@ -296,9 +296,7 @@ class IptablesFirewallDriver(base_firewall.IptablesFirewallDriver):
     def unfilter_instance(self, instance, network_info):
         # NOTE(salvatore-orlando):
         # Overriding base class method for applying nwfilter operation
-        if self.instances.pop(instance['id'], None):
+        if self.instance_info.pop(instance['id'], None):
-            # NOTE(vish): use the passed info instead of the stored info
-            self.network_infos.pop(instance['id'])
             self.remove_filters_for_instance(instance)
             self.iptables.apply()
             self.nwfilter.unfilter_instance(instance, network_info)