Merge "DVR: Centralized FloatingIPs are not cleared after migration."

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):

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()
+            centralized_cidrs = self.get_centralized_fip_cidr_set()
-        existing_centralized_cidrs = self.centralized_floatingips_set
+        return fip_cidrs | centralized_cidrs
-        return fip_cidrs | centralized_cidrs | existing_centralized_cidrs
 
     def remove_centralized_floatingip(self, fip_cidr):
         """Function to handle the centralized Floatingip remove."""

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)

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)

neutron/db/l3_dvrscheduler_db.py

@@ -595,12 +595,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:

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(

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):
+    def test_floating_ip_migrate_when_unbound_port_is_bound_to_a_host(self):
-        """Test to check floating ips migrate from unboun to bound host."""
+        """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):
 

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(
@@ -3773,8 +3769,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()

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 = {