Implement "ip route delete" command using Pyroute2

Change-Id: I960455d6a9bc1b633d485c42a26b3a254731558e
Related-Bug: #1492714

neutron/agent/l3/dvr_local_router.py

@@ -17,7 +17,6 @@ import collections
 
 import netaddr
 from neutron_lib import constants as lib_constants
-from neutron_lib import exceptions
 from oslo_log import log as logging
 from oslo_utils import excutils
 import six
@@ -26,6 +25,7 @@ from neutron.agent.l3 import dvr_fip_ns
 from neutron.agent.l3 import dvr_router_base
 from neutron.agent.linux import ip_lib
 from neutron.common import utils as common_utils
+from neutron.privileged.agent.linux import ip_lib as priv_ip_lib
 
 LOG = logging.getLogger(__name__)
 # xor-folding mask used for IPv6 rule index
@@ -180,7 +180,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
 
             device = ip_lib.IPDevice(fip_2_rtr_name, namespace=fip_ns_name)
 
-            device.route.delete_route(fip_cidr, str(rtr_2_fip.ip))
+            device.route.delete_route(fip_cidr, via=str(rtr_2_fip.ip))
             return device
 
     def floating_ip_moved_dist(self, fip):
@@ -322,7 +322,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
                                          snat_idx):
         try:
             ns_ip_device.route.delete_gateway(gw_ip_addr, table=snat_idx)
-        except exceptions.DeviceNotFoundError:
+        except priv_ip_lib.NetworkInterfaceNotFound:
             pass
 
     def _stale_ip_rule_cleanup(self, namespace, ns_ipd, ip_version):
@@ -676,7 +676,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
                 return
             for subnet in router_port['subnets']:
                 rtr_port_cidr = subnet['cidr']
-                device.route.delete_route(rtr_port_cidr, str(rtr_2_fip_ip))
+                device.route.delete_route(rtr_port_cidr, via=str(rtr_2_fip_ip))
 
     def _add_interface_route_to_fip_ns(self, router_port):
         rtr_2_fip_ip, fip_2_rtr_name = self.get_rtr_fip_ip_and_interface_name()

neutron/agent/linux/ip_lib.py

@@ -23,7 +23,6 @@ from neutron_lib import constants
 from neutron_lib import exceptions
 from oslo_config import cfg
 from oslo_log import log as logging
-from oslo_utils import excutils
 from pyroute2.netlink import exceptions as netlink_exceptions
 from pyroute2.netlink import rtnl
 from pyroute2.netlink.rtnl import ifaddrmsg
@@ -586,38 +585,13 @@ class IpRouteCommand(IpDeviceCommandBase):
         super(IpRouteCommand, self).__init__(parent)
         self._table = table
 
-    def table(self, table):
-        """Return an instance of IpRouteCommand which works on given table"""
-        return IpRouteCommand(self._parent, table)
-
-    def _table_args(self, override=None):
-        if override:
-            return ['table', override]
-        return ['table', self._table] if self._table else []
-
-    def _dev_args(self):
-        return ['dev', self.name] if self.name else []
-
     def add_gateway(self, gateway, metric=None, table=None, scope='global'):
         self.add_route(None, via=gateway, table=table, metric=metric,
                        scope=scope)
 
-    def _run_as_root_detect_device_not_found(self, options, args):
+    def delete_gateway(self, gateway, table=None, scope=None):
-        try:
+        self.delete_route(None, device=self.name, via=gateway, table=table,
-            return self._as_root(options, tuple(args))
+                          scope=scope)
-        except RuntimeError as rte:
-            with excutils.save_and_reraise_exception() as ctx:
-                if "Cannot find device" in str(rte):
-                    ctx.reraise = False
-                    raise exceptions.DeviceNotFoundError(device_name=self.name)
-
-    def delete_gateway(self, gateway, table=None):
-        ip_version = common_utils.get_ip_version(gateway)
-        args = ['del', 'default',
-                'via', gateway]
-        args += self._dev_args()
-        args += self._table_args(table)
-        self._run_as_root_detect_device_not_found([ip_version], args)
 
     def list_routes(self, ip_version, scope=None, via=None, table=None,
                     **kwargs):
@@ -633,7 +607,7 @@ class IpRouteCommand(IpDeviceCommandBase):
         self.add_route(cidr, scope='link')
 
     def delete_onlink_route(self, cidr):
-        self.delete_route(cidr, scope='link')
+        self.delete_route(cidr, device=self.name, scope='link')
 
     def get_gateway(self, scope=None, table=None,
                     ip_version=constants.IP_VERSION_4):
@@ -643,11 +617,9 @@ class IpRouteCommand(IpDeviceCommandBase):
                 return route
 
     def flush(self, ip_version, table=None, **kwargs):
-        args = ['flush']
+        for route in self.list_routes(ip_version, table=table):
-        args += self._table_args(table)
+            self.delete_route(route['cidr'], device=route['device'],
-        for k, v in kwargs.items():
+                              via=route['via'], table=table, **kwargs)
-            args += [k, v]
-        self._as_root([ip_version], tuple(args))
 
     def add_route(self, cidr, via=None, table=None, metric=None, scope=None,
                   **kwargs):
@@ -655,16 +627,11 @@ class IpRouteCommand(IpDeviceCommandBase):
         add_ip_route(self._parent.namespace, cidr, device=self.name, via=via,
                      table=table, metric=metric, scope=scope, **kwargs)
 
-    def delete_route(self, cidr, via=None, table=None, **kwargs):
+    def delete_route(self, cidr, device=None, via=None, table=None, scope=None,
-        ip_version = common_utils.get_ip_version(cidr)
+                     **kwargs):
-        args = ['del', cidr]
+        table = table or self._table
-        if via:
+        delete_ip_route(self._parent.namespace, cidr, device=device, via=via,
-            args += ['via', via]
+                        table=table, scope=scope, **kwargs)
-        args += self._dev_args()
-        args += self._table_args(table)
-        for k, v in kwargs.items():
-            args += [k, v]
-        self._run_as_root_detect_device_not_found([ip_version], args)
 
 
 class IPRoute(SubProcessBase):
@@ -1531,3 +1498,14 @@ def list_ip_routes(namespace, ip_version, scope=None, via=None, table=None,
         ret = [route for route in ret if route['via'] == via]
 
     return ret
+
+
+def delete_ip_route(namespace, cidr, device=None, via=None, table=None,
+                    scope=None, **kwargs):
+    """Delete an IP route"""
+    if table:
+        table = IP_RULE_TABLES.get(table, table)
+    ip_version = common_utils.get_ip_version(cidr or via)
+    privileged.delete_ip_route(namespace, cidr, ip_version,
+                               device=device, via=via, table=table,
+                               scope=scope, **kwargs)

neutron/privileged/agent/linux/ip_lib.py

@@ -650,29 +650,56 @@ def delete_ip_rule(namespace, **kwargs):
         raise
 
 
+def _make_pyroute2_route_args(namespace, ip_version, cidr, device, via, table,
+                              metric, scope, protocol):
+    """Returns a dictionary of arguments to be used in pyroute route commands
+
+    :param namespace: (string) name of the namespace
+    :param ip_version: (int) [4, 6]
+    :param cidr: (string) source IP or CIDR address (IPv4, IPv6)
+    :param device: (string) input interface name
+    :param via: (string) gateway IP address
+    :param table: (string, int) table number or name
+    :param metric: (int) route metric
+    :param scope: (int) route scope
+    :param protocol: (string) protocol name (pyroute2.netlink.rtnl.rt_proto)
+    :return: a dictionary with the kwargs needed in pyroute rule commands
+    """
+    args = {'family': _IP_VERSION_FAMILY_MAP[ip_version]}
+    if not scope:
+        scope = 'global' if via else 'link'
+    scope = _get_scope_name(scope)
+    if scope:
+        args['scope'] = scope
+    if cidr:
+        args['dst'] = cidr
+    if device:
+        args['oif'] = get_link_id(device, namespace)
+    if via:
+        args['gateway'] = via
+    if table:
+        args['table'] = int(table)
+    if metric:
+        args['priority'] = int(metric)
+    if protocol:
+        args['proto'] = protocol
+    return args
+
+
 @privileged.default.entrypoint
+# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads
+# we need to lock this function to workaround this issue.
+# For details please check https://bugs.launchpad.net/neutron/+bug/1811515
 @lockutils.synchronized("privileged-ip-lib")
 def add_ip_route(namespace, cidr, ip_version, device=None, via=None,
                  table=None, metric=None, scope=None, **kwargs):
     """Add an IP route"""
+    kwargs.update(_make_pyroute2_route_args(
+        namespace, ip_version, cidr, device, via, table, metric, scope,
+        'static'))
     try:
         with get_iproute(namespace) as ip:
-            family = _IP_VERSION_FAMILY_MAP[ip_version]
+            ip.route('replace', **kwargs)
-            if not scope:
-                scope = 'global' if via else 'link'
-            scope = _get_scope_name(scope)
-            if cidr:
-                kwargs['dst'] = cidr
-            if via:
-                kwargs['gateway'] = via
-            if table:
-                kwargs['table'] = int(table)
-            if device:
-                kwargs['oif'] = get_link_id(device, namespace)
-            if metric:
-                kwargs['priority'] = int(metric)
-            ip.route('replace', family=family, scope=scope, proto='static',
-                     **kwargs)
     except OSError as e:
         if e.errno == errno.ENOENT:
             raise NetworkNamespaceNotFound(netns_name=namespace)
@@ -680,17 +707,36 @@ def add_ip_route(namespace, cidr, ip_version, device=None, via=None,
 
 
 @privileged.default.entrypoint
+# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads
+# we need to lock this function to workaround this issue.
+# For details please check https://bugs.launchpad.net/neutron/+bug/1811515
 @lockutils.synchronized("privileged-ip-lib")
 def list_ip_routes(namespace, ip_version, device=None, table=None, **kwargs):
     """List IP routes"""
+    kwargs.update(_make_pyroute2_route_args(
+        namespace, ip_version, None, device, None, table, None, None, None))
     try:
         with get_iproute(namespace) as ip:
-            family = _IP_VERSION_FAMILY_MAP[ip_version]
+            return make_serializable(ip.route('show', **kwargs))
-            if table:
+    except OSError as e:
-                kwargs['table'] = table
+        if e.errno == errno.ENOENT:
-            if device:
+            raise NetworkNamespaceNotFound(netns_name=namespace)
-                kwargs['oif'] = get_link_id(device, namespace)
+        raise
-            return make_serializable(ip.route('show', family=family, **kwargs))
+
+
+@privileged.default.entrypoint
+# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads
+# we need to lock this function to workaround this issue.
+# For details please check https://bugs.launchpad.net/neutron/+bug/1811515
+@lockutils.synchronized("privileged-ip-lib")
+def delete_ip_route(namespace, cidr, ip_version, device=None, via=None,
+                    table=None, scope=None, **kwargs):
+    """Delete an IP route"""
+    kwargs.update(_make_pyroute2_route_args(
+        namespace, ip_version, cidr, device, via, table, None, scope, None))
+    try:
+        with get_iproute(namespace) as ip:
+            ip.route('del', **kwargs)
     except OSError as e:
         if e.errno == errno.ENOENT:
             raise NetworkNamespaceNotFound(netns_name=namespace)

neutron/tests/functional/agent/linux/test_ip_lib.py

@@ -846,7 +846,17 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase):
         self.cidrs = ['192.168.0.0/24', '10.0.0.0/8', '2001::/64', 'faaa::/96']
 
     def _assert_route(self, ip_version, table=None, source_prefix=None,
-                      cidr=None, scope=None, via=None, metric=None):
+                      cidr=None, scope=None, via=None, metric=None,
+                      not_in=False):
+        if not_in:
+            fn = lambda: cmp not in self.device.route.list_routes(ip_version,
+                                                                  table=table)
+            msg = 'Route found: %s'
+        else:
+            fn = lambda: cmp in self.device.route.list_routes(ip_version,
+                                                              table=table)
+            msg = 'Route not found: %s'
+
         if cidr:
             ip_version = utils.get_ip_version(cidr)
         else:
@@ -868,11 +878,9 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase):
                'via': via,
                'priority': metric}
         try:
-            utils.wait_until_true(lambda: cmp in self.device.route.list_routes(
+            utils.wait_until_true(fn, timeout=5)
-                ip_version, table=table), timeout=5)
         except utils.WaitTimeout:
-            raise self.fail('Route not found: %s' % cmp)
+            raise self.fail(msg % cmp)
-        return True
 
     def test_add_route_table(self):
         tables = (None, 1, 253, 254, 255)
@@ -929,7 +937,7 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase):
         routes = self.device.route.list_onlink_routes(constants.IP_VERSION_4)
         self.assertEqual(len(cidr_ipv4), len(routes))
 
-    def test_get_gateway(self):
+    def test_get_and_delete_gateway(self):
         gateways = (str(netaddr.IPNetwork(self.device_cidr_ipv4).ip),
                     str(netaddr.IPNetwork(self.device_cidr_ipv6).ip + 1))
         scopes = ('global', 'site', 'link')
@@ -944,4 +952,33 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase):
                                metric=metric, table=table)
             self.assertEqual(gateway, self.device.route.get_gateway(
                 ip_version=ip_version, table=table)['via'])
-            self.device.route.delete_gateway(gateway, table=table)
+
+            self.device.route.delete_gateway(gateway, table=table, scope=scope)
+            self.assertIsNone(self.device.route.get_gateway(
+                ip_version=ip_version, table=table))
+
+    def test_delete_route(self):
+        scopes = ('global', 'site', 'link')
+        tables = (None, 1, 254, 255)
+        for cidr, scope, table in itertools.product(
+                self.cidrs, scopes, tables):
+            ip_version = utils.get_ip_version(cidr)
+            self.device.route.add_route(cidr, table=table, scope=scope)
+            self._assert_route(ip_version, cidr=cidr, scope=scope, table=table)
+
+            self.device.route.delete_route(cidr, table=table, scope=scope)
+            self._assert_route(ip_version, cidr=cidr, scope=scope, table=table,
+                               not_in=True)
+
+    def test_flush(self):
+        tables = (None, 1, 200)
+        ip_versions = (constants.IP_VERSION_4, constants.IP_VERSION_6)
+        for cidr, table in itertools.product(self.cidrs, tables):
+            self.device.route.add_route(cidr, table=table)
+
+        for ip_version, table in itertools.product(ip_versions, tables):
+            routes = self.device.route.list_routes(ip_version, table=table)
+            self.assertGreater(len(routes), 0)
+            self.device.route.flush(ip_version, table=table)
+            routes = self.device.route.list_routes(ip_version, table=table)
+            self.assertEqual([], routes)

neutron/tests/unit/agent/l3/test_dvr_local_router.py

@@ -386,7 +386,8 @@ class TestDvrRouterOperations(base.BaseTestCase):
         ri.floating_ip_removed_dist(fip_cidr)
         self.mock_delete_ip_rule.assert_called_with(
             ri.router_namespace.name, ip=fixed_ip, table=16, priority=FIP_PRI)
-        mIPDevice().route.delete_route.assert_called_with(fip_cidr, str(s.ip))
+        mIPDevice().route.delete_route.assert_called_with(fip_cidr,
+                                                          via=str(s.ip))
         ri.fip_ns.local_subnets.allocate.assert_not_called()
 
     @mock.patch.object(ip_lib, 'add_ip_rule')

neutron/tests/unit/agent/linux/test_ip_lib.py

@@ -841,109 +841,6 @@ class TestIpAddrCommand(TestIPCmdBase):
         self.assertEqual(0, len(retval))
 
 
-class TestIpRouteCommand(TestIPCmdBase):
-    def setUp(self):
-        super(TestIpRouteCommand, self).setUp()
-        self.parent.name = 'eth0'
-        self.command = 'route'
-        self.route_cmd = ip_lib.IpRouteCommand(self.parent)
-        self.ip_version = constants.IP_VERSION_4
-        self.table = 14
-        self.metric = 100
-        self.cidr = '192.168.45.100/24'
-        self.ip = '10.0.0.1'
-        self.gateway = '192.168.45.100'
-        self.test_cases = [{'sample': GATEWAY_SAMPLE1,
-                            'expected': {'gateway': '10.35.19.254',
-                                         'metric': 100}},
-                           {'sample': GATEWAY_SAMPLE2,
-                            'expected': {'gateway': '10.35.19.254',
-                                         'metric': 100}},
-                           {'sample': GATEWAY_SAMPLE3,
-                            'expected': None},
-                           {'sample': GATEWAY_SAMPLE4,
-                            'expected': {'gateway': '10.35.19.254'}},
-                           {'sample': GATEWAY_SAMPLE5,
-                            'expected': {'gateway': '192.168.99.1'}},
-                           {'sample': GATEWAY_SAMPLE6,
-                            'expected': {'gateway': '192.168.99.1',
-                                         'metric': 100}},
-                           {'sample': GATEWAY_SAMPLE7,
-                            'expected': {'metric': 1}}]
-
-    def test_del_gateway_cannot_find_device(self):
-        self.parent._as_root.side_effect = RuntimeError("Cannot find device")
-
-        exc = self.assertRaises(exceptions.DeviceNotFoundError,
-                          self.route_cmd.delete_gateway,
-                          self.gateway, table=self.table)
-        self.assertIn(self.parent.name, str(exc))
-
-    def test_del_gateway_other_error(self):
-        self.parent._as_root.side_effect = RuntimeError()
-
-        self.assertRaises(RuntimeError, self.route_cmd.delete_gateway,
-                          self.gateway, table=self.table)
-
-    def test_flush_route_table(self):
-        self.route_cmd.flush(self.ip_version, self.table)
-        self._assert_sudo([self.ip_version], ('flush', 'table', self.table))
-
-    def test_delete_route(self):
-        self.route_cmd.delete_route(self.cidr, self.ip, self.table)
-        self._assert_sudo([self.ip_version],
-                          ('del', self.cidr,
-                           'via', self.ip,
-                           'dev', self.parent.name,
-                           'table', self.table))
-
-    def test_delete_route_no_via(self):
-        self.route_cmd.delete_route(self.cidr, table=self.table)
-        self._assert_sudo([self.ip_version],
-                          ('del', self.cidr,
-                           'dev', self.parent.name,
-                           'table', self.table))
-
-    def test_delete_route_with_scope(self):
-        self.route_cmd.delete_route(self.cidr, scope='link')
-        self._assert_sudo([self.ip_version],
-                          ('del', self.cidr,
-                           'dev', self.parent.name,
-                           'scope', 'link'))
-
-    def test_delete_route_no_device(self):
-        self.parent._as_root.side_effect = RuntimeError("Cannot find device")
-        self.assertRaises(exceptions.DeviceNotFoundError,
-                          self.route_cmd.delete_route,
-                          self.cidr, self.ip, self.table)
-
-
-class TestIPv6IpRouteCommand(TestIpRouteCommand):
-    def setUp(self):
-        super(TestIPv6IpRouteCommand, self).setUp()
-        self.ip_version = constants.IP_VERSION_6
-        self.cidr = '2001:db8::/64'
-        self.ip = '2001:db8::100'
-        self.gateway = '2001:db8::1'
-        self.test_cases = [{'sample': IPv6_GATEWAY_SAMPLE1,
-                            'expected':
-                            {'gateway': '2001:470:9:1224:4508:b885:5fb:740b',
-                             'metric': 100}},
-                           {'sample': IPv6_GATEWAY_SAMPLE2,
-                            'expected':
-                            {'gateway': '2001:470:9:1224:4508:b885:5fb:740b',
-                             'metric': 100}},
-                           {'sample': IPv6_GATEWAY_SAMPLE3,
-                            'expected': None},
-                           {'sample': IPv6_GATEWAY_SAMPLE4,
-                            'expected':
-                            {'gateway': 'fe80::dfcc:aaff:feb9:76ce'}},
-                           {'sample': IPv6_GATEWAY_SAMPLE5,
-                            'expected':
-                            {'gateway': '2001:470:9:1224:4508:b885:5fb:740b',
-                             'metric': 1024}}]
-
-
 class TestIpNetnsCommand(TestIPCmdBase):
     def setUp(self):
         super(TestIpNetnsCommand, self).setUp()