Optimize ipset usage in IptablesFirewallDriver

Currently, IptablesFirewallDriver._update_ipset_members() iterates
through a list of security group IDs and makes a call to
IpsetManager.set_members() for each security group ID in the list. The
problem is that set_members() is repeatedly called with the same
arguments over and over again because the list of security group IDs
contains duplicates. These duplicated calls are unnecessary because they
are idempotent.

For instance, with a security group of 50 rules created in this manner:
    neutron security-group-rule-create $SECGRP --remote_group_id $SECGRP
        --protocol tcp --port_range_min $i --port_range_max $i

Adding a server to that security group will cause 50 calls to
IpsetManager.set_members() because the list of security group IDs is 50 of
the same ID. Only one call to IpsetManager.set_members() is necessary
per security group ID.

This patch converts that list of security group IDs into a set, which
eliminates the duplicate idempotent calls to
IpsetManager.set_members() with the same arguments. This will affect
performance by reducing the amount of file locking around ipset when
adding servers to security groups.

Change-Id: Id2c8c8c1093c8abcf1fd897b23b0358aeb55b526
Closes-Bug: 1466921

diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py
index a686e0e82cc0688e53f4c0dc7889a53510e3c219..ff12802e1631f9d1fec73d8a40ae4482042954df 100644 (file)
--- a/neutron/agent/linux/iptables_firewall.py
+++ b/neutron/agent/linux/iptables_firewall.py
@@ -432,14 +432,14 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
 
     def _get_remote_sg_ids(self, port, direction=None):
         sg_ids = port.get('security_groups', [])
-        remote_sg_ids = {constants.IPv4: [], constants.IPv6: []}
+        remote_sg_ids = {constants.IPv4: set(), constants.IPv6: set()}
         for sg_id in sg_ids:
             for rule in self.sg_rules.get(sg_id, []):
                 if not direction or rule['direction'] == direction:
                     remote_sg_id = rule.get('remote_group_id')
                     ether_type = rule.get('ethertype')
                     if remote_sg_id and ether_type:
-                        remote_sg_ids[ether_type].append(remote_sg_id)
+                        remote_sg_ids[ether_type].add(remote_sg_id)
         return remote_sg_ids
 
     def _add_rules_by_security_group(self, port, direction):
@@ -651,8 +651,8 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
                                 constants.IPv6: set()}
         for port in filtered_ports:
             remote_sg_ids = self._get_remote_sg_ids(port)
-            for ip_version, sg_ids in six.iteritems(remote_sg_ids):
-                remote_group_id_sets[ip_version].update(sg_ids)
+            for ip_version in (constants.IPv4, constants.IPv6):
+                remote_group_id_sets[ip_version] |= remote_sg_ids[ip_version]
         return remote_group_id_sets
 
     def _determine_sg_rules_to_remove(self, filtered_ports):

diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py
index 7491d5a8740b49ea7ff44f75994c393ac44ef380..96cf75a98226271c01181b26f8647defef942813 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py
+++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py
@@ -1507,6 +1507,17 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase):
             {_IPv4: set([FAKE_SGID]), _IPv6: set([OTHER_SGID])},
             self.firewall._get_remote_sg_ids_sets_by_ipversion(ports))
 
+    def test_get_remote_sg_ids(self):
+        self.firewall.sg_rules = self._fake_sg_rules(
+            remote_groups={_IPv4: [FAKE_SGID, FAKE_SGID, FAKE_SGID],
+                           _IPv6: [OTHER_SGID, OTHER_SGID, OTHER_SGID]})
+
+        port = self._fake_port()
+
+        self.assertEqual(
+            {_IPv4: set([FAKE_SGID]), _IPv6: set([OTHER_SGID])},
+            self.firewall._get_remote_sg_ids(port))
+
     def test_determine_sg_rules_to_remove(self):
         self.firewall.pre_sg_rules = self._fake_sg_rules(sg_id=OTHER_SGID)
         ports = [self._fake_port()]