From cd0cc47a6ab0f7968a8c24e9d477909c45e4ae87 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Thu, 11 Oct 2018 23:25:44 -0700 Subject: [PATCH] DVR: Centralized FloatingIPs are not cleared after migration. With DVR routers, if a port is associated with a FloatingIP, before it is used by a VM, the FloatingIP will be initially started at the Network Node SNAT Namespace, since the port is not bound to any host. Then when the port is attached to a VM, the port gets its host binding, and then the FloatingIP setup should be migrated to the Compute host and the original FloatingIP in the Network Node SNAT Namespace should be cleared. But the original FloatingIP setup in SNAT Namespace was not cleared by the agent. This patch addresses the issue. Change-Id: I55a16bcc0020087aa1abe76f5bc85cd64ccdaecd Closes-Bug: #1796491 --- neutron/agent/l3/dvr_edge_ha_router.py | 7 +++-- neutron/agent/l3/dvr_edge_router.py | 14 +++++---- neutron/agent/l3/dvr_local_router.py | 10 ++----- neutron/agent/l3/router_info.py | 7 ++--- neutron/db/l3_dvrscheduler_db.py | 5 ++-- .../tests/functional/agent/l3/framework.py | 5 ++++ .../functional/agent/l3/test_dvr_router.py | 26 +++++++++++++--- neutron/tests/unit/agent/l3/test_agent.py | 22 +++++--------- .../unit/scheduler/test_l3_agent_scheduler.py | 30 +++++++++++++++++++ 9 files changed, 85 insertions(+), 41 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_ha_router.py b/neutron/agent/l3/dvr_edge_ha_router.py index 9c9035f4ab7..f49093919e2 100644 --- a/neutron/agent/l3/dvr_edge_ha_router.py +++ b/neutron/agent/l3/dvr_edge_ha_router.py @@ -76,9 +76,12 @@ class DvrEdgeHaRouter(dvr_edge_router.DvrEdgeRouter, super(DvrEdgeHaRouter, self).remove_centralized_floatingip( fip_cidr) - def _get_centralized_fip_cidr_set(self): + def get_centralized_fip_cidr_set(self): + ex_gw_port = self.get_ex_gw_port() + if not ex_gw_port: + return set() interface_name = self.get_snat_external_device_interface_name( - self.get_ex_gw_port()) + ex_gw_port) return set(self._get_cidrs_from_keepalived(interface_name)) def external_gateway_added(self, ex_gw_port, interface_name): diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index 36ba61065d1..f624da415aa 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/neutron/agent/l3/dvr_edge_router.py @@ -267,10 +267,13 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): long_name = router.EXTERNAL_DEV_PREFIX + ex_gw_port['id'] return long_name[:self.driver.DEV_NAME_LEN] - def _get_centralized_fip_cidr_set(self): + def get_centralized_fip_cidr_set(self): """Returns the fip_cidr set for centralized floatingips.""" + ex_gw_port = self.get_ex_gw_port() + if not ex_gw_port: + return set() interface_name = self.get_snat_external_device_interface_name( - self.get_ex_gw_port()) + ex_gw_port) device = ip_lib.IPDevice( interface_name, namespace=self.snat_namespace.name) return set([addr['cidr'] for addr in device.addr.list()]) @@ -285,11 +288,10 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): """ fip_cidrs = super(DvrEdgeRouter, self).get_router_cidrs(device) centralized_cidrs = set() - # Call _get_centralized_fip_cidr only when snat_namespace exists + # Call get_centralized_fip_cidr_set only when snat_namespace exists if self.get_ex_gw_port() and self.snat_namespace.exists(): - centralized_cidrs = self._get_centralized_fip_cidr_set() - existing_centralized_cidrs = self.centralized_floatingips_set - return fip_cidrs | centralized_cidrs | existing_centralized_cidrs + centralized_cidrs = self.get_centralized_fip_cidr_set() + return fip_cidrs | centralized_cidrs def remove_centralized_floatingip(self, fip_cidr): """Function to handle the centralized Floatingip remove.""" diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 5b0f5edb4d4..961187f3313 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -42,16 +42,12 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): super(DvrLocalRouter, self).__init__(host, *args, **kwargs) self.floating_ips_dict = {} - self.centralized_floatingips_set = set() # Linklocal subnet for router and floating IP namespace link self.rtr_fip_subnet = None self.rtr_fip_connect = False self.fip_ns = None self._pending_arp_set = set() - def get_centralized_router_cidrs(self): - return self.centralized_floatingips_set - def migrate_centralized_floating_ip(self, fip, interface_name, device): # Remove the centralized fip first and then add fip to the host ip_cidr = common_utils.ip_to_cidr(fip['floating_ip_address']) @@ -113,8 +109,6 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): """Add floating IP to respective namespace based on agent mode.""" if fip.get(lib_constants.DVR_SNAT_BOUND): floating_ip_status = self.add_centralized_floatingip(fip, fip_cidr) - if floating_ip_status == lib_constants.FLOATINGIP_STATUS_ACTIVE: - self.centralized_floatingips_set.add(fip_cidr) return floating_ip_status if not self._check_if_floatingip_bound_to_host(fip): # TODO(Swami): Need to figure out what status @@ -172,9 +166,9 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): def floating_ip_removed_dist(self, fip_cidr): """Remove floating IP from FIP namespace.""" - if fip_cidr in self.centralized_floatingips_set: + centralized_fip_cidrs = self.get_centralized_fip_cidr_set() + if fip_cidr in centralized_fip_cidrs: self.remove_centralized_floatingip(fip_cidr) - self.centralized_floatingips_set.remove(fip_cidr) return floating_ip = fip_cidr.split('/')[0] fip_2_rtr_name = self.fip_ns.get_int_device_name(self.router_id) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index beac1c8c903..9b5f1ff36eb 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -326,7 +326,7 @@ class RouterInfo(object): def get_router_cidrs(self, device): return set([addr['cidr'] for addr in device.addr.list()]) - def get_centralized_router_cidrs(self): + def get_centralized_fip_cidr_set(self): return set() def process_floating_ip_addresses(self, interface_name): @@ -346,7 +346,7 @@ class RouterInfo(object): existing_cidrs = self.get_router_cidrs(device) new_cidrs = set() gw_cidrs = self._get_gw_ips_cidr() - + centralized_fip_cidrs = self.get_centralized_fip_cidr_set() floating_ips = self.get_floating_ips() # Loop once to ensure that floating ips are configured. for fip in floating_ips: @@ -354,7 +354,6 @@ class RouterInfo(object): ip_cidr = common_utils.ip_to_cidr(fip_ip) new_cidrs.add(ip_cidr) fip_statuses[fip['id']] = lib_constants.FLOATINGIP_STATUS_ACTIVE - cent_router_cidrs = self.get_centralized_router_cidrs() if ip_cidr not in existing_cidrs: fip_statuses[fip['id']] = self.add_floating_ip( @@ -369,7 +368,7 @@ class RouterInfo(object): {'old': self.fip_map[fip_ip], 'new': fip['fixed_ip_address']}) fip_statuses[fip['id']] = self.move_floating_ip(fip) - elif (ip_cidr in cent_router_cidrs and + elif (ip_cidr in centralized_fip_cidrs and fip.get('host') == self.host): LOG.debug("Floating IP is migrating from centralized " "to distributed: %s", fip) diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py index a82bfe2e957..a2a86b264cd 100644 --- a/neutron/db/l3_dvrscheduler_db.py +++ b/neutron/db/l3_dvrscheduler_db.py @@ -496,12 +496,11 @@ def _notify_l3_agent_port_update(resource, event, trigger, **kwargs): # If dest_host is set, then the port profile has changed # and this port is in migration. The call below will # pre-create the router on the new host - # No need to check for device_owner since we are scheduling - # the routers without checking for device_owner. # If the original_port is None, then it is a migration # from unbound to bound. if (is_new_port_binding_changed or dest_host): - if original_port[portbindings.HOST_ID] is None: + if (not original_port[portbindings.HOST_ID] and + not original_port['device_owner']): l3plugin.dvr_handle_new_service_port(context, new_port, unbound_migrate=True) else: diff --git a/neutron/tests/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index f70786a5c6e..f21afb551ff 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -658,6 +658,11 @@ class L3AgentTestFramework(base.BaseSudoTestCase): self._assert_ip_address_on_interface(namespace, interface, ip_address) + def _assert_ip_address_not_on_interface( + self, namespace, interface, ip_address): + self.assertNotIn( + ip_address, self._get_addresses_on_device(namespace, interface)) + def _assert_ip_address_on_interface(self, namespace, interface, ip_address): self.assertIn( diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index ae82180d577..571896e5a7b 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -918,6 +918,14 @@ class TestDvrRouter(framework.L3AgentTestFramework): str(iptables_manager.IptablesRule(rule[0], rule[1])), rules) return True + def _assert_iptables_rules_not_exist( + self, router_iptables_manager, table_name, expected_rules): + rules = router_iptables_manager.get_rules_for_table(table_name) + for rule in expected_rules: + self.assertNotIn( + str(iptables_manager.IptablesRule(rule[0], rule[1])), rules) + return True + def test_prevent_snat_rule_exist_on_restarted_agent(self): self.agent.conf.agent_mode = 'dvr_snat' router_info = self.generate_dvr_router_info() @@ -1112,8 +1120,8 @@ class TestDvrRouter(framework.L3AgentTestFramework): self._assert_iptables_rules_exist( router1.snat_iptables_manager, 'nat', expected_rules) - def test_floating_ip_migration_from_unbound_to_bound(self): - """Test to check floating ips migrate from unboun to bound host.""" + def test_floating_ip_migrate_when_unbound_port_is_bound_to_a_host(self): + """Test to check floating ips migrate from unbound to bound host.""" self.agent.conf.agent_mode = lib_constants.L3_AGENT_MODE_DVR_SNAT router_info = self.generate_dvr_router_info( enable_floating_ip=True, enable_centralized_fip=True, @@ -1158,8 +1166,18 @@ class TestDvrRouter(framework.L3AgentTestFramework): qrouter_ns = router_updated.ns_name fixed_ip_dist = distributed_fip['fixed_ip_address'] + self._assert_snat_namespace_exists(router_updated) snat_ns = router_updated.snat_namespace.name fixed_ip_cent = centralized_floatingip['fixed_ip_address'] + router_updated.get_centralized_fip_cidr_set = mock.Mock( + return_value=set(["19.4.4.3/32"])) + self.assertTrue(self._assert_iptables_rules_not_exist( + router_updated.snat_iptables_manager, 'nat', expected_rules)) + port = router_updated.get_ex_gw_port() + interface_name = router_updated.get_external_device_name(port['id']) + self._assert_ip_address_not_on_interface( + snat_ns, interface_name, + centralized_floatingip['floating_ip_address']) self.assertTrue(self._fixed_ip_rule_exists(qrouter_ns, fixed_ip_dist)) self.assertFalse(self._fixed_ip_rule_exists(snat_ns, fixed_ip_dist)) self.assertTrue(self._fixed_ip_rule_exists(qrouter_ns, fixed_ip_cent)) @@ -1390,7 +1408,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): @mock.patch.object(dvr_local_router.DvrLocalRouter, 'connect_rtr_2_fip') @mock.patch.object( - dvr_ha_router.DvrEdgeHaRouter, '_get_centralized_fip_cidr_set') + dvr_ha_router.DvrEdgeHaRouter, 'get_centralized_fip_cidr_set') def test_dvr_ha_router_with_centralized_fip_calls_keepalived_cidr( self, connect_rtr_2_fip_mock, fip_cidr_centralized_mock): @@ -1409,7 +1427,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): @mock.patch.object(dvr_local_router.DvrLocalRouter, 'connect_rtr_2_fip') @mock.patch.object( - dvr_edge_router.DvrEdgeRouter, '_get_centralized_fip_cidr_set') + dvr_edge_router.DvrEdgeRouter, 'get_centralized_fip_cidr_set') def test_dvr_router_with_centralized_fip_calls_keepalived_cidr( self, connect_rtr_2_fip_mock, fip_cidr_centralized_mock): diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 555685aae38..f02dfd3baa3 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -1145,8 +1145,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): ri.iptables_manager.ipv4['nat'] = mock.MagicMock() ex_gw_port = {'id': _uuid(), 'network_id': mock.sentinel.ext_net_id} - ri.get_centralized_router_cidrs = mock.Mock( - return_value=set()) ri.add_floating_ip = mock.Mock( return_value=lib_constants.FLOATINGIP_STATUS_ACTIVE) with mock.patch.object(lla.LinkLocalAllocator, '_write'): @@ -1331,6 +1329,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): with mock.patch.object(ri, 'get_floating_ips') as fips, \ mock.patch.object(ri, 'add_centralized_floatingip') as add_fip, \ + mock.patch.object(ri, 'get_centralized_fip_cidr_set' + ) as get_fip_cidrs, \ mock.patch.object(ri, 'get_floating_agent_gw_interface' ) as fip_gw_port, \ mock.patch.object(ri.fip_ns, @@ -1351,22 +1351,20 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): status = ri.floating_ip_added_dist(fips, "192.168.0.1/32") add_fip.assert_called_once_with(fips, "192.168.0.1/32") self.assertEqual(lib_constants.FLOATINGIP_STATUS_ACTIVE, status) - self.assertEqual(set(["192.168.0.1/32"]), - ri.centralized_floatingips_set) # Now let us add the second fip status = ri.floating_ip_added_dist(fips, "192.168.0.2/32") self.assertEqual(lib_constants.FLOATINGIP_STATUS_ACTIVE, status) - self.assertEqual(set(["192.168.0.2/32", "192.168.0.1/32"]), - ri.centralized_floatingips_set) device = mock.Mock() + get_fip_cidrs.return_value = set( + ["192.168.0.2/32", "192.168.0.1/32"]) self.assertEqual(set(["192.168.0.2/32", "192.168.0.1/32"]), ri.get_router_cidrs(device)) ri.floating_ip_removed_dist("192.168.0.1/32") rem_fip.assert_called_once_with("192.168.0.1/32") + self.assertTrue(get_fip_cidrs.called) + get_fip_cidrs.return_value = set(["192.168.0.2/32"]) self.assertEqual(set(["192.168.0.2/32"]), ri.get_router_cidrs(device)) - self.assertEqual(set(["192.168.0.2/32"]), - ri.centralized_floatingips_set) @mock.patch.object(lla.LinkLocalAllocator, '_write') def test_create_dvr_fip_interfaces_for_late_binding(self, lla_write): @@ -1987,7 +1985,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): agent.plugin_rpc, 'update_floatingip_statuses' ) as mock_update_fip_status,\ mock.patch.object( - ri, 'get_centralized_router_cidrs') as cent_cidrs,\ + ri, 'get_centralized_fip_cidr_set') as cent_cidrs,\ mock.patch.object(ri, 'get_router_cidrs') as mock_get_cidrs: cent_cidrs.return_value = set() mock_get_cidrs.return_value = set( @@ -2063,7 +2061,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): agent.plugin_rpc, 'update_floatingip_statuses' ) as mock_update_fip_status,\ mock.patch.object( - ri, 'get_centralized_router_cidrs') as cent_cidrs,\ + ri, 'get_centralized_fip_cidr_set') as cent_cidrs,\ mock.patch.object(ri, 'get_router_cidrs') as mock_get_cidrs: mock_get_cidrs.return_value = set() cent_cidrs.return_value = set() @@ -2091,8 +2089,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router, **self.ri_kwargs) ri.external_gateway_added = mock.Mock() - ri.get_centralized_router_cidrs = mock.Mock( - return_value=set()) ri.process() # Assess the call for putting the floating IP up was performed mock_update_fip_status.assert_called_once_with( @@ -3693,8 +3689,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): pf_used_fip + need_to_remove_fip + [gw_cidr]) ri.iptables_manager.ipv4['nat'] = mock.MagicMock() mock_get_gw_cidr.return_value = set([gw_cidr['cidr']]) - ri.get_centralized_router_cidrs = mock.Mock( - return_value=set()) ri.add_floating_ip = mock.Mock( return_value=lib_constants.FLOATINGIP_STATUS_ACTIVE) ri.remove_floating_ip = mock.Mock() diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index ea7336a8c26..8289bfb1365 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py @@ -882,6 +882,36 @@ class L3DvrSchedulerTestCase(L3SchedulerBaseMixin, self.assertTrue( l3plugin.update_arp_entry_for_dvr_service_port.called) + def test__notify_l3_agent_when_unbound_port_migrates_to_bound_host(self): + port_id = 'fake-port' + kwargs = { + 'context': self.adminContext, + 'original_port': { + 'id': port_id, + portbindings.HOST_ID: '', + 'device_owner': '', + 'admin_state_up': True, + }, + 'port': { + 'id': port_id, + portbindings.HOST_ID: 'vm-host', + 'device_owner': DEVICE_OWNER_COMPUTE, + 'mac_address': '02:04:05:17:18:19' + }, + } + port = kwargs.get('port') + plugin = directory.get_plugin() + l3plugin = mock.Mock() + l3plugin.supported_extension_aliases = [ + 'router', constants.L3_AGENT_SCHEDULER_EXT_ALIAS, + constants.L3_DISTRIBUTED_EXT_ALIAS + ] + directory.add_plugin(plugin_constants.L3, l3plugin) + l3_dvrscheduler_db._notify_l3_agent_port_update( + 'port', 'after_update', plugin, **kwargs) + l3plugin.dvr_handle_new_service_port.assert_called_once_with( + self.adminContext, port, unbound_migrate=True) + def test__notify_l3_agent_update_port_no_removing_routers(self): port_id = 'fake-port' kwargs = {