From f80f06a71de6e235c71e0333448cf59e44745c45 Mon Sep 17 00:00:00 2001 From: Roman Podolyaka Date: Wed, 20 Mar 2013 18:50:50 +0200 Subject: [PATCH] Use wrappers instead of direct calls to ip route. - extract the logic of ip route wrapper into a separate class to drop dependency on a specific network device - add route wrapper to IPWrapper class - use IPWrapper instead of direct calls to ip route in l3 agent - update tests Fixes bug 1133133. Change-Id: Ic7174b0676d1a565909bb5f6f950376cf8fae8d2 --- 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, 181 insertions(+), 115 deletions(-) diff --git a/quantum/agent/dhcp_agent.py b/quantum/agent/dhcp_agent.py index 3993b56ce..3c32bf5ce 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(interface_name) + device.route.pullup_route() 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 39ea511fc..345df17b4 100644 --- a/quantum/agent/l3_agent.py +++ b/quantum/agent/l3_agent.py @@ -416,14 +416,8 @@ class L3NATAgent(manager.Manager): gw_ip = ex_gw_port['subnet']['gateway_ip'] if ex_gw_port['subnet']['gateway_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) + ip = ip_lib.IPWrapper(self.root_helper, ri.ns_name()) + ip.route.add_gateway(gw_ip) for (c, r) in self.external_gateway_nat_rules(ex_gw_ip, internal_cidrs, @@ -645,19 +639,9 @@ class L3NATAgent(manager.Manager): def after_start(self): LOG.info(_("L3 agent started")) - 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): + ip = ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name()) + new_routes = ri.router['routes'] old_routes = ri.routes adds, removes = common_utils.diff_list_of_dict(old_routes, @@ -668,11 +652,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 - self._update_routing_table(ri, 'replace', route) + + ip.route.add(route['destination'], route['nexthop']) for route in removes: LOG.debug(_("Removed route entry is '%s'"), route) - self._update_routing_table(ri, 'delete', route) + ip.route.delete(route['destination'], route['nexthop']) ri.routes = new_routes diff --git a/quantum/agent/linux/ip_lib.py b/quantum/agent/linux/ip_lib.py index 5207c230e..0d1c046a7 100644 --- a/quantum/agent/linux/ip_lib.py +++ b/quantum/agent/linux/ip_lib.py @@ -63,6 +63,7 @@ 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) @@ -134,7 +135,7 @@ class IPDevice(SubProcessBase): self.name = name self.link = IpLinkCommand(self) self.addr = IpAddrCommand(self) - self.route = IpRouteCommand(self) + self.route = IpRouteDeviceCommand(self) def __eq__(self, other): return (other is not None and self.name == other.name @@ -300,35 +301,37 @@ class IpAddrCommand(IpDeviceCommandBase): return retval -class IpRouteCommand(IpDeviceCommandBase): +class IpRouteCommand(IpCommandBase): COMMAND = 'route' - def add_gateway(self, gateway, metric=None): - args = ['replace', 'default', 'via', gateway] + def add_gateway(self, gw, metric=None, dev=None): + """Adds a default gateway or replaces an existing one.""" + + args = ['replace', 'default', 'via', gw] if metric: args += ['metric', metric] - args += ['dev', self.name] + if dev: + args += ['dev', dev] + self._as_root(*args) - def delete_gateway(self, gateway): - self._as_root('del', - 'default', - 'via', - gateway, - 'dev', - self.name) + def delete_gateway(self, gw, dev=None): + args = ['delete', 'default', 'via', gw] + if dev: + args += ['dev', dev] - def get_gateway(self, scope=None, filters=None): - if filters is None: - filters = [] - - retval = None + self._as_root(*args) + def get_gateway(self, scope=None, filters=None, dev=None): + args = ['list'] + if dev: + args += ['dev', self.name] if scope: - filters += ['scope', scope] + args += ['scope', scope] + if filters: + args += filters - route_list_lines = self._run('list', 'dev', self.name, - *filters).split('\n') + route_list_lines = self._run(*args).split('\n') default_route_line = next((x.strip() for x in route_list_lines if x.strip().startswith('default')), None) @@ -341,7 +344,7 @@ class IpRouteCommand(IpDeviceCommandBase): if parts_has_metric: retval.update(metric=int(parts[metric_index])) - return retval + return retval def pullup_route(self, interface_name): """ @@ -383,6 +386,49 @@ class IpRouteCommand(IpDeviceCommandBase): 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 0ac24a2c0..538c34216 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') - ip_cls = self.ip_cls_p.start() + self.ip_cls = self.ip_cls_p.start() self.mock_ip = mock.MagicMock() - ip_cls.return_value = self.mock_ip + self.ip_cls.return_value = self.mock_ip self.l3pluginApi_cls_p = mock.patch( 'quantum.agent.l3_agent.L3PluginApi') @@ -210,57 +210,15 @@ class TestBasicRouterOperations(base.BaseTestCase): def testAgentRemoveFloatingIP(self): self._test_floating_ip_action('remove') - 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) + def _check_route_calls(self, action, calls, namespace): + mocks = { + 'replace': self.mock_ip.route.add, + 'delete': self.mock_ip.route.delete + } - 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) + m = mocks[action] + m.assert_has_calls([mock.call(*call) for call in calls], + any_order=True) def testRoutesUpdated(self): self._test_routes_updated(namespace=True) @@ -288,28 +246,24 @@ class TestBasicRouterOperations(base.BaseTestCase): ri.router['routes'] = fake_new_routes agent.routes_updated(ri) - 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']] + expected = [['110.100.30.0/24', '10.100.10.30'], + ['110.100.31.0/24', '10.100.10.30']] - self._check_agent_method_called(agent, expected, namespace) + self._check_route_calls('replace', expected, ri.ns_name()) 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 = [['ip', 'route', 'delete', 'to', '110.100.31.0/24', - 'via', '10.100.10.30']] + expected = [['110.100.31.0/24', '10.100.10.30']] - self._check_agent_method_called(agent, expected, namespace) + self._check_route_calls('delete', expected, ri.ns_name()) fake_new_routes = [] ri.router['routes'] = fake_new_routes agent.routes_updated(ri) - expected = [['ip', 'route', 'delete', 'to', '110.100.30.0/24', - 'via', '10.100.10.30']] - self._check_agent_method_called(agent, expected, namespace) + expected = [['110.100.30.0/24', '10.100.10.30']] + self._check_route_calls('delete', expected, ri.ns_name()) def testProcessRouter(self): diff --git a/quantum/tests/unit/test_linux_ip_lib.py b/quantum/tests/unit/test_linux_ip_lib.py index 47b40631d..25b0227ec 100644 --- a/quantum/tests/unit/test_linux_ip_lib.py +++ b/quantum/tests/unit/test_linux_ip_lib.py @@ -549,25 +549,54 @@ 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 - self.route_cmd.add_gateway(gateway, metric) + dev = 'eth0' + + self.route_cmd.add_gateway(gateway, metric, dev) self._assert_sudo([], ('replace', 'default', 'via', gateway, 'metric', metric, - 'dev', self.parent.name)) + 'dev', dev)) - def test_del_gateway(self): + def test_delete_gateway_with_dev(self): gateway = '192.168.45.100' - self.route_cmd.delete_gateway(gateway) + dev = 'eth0' + + self.route_cmd.delete_gateway(gateway, dev) self._assert_sudo([], - ('del', 'default', 'via', gateway, - 'dev', self.parent.name)) + ('delete', 'default', 'via', gateway, + 'dev', dev)) def test_get_gateway(self): test_cases = [{'sample': GATEWAY_SAMPLE1, @@ -614,6 +643,59 @@ 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()