From f7f09c79e561931bd80b0b7e17faca54926102bd 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 (cherry picked from commit cd0cc47a6ab0f7968a8c24e9d477909c45e4ae87) --- 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 | 20 +++++-------- .../unit/scheduler/test_l3_agent_scheduler.py | 30 +++++++++++++++++++ 9 files changed, 85 insertions(+), 39 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_ha_router.py b/neutron/agent/l3/dvr_edge_ha_router.py index 6851c926b89..57173e05d44 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 146518d54ca..0a5a1d9f191 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/neutron/agent/l3/dvr_edge_router.py @@ -268,10 +268,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()]) @@ -286,11 +289,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 75fe7b7961a..157e3c4428f 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(n_const.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 1b8d9a10bf1..3fee1883626 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -324,7 +324,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): @@ -344,7 +344,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: @@ -352,7 +352,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( @@ -367,7 +366,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 3b92c8ff383..b2e92dd63d3 100644 --- a/neutron/db/l3_dvrscheduler_db.py +++ b/neutron/db/l3_dvrscheduler_db.py @@ -472,12 +472,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 ecf8e7654c7..398b2b65283 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -652,6 +652,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 63d7d0a9b94..b81d8604583 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -914,6 +914,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() @@ -1108,8 +1116,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, @@ -1154,8 +1162,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)) @@ -1385,7 +1403,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): @@ -1404,7 +1422,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 c87da35fa0c..c544a10caf9 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -1125,8 +1125,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'): @@ -1309,6 +1307,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, @@ -1329,22 +1329,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): @@ -1958,7 +1956,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( @@ -2033,7 +2031,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() @@ -2061,8 +2059,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( diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index 73b0115e5af..239b9795a3a 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py @@ -885,6 +885,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 = {