defer firewall updates to iptables data structures
One of two patches that fixes bug #1194438. In the iptables firewall driver, each port method (update_port_filter, prepare_port_filter, remove_port_filter) makes O(N) calls, where N=len(firewall.ports), to IptablesManager methods that update dozens of data structures. When the firewall methods are called in sequence, e.g., by SecurityGroupAgentRpcMixin, the calls to IptablesManager's methods start to add up. This patch changes IptablesFirewallDriver to defer and coalesce calls to IptablesManager. Now a sequence of M port method calls results in O(N) calls to IptablesManager methods instead of O(N*M) as before. Change-Id: If17eeaec197beae8b8aecffca1f19d4535a7226e
This commit is contained in:
parent
ca421e7e62
commit
e31c9fd3fe
@ -47,6 +47,8 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
|
|||||||
# list of port which has security group
|
# list of port which has security group
|
||||||
self.filtered_ports = {}
|
self.filtered_ports = {}
|
||||||
self._add_fallback_chain_v4v6()
|
self._add_fallback_chain_v4v6()
|
||||||
|
self._defer_apply = False
|
||||||
|
self._pre_defer_filtered_ports = None
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def ports(self):
|
def ports(self):
|
||||||
@ -84,8 +86,12 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
|
|||||||
|
|
||||||
def _setup_chains(self):
|
def _setup_chains(self):
|
||||||
"""Setup ingress and egress chain for a port."""
|
"""Setup ingress and egress chain for a port."""
|
||||||
|
if not self._defer_apply:
|
||||||
|
self._setup_chains_apply(self.filtered_ports)
|
||||||
|
|
||||||
|
def _setup_chains_apply(self, ports):
|
||||||
self._add_chain_by_name_v4v6(SG_CHAIN)
|
self._add_chain_by_name_v4v6(SG_CHAIN)
|
||||||
for port in self.filtered_ports.values():
|
for port in ports.values():
|
||||||
self._setup_chain(port, INGRESS_DIRECTION)
|
self._setup_chain(port, INGRESS_DIRECTION)
|
||||||
self._setup_chain(port, EGRESS_DIRECTION)
|
self._setup_chain(port, EGRESS_DIRECTION)
|
||||||
self.iptables.ipv4['filter'].add_rule(SG_CHAIN, '-j ACCEPT')
|
self.iptables.ipv4['filter'].add_rule(SG_CHAIN, '-j ACCEPT')
|
||||||
@ -93,7 +99,11 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
|
|||||||
|
|
||||||
def _remove_chains(self):
|
def _remove_chains(self):
|
||||||
"""Remove ingress and egress chain for a port."""
|
"""Remove ingress and egress chain for a port."""
|
||||||
for port in self.filtered_ports.values():
|
if not self._defer_apply:
|
||||||
|
self._remove_chains_apply(self.filtered_ports)
|
||||||
|
|
||||||
|
def _remove_chains_apply(self, ports):
|
||||||
|
for port in ports.values():
|
||||||
self._remove_chain(port, INGRESS_DIRECTION)
|
self._remove_chain(port, INGRESS_DIRECTION)
|
||||||
self._remove_chain(port, EGRESS_DIRECTION)
|
self._remove_chain(port, EGRESS_DIRECTION)
|
||||||
self._remove_chain(port, IP_SPOOF_FILTER)
|
self._remove_chain(port, IP_SPOOF_FILTER)
|
||||||
@ -307,10 +317,18 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
|
|||||||
'%s%s' % (CHAIN_NAME_PREFIX[direction], port['device'][3:]))
|
'%s%s' % (CHAIN_NAME_PREFIX[direction], port['device'][3:]))
|
||||||
|
|
||||||
def filter_defer_apply_on(self):
|
def filter_defer_apply_on(self):
|
||||||
self.iptables.defer_apply_on()
|
if not self._defer_apply:
|
||||||
|
self.iptables.defer_apply_on()
|
||||||
|
self._pre_defer_filtered_ports = dict(self.filtered_ports)
|
||||||
|
self._defer_apply = True
|
||||||
|
|
||||||
def filter_defer_apply_off(self):
|
def filter_defer_apply_off(self):
|
||||||
self.iptables.defer_apply_off()
|
if self._defer_apply:
|
||||||
|
self._defer_apply = False
|
||||||
|
self._remove_chains_apply(self._pre_defer_filtered_ports)
|
||||||
|
self._pre_defer_filtered_ports = None
|
||||||
|
self._setup_chains_apply(self.filtered_ports)
|
||||||
|
self.iptables.defer_apply_off()
|
||||||
|
|
||||||
|
|
||||||
class OVSHybridIptablesFirewallDriver(IptablesFirewallDriver):
|
class OVSHybridIptablesFirewallDriver(IptablesFirewallDriver):
|
||||||
|
@ -15,6 +15,8 @@
|
|||||||
# License for the specific language governing permissions and limitations
|
# License for the specific language governing permissions and limitations
|
||||||
# under the License.
|
# under the License.
|
||||||
|
|
||||||
|
import copy
|
||||||
|
|
||||||
import mock
|
import mock
|
||||||
from mock import call
|
from mock import call
|
||||||
from oslo.config import cfg
|
from oslo.config import cfg
|
||||||
@ -935,6 +937,70 @@ class IptablesFirewallTestCase(base.BaseTestCase):
|
|||||||
self.iptables_inst.assert_has_calls([call.defer_apply_on(),
|
self.iptables_inst.assert_has_calls([call.defer_apply_on(),
|
||||||
call.defer_apply_off()])
|
call.defer_apply_off()])
|
||||||
|
|
||||||
|
def _mock_chain_applies(self):
|
||||||
|
class CopyingMock(mock.MagicMock):
|
||||||
|
"""Copies arguments so mutable arguments can be asserted on.
|
||||||
|
|
||||||
|
Copied verbatim from unittest.mock documentation.
|
||||||
|
"""
|
||||||
|
def __call__(self, *args, **kwargs):
|
||||||
|
args = copy.deepcopy(args)
|
||||||
|
kwargs = copy.deepcopy(kwargs)
|
||||||
|
return super(CopyingMock, self).__call__(*args, **kwargs)
|
||||||
|
# Need to use CopyingMock because _{setup,remove}_chains_apply are
|
||||||
|
# usually called with that's modified between calls (i.e.,
|
||||||
|
# self.firewall.filtered_ports).
|
||||||
|
chain_applies = CopyingMock()
|
||||||
|
self.firewall._setup_chains_apply = chain_applies.setup
|
||||||
|
self.firewall._remove_chains_apply = chain_applies.remove
|
||||||
|
return chain_applies
|
||||||
|
|
||||||
|
def test_mock_chain_applies(self):
|
||||||
|
chain_applies = self._mock_chain_applies()
|
||||||
|
port_prepare = {'device': 'd1', 'mac_address': 'prepare'}
|
||||||
|
port_update = {'device': 'd1', 'mac_address': 'update'}
|
||||||
|
self.firewall.prepare_port_filter(port_prepare)
|
||||||
|
self.firewall.update_port_filter(port_update)
|
||||||
|
self.firewall.remove_port_filter(port_update)
|
||||||
|
chain_applies.assert_has_calls([call.remove({}),
|
||||||
|
call.setup({'d1': port_prepare}),
|
||||||
|
call.remove({'d1': port_prepare}),
|
||||||
|
call.setup({'d1': port_update}),
|
||||||
|
call.remove({'d1': port_update}),
|
||||||
|
call.setup({})])
|
||||||
|
|
||||||
|
def test_defer_chain_apply_need_pre_defer_copy(self):
|
||||||
|
chain_applies = self._mock_chain_applies()
|
||||||
|
port = self._fake_port()
|
||||||
|
device2port = {port['device']: port}
|
||||||
|
self.firewall.prepare_port_filter(port)
|
||||||
|
with self.firewall.defer_apply():
|
||||||
|
self.firewall.remove_port_filter(port)
|
||||||
|
chain_applies.assert_has_calls([call.remove({}),
|
||||||
|
call.setup(device2port),
|
||||||
|
call.remove(device2port),
|
||||||
|
call.setup({})])
|
||||||
|
|
||||||
|
def test_defer_chain_apply_coalesce_simple(self):
|
||||||
|
chain_applies = self._mock_chain_applies()
|
||||||
|
port = self._fake_port()
|
||||||
|
with self.firewall.defer_apply():
|
||||||
|
self.firewall.prepare_port_filter(port)
|
||||||
|
self.firewall.update_port_filter(port)
|
||||||
|
self.firewall.remove_port_filter(port)
|
||||||
|
chain_applies.assert_has_calls([call.remove({}), call.setup({})])
|
||||||
|
|
||||||
|
def test_defer_chain_apply_coalesce_multiple_ports(self):
|
||||||
|
chain_applies = self._mock_chain_applies()
|
||||||
|
port1 = {'device': 'd1', 'mac_address': 'mac1'}
|
||||||
|
port2 = {'device': 'd2', 'mac_address': 'mac2'}
|
||||||
|
device2port = {'d1': port1, 'd2': port2}
|
||||||
|
with self.firewall.defer_apply():
|
||||||
|
self.firewall.prepare_port_filter(port1)
|
||||||
|
self.firewall.prepare_port_filter(port2)
|
||||||
|
chain_applies.assert_has_calls([call.remove({}),
|
||||||
|
call.setup(device2port)])
|
||||||
|
|
||||||
def test_ip_spoofing_filter_with_multiple_ips(self):
|
def test_ip_spoofing_filter_with_multiple_ips(self):
|
||||||
port = {'device': 'tapfake_dev',
|
port = {'device': 'tapfake_dev',
|
||||||
'mac_address': 'ff:ff:ff:ff',
|
'mac_address': 'ff:ff:ff:ff',
|
||||||
|
Loading…
Reference in New Issue
Block a user