From 02ad1b7909628f435515eca043c5b849d937176c Mon Sep 17 00:00:00 2001
From: Slawek Kaplonski <skaplons@redhat.com>
Date: Tue, 25 Aug 2020 14:44:26 +0200
Subject: [PATCH] Fix migration from the HA to non-HA routers

In case if during switching HA router to be down, there will be any
failure, router_info will be stored in L3 agent's cache as HaRouter.
In case when next update on the router is migration to non-HA router
this is wrong class and it causes other issues, e.g. with
remove_vip_by_ip_address() which is correct only for HA routers.

This patch fixes that issue by adding check of the router's ha and
distributed flags and update local cache with new router_info class
in case if at least one of those flags don't match.

Change-Id: Ib0d3a501f88c149baea7d715c7cfe5811bc85e4f
Closes-Bug: #1892846
(cherry picked from commit 489e0ead7297e3b17ca2bf8c4bea9701ad14a939)
---
 neutron/agent/l3/agent.py                 | 17 +++++++
 neutron/tests/unit/agent/l3/test_agent.py | 60 +++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py
index 6fe209ac6f6..89e9abb4675 100644
--- a/neutron/agent/l3/agent.py
+++ b/neutron/agent/l3/agent.py
@@ -527,6 +527,23 @@ class L3NATAgent(ha.AgentMixin,
 
     def _process_updated_router(self, router):
         ri = self.router_info[router['id']]
+
+        router_ha = router.get('ha')
+        router_distributed = router.get('distributed')
+        if ((router_ha is not None and ri.router.get('ha') != router_ha) or
+                (router_distributed is not None and
+                 ri.router.get('distributed') != router_distributed)):
+            LOG.warning('Type of the router %(id)s changed. '
+                        'Old type: ha=%(old_ha)s; distributed=%(old_dvr)s; '
+                        'New type: ha=%(new_ha)s; distributed=%(new_dvr)s',
+                        {'id': router['id'],
+                         'old_ha': ri.router.get('ha'),
+                         'old_dvr': ri.router.get('distributed'),
+                         'new_ha': router.get('ha'),
+                         'new_dvr': router.get('distributed')})
+            ri = self._create_router(router['id'], router)
+            self.router_info[router['id']] = ri
+
         is_dvr_snat_agent = (self.conf.agent_mode ==
                              lib_const.L3_AGENT_MODE_DVR_SNAT)
         is_dvr_only_agent = (self.conf.agent_mode in
diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index fa8ee2d0f3f..a1e96c386c3 100644
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -2820,6 +2820,16 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         router.distributed = True
         router.ha = True
         router_info = mock.MagicMock()
+
+        def mock_get(name):
+            if name == 'ha':
+                return router.ha
+            if name == 'distributed':
+                return router.distributed
+            return mock.Mock()
+
+        router_info.router.get.side_effect = mock_get
+
         agent.router_info[router.id] = router_info
         updated_router = {'id': '1234',
                           'distributed': True,
@@ -2852,6 +2862,16 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         router._ha_interface = True
         router.ha = True
         router_info = mock.MagicMock()
+
+        def mock_get(name):
+            if name == 'ha':
+                return router.ha
+            if name == 'distributed':
+                return router.distributed
+            return mock.Mock()
+
+        router_info.router.get.side_effect = mock_get
+
         agent.router_info[router.id] = router_info
         updated_router = {'id': '1234',
                           'distributed': True, 'ha': True,
@@ -2997,6 +3017,46 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         agent._process_router_if_compatible(router)
         self.assertIn(router['id'], agent.router_info)
 
+    def test_process_router_if_compatible_type_match(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'ha': False, 'distributed': False,
+                  'external_gateway_info': {'network_id': 'aaa'}}
+
+        ri = mock.Mock(router=router)
+        agent.router_info[router['id']] = ri
+        with mock.patch.object(agent, "_create_router") as create_router_mock:
+            agent._process_router_if_compatible(router)
+        create_router_mock.assert_not_called()
+        self.assertIn(router['id'], agent.router_info)
+        self.assertFalse(agent.router_info[router['id']].router['ha'])
+        self.assertFalse(agent.router_info[router['id']].router['distributed'])
+
+    def test_process_router_if_compatible_type_changed(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'revision_number': 1,
+                  'ha': True, 'distributed': False,
+                  'external_gateway_info': {'network_id': 'aaa'}}
+
+        ri = mock.Mock(router=router)
+        agent.router_info[router['id']] = ri
+        new_router = copy.deepcopy(router)
+        new_router['ha'] = False
+        with mock.patch.object(agent, "_create_router") as create_router_mock:
+            agent._process_router_if_compatible(new_router)
+        create_router_mock.assert_called_once_with(
+            new_router['id'], new_router)
+        self.assertIn(router['id'], agent.router_info)
+        self.assertFalse(agent.router_info[router['id']].router['ha'])
+        self.assertFalse(agent.router_info[router['id']].router['distributed'])
+
     def test_nonexistent_interface_driver(self):
         self.conf.set_override('interface_driver', None)
         self.assertRaises(SystemExit, l3_agent.L3NATAgent,