From 2cde9609c972e4fc4dd11a5c4caa342a3c1a3272 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Thu, 3 Jun 2021 14:49:45 +0000 Subject: [PATCH] Populate self.floating_ips_dict using "ip rule" information When the L3 agent starts, reads the floating IP rule priority from a state file created by "FipRulePriorityAllocator". In case of not having all floating IPs registers in this file, the method: - Creates a new priority for this floating IP. - Creates the "ip rule" in the namespace. - Adds a new entry in "self.floating_ips_dict". All "ip rules" present in the namespace that do not match the registered fixed IP address ("from") and the priority assigned are deleted. Closes-Bug: #1891673 Closes-Bug: #1929821 Conflicts: neutron/tests/unit/agent/l3/test_dvr_local_router.py Change-Id: Ia3fbde3304ab5f3c309dc62dbf58274afbcf4614 (cherry picked from commit a03c240ef4ea1d4b874b618dbd0163a3a2f7024c) (cherry picked from commit b4ad1a2775d00cd6d14bd4766a0a1c5c41332d89) --- neutron/agent/l3/dvr_local_router.py | 76 ++++++++++++++----- .../agent/l3/fip_rule_priority_allocator.py | 3 + .../tests/functional/agent/l3/framework.py | 5 +- neutron/tests/unit/agent/l3/test_agent.py | 8 +- .../unit/agent/l3/test_dvr_local_router.py | 74 +++++++++++------- neutron/tests/unit/agent/linux/test_pd.py | 8 +- 6 files changed, 116 insertions(+), 58 deletions(-) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index f50d7bc750a..0cd90a79eab 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/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']) + if not ex_gw_port: + return - 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) + 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.""" diff --git a/neutron/agent/l3/fip_rule_priority_allocator.py b/neutron/agent/l3/fip_rule_priority_allocator.py index 8fcb8f98e6d..8948067b3b7 100644 --- a/neutron/agent/l3/fip_rule_priority_allocator.py +++ b/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. diff --git a/neutron/tests/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index 1cfc16eba93..90481e47fb4 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/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) diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 193ad0d887c..12e6c68a71d 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/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) diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index 90fda10e4a6..25f9ea2c407 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/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() diff --git a/neutron/tests/unit/agent/linux/test_pd.py b/neutron/tests/unit/agent/linux/test_pd.py index 93985a83584..5e1cdbdd1b0 100644 --- a/neutron/tests/unit/agent/linux/test_pd.py +++ b/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'