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

diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index c1336355e6e737855c03a5812d412ac3075d4261..8a5695cc08c93c23b4b37b7e413ae5f3439cbef8 100644 (file)
--- 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 435791b931301b8aa7b3b52138c05f2128bb44bb..ed1e91e98f7a792cbea8094470a564cdc55d0b79 100644 (file)
--- 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 a22b46b4f5ab38f4bb3419e7fdf579294b8152f7..586f70bb9b30d878f0d7c2c66c864299a73a1101 100644 (file)
--- 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 296d93f80d9f4d33a20089e92204d7c5e347bad8..2bf4f303515b4634bca28c65c068089dc6ce750e 100644 (file)
--- 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 ad08444c15732c2c20e9f5a1d924e356ad1a4cad..8bbc210dd9b3c3b89916fba1e1bde1f836ad8a0b 100644 (file)
--- 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')],