openstack/neutron-lbaas
Code Issues Proposed changes

Use wrappers instead of direct calls to ip route.

Browse Source 
- 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
This commit is contained in:
Roman Podolyaka 2013-03-20 18:50:50 +02:00
parent 2ac053e9cf
commit f80f06a71d
5 changed files with 181 additions and 115 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
quantum/agent/dhcp_agent.py
View File

@ -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)

30
quantum/agent/l3_agent.py
View File

@ -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

88
quantum/agent/linux/ip_lib.py
View File

@ -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'

80
quantum/tests/unit/test_l3_agent.py
View File

@ -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):

96
quantum/tests/unit/test_linux_ip_lib.py
View File

@ -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()