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 <gong.yongsheng@99cloud.net>
Related-Bug: 1414640
Change-Id: I1406b1876c3a47b110818686b42e5f2f688154fa

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.

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('}')
 

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