From 176c483cdf2bc8eff7cfff4a9b0ac0dd98239da1 Mon Sep 17 00:00:00 2001 From: Vishvananda Ishaya Date: Wed, 7 Aug 2013 11:02:59 -0700 Subject: [PATCH] Fix simultaneous timeout with smart iptables usage Simultaneous boots were getting timeouts in allocate_for_network. This was due to our inefficient use of iptables where we attempt to recreate the same rules over and over using initialize_gateway, plug, and restart_dhcp. This patch makes the iptables code a little bit smarter by only shelling out if the rule list has changed. Note that there is a small difference in the interpretation of duplicate rules. Previously, the last copy of the rule would win whereas now the first copy of the rule will win. Multiple copies of the same rule were never supported. This means the only thing that one needs to remember is it is necessary to remove a rule and re-add it if you want to move it later in the list. Fixes bug 1199433 Change-Id: I9920f8d6aa269d9b3bfd88a8dbbb00f859deb158 --- nova/network/linux_net.py | 37 +++++++++++++++++++++++---- nova/tests/fake_network.py | 2 +- nova/tests/network/test_manager.py | 4 +-- nova/tests/test_iptables_network.py | 25 ++++++++++++++++++ nova/tests/virt/xenapi/test_xenapi.py | 4 +-- 5 files changed, 62 insertions(+), 10 deletions(-) diff --git a/nova/network/linux_net.py b/nova/network/linux_net.py index 4f8efa9986..a4ce92dc14 100644 --- a/nova/network/linux_net.py +++ b/nova/network/linux_net.py @@ -184,6 +184,7 @@ class IptablesTable(object): self.chains = set() self.unwrapped_chains = set() self.remove_chains = set() + self.dirty = True def add_chain(self, name, wrap=True): """Adds a named chain to the table. @@ -201,6 +202,7 @@ class IptablesTable(object): self.chains.add(name) else: self.unwrapped_chains.add(name) + self.dirty = True def remove_chain(self, name, wrap=True): """Remove named chain. @@ -220,6 +222,7 @@ class IptablesTable(object): LOG.warn(_('Attempted to remove chain %s which does not exist'), name) return + self.dirty = True # non-wrapped chains and rules need to be dealt with specially, # so we keep a list of them to be iterated over in apply() @@ -257,7 +260,12 @@ class IptablesTable(object): if '$' in rule: rule = ' '.join(map(self._wrap_target_chain, rule.split(' '))) - self.rules.append(IptablesRule(chain, rule, wrap, top)) + rule_obj = IptablesRule(chain, rule, wrap, top) + if rule_obj in self.rules: + LOG.debug(_("Skipping duplicate iptables rule addition")) + else: + self.rules.append(IptablesRule(chain, rule, wrap, top)) + self.dirty = True def _wrap_target_chain(self, s): if s.startswith('$'): @@ -276,6 +284,7 @@ class IptablesTable(object): self.rules.remove(IptablesRule(chain, rule, wrap, top)) if not wrap: self.remove_rules.append(IptablesRule(chain, rule, wrap, top)) + self.dirty = True except ValueError: LOG.warn(_('Tried to remove rule that was not there:' ' %(chain)r %(rule)r %(wrap)r %(top)r'), @@ -288,12 +297,17 @@ class IptablesTable(object): regex = re.compile(regex) num_rules = len(self.rules) self.rules = filter(lambda r: not regex.match(str(r)), self.rules) - return num_rules - len(self.rules) + removed = num_rules - len(self.rules) + if removed > 0: + self.dirty = True + return removed def empty_chain(self, chain, wrap=True): """Remove all rules from a chain.""" chained_rules = [rule for rule in self.rules if rule.chain == chain and rule.wrap == wrap] + if chained_rules: + self.dirty = True for rule in chained_rules: self.rules.remove(rule) @@ -389,13 +403,25 @@ class IptablesManager(object): def defer_apply_off(self): self.iptables_apply_deferred = False - self._apply() + self.apply() + + def dirty(self): + for table in self.ipv4.itervalues(): + if table.dirty: + return True + if CONF.use_ipv6: + for table in self.ipv6.itervalues(): + if table.dirty: + return True + return False def apply(self): if self.iptables_apply_deferred: return - - self._apply() + if self.dirty(): + self._apply() + else: + LOG.debug(_("Skipping apply due to lack of new rules")) @utils.synchronized('iptables', external=True) def _apply(self): @@ -419,6 +445,7 @@ class IptablesManager(object): start, end = self._find_table(all_lines, table_name) all_lines[start:end] = self._modify_rules( all_lines[start:end], table, table_name) + table.dirty = False self.execute('%s-restore' % (cmd,), '-c', run_as_root=True, process_input='\n'.join(all_lines), attempts=5) diff --git a/nova/tests/fake_network.py b/nova/tests/fake_network.py index 12d518aef3..40a0842eb8 100644 --- a/nova/tests/fake_network.py +++ b/nova/tests/fake_network.py @@ -334,7 +334,7 @@ def fake_get_instance_nw_info(stubs, num_networks=1, ips_per_vif=2, subnet_v6 = dict( cidr='2001:db8:0:%x::/64' % i, - gateway='fe80::def') + gateway='2001:db8:0:%x::1' % i) return [subnet_v4, subnet_v6] def get_network_by_uuid(context, uuid): diff --git a/nova/tests/network/test_manager.py b/nova/tests/network/test_manager.py index 33a7c24334..948684870c 100644 --- a/nova/tests/network/test_manager.py +++ b/nova/tests/network/test_manager.py @@ -177,7 +177,7 @@ class FlatNetworkTestCase(test.TestCase): 'dhcp_server': '192.168.1.1', 'dns': ['192.168.%d.3' % nid, '192.168.%d.4' % nid], 'gateway': '192.168.%d.1' % nid, - 'gateway_v6': 'fe80::def', + 'gateway_v6': '2001:db8:0:1::1', 'ip6s': 'DONTCARE', 'ips': 'DONTCARE', 'label': 'test%d' % nid, @@ -197,7 +197,7 @@ class FlatNetworkTestCase(test.TestCase): check = [{'enabled': 'DONTCARE', 'ip': '2001:db8:0:1::%x' % nid, 'netmask': 64, - 'gateway': 'fe80::def'}] + 'gateway': '2001:db8:0:1::1'}] self.assertThat(info['ip6s'], matchers.DictListMatches(check)) num_fixed_ips = len(info['ips']) diff --git a/nova/tests/test_iptables_network.py b/nova/tests/test_iptables_network.py index 0aaae7df0b..51fc061552 100644 --- a/nova/tests/test_iptables_network.py +++ b/nova/tests/test_iptables_network.py @@ -88,6 +88,31 @@ class IptablesManagerTestCase(test.TestCase): super(IptablesManagerTestCase, self).setUp() self.manager = linux_net.IptablesManager() + def test_duplicate_rules_no_dirty(self): + table = self.manager.ipv4['filter'] + table.dirty = False + num_rules = len(table.rules) + table.add_rule('FORWARD', '-s 1.2.3.4/5 -j DROP') + self.assertEqual(len(table.rules), num_rules + 1) + self.assertTrue(table.dirty) + table.dirty = False + num_rules = len(table.rules) + table.add_rule('FORWARD', '-s 1.2.3.4/5 -j DROP') + self.assertEqual(len(table.rules), num_rules) + self.assertFalse(table.dirty) + + def test_clean_tables_no_apply(self): + for table in self.manager.ipv4.itervalues(): + table.dirty = False + for table in self.manager.ipv6.itervalues(): + table.dirty = False + + def error_apply(): + raise test.TestingException() + + self.stubs.Set(self.manager, '_apply', error_apply) + self.manager.apply() + def test_filter_rules_are_wrapped(self): current_lines = self.sample_filter diff --git a/nova/tests/virt/xenapi/test_xenapi.py b/nova/tests/virt/xenapi/test_xenapi.py index 069dd87b24..8a7b2cc495 100644 --- a/nova/tests/virt/xenapi/test_xenapi.py +++ b/nova/tests/virt/xenapi/test_xenapi.py @@ -619,11 +619,11 @@ class XenAPIVMTestCase(stubs.XenAPITestBase): {'broadcast': '192.168.1.255', 'dns': ['192.168.1.4', '192.168.1.3'], 'gateway': '192.168.1.1', - 'gateway_v6': 'fe80::def', + 'gateway_v6': '2001:db8:0:1::1', 'ip6s': [{'enabled': '1', 'ip': '2001:db8:0:1::1', 'netmask': 64, - 'gateway': 'fe80::def'}], + 'gateway': '2001:db8:0:1::1'}], 'ips': [{'enabled': '1', 'ip': '192.168.1.100', 'netmask': '255.255.255.0',