From 7bff99ac4a5ef0d4b2cc6f77f5679bb8e01f86d7 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Fri, 1 Sep 2017 13:52:51 -0400 Subject: [PATCH] DVR: Always initialize floating IP host With a recent change to the neutron server code, the server was processing floating IPs that were not bound to the respective agent during fullsync operation. Change to always initialize floating IP host info so callers can determine if info should be sent to an agent or not. Also changed the logic that decides when the server should send a floating IP to an agent to be easier to understand. Closes-bug: #1713927 Change-Id: Ic916225e0a11c3fb8cd94437ca063e0d3295a569 --- neutron/common/constants.py | 4 + neutron/db/l3_dvr_db.py | 93 +++++++++---------- .../l3_router/test_l3_dvr_router_plugin.py | 81 ++++++++++++++++ neutron/tests/unit/db/test_l3_dvr_db.py | 7 +- 4 files changed, 134 insertions(+), 51 deletions(-) diff --git a/neutron/common/constants.py b/neutron/common/constants.py index 0bc56c2c10d..6efff0c7bb7 100644 --- a/neutron/common/constants.py +++ b/neutron/common/constants.py @@ -172,6 +172,10 @@ VALID_FLOATINGIP_STATUS = (lib_constants.FLOATINGIP_STATUS_ACTIVE, lib_constants.FLOATINGIP_STATUS_DOWN, lib_constants.FLOATINGIP_STATUS_ERROR) +# Floating IP host binding states +FLOATING_IP_HOST_UNBOUND = "FLOATING_IP_HOST_UNBOUND" +FLOATING_IP_HOST_NEEDS_BINDING = "FLOATING_IP_HOST_NEEDS_BINDING" + # Possible types of values (e.g. in QoS rule types) VALUES_TYPE_CHOICES = "choices" VALUES_TYPE_RANGE = "range" diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index fe8f554322f..cda3d6473be 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -630,57 +630,55 @@ class _DVRAgentInterfaceMixin(object): return routers_dict + @staticmethod + def _get_floating_ip_host(floating_ip): + """Function to return floating IP host binding state + + By default, floating IPs are not bound to any host. Instead + of using the empty string to denote this use a constant. + """ + return floating_ip.get('host', l3_const.FLOATING_IP_HOST_UNBOUND) + def _process_floating_ips_dvr(self, context, routers_dict, floating_ips, host, agent): LOG.debug("FIP Agent : %s ", agent.id) for floating_ip in floating_ips: router = routers_dict.get(floating_ip['router_id']) if router: - router_floatingips = router.get(const.FLOATINGIP_KEY, []) if router['distributed']: - fip_host = floating_ip.get('host') - fip_dest_host = floating_ip.get('dest_host') - # Skip if floatingip need not be processed for the - # given agent. - if self._should_skip_floating_ip_processed_for_given_agent( - floating_ip, fip_host, fip_dest_host, agent): + if self._skip_floating_ip_for_mismatched_agent_or_host( + floating_ip, agent, host): continue - # Also skip floatingip if the fip port have a host defined - # and if the host does not match. - if self._check_floating_ip_not_valid_for_given_host( - fip_host, fip_dest_host, host): - continue - LOG.debug("Floating IP host: %s", fip_host) + router_floatingips = router.get(const.FLOATINGIP_KEY, []) router_floatingips.append(floating_ip) router[const.FLOATINGIP_KEY] = router_floatingips - def _check_floating_ip_not_valid_for_given_host( - self, fip_host, fip_dest_host, host): - """Function to check if floatingip host match for the given agent. + def _skip_floating_ip_for_mismatched_agent_or_host(self, floating_ip, + agent, host): + """Function to check if floating IP processing can be skipped.""" + fip_host = self._get_floating_ip_host(floating_ip) - Check if the given floatingip host matches with the requesting - host when floatingip dest_host is None. - If floatingip dest_host is not None it means that the floatingip - is migrating to a new compute host and the original host will not - match. - """ - host_mismatch = ( - fip_host != host and fip_dest_host is None) - return (fip_host is not None and host_mismatch) + # Skip if it is unbound + if fip_host == l3_const.FLOATING_IP_HOST_UNBOUND: + return True - def _should_skip_floating_ip_processed_for_given_agent( - self, floating_ip, fip_host, fip_dest_host, agent): - """Function to check if floatingip need to be processed or skipped. - - Skip if host and dest_host is none and the agent - requesting is not dvr_snat agent, and the fip has - not already been assigned 'dvr_snat_bound' state. - """ + # Skip if the given agent is in the wrong mode - SNAT bound + # requires DVR_SNAT agent. agent_mode = self._get_agent_mode(agent) - return (fip_host is None and (fip_dest_host is None) and - agent_mode in [const.L3_AGENT_MODE_LEGACY, - const.L3_AGENT_MODE_DVR] and - not floating_ip.get(l3_const.DVR_SNAT_BOUND)) + if (agent_mode in [const.L3_AGENT_MODE_LEGACY, + const.L3_AGENT_MODE_DVR] and + floating_ip.get(l3_const.DVR_SNAT_BOUND)): + return True + + # Skip if it is bound, but not to the given host + fip_dest_host = floating_ip.get('dest_host') + if (fip_host != l3_const.FLOATING_IP_HOST_NEEDS_BINDING and + fip_host != host and fip_dest_host is None): + return True + + # not being skipped, log host + LOG.debug("Floating IP host: %s", fip_host) + return False def _get_fip_agent_gw_ports(self, context, fip_agent_id): """Return list of floating agent gateway ports for the agent.""" @@ -744,30 +742,29 @@ class _DVRAgentInterfaceMixin(object): port_dict.update({port['id']: port}) # Add the port binding host to the floatingip dictionary for fip in floating_ips: + # Assume no host binding required + fip['host'] = l3_const.FLOATING_IP_HOST_UNBOUND vm_port = port_dict.get(fip['port_id'], None) if vm_port: + # Default value if host port-binding required + fip['host'] = l3_const.FLOATING_IP_HOST_NEEDS_BINDING port_host = vm_port[portbindings.HOST_ID] if port_host: - fip['host'] = port_host fip['dest_host'] = ( self._get_dvr_migrating_service_port_hostid( context, fip['port_id'], port=vm_port)) vm_port_agent_mode = vm_port.get('agent', None) - if vm_port_agent_mode == ( + if vm_port_agent_mode != ( l3_const.L3_AGENT_MODE_DVR_NO_EXTERNAL): - # For floatingip configured on ports that - # reside on 'dvr_no_external' agent, get rid of - # the fip host binding since it would be created + # For floatingip configured on ports that do not + # reside on a 'dvr_no_external' agent, add the + # fip host binding, else it will be created # in the 'dvr_snat' agent. - fip['host'] = None - else: - # If no port-binding assign the fip['host'] - # value to None. - fip['host'] = None + fip['host'] = port_host # Handle the case were there is no host binding # for the private ports that are associated with # floating ip. - if not fip['host'] or fip['host'] is None: + if fip['host'] == l3_const.FLOATING_IP_HOST_NEEDS_BINDING: fip[l3_const.DVR_SNAT_BOUND] = True routers_dict = self._process_routers(context, routers, agent) self._process_floating_ips_dvr(context, routers_dict, diff --git a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py index 55aa4a950fd..3e80d2f9ffd 100644 --- a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py +++ b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py @@ -748,6 +748,87 @@ class L3DvrTestCase(L3DvrTestCaseBase): floatingips = router_info[0][constants.FLOATINGIP_KEY] self.assertTrue(floatingips[0][n_const.DVR_SNAT_BOUND]) + def test_dvr_process_floatingips_for_dvr_on_full_sync(self): + HOST1 = 'host1' + helpers.register_l3_agent( + host=HOST1, agent_mode=constants.L3_AGENT_MODE_DVR) + router = self._create_router(ha=False) + private_net1 = self._make_network(self.fmt, 'net1', True) + kwargs = {'arg_list': (external_net.EXTERNAL,), + external_net.EXTERNAL: True} + ext_net = self._make_network(self.fmt, '', True, **kwargs) + self._make_subnet( + self.fmt, ext_net, '10.20.0.1', '10.20.0.0/24', + ip_version=4, enable_dhcp=True) + # Schedule the router to the dvr_snat node + self.l3_plugin.schedule_router(self.context, + router['id'], + candidates=[self.l3_agent]) + + # Set gateway to router + self.l3_plugin._update_router_gw_info( + self.context, router['id'], + {'network_id': ext_net['network']['id']}) + private_subnet1 = self._make_subnet( + self.fmt, + private_net1, + '10.1.0.1', + cidr='10.1.0.0/24', + ip_version=4, + enable_dhcp=True) + with self.port( + subnet=private_subnet1, + device_owner=DEVICE_OWNER_COMPUTE) as int_port1,\ + self.port( + subnet=private_subnet1) as int_port2: + self.l3_plugin.add_router_interface( + self.context, router['id'], + {'subnet_id': private_subnet1['subnet']['id']}) + router_handle = ( + self.l3_plugin.list_active_sync_routers_on_active_l3_agent( + self.context, self.l3_agent['host'], [router['id']])) + self.assertEqual(self.l3_agent['host'], + router_handle[0]['gw_port_host']) + with mock.patch.object(self.l3_plugin, + '_l3_rpc_notifier') as l3_notifier: + self.core_plugin.update_port( + self.context, int_port1['port']['id'], + {'port': {portbindings.HOST_ID: HOST1}}) + # Now let us assign the floatingip to the bound port + # and unbound port. + fip1 = {'floating_network_id': ext_net['network']['id'], + 'router_id': router['id'], + 'port_id': int_port1['port']['id'], + 'tenant_id': int_port1['port']['tenant_id']} + self.l3_plugin.create_floatingip( + self.context, {'floatingip': fip1}) + expected_routers_updated_calls = [ + mock.call(self.context, mock.ANY, HOST1)] + l3_notifier.routers_updated_on_host.assert_has_calls( + expected_routers_updated_calls) + self.assertFalse(l3_notifier.routers_updated.called) + fip2 = {'floating_network_id': ext_net['network']['id'], + 'router_id': router['id'], + 'port_id': int_port2['port']['id'], + 'tenant_id': int_port2['port']['tenant_id']} + self.l3_plugin.create_floatingip( + self.context, {'floatingip': fip2}) + router_info = ( + self.l3_plugin.list_active_sync_routers_on_active_l3_agent( + self.context, self.l3_agent['host'], [router['id']])) + floatingips = router_info[0][constants.FLOATINGIP_KEY] + self.assertEqual(1, len(floatingips)) + self.assertTrue(floatingips[0][n_const.DVR_SNAT_BOUND]) + self.assertEqual(n_const.FLOATING_IP_HOST_NEEDS_BINDING, + floatingips[0]['host']) + router1_info = ( + self.l3_plugin.list_active_sync_routers_on_active_l3_agent( + self.context, HOST1, [router['id']])) + floatingips = router1_info[0][constants.FLOATINGIP_KEY] + self.assertEqual(1, len(floatingips)) + self.assertEqual(HOST1, floatingips[0]['host']) + self.assertIsNone(floatingips[0]['dest_host']) + def test_dvr_router_centralized_floating_ip(self): HOST1 = 'host1' helpers.register_l3_agent( diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 55204069706..0f47d2e18d0 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -363,9 +363,10 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): 'id': _uuid(), 'port_id': _uuid(), 'router_id': 'foo_router_id', - 'host': hostid } - if not hostid: + if hostid is not None: + floatingip['host'] = hostid + else: hostid = 'not_my_host_id' routers = { 'foo_router_id': router @@ -389,7 +390,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): hostid, agent) return (router, floatingip) - def test_floatingip_on_port_not_host(self): + def test_floatingip_on_port_no_host_key(self): router, fip = self._floatingip_on_port_test_setup(None) self.assertNotIn(const.FLOATINGIP_KEY, router)