From e214b56da9205be7ba927142cc92e4f69ad09b01 Mon Sep 17 00:00:00 2001 From: Assaf Muller Date: Mon, 2 Mar 2015 11:29:51 -0500 Subject: [PATCH] Simplify keepalived.virtual_routes keepalived.virtual_routes previously held one list of virtual routes of different kinds, and the HA router class manipulated that list directly. The list held both the default gateway virtual route, and any extra routes. This means that when adding extra routes for example, the HA router would first have to remove all routes that are not default gateway routes, then add the extra routes received via RPC. This is messy because: a) It's needlessly complicated b) It's fragile c) There's zero separation of concerns (HA router should not know how keepalived maintains its list of virtual routes) d) It requires changes to the management of the default gateway and virtual routes just to add another type of extra routes This patch solves these issues by separating the persistency of virtual routes according to their role. Co-Authored-By: gong yong sheng Related-Bug: 1414640 Change-Id: I1406b1876c3a47b110818686b42e5f2f688154fa --- neutron/agent/l3/ha_router.py | 24 ++++-------- neutron/agent/linux/keepalived.py | 35 ++++++++++++++--- .../tests/unit/agent/linux/test_keepalived.py | 39 ++++++++++++++++++- 3 files changed, 76 insertions(+), 22 deletions(-) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 4b88a3e2a81..d66a9d43956 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -179,20 +179,15 @@ def routes_updated(self): new_routes = self.router['routes'] instance = self._get_keepalived_instance() - - # Filter out all of the old routes while keeping only the default route - default_gw = (n_consts.IPv6_ANY, n_consts.IPv4_ANY) - instance.virtual_routes = [route for route in instance.virtual_routes - if route.destination in default_gw] - for route in new_routes: - instance.virtual_routes.append(keepalived.KeepalivedVirtualRoute( - route['destination'], - route['nexthop'])) - + instance.virtual_routes.extra_routes = [ + keepalived.KeepalivedVirtualRoute( + route['destination'], route['nexthop']) + for route in new_routes] self.routes = new_routes def _add_default_gw_virtual_route(self, ex_gw_port, interface_name): subnets = ex_gw_port.get('subnets', []) + default_gw_rts = [] for subnet in subnets: gw_ip = subnet['gateway_ip'] if gw_ip: @@ -202,12 +197,9 @@ def _add_default_gw_virtual_route(self, ex_gw_port, interface_name): netaddr.IPAddress(gw_ip).version == 4 else n_consts.IPv6_ANY) instance = self._get_keepalived_instance() - instance.virtual_routes = ( - [route for route in instance.virtual_routes - if route.destination != default_gw]) - instance.virtual_routes.append( - keepalived.KeepalivedVirtualRoute( - default_gw, gw_ip, interface_name)) + default_gw_rts.append(keepalived.KeepalivedVirtualRoute( + default_gw, gw_ip, interface_name)) + instance.virtual_routes.gateway_routes = default_gw_rts def _should_delete_ipv6_lladdr(self, ipv6_lladdr): """Only the master should have any IP addresses configured. diff --git a/neutron/agent/linux/keepalived.py b/neutron/agent/linux/keepalived.py index 58d5120ed53..0ee258ff0e1 100644 --- a/neutron/agent/linux/keepalived.py +++ b/neutron/agent/linux/keepalived.py @@ -107,6 +107,32 @@ def build_config(self): return output +class KeepalivedInstanceRoutes(object): + def __init__(self): + self.gateway_routes = [] + self.extra_routes = [] + + def remove_routes_on_interface(self, interface_name): + self.gateway_routes = [gw_rt for gw_rt in self.gateway_routes + if gw_rt.interface_name != interface_name] + # NOTE(amuller): extra_routes are initialized from the router's + # 'routes' attribute. These routes do not have an interface + # parameter and so cannot be removed via an interface_name lookup. + + @property + def routes(self): + return self.gateway_routes + self.extra_routes + + def __len__(self): + return len(self.routes) + + def build_config(self): + return itertools.chain([' virtual_routes {'], + (' %s' % route.build_config() + for route in self.routes), + [' }']) + + class KeepalivedInstance(object): """Instance section of a keepalived configuration.""" @@ -127,7 +153,7 @@ def __init__(self, state, interface, vrouter_id, ha_cidrs, self.mcast_src_ip = mcast_src_ip self.track_interfaces = [] self.vips = [] - self.virtual_routes = [] + self.virtual_routes = KeepalivedInstanceRoutes() self.authentication = None metadata_cidr = '169.254.169.254/32' self.primary_vip_range = get_free_range( @@ -148,8 +174,7 @@ def remove_vips_vroutes_by_interface(self, interface_name): self.vips = [vip for vip in self.vips if vip.interface_name != interface_name] - self.virtual_routes = [vroute for vroute in self.virtual_routes - if vroute.interface_name != interface_name] + self.virtual_routes.remove_routes_on_interface(interface_name) def remove_vip_by_ip_address(self, ip_address): self.vips = [vip for vip in self.vips @@ -243,8 +268,8 @@ def build_config(self): config.extend(self._build_vips_config()) - if self.virtual_routes: - config.extend(self._build_virtual_routes_config()) + if len(self.virtual_routes): + config.extend(self.virtual_routes.build_config()) config.append('}') diff --git a/neutron/tests/unit/agent/linux/test_keepalived.py b/neutron/tests/unit/agent/linux/test_keepalived.py index 2770cc62575..7d6e9806f9e 100644 --- a/neutron/tests/unit/agent/linux/test_keepalived.py +++ b/neutron/tests/unit/agent/linux/test_keepalived.py @@ -87,7 +87,7 @@ def _get_config(self): virtual_route = keepalived.KeepalivedVirtualRoute(n_consts.IPv4_ANY, "192.168.1.1", "eth1") - instance1.virtual_routes.append(virtual_route) + instance1.virtual_routes.gateway_routes = [virtual_route] instance2 = keepalived.KeepalivedInstance('MASTER', 'eth4', 2, ['169.254.192.0/18'], @@ -189,6 +189,43 @@ def test_state_exception(self): invalid_auth_type, 'some_password') +class KeepalivedInstanceRoutesTestCase(base.BaseTestCase): + @classmethod + def _get_instance_routes(cls): + routes = keepalived.KeepalivedInstanceRoutes() + default_gw_eth0 = keepalived.KeepalivedVirtualRoute( + '0.0.0.0/0', '1.0.0.254', 'eth0') + default_gw_eth1 = keepalived.KeepalivedVirtualRoute( + '::/0', 'fe80::3e97:eff:fe26:3bfa/64', 'eth1') + routes.gateway_routes = [default_gw_eth0, default_gw_eth1] + extra_routes = [ + keepalived.KeepalivedVirtualRoute('10.0.0.0/8', '1.0.0.1'), + keepalived.KeepalivedVirtualRoute('20.0.0.0/8', '2.0.0.2')] + routes.extra_routes = extra_routes + return routes + + def test_routes(self): + routes = self._get_instance_routes() + self.assertEqual(len(routes.routes), 4) + + def test_remove_routes_on_interface(self): + routes = self._get_instance_routes() + routes.remove_routes_on_interface('eth0') + self.assertEqual(len(routes.routes), 3) + routes.remove_routes_on_interface('eth1') + self.assertEqual(len(routes.routes), 2) + + def test_build_config(self): + expected = """ virtual_routes { + 0.0.0.0/0 via 1.0.0.254 dev eth0 + ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 + 10.0.0.0/8 via 1.0.0.1 + 20.0.0.0/8 via 2.0.0.2 + }""" + routes = self._get_instance_routes() + self.assertEqual(expected, '\n'.join(routes.build_config())) + + class KeepalivedInstanceTestCase(base.BaseTestCase, KeepalivedConfBaseMixin): def test_get_primary_vip(self):