From 2175c85d8347414e1bcce7e19e500029183d5da2 Mon Sep 17 00:00:00 2001
From: Sudhakar <sudhakar.gariganti@gmail.com>
Date: Mon, 3 Mar 2014 15:35:20 +0530
Subject: [PATCH] Improve iptables_manager _modify_rules() method

As the number of ports per default security group increases, the
number of iptables entries on the Compute Node grows.  Because of
this, there is a gradual increase in the time taken to apply chains
and rules.

Currently we are using list comprehensions to find if a new chain or
rule matches an existing one.  Instead, walk through the list in
reverse to find a matching entry.

Added a new method, _find_last_entry(), to return the entry we are
searching for.

Change-Id: I3585479ffa00be556b8b21dc9dbd6b36ad37f4de
Closes-Bug: #1302272
Related-Bug: #1253993
---
 neutron/agent/linux/iptables_manager.py     | 40 ++++++++++-----------
 neutron/tests/unit/test_iptables_manager.py | 24 +++++++++++++
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 92241ece2c..fb0a21ff5b 100644
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -457,6 +457,13 @@ class IptablesManager(object):
 
         return rules_index
 
+    def _find_last_entry(self, filter_list, match_str):
+        # find a matching entry, starting from the bottom
+        for s in reversed(filter_list):
+            s = s.strip()
+            if match_str in s:
+                return s
+
     def _modify_rules(self, current_lines, table, table_name):
         unwrapped_chains = table.unwrapped_chains
         chains = table.chains
@@ -489,19 +496,14 @@ class IptablesManager(object):
         for chain in all_chains:
             chain_str = str(chain).strip()
 
-            orig_filter = [s for s in old_filter if chain_str in s.strip()]
-            dup_filter = [s for s in new_filter if chain_str in s.strip()]
+            old = self._find_last_entry(old_filter, chain_str)
+            if not old:
+                dup = self._find_last_entry(new_filter, chain_str)
             new_filter = [s for s in new_filter if chain_str not in s.strip()]
 
             # if no old or duplicates, use original chain
-            if orig_filter:
-                # grab the last entry, if there is one
-                old = orig_filter[-1]
-                chain_str = str(old).strip()
-            elif dup_filter:
-                # grab the last entry, if there is one
-                dup = dup_filter[-1]
-                chain_str = str(dup).strip()
+            if old or dup:
+                chain_str = str(old or dup)
             else:
                 # add-on the [packet:bytes]
                 chain_str += ' - [0:0]'
@@ -517,21 +519,17 @@ class IptablesManager(object):
             # Further down, we weed out duplicates from the bottom of the
             # list, so here we remove the dupes ahead of time.
 
-            orig_filter = [s for s in old_filter if rule_str in s.strip()]
-            dup_filter = [s for s in new_filter if rule_str in s.strip()]
+            old = self._find_last_entry(old_filter, rule_str)
+            if not old:
+                dup = self._find_last_entry(new_filter, rule_str)
             new_filter = [s for s in new_filter if rule_str not in s.strip()]
 
             # if no old or duplicates, use original rule
-            if orig_filter:
-                # grab the last entry, if there is one
-                old = orig_filter[-1]
-                rule_str = str(old).strip()
-            elif dup_filter:
-                # grab the last entry, if there is one
-                dup = dup_filter[-1]
-                rule_str = str(dup).strip()
+            if old or dup:
+                rule_str = str(old or dup)
                 # backup one index so we write the array correctly
-                rules_index -= 1
+                if not old:
+                    rules_index -= 1
             else:
                 # add-on the [packet:bytes]
                 rule_str = '[0:0] ' + rule_str
diff --git a/neutron/tests/unit/test_iptables_manager.py b/neutron/tests/unit/test_iptables_manager.py
index af23432bf1..5e16406bd8 100644
--- a/neutron/tests/unit/test_iptables_manager.py
+++ b/neutron/tests/unit/test_iptables_manager.py
@@ -670,6 +670,30 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
 
         tools.verify_mock_calls(self.execute, expected_calls_and_values)
 
+    def _test_find_last_entry(self, find_str):
+        filter_list = [':neutron-filter-top - [0:0]',
+                       ':%(bn)s-FORWARD - [0:0]',
+                       ':%(bn)s-INPUT - [0:0]',
+                       ':%(bn)s-local - [0:0]',
+                       ':%(wrap)s - [0:0]',
+                       ':%(bn)s-OUTPUT - [0:0]',
+                       '[0:0] -A FORWARD -j neutron-filter-top',
+                       '[0:0] -A OUTPUT -j neutron-filter-top'
+                       % IPTABLES_ARG]
+
+        return self.iptables._find_last_entry(filter_list, find_str)
+
+    def test_find_last_entry_old_dup(self):
+        find_str = 'neutron-filter-top'
+        match_str = '[0:0] -A OUTPUT -j neutron-filter-top'
+        ret_str = self._test_find_last_entry(find_str)
+        self.assertEqual(ret_str, match_str)
+
+    def test_find_last_entry_none(self):
+        find_str = 'neutron-filter-NOTFOUND'
+        ret_str = self._test_find_last_entry(find_str)
+        self.assertIsNone(ret_str)
+
 
 class IptablesManagerStateLessTestCase(base.BaseTestCase):