From 55f2bc05a0029888a1de1b79838d1e3a7aaac34f Mon Sep 17 00:00:00 2001 From: Aaron Rosen Date: Wed, 3 Apr 2013 15:13:20 -0700 Subject: [PATCH] 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 f80f06a71de6e235c71e0333448cf59e44745c45. Change-Id: Ic93eec0d95ad20baf53cc97476fcdff1b308ae91 --- quantum/agent/dhcp_agent.py | 2 +- quantum/agent/l3_agent.py | 30 ++++++-- quantum/agent/linux/ip_lib.py | 88 ++++++----------------- quantum/tests/unit/test_l3_agent.py | 80 ++++++++++++++++----- quantum/tests/unit/test_linux_ip_lib.py | 96 ++----------------------- 5 files changed, 115 insertions(+), 181 deletions(-) diff --git a/quantum/agent/dhcp_agent.py b/quantum/agent/dhcp_agent.py index 3c32bf5ce..3993b56ce 100644 --- a/quantum/agent/dhcp_agent.py +++ b/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) diff --git a/quantum/agent/l3_agent.py b/quantum/agent/l3_agent.py index 345df17b4..39ea511fc 100644 --- a/quantum/agent/l3_agent.py +++ b/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()) - ip.route.add_gateway(gw_ip) + cmd = ['route', 'add', 'default', 'gw', 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): - ip = ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name()) + def _update_routing_table(self, ri, operation, route): + 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) - - ip.route.add(route['destination'], route['nexthop']) + #replace success even if there is no existing route + 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 diff --git a/quantum/agent/linux/ip_lib.py b/quantum/agent/linux/ip_lib.py index 0d1c046a7..5207c230e 100644 --- a/quantum/agent/linux/ip_lib.py +++ b/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): - """Adds a default gateway or replaces an existing one.""" - - args = ['replace', 'default', 'via', gw] + def add_gateway(self, gateway, metric=None): + args = ['replace', 'default', 'via', gateway] if metric: args += ['metric', metric] - if dev: - args += ['dev', dev] - + args += ['dev', self.name] self._as_root(*args) - def delete_gateway(self, gw, dev=None): - args = ['delete', 'default', 'via', gw] - if dev: - args += ['dev', dev] + def delete_gateway(self, gateway): + self._as_root('del', + 'default', + '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] - if filters: - args += filters + filters += ['scope', scope] - 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' diff --git a/quantum/tests/unit/test_l3_agent.py b/quantum/tests/unit/test_l3_agent.py index 538c34216..0ac24a2c0 100644 --- a/quantum/tests/unit/test_l3_agent.py +++ b/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): - mocks = { - 'replace': self.mock_ip.route.add, - 'delete': self.mock_ip.route.delete - } + def _check_agent_method_called(self, agent, calls, namespace): + if namespace: + self.mock_ip.netns.execute.assert_has_calls( + [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] - m.assert_has_calls([mock.call(*call) for call in calls], - any_order=True) + def _test_routing_table_update(self, namespace): + if not namespace: + 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'], - ['110.100.31.0/24', '10.100.10.30']] + expected = [['ip', 'route', 'replace', 'to', '110.100.30.0/24', + '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']] - self._check_route_calls('delete', expected, ri.ns_name()) + expected = [['ip', 'route', 'delete', 'to', '110.100.30.0/24', + 'via', '10.100.10.30']] + self._check_agent_method_called(agent, expected, namespace) def testProcessRouter(self): diff --git a/quantum/tests/unit/test_linux_ip_lib.py b/quantum/tests/unit/test_linux_ip_lib.py index 25b0227ec..47b40631d 100644 --- a/quantum/tests/unit/test_linux_ip_lib.py +++ b/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, dev) + self.route_cmd.add_gateway(gateway, metric) 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, dev) + self.route_cmd.delete_gateway(gateway) self._assert_sudo([], - ('delete', 'default', 'via', gateway, - 'dev', dev)) + ('del', 'default', 'via', gateway, + '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()