From 92b946e90b6cdd90deba2097196970afaaaf8dab Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Wed, 27 May 2015 20:12:27 +0000 Subject: [PATCH] Use a single method to remove an address with its conntrack state I just noticed a pattern and I thought I'd throw this up for discussion. It has occurred to me that this addition sort of breaks the ip_lib paradigm of wrapping ip commands without any additional useful abstraction. Any better ideas? Change-Id: Ibd34bf4a721c153aca916e294e58adb4a28379e4 --- neutron/agent/l3/router_info.py | 3 +- neutron/agent/linux/interface.py | 39 +------------------ neutron/agent/linux/ip_lib.py | 35 +++++++++++++++++ .../tests/unit/agent/l3/test_legacy_router.py | 5 +-- .../tests/unit/agent/linux/test_interface.py | 10 +++-- 5 files changed, 44 insertions(+), 48 deletions(-) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index c1336355e6e..8a5695cc08c 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -210,8 +210,7 @@ class RouterInfo(object): raise NotImplementedError() def remove_floating_ip(self, device, ip_cidr): - device.addr.delete(ip_cidr) - self.driver.delete_conntrack_state(namespace=self.ns_name, ip=ip_cidr) + device.delete_addr_and_conntrack_state(ip_cidr) def get_router_cidrs(self, device): return set([addr['cidr'] for addr in device.addr.list()]) diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index 435791b9313..ed1e91e98f7 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -113,8 +113,7 @@ class LinuxInterfaceDriver(object): # clean up any old addresses for ip_cidr in previous: if ip_cidr not in preserve_ips: - device.addr.delete(ip_cidr) - self.delete_conntrack_state(namespace=namespace, ip=ip_cidr) + device.delete_addr_and_conntrack_state(ip_cidr) for gateway_ip in gateway_ips or []: device.route.add_gateway(gateway_ip) @@ -131,42 +130,6 @@ class LinuxInterfaceDriver(object): for route in existing_onlink_routes - new_onlink_routes: device.route.delete_onlink_route(route) - def delete_conntrack_state(self, namespace, ip): - """Delete conntrack state associated with an IP address. - - This terminates any active connections through an IP. Call this soon - after removing the IP address from an interface so that new connections - cannot be created before the IP address is gone. - - namespace: the name of the namespace where the IP has been configured - ip: the IP address for which state should be removed. This can be - passed as a string with or without /NN. A netaddr.IPAddress or - netaddr.Network representing the IP address can also be passed. - """ - ip_str = str(netaddr.IPNetwork(ip).ip) - ip_wrapper = ip_lib.IPWrapper(namespace=namespace) - - # Delete conntrack state for ingress traffic - # If 0 flow entries have been deleted - # conntrack -D will return 1 - try: - ip_wrapper.netns.execute(["conntrack", "-D", "-d", ip_str], - check_exit_code=True, - extra_ok_codes=[1]) - - except RuntimeError: - LOG.exception(_LE("Failed deleting ingress connection state of" - " floatingip %s"), ip_str) - - # Delete conntrack state for egress traffic - try: - ip_wrapper.netns.execute(["conntrack", "-D", "-q", ip_str], - check_exit_code=True, - extra_ok_codes=[1]) - except RuntimeError: - LOG.exception(_LE("Failed deleting egress connection state of" - " floatingip %s"), ip_str) - def check_bridge_exists(self, bridge): if not ip_lib.device_exists(bridge): raise exceptions.BridgeDoesNotExist(bridge=bridge) diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index a22b46b4f5a..586f70bb9b3 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -211,6 +211,41 @@ class IPDevice(SubProcessBase): def __str__(self): return self.name + def delete_addr_and_conntrack_state(self, cidr): + """Delete an address along with its conntrack state + + This terminates any active connections through an IP. + + cidr: the IP address for which state should be removed. This can be + passed as a string with or without /NN. A netaddr.IPAddress or + netaddr.Network representing the IP address can also be passed. + """ + self.addr.delete(cidr) + + ip_str = str(netaddr.IPNetwork(cidr).ip) + ip_wrapper = IPWrapper(namespace=self.namespace) + + # Delete conntrack state for ingress traffic + # If 0 flow entries have been deleted + # conntrack -D will return 1 + try: + ip_wrapper.netns.execute(["conntrack", "-D", "-d", ip_str], + check_exit_code=True, + extra_ok_codes=[1]) + + except RuntimeError: + LOG.exception(_LE("Failed deleting ingress connection state of" + " floatingip %s"), ip_str) + + # Delete conntrack state for egress traffic + try: + ip_wrapper.netns.execute(["conntrack", "-D", "-q", ip_str], + check_exit_code=True, + extra_ok_codes=[1]) + except RuntimeError: + LOG.exception(_LE("Failed deleting egress connection state of" + " floatingip %s"), ip_str) + class IpCommandBase(object): COMMAND = '' diff --git a/neutron/tests/unit/agent/l3/test_legacy_router.py b/neutron/tests/unit/agent/l3/test_legacy_router.py index 296d93f80d9..2bf4f303515 100644 --- a/neutron/tests/unit/agent/l3/test_legacy_router.py +++ b/neutron/tests/unit/agent/l3/test_legacy_router.py @@ -46,10 +46,7 @@ class TestBasicRouterOperations(BasicRouterTestCaseFramework): ri.remove_floating_ip(device, cidr) - device.addr.delete.assert_called_once_with(cidr) - self.driver.delete_conntrack_state.assert_called_once_with( - ip=cidr, - namespace=ri.ns_name) + device.delete_addr_and_conntrack_state.assert_called_once_with(cidr) @mock.patch.object(ip_lib, 'send_gratuitous_arp') diff --git a/neutron/tests/unit/agent/linux/test_interface.py b/neutron/tests/unit/agent/linux/test_interface.py index ad08444c157..8bbc210dd9b 100644 --- a/neutron/tests/unit/agent/linux/test_interface.py +++ b/neutron/tests/unit/agent/linux/test_interface.py @@ -94,7 +94,7 @@ class TestABCDriver(TestBase): [mock.call('tap0', namespace=ns), mock.call().addr.list(filters=['permanent']), mock.call().addr.add('192.168.1.2/24'), - mock.call().addr.delete('172.16.77.240/24'), + mock.call().delete_addr_and_conntrack_state('172.16.77.240/24'), mock.call().route.list_onlink_routes(constants.IP_VERSION_4), mock.call().route.list_onlink_routes(constants.IP_VERSION_6), mock.call().route.add_onlink_route('172.20.0.0/24')]) @@ -127,6 +127,7 @@ class TestABCDriver(TestBase): mock.call().addr.list(filters=['permanent']), mock.call().addr.add('192.168.1.2/24')]) self.assertFalse(self.ip_dev().addr.delete.called) + self.assertFalse(self.ip_dev().delete_addr_and_conntrack_state.called) def _test_l3_init_with_ipv6(self, include_gw_ip): addresses = [dict(scope='global', @@ -147,7 +148,8 @@ class TestABCDriver(TestBase): [mock.call('tap0', namespace=ns), mock.call().addr.list(filters=['permanent']), mock.call().addr.add('2001:db8:a::124/64'), - mock.call().addr.delete('2001:db8:a::123/64')]) + mock.call().delete_addr_and_conntrack_state( + '2001:db8:a::123/64')]) if include_gw_ip: expected_calls += ( [mock.call().route.add_gateway('2001:db8:a::1')]) @@ -180,8 +182,8 @@ class TestABCDriver(TestBase): mock.call().addr.list(filters=['permanent']), mock.call().addr.add('192.168.1.2/24'), mock.call().addr.add('2001:db8:a::124/64'), - mock.call().addr.delete('172.16.77.240/24'), - mock.call().addr.delete('2001:db8:a::123/64'), + mock.call().delete_addr_and_conntrack_state('172.16.77.240/24'), + mock.call().delete_addr_and_conntrack_state('2001:db8:a::123/64'), mock.call().route.list_onlink_routes(constants.IP_VERSION_4), mock.call().route.list_onlink_routes(constants.IP_VERSION_6), mock.call().route.add_onlink_route('172.20.0.0/24')],