Revert "Use wrappers instead of direct calls to ip route."

This commit somehow broken floating ips with the l3 agent so we should revert
it till we figure out why.

This reverts commit f80f06a71d.

Change-Id: Ic93eec0d95ad20baf53cc97476fcdff1b308ae91

quantum/agent/dhcp_agent.py

@@ -566,7 +566,7 @@ class DeviceManager(object):
         if namespace is None:
             device = ip_lib.IPDevice(interface_name,
                                      self.root_helper)
-            device.route.pullup_route()
+            device.route.pullup_route(interface_name)
 
         if self.conf.enable_metadata_network:
             meta_cidr = netaddr.IPNetwork(METADATA_DEFAULT_IP)

quantum/agent/l3_agent.py

@@ -416,8 +416,14 @@ class L3NATAgent(manager.Manager):
 
         gw_ip = ex_gw_port['subnet']['gateway_ip']
         if ex_gw_port['subnet']['gateway_ip']:
-            ip = ip_lib.IPWrapper(self.root_helper, ri.ns_name())
+            cmd = ['route', 'add', 'default', 'gw', gw_ip]
-            ip.route.add_gateway(gw_ip)
+            if self.conf.use_namespaces:
+                ip_wrapper = ip_lib.IPWrapper(self.root_helper,
+                                              namespace=ri.ns_name())
+                ip_wrapper.netns.execute(cmd, check_exit_code=False)
+            else:
+                utils.execute(cmd, check_exit_code=False,
+                              root_helper=self.root_helper)
 
         for (c, r) in self.external_gateway_nat_rules(ex_gw_ip,
                                                       internal_cidrs,
@@ -639,9 +645,19 @@ class L3NATAgent(manager.Manager):
     def after_start(self):
         LOG.info(_("L3 agent started"))
 
-    def routes_updated(self, ri):
+    def _update_routing_table(self, ri, operation, route):
-        ip = ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name())
+        cmd = ['ip', 'route', operation, 'to', route['destination'],
+               'via', route['nexthop']]
+        #TODO(nati) move this code to iplib
+        if self.conf.use_namespaces:
+            ip_wrapper = ip_lib.IPWrapper(self.conf.root_helper,
+                                          namespace=ri.ns_name())
+            ip_wrapper.netns.execute(cmd, check_exit_code=False)
+        else:
+            utils.execute(cmd, check_exit_code=False,
+                          root_helper=self.conf.root_helper)
 
+    def routes_updated(self, ri):
         new_routes = ri.router['routes']
         old_routes = ri.routes
         adds, removes = common_utils.diff_list_of_dict(old_routes,
@@ -652,11 +668,11 @@ class L3NATAgent(manager.Manager):
             for del_route in removes:
                 if route['destination'] == del_route['destination']:
                     removes.remove(del_route)
-
+            #replace success even if there is no existing route
-            ip.route.add(route['destination'], route['nexthop'])
+            self._update_routing_table(ri, 'replace', route)
         for route in removes:
             LOG.debug(_("Removed route entry is '%s'"), route)
-            ip.route.delete(route['destination'], route['nexthop'])
+            self._update_routing_table(ri, 'delete', route)
         ri.routes = new_routes
 
 

quantum/agent/linux/ip_lib.py

@@ -63,7 +63,6 @@ class IPWrapper(SubProcessBase):
         super(IPWrapper, self).__init__(root_helper=root_helper,
                                         namespace=namespace)
         self.netns = IpNetnsCommand(self)
-        self.route = IpRouteCommand(self)
 
     def device(self, name):
         return IPDevice(name, self.root_helper, self.namespace)
@@ -135,7 +134,7 @@ class IPDevice(SubProcessBase):
         self.name = name
         self.link = IpLinkCommand(self)
         self.addr = IpAddrCommand(self)
-        self.route = IpRouteDeviceCommand(self)
+        self.route = IpRouteCommand(self)
 
     def __eq__(self, other):
         return (other is not None and self.name == other.name
@@ -301,37 +300,35 @@ class IpAddrCommand(IpDeviceCommandBase):
         return retval
 
 
-class IpRouteCommand(IpCommandBase):
+class IpRouteCommand(IpDeviceCommandBase):
     COMMAND = 'route'
 
-    def add_gateway(self, gw, metric=None, dev=None):
+    def add_gateway(self, gateway, metric=None):
-        """Adds a default gateway or replaces an existing one."""
+        args = ['replace', 'default', 'via', gateway]
-
-        args = ['replace', 'default', 'via', gw]
         if metric:
             args += ['metric', metric]
-        if dev:
+        args += ['dev', self.name]
-            args += ['dev', dev]
-
         self._as_root(*args)
 
-    def delete_gateway(self, gw, dev=None):
+    def delete_gateway(self, gateway):
-        args = ['delete', 'default', 'via', gw]
+        self._as_root('del',
-        if dev:
+                      'default',
-            args += ['dev', dev]
+                      'via',
+                      gateway,
+                      'dev',
+                      self.name)
 
-        self._as_root(*args)
+    def get_gateway(self, scope=None, filters=None):
+        if filters is None:
+            filters = []
+
+        retval = None
 
-    def get_gateway(self, scope=None, filters=None, dev=None):
-        args = ['list']
-        if dev:
-            args += ['dev', self.name]
         if scope:
-            args += ['scope', scope]
+            filters += ['scope', scope]
-        if filters:
-            args += filters
 
-        route_list_lines = self._run(*args).split('\n')
+        route_list_lines = self._run('list', 'dev', self.name,
+                                     *filters).split('\n')
         default_route_line = next((x.strip() for x in
                                    route_list_lines if
                                    x.strip().startswith('default')), None)
@@ -344,7 +341,7 @@ class IpRouteCommand(IpCommandBase):
             if parts_has_metric:
                 retval.update(metric=int(parts[metric_index]))
 
-            return retval
+        return retval
 
     def pullup_route(self, interface_name):
         """
@@ -386,49 +383,6 @@ class IpRouteCommand(IpCommandBase):
                     self._as_root('append', subnet, 'proto', 'kernel',
                                   'dev', device)
 
-    def add(self, destination, nexthop, dev=None):
-        """Adds a new route or replaces an existing one."""
-
-        args = ['replace', 'to', destination, 'via', nexthop]
-        if dev:
-            args += ['dev', dev]
-
-        self._as_root(*args)
-
-    def delete(self, destination, nexthop, dev=None):
-        args = ['delete', 'to', destination, 'via', nexthop]
-        if dev:
-            args += ['dev', dev]
-
-        self._as_root(*args)
-
-
-class IpRouteDeviceCommand(IpDeviceCommandBase):
-    """Wrapper for ip route actions which are bound to a specific device"""
-
-    def __init__(self, parent):
-        super(IpRouteDeviceCommand, self).__init__(parent)
-
-        self._route = IpRouteCommand(parent)
-
-    def add_gateway(self, gw, metric=None):
-        return self._route.add_gateway(gw, metric, self.name)
-
-    def delete_gateway(self, gw):
-        return self._route.delete_gateway(gw, self.name)
-
-    def get_gateway(self, scope=None, filters=None):
-        return self._route.get_gateway(scope, filters, self.name)
-
-    def pullup_route(self):
-        return self._route.pullup_route(self.name)
-
-    def add(self, destination, nexthop):
-        return self._route.add(destination, nexthop, self.name)
-
-    def delete(self, destination, nexthop):
-        return self._route.delete(destination, nexthop, self.name)
-
 
 class IpNetnsCommand(IpCommandBase):
     COMMAND = 'netns'

quantum/tests/unit/test_l3_agent.py

@@ -66,9 +66,9 @@ class TestBasicRouterOperations(base.BaseTestCase):
         driver_cls.return_value = self.mock_driver
 
         self.ip_cls_p = mock.patch('quantum.agent.linux.ip_lib.IPWrapper')
-        self.ip_cls = self.ip_cls_p.start()
+        ip_cls = self.ip_cls_p.start()
         self.mock_ip = mock.MagicMock()
-        self.ip_cls.return_value = self.mock_ip
+        ip_cls.return_value = self.mock_ip
 
         self.l3pluginApi_cls_p = mock.patch(
             'quantum.agent.l3_agent.L3PluginApi')
@@ -210,15 +210,57 @@ class TestBasicRouterOperations(base.BaseTestCase):
     def testAgentRemoveFloatingIP(self):
         self._test_floating_ip_action('remove')
 
-    def _check_route_calls(self, action, calls, namespace):
+    def _check_agent_method_called(self, agent, calls, namespace):
-        mocks = {
+        if namespace:
-            'replace': self.mock_ip.route.add,
+            self.mock_ip.netns.execute.assert_has_calls(
-            'delete': self.mock_ip.route.delete
+                [mock.call(call, check_exit_code=False) for call in calls],
-        }
+                any_order=True)
+        else:
+            self.utils_exec.assert_has_calls([
+                mock.call(call, root_helper='sudo',
+                          check_exit_code=False) for call in calls],
+                any_order=True)
 
-        m = mocks[action]
+    def _test_routing_table_update(self, namespace):
-        m.assert_has_calls([mock.call(*call) for call in calls],
+        if not namespace:
-                           any_order=True)
+            self.conf.set_override('use_namespaces', False)
+
+        router_id = _uuid()
+        ri = l3_agent.RouterInfo(router_id, self.conf.root_helper,
+                                 self.conf.use_namespaces,
+                                 None)
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        fake_route1 = {'destination': '135.207.0.0/16',
+                       'nexthop': '1.2.3.4'}
+        fake_route2 = {'destination': '135.207.111.111/32',
+                       'nexthop': '1.2.3.4'}
+
+        agent._update_routing_table(ri, 'replace', fake_route1)
+        expected = [['ip', 'route', 'replace', 'to', '135.207.0.0/16',
+                     'via', '1.2.3.4']]
+        self._check_agent_method_called(agent, expected, namespace)
+
+        agent._update_routing_table(ri, 'delete', fake_route1)
+        expected = [['ip', 'route', 'delete', 'to', '135.207.0.0/16',
+                     'via', '1.2.3.4']]
+        self._check_agent_method_called(agent, expected, namespace)
+
+        agent._update_routing_table(ri, 'replace', fake_route2)
+        expected = [['ip', 'route', 'replace', 'to', '135.207.111.111/32',
+                     'via', '1.2.3.4']]
+        self._check_agent_method_called(agent, expected, namespace)
+
+        agent._update_routing_table(ri, 'delete', fake_route2)
+        expected = [['ip', 'route', 'delete', 'to', '135.207.111.111/32',
+                     'via', '1.2.3.4']]
+        self._check_agent_method_called(agent, expected, namespace)
+
+    def testAgentRoutingTableUpdated(self):
+        self._test_routing_table_update(namespace=True)
+
+    def testAgentRoutingTableUpdatedNoNameSpace(self):
+        self._test_routing_table_update(namespace=False)
 
     def testRoutesUpdated(self):
         self._test_routes_updated(namespace=True)
@@ -246,24 +288,28 @@ class TestBasicRouterOperations(base.BaseTestCase):
         ri.router['routes'] = fake_new_routes
         agent.routes_updated(ri)
 
-        expected = [['110.100.30.0/24', '10.100.10.30'],
+        expected = [['ip', 'route', 'replace', 'to', '110.100.30.0/24',
-                    ['110.100.31.0/24', '10.100.10.30']]
+                    'via', '10.100.10.30'],
+                    ['ip', 'route', 'replace', 'to', '110.100.31.0/24',
+                    'via', '10.100.10.30']]
 
-        self._check_route_calls('replace', expected, ri.ns_name())
+        self._check_agent_method_called(agent, expected, namespace)
 
         fake_new_routes = [{'destination': "110.100.30.0/24",
                             'nexthop': "10.100.10.30"}]
         ri.router['routes'] = fake_new_routes
         agent.routes_updated(ri)
-        expected = [['110.100.31.0/24', '10.100.10.30']]
+        expected = [['ip', 'route', 'delete', 'to', '110.100.31.0/24',
+                    'via', '10.100.10.30']]
 
-        self._check_route_calls('delete', expected, ri.ns_name())
+        self._check_agent_method_called(agent, expected, namespace)
         fake_new_routes = []
         ri.router['routes'] = fake_new_routes
         agent.routes_updated(ri)
 
-        expected = [['110.100.30.0/24', '10.100.10.30']]
+        expected = [['ip', 'route', 'delete', 'to', '110.100.30.0/24',
-        self._check_route_calls('delete', expected, ri.ns_name())
+                    'via', '10.100.10.30']]
+        self._check_agent_method_called(agent, expected, namespace)
 
     def testProcessRouter(self):
 

quantum/tests/unit/test_linux_ip_lib.py

@@ -549,54 +549,25 @@ class TestIpAddrCommand(TestIPCmdBase):
 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)
 
-    def test_add(self):
-        dst = '10.0.1.0/24'
-        nexthop = '192.168.1.1'
-
-        self.route_cmd.add(dst, nexthop)
-        self._assert_sudo([], ('replace', 'to', dst, 'via', nexthop))
-
-    def test_delete(self):
-        dst = '10.0.1.0/24'
-        nexthop = '192.168.1.1'
-
-        self.route_cmd.delete(dst, nexthop)
-        self._assert_sudo([], ('delete', 'to', dst, 'via', nexthop))
-
     def test_add_gateway(self):
-        gw = '10.0.1.1'
-
-        self.route_cmd.add_gateway(gw)
-        self._assert_sudo([], ('replace', 'default', 'via', gw))
-
-    def test_delete_gateway(self):
-        gw = '10.0.1.1'
-
-        self.route_cmd.delete_gateway(gw)
-        self._assert_sudo([], ('delete', 'default', 'via', gw))
-
-    def test_add_gateway_with_metric_dev(self):
         gateway = '192.168.45.100'
         metric = 100
-        dev = 'eth0'
+        self.route_cmd.add_gateway(gateway, metric)
-
-        self.route_cmd.add_gateway(gateway, metric, dev)
         self._assert_sudo([],
                           ('replace', 'default', 'via', gateway,
                            'metric', metric,
-                           'dev', dev))
+                           'dev', self.parent.name))
 
-    def test_delete_gateway_with_dev(self):
+    def test_del_gateway(self):
         gateway = '192.168.45.100'
-        dev = 'eth0'
+        self.route_cmd.delete_gateway(gateway)
-
-        self.route_cmd.delete_gateway(gateway, dev)
         self._assert_sudo([],
-                          ('delete', 'default', 'via', gateway,
+                          ('del', 'default', 'via', gateway,
-                           'dev', dev))
+                           'dev', self.parent.name))
 
     def test_get_gateway(self):
         test_cases = [{'sample': GATEWAY_SAMPLE1,
@@ -643,59 +614,6 @@ class TestIpRouteCommand(TestIPCmdBase):
         self.assertEqual(len(self.parent._run.mock_calls), 2)
 
 
-class TestIpRouteDeviceCommand(TestIPCmdBase):
-    def setUp(self):
-        super(TestIpRouteDeviceCommand, self).setUp()
-        self.parent.name = 'eth0'
-        self.command = 'route'
-        self.route_cmd = ip_lib.IpRouteDeviceCommand(self.parent)
-
-        route_p = mock.patch.object(self.route_cmd, '_route')
-        self._route = route_p.start()
-
-    def test_add_gateway(self):
-        gw = '10.0.0.1'
-        metric = 100
-
-        self.route_cmd.add_gateway(gw, metric)
-        self._route.add_gateway.assert_called_once_with(gw, metric,
-                                                        self.route_cmd.name)
-
-    def test_delete_gateway(self):
-        gw = '10.0.0.1'
-
-        self.route_cmd.delete_gateway(gw)
-        self._route.delete_gateway.assert_called_once_with(gw,
-                                                           self.route_cmd.name)
-
-    def test_get_gateway(self):
-        scope = 'scope'
-        filters = []
-
-        self.route_cmd.get_gateway(scope, filters)
-        self._route.get_gateway.assert_called_once_with(scope, filters,
-                                                        self.route_cmd.name)
-
-    def test_add(self):
-        dst = '8.8.8.8'
-        via = '10.0.1.4'
-
-        self.route_cmd.add(dst, via)
-        self._route.add.assert_called_once_with(dst, via, self.route_cmd.name)
-
-    def test_delete(self):
-        dst = '8.8.8.8'
-        via = '10.0.1.4'
-
-        self.route_cmd.delete(dst, via)
-        self._route.delete.assert_called_once_with(dst, via,
-                                                   self.route_cmd.name)
-
-    def test_pullup_route(self):
-        self.route_cmd.pullup_route()
-        self._route.pullup_route.assert_called_once_with(self.route_cmd.name)
-
-
 class TestIpNetnsCommand(TestIPCmdBase):
     def setUp(self):
         super(TestIpNetnsCommand, self).setUp()