Always send status update for processed floating ips

Currently l3 agent skips status update for floating ips in case
status didn't change: this might be wrong if status has changed
on server side while agent was processing. See bug for details.

L3 agent skips floating ip processing in case ip address exists
on external device. So we can still skip status update for such
floating ips.

Closes-Bug: #1505557
Change-Id: I908fe5a0555f68ab85e7d199c36a903b915e103f

diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index d6e6302e90c3f00f1a8a4ce6cd1d95d51558a1ad..30d01122cfb84827420e0bb15bf01289c0cd20c9 100644 (file)
--- a/neutron/agent/l3/router_info.py
+++ b/neutron/agent/l3/router_info.py
@@ -233,11 +233,10 @@ class RouterInfo(object):
                 LOG.debug('Floating ip %(id)s added, status %(status)s',
                           {'id': fip['id'],
                            'status': fip_statuses.get(fip['id'])})
-
+            elif fip_statuses[fip['id']] == fip['status']:
                 # mark the status as not changed. we can't remove it because
                 # that's how the caller determines that it was removed
-                if fip_statuses[fip['id']] == fip['status']:
-                    fip_statuses[fip['id']] = FLOATINGIP_STATUS_NOCHANGE
+                fip_statuses[fip['id']] = FLOATINGIP_STATUS_NOCHANGE
         fips_to_remove = (
             ip_cidr for ip_cidr in existing_cidrs - new_cidrs
             if common_utils.is_cidr_host(ip_cidr))

diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index 9bf76b63069f7d16a2178d97a58d9b0cc8f60670..6b66df4a01fc5298a2ba1639a4ce6b63b94a6a72 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -1415,25 +1415,53 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
 
     def test_process_router_floatingip_nochange(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        router = l3_test_common.prepare_router_data(num_internal_ports=1)
+        fip1 = {'id': _uuid(), 'floating_ip_address': '8.8.8.8',
+                'fixed_ip_address': '7.7.7.7', 'status': 'ACTIVE',
+                'port_id': router[l3_constants.INTERFACE_KEY][0]['id']}
+        fip2 = copy.copy(fip1)
+        fip2.update({'id': _uuid(), 'status': 'DOWN',
+                     'floating_ip_address': '9.9.9.9'})
+        router[l3_constants.FLOATINGIP_KEY] = [fip1, fip2]
+
+        ri = legacy_router.LegacyRouter(router['id'], router,
+                                        **self.ri_kwargs)
+        ri.external_gateway_added = mock.Mock()
         with mock.patch.object(
             agent.plugin_rpc, 'update_floatingip_statuses'
-        ) as mock_update_fip_status:
-            router = l3_test_common.prepare_router_data(num_internal_ports=1)
-            fip1 = {'id': _uuid(), 'floating_ip_address': '8.8.8.8',
-                    'fixed_ip_address': '7.7.7.7', 'status': 'ACTIVE',
-                    'port_id': router[l3_constants.INTERFACE_KEY][0]['id']}
-            fip2 = copy.copy(fip1)
-            fip2.update({'id': _uuid(), 'status': 'DOWN'})
-            router[l3_constants.FLOATINGIP_KEY] = [fip1, fip2]
-
-            ri = legacy_router.LegacyRouter(router['id'], router,
-                                            **self.ri_kwargs)
-            ri.external_gateway_added = mock.Mock()
+        ) as mock_update_fip_status,\
+                mock.patch.object(ri, 'get_router_cidrs') as mock_get_cidrs:
+            mock_get_cidrs.return_value = set(
+                [fip1['floating_ip_address'] + '/32'])
             ri.process(agent)
-            # make sure only the one that went from DOWN->ACTIVE was sent
+            # make sure only the one that wasn't in existing cidrs was sent
             mock_update_fip_status.assert_called_once_with(
                 mock.ANY, ri.router_id, {fip2['id']: 'ACTIVE'})
 
+    def test_process_router_floatingip_status_update_if_processed(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        router = l3_test_common.prepare_router_data(num_internal_ports=1)
+        fip1 = {'id': _uuid(), 'floating_ip_address': '8.8.8.8',
+                'fixed_ip_address': '7.7.7.7', 'status': 'ACTIVE',
+                'port_id': router[l3_constants.INTERFACE_KEY][0]['id']}
+        fip2 = copy.copy(fip1)
+        fip2.update({'id': _uuid(), 'status': 'DOWN', })
+        router[l3_constants.FLOATINGIP_KEY] = [fip1, fip2]
+
+        ri = legacy_router.LegacyRouter(router['id'], router,
+                                        **self.ri_kwargs)
+        ri.external_gateway_added = mock.Mock()
+        with mock.patch.object(
+            agent.plugin_rpc, 'update_floatingip_statuses'
+        ) as mock_update_fip_status,\
+                mock.patch.object(ri, 'get_router_cidrs') as mock_get_cidrs:
+            mock_get_cidrs.return_value = set()
+            ri.process(agent)
+            # make sure both was sent since not existed in existing cidrs
+            mock_update_fip_status.assert_called_once_with(
+                mock.ANY, ri.router_id, {fip1['id']: 'ACTIVE',
+                                         fip2['id']: 'ACTIVE'})
+
     def test_process_router_floatingip_disabled(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         with mock.patch.object(

diff --git a/neutron/tests/unit/agent/l3/test_router_info.py b/neutron/tests/unit/agent/l3/test_router_info.py
index 66cafa41f4fa5b5a6e3a1c2d909a20a1fcb5a1a1..3033e571a3dba3751df2399a3e22843661092eb1 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_router_info.py
+++ b/neutron/tests/unit/agent/l3/test_router_info.py
@@ -258,7 +258,8 @@ class TestFloatingIpWithMockDevice(BasicRouterTestCaseFramework):
         fip = {
             'id': fip_id, 'port_id': _uuid(),
             'floating_ip_address': '15.1.2.3',
-            'fixed_ip_address': '192.168.0.2'
+            'fixed_ip_address': '192.168.0.2',
+            'status': l3_constants.FLOATINGIP_STATUS_DOWN
         }
 
         IPDevice.return_value = device = mock.Mock()