Merge "Populate self.floating_ips_dict using "ip rule" information" into stable/train

neutron/agent/l3/dvr_local_router.py

@@ -46,27 +46,63 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
         self.rtr_fip_connect = False
         self.fip_ns = None
         self._pending_arp_set = set()
+        self._load_used_fip_information()
 
-        self.load_used_fip_information()
+    def _load_used_fip_information(self):
+        """Load FIP from the FipRulePriorityAllocator state file.
 
-    def load_used_fip_information(self):
-        """Some information needed to remove a floating ip e.g. it's associated
-        ip rule priorities, are stored in memory to avoid extra db lookups.
-        Since this is lost on agent restart we need to reload them.
+        If, for any reason, the FIP is not stored in the state file, this
+        method reads the namespace "ip rule" list and search for the
+        corresponding fixed IP of the FIP. If present, this "ip rule" is
+        (1) deleted, (2) a new rule priority is allocated and (3) the "ip rule"
+        is written again with the new assigned priority.
+
+        At the end of the method, all existing "ip rule" registers in
+        FIP_RT_TBL table (where FIP rules are stored) that don't match with
+        any register memoized in self._rule_priorities is deleted.
         """
         ex_gw_port = self.get_ex_gw_port()
-        if ex_gw_port:
-            fip_ns = self.agent.get_fip_ns(ex_gw_port['network_id'])
-
-            for fip in self.get_floating_ips():
-                floating_ip = fip['floating_ip_address']
-                fixed_ip = fip['fixed_ip_address']
-                rule_pr = fip_ns.lookup_rule_priority(floating_ip)
-                if rule_pr:
-                    self.floating_ips_dict[floating_ip] = (fixed_ip, rule_pr)
-                else:
-                    LOG.error("Rule priority not found for floating ip %s",
-                              floating_ip)
+        if not ex_gw_port:
+            return
+
+        fip_ns = self.agent.get_fip_ns(ex_gw_port['network_id'])
+        for fip in self.get_floating_ips():
+            floating_ip = fip['floating_ip_address']
+            fixed_ip = fip['fixed_ip_address']
+            if not fixed_ip:
+                continue
+
+            rule_pr = fip_ns.lookup_rule_priority(floating_ip)
+            if rule_pr:
+                self.floating_ips_dict[floating_ip] = (fixed_ip, rule_pr)
+                continue
+
+            rule_pr = fip_ns.allocate_rule_priority(floating_ip)
+            ip_lib.add_ip_rule(self.ns_name, fixed_ip,
+                               table=dvr_fip_ns.FIP_RT_TBL,
+                               priority=rule_pr)
+            self.floating_ips_dict[floating_ip] = (fixed_ip, rule_pr)
+
+        self._cleanup_unused_fip_ip_rules()
+
+    def _cleanup_unused_fip_ip_rules(self):
+        if not self.router_namespace.exists():
+            # It could be a new router, thus the namespace is not created yet.
+            return
+
+        ip_rules = ip_lib.list_ip_rules(self.ns_name,
+                                        lib_constants.IP_VERSION_4)
+        ip_rules = [ipr for ipr in ip_rules
+                    if ipr['table'] == dvr_fip_ns.FIP_RT_TBL]
+        for ip_rule in ip_rules:
+            for fixed_ip, rule_pr in self.floating_ips_dict.values():
+                if (ip_rule['from'] == fixed_ip and
+                        ip_rule['priority'] == rule_pr):
+                    break
+            else:
+                ip_lib.delete_ip_rule(self.ns_name, ip_rule['from'],
+                                      table=dvr_fip_ns.FIP_RT_TBL,
+                                      priority=ip_rule['priority'])
 
     def migrate_centralized_floating_ip(self, fip, interface_name, device):
         # Remove the centralized fip first and then add fip to the host
@@ -182,10 +218,10 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
                                   priority=int(str(rule_pr)))
             self.fip_ns.deallocate_rule_priority(floating_ip)
         else:
-            LOG.error("Unable to find necessary information to complete "
-                      "removal of floating ip rules for %s - will require "
-                      "manual cleanup (see LP 1891673 for details).",
-                      floating_ip)
+            LOG.error('Floating IP %s not stored in this agent. Because of '
+                      'the initilization method "_load_used_fip_information", '
+                      'all floating IPs should be memoized in the local '
+                      'memory.', floating_ip)
 
     def floating_ip_removed_dist(self, fip_cidr):
         """Remove floating IP from FIP namespace."""

neutron/agent/l3/fip_rule_priority_allocator.py

@@ -31,6 +31,9 @@ class FipPriority(object):
         else:
             return False
 
+    def __int__(self):
+        return int(self.index)
+
 
 class FipRulePriorityAllocator(ItemAllocator):
     """Manages allocation of floating ips rule priorities.

neutron/tests/functional/agent/l3/framework.py

@@ -27,6 +27,7 @@ import testtools
 
 from neutron.agent.common import ovs_lib
 from neutron.agent.l3 import agent as neutron_l3_agent
+from neutron.agent.l3 import dvr_local_router
 from neutron.agent.l3 import namespaces
 from neutron.agent.l3 import router_info as l3_router_info
 from neutron.agent import l3_agent as l3_agent_main
@@ -405,7 +406,9 @@ class L3AgentTestFramework(base.BaseSudoTestCase):
             ovsbr.clear_db_attribute('Port', device_name, 'tag')
 
         with mock.patch(OVS_INTERFACE_DRIVER + '.plug_new', autospec=True) as (
-                ovs_plug):
+                ovs_plug), \
+                mock.patch.object(dvr_local_router.DvrLocalRouter,
+                                  '_load_used_fip_information'):
             ovs_plug.side_effect = new_ovs_plug
             agent._process_added_router(router)
 

neutron/tests/unit/agent/l3/test_agent.py

@@ -215,6 +215,9 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
     def setUp(self):
         super(TestBasicRouterOperations, self).setUp()
         self.useFixture(IptablesFixture())
+        self._mock_load_fip = mock.patch.object(
+            dvr_local_router.DvrLocalRouter, '_load_used_fip_information')
+        self.mock_load_fip = self._mock_load_fip.start()
 
     def test_request_id_changes(self):
         a = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1214,11 +1217,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             self.assertIsNone(res_ip)
             self.assertTrue(log_error.called)
 
-    @mock.patch.object(dvr_router.DvrEdgeRouter, 'load_used_fip_information')
     @mock.patch.object(dvr_router_base.LOG, 'error')
-    def test_get_snat_port_for_internal_port_ipv6_same_port(self,
-                                                            log_error,
-                                                            load_used_fips):
+    def test_get_snat_port_for_internal_port_ipv6_same_port(self, log_error):
         router = l3_test_common.prepare_router_data(
             ip_version=lib_constants.IP_VERSION_4, enable_snat=True,
             num_internal_ports=1)

neutron/tests/unit/agent/l3/test_dvr_local_router.py

@@ -12,6 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import copy
+
 import mock
 from neutron_lib.api.definitions import portbindings
 from neutron_lib import constants as lib_constants
@@ -22,6 +24,7 @@ from oslo_utils import uuidutils
 from neutron.agent.l3 import agent as l3_agent
 from neutron.agent.l3 import dvr_edge_ha_router as dvr_edge_ha_rtr
 from neutron.agent.l3 import dvr_edge_router as dvr_edge_rtr
+from neutron.agent.l3 import dvr_fip_ns
 from neutron.agent.l3 import dvr_local_router as dvr_router
 from neutron.agent.l3 import link_local_allocator as lla
 from neutron.agent.l3 import router_info
@@ -38,6 +41,9 @@ from neutron.tests.common import l3_test_common
 _uuid = uuidutils.generate_uuid
 FIP_PRI = 32768
 HOSTNAME = 'myhost'
+FIP_RULE_PRIO_LIST = [['fip_1', 'fixed_ip_1', 'prio_1'],
+                      ['fip_2', 'fixed_ip_2', 'prio_2'],
+                      ['fip_3', 'fixed_ip_3', 'prio_3']]
 
 
 class TestDvrRouterOperations(base.BaseTestCase):
@@ -116,6 +122,10 @@ class TestDvrRouterOperations(base.BaseTestCase):
             'oslo_service.loopingcall.FixedIntervalLoopingCall')
         self.looping_call_p.start()
 
+        self.mock_load_fip_p = mock.patch.object(dvr_router.DvrLocalRouter,
+                                                 '_load_used_fip_information')
+        self.mock_load_fip = self.mock_load_fip_p.start()
+
         subnet_id_1 = _uuid()
         subnet_id_2 = _uuid()
         self.snat_ports = [{'subnets': [{'cidr': '152.2.0.0/16',
@@ -154,9 +164,7 @@ class TestDvrRouterOperations(base.BaseTestCase):
         kwargs['router'] = router
         kwargs['agent_conf'] = self.conf
         kwargs['interface_driver'] = mock.Mock()
-        with mock.patch.object(dvr_router.DvrLocalRouter,
-                               'load_used_fip_information'):
-            return dvr_router.DvrLocalRouter(HOSTNAME, **kwargs)
+        return dvr_router.DvrLocalRouter(HOSTNAME, **kwargs)
 
     def _set_ri_kwargs(self, agent, router_id, router):
         self.ri_kwargs['agent'] = agent
@@ -224,32 +232,40 @@ class TestDvrRouterOperations(base.BaseTestCase):
             self.assertTrue(
                 ri.fip_ns.create_rtr_2_fip_link.called)
 
-    def test_load_used_fip_information(self):
-        router = mock.MagicMock()
-        with mock.patch.object(dvr_router.DvrLocalRouter,
-                               'get_floating_ips') as mock_get_floating_ips:
-            with mock.patch.object(dvr_router.DvrLocalRouter,
-                                   'get_ex_gw_port') as mock_ext_port:
-                mock_ext_port.return_value = {'network_id': _uuid()}
-                fip = {'id': _uuid(),
-                       'host': HOSTNAME,
-                       'floating_ip_address': '15.1.2.3',
-                       'fixed_ip_address': '192.168.0.1',
-                       'floating_network_id': _uuid(),
-                       'port_id': _uuid()}
-                fip_ns = mock.MagicMock()
-                fip_ns.lookup_rule_priority.return_value = 1234
-                mock_get_floating_ips.return_value = [fip]
-                mock_agent = mock.MagicMock()
-                mock_agent.get_fip_ns.return_value = fip_ns
-                kwargs = {'agent': mock_agent,
-                          'router_id': _uuid(),
-                          'router': mock.Mock(),
-                          'agent_conf': self.conf,
-                          'interface_driver': mock.Mock()}
-                router = dvr_router.DvrLocalRouter(HOSTNAME, **kwargs)
-                self.assertEqual({'15.1.2.3': ('192.168.0.1', 1234)},
-                                 router.floating_ips_dict)
+    @mock.patch.object(ip_lib, 'add_ip_rule')
+    def test__load_used_fip_information(self, mock_add_ip_rule):
+        # This test will simulate how "DvrLocalRouter" reloads the FIP
+        # information from both the FipNamespace._rule_priorities state file
+        # and the namespace "ip rule" list.
+        router = self._create_router()
+        self.mock_load_fip_p.stop()
+        fip_ns = router.agent.get_fip_ns('net_id')
+
+        # To simulate a partially populated FipNamespace._rule_priorities
+        # state file, we load all FIPs but last.
+        fip_rule_prio_list = copy.deepcopy(FIP_RULE_PRIO_LIST)
+        for idx, (fip, _, _) in enumerate(FIP_RULE_PRIO_LIST[:-1]):
+            prio = fip_ns.allocate_rule_priority(fip)
+            fip_rule_prio_list[idx][2] = prio
+
+        fips = [{'floating_ip_address': fip, 'fixed_ip_address': fixed_ip} for
+                fip, fixed_ip, _ in fip_rule_prio_list]
+        with mock.patch.object(dvr_fip_ns.FipNamespace,
+                               'allocate_rule_priority',
+                               return_value=fip_rule_prio_list[-1][2]), \
+                mock.patch.object(router, '_cleanup_unused_fip_ip_rules'), \
+                mock.patch.object(router, 'get_floating_ips',
+                                  return_value=fips):
+            router._load_used_fip_information()
+
+        mock_add_ip_rule.assert_called_once_with(
+            router.ns_name, fip_rule_prio_list[2][1],
+            table=dvr_fip_ns.FIP_RT_TBL, priority=fip_rule_prio_list[-1][2])
+        self.assertEqual(3, len(router.floating_ips_dict))
+        ret = [[fip, fixed_ip, prio] for fip, (fixed_ip, prio) in
+               router.floating_ips_dict.items()]
+        self.assertEqual(sorted(ret, key=lambda ret: ret[0]),
+                         fip_rule_prio_list)
 
     def test_get_floating_ips_dvr(self):
         router = mock.MagicMock()

neutron/tests/unit/agent/linux/test_pd.py

@@ -47,8 +47,8 @@ class TestPrefixDelegation(tests_base.DietTestCase):
         self.assertEqual(ns_name, pd_router.get('ns_name'))
 
     @mock.patch.object(dvr_edge_router.DvrEdgeRouter,
-                       'load_used_fip_information')
-    def test_add_update_dvr_edge_router(self, load_used_fip_info):
+                       '_load_used_fip_information')
+    def test_add_update_dvr_edge_router(self, *args):
         l3_agent = mock.Mock()
         l3_agent.pd.routers = {}
         router_id = '1'
@@ -62,8 +62,8 @@ class TestPrefixDelegation(tests_base.DietTestCase):
         self._test_add_update_pd(l3_agent, ri, ns_name)
 
     @mock.patch.object(dvr_local_router.DvrLocalRouter,
-                       'load_used_fip_information')
-    def test_add_update_dvr_local_router(self, load_used_fip_info):
+                       '_load_used_fip_information')
+    def test_add_update_dvr_local_router(self, *args):
         l3_agent = mock.Mock()
         l3_agent.pd.routers = {}
         router_id = '1'