From 3b2b7f4fe7bacb99028b5cba7ac7a8e6c412d965 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Thu, 18 Feb 2021 12:48:13 +0100 Subject: [PATCH] Remove update_initial_state() method from the HA router This method was intended to check state of the HA router on the node and update it in the neutron server. Patch [1] added check of the initial status to the neutron_keepalived_state_change_monitor process. It also could cause some race conditions and event which is setting correct state of the router will be not processed thus router may endup with two nodes with "primary" state in the Neutron's DB. Neutron_keepalived_state_change_monitor was notifying agent about router's initial state only if this state was 'primary'. Now it will notify agent always to let agent set router's state as 'backup' if needed (that was previously done by this removed update_initial_state() method). [1] https://review.opendev.org/c/openstack/neutron/+/642295 Conflicts: neutron/agent/l3/ha_router.py neutron/agent/l3/keepalived_state_change.py neutron/tests/unit/agent/l3/test_dvr_local_router.py Change-Id: I2cc58c30cf844ee0ecf0611ecdec430086464790 Closes-Bug: #1916022 (cherry picked from commit 0d8ae1576724b196580c9af3782fd3c9e9072bef) --- neutron/agent/l3/agent.py | 2 -- neutron/agent/l3/ha_router.py | 13 +------------ neutron/agent/l3/keepalived_state_change.py | 4 ++-- .../tests/unit/agent/l3/test_dvr_local_router.py | 16 +++++++--------- neutron/tests/unit/agent/l3/test_ha_router.py | 3 +-- 5 files changed, 11 insertions(+), 27 deletions(-) diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index 0af0d0512b0..4ae13f7b7b6 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -442,7 +442,6 @@ class L3NATAgent(ha.AgentMixin, if router.get('ha'): features.append('ha') - kwargs['state_change_callback'] = self.enqueue_state_change if router.get('distributed') and router.get('ha'): # Case 1: If the router contains information about the HA interface @@ -457,7 +456,6 @@ class L3NATAgent(ha.AgentMixin, if (not router.get(lib_const.HA_INTERFACE_KEY) or self.conf.agent_mode != lib_const.L3_AGENT_MODE_DVR_SNAT): features.remove('ha') - kwargs.pop('state_change_callback') return self.router_factory.create(features, **kwargs) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 1811123405e..c66839fc98b 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -66,12 +66,11 @@ class HaRouterNamespace(namespaces.RouterNamespace): class HaRouter(router.RouterInfo): - def __init__(self, state_change_callback, *args, **kwargs): + def __init__(self, *args, **kwargs): super(HaRouter, self).__init__(*args, **kwargs) self.ha_port = None self.keepalived_manager = None - self.state_change_callback = state_change_callback self._ha_state = None self._ha_state_path = None @@ -151,7 +150,6 @@ class HaRouter(router.RouterInfo): self._init_keepalived_manager(process_monitor) self._check_and_set_real_state() self.ha_network_added() - self.update_initial_state(self.state_change_callback) self.spawn_state_change_monitor(process_monitor) def _init_keepalived_manager(self, process_monitor): @@ -443,15 +441,6 @@ class HaRouter(router.RouterInfo): except common_utils.WaitTimeout: pm.disable(sig=str(int(signal.SIGKILL))) - def update_initial_state(self, callback): - addresses = ip_lib.get_devices_with_ip(self.ha_namespace, - name=self.get_ha_device_name()) - cidrs = (address['cidr'] for address in addresses) - ha_cidr = self._get_primary_vip() - state = 'master' if ha_cidr in cidrs else 'backup' - self.ha_state = state - callback(self.router_id, state) - @staticmethod def _gateway_ports_equal(port1, port2): def _get_filtered_dict(d, ignore): diff --git a/neutron/agent/l3/keepalived_state_change.py b/neutron/agent/l3/keepalived_state_change.py index da0a78fe402..7fd9e4269ec 100644 --- a/neutron/agent/l3/keepalived_state_change.py +++ b/neutron/agent/l3/keepalived_state_change.py @@ -106,12 +106,12 @@ class MonitorDaemon(daemon.Daemon): for address in ip.addr.list(): if address.get('cidr') == self.cidr: state = 'master' - self.write_state_change(state) - self.notify_agent(state) break LOG.debug('Initial status of router %s is %s', self.router_id, state) + self.write_state_change(state) + self.notify_agent(state) except Exception: LOG.exception('Failed to get initial status of router %s', self.router_id) diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index c64ad9f9b53..cc4261d7b96 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -860,7 +860,7 @@ class TestDvrRouterOperations(base.BaseTestCase): fip = {'id': _uuid()} fip_cidr = '11.22.33.44/24' - ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri.is_router_master = mock.Mock(return_value=False) ri._add_vip = mock.Mock() interface_name = ri.get_snat_external_device_interface_name( @@ -871,7 +871,7 @@ class TestDvrRouterOperations(base.BaseTestCase): router[lib_constants.HA_INTERFACE_KEY]['status'] = 'DOWN' self._set_ri_kwargs(agent, router['id'], router) - ri_1 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri_1 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri_1.is_router_master = mock.Mock(return_value=True) ri_1._add_vip = mock.Mock() interface_name = ri_1.get_snat_external_device_interface_name( @@ -882,7 +882,7 @@ class TestDvrRouterOperations(base.BaseTestCase): router[lib_constants.HA_INTERFACE_KEY]['status'] = 'ACTIVE' self._set_ri_kwargs(agent, router['id'], router) - ri_2 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri_2 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri_2.is_router_master = mock.Mock(return_value=True) ri_2._add_vip = mock.Mock() interface_name = ri_2.get_snat_external_device_interface_name( @@ -904,14 +904,14 @@ class TestDvrRouterOperations(base.BaseTestCase): self._set_ri_kwargs(agent, router['id'], router) fip_cidr = '11.22.33.44/24' - ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri.is_router_master = mock.Mock(return_value=False) ri._remove_vip = mock.Mock() ri.remove_centralized_floatingip(fip_cidr) ri._remove_vip.assert_called_once_with(fip_cidr) super_remove_centralized_floatingip.assert_not_called() - ri1 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri1 = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri1.is_router_master = mock.Mock(return_value=True) ri1._remove_vip = mock.Mock() ri1.remove_centralized_floatingip(fip_cidr) @@ -927,10 +927,9 @@ class TestDvrRouterOperations(base.BaseTestCase): router[lib_constants.HA_INTERFACE_KEY]['status'] = 'ACTIVE' self.mock_driver.unplug.reset_mock() self._set_ri_kwargs(agent, router['id'], router) - ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri._ha_state_path = self.get_temp_file_path('router_ha_state') ri._create_snat_namespace = mock.Mock() - ri.update_initial_state = mock.Mock() ri._plug_external_gateway = mock.Mock() ri.initialize(mock.Mock()) ri._create_dvr_gateway(mock.Mock(), mock.Mock()) @@ -946,14 +945,13 @@ class TestDvrRouterOperations(base.BaseTestCase): self.mock_driver.unplug.reset_mock() self._set_ri_kwargs(agent, router['id'], router) - ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, [], **self.ri_kwargs) + ri = dvr_edge_ha_rtr.DvrEdgeHaRouter(HOSTNAME, **self.ri_kwargs) ri._ha_state_path = self.get_temp_file_path('router_ha_state') with open(ri._ha_state_path, "w") as f: f.write("master") ri._create_snat_namespace = mock.Mock() - ri.update_initial_state = mock.Mock() ri._plug_external_gateway = mock.Mock() with mock.patch("neutron.agent.linux.keepalived." "KeepalivedManager.check_processes", diff --git a/neutron/tests/unit/agent/l3/test_ha_router.py b/neutron/tests/unit/agent/l3/test_ha_router.py index efab9cd8f45..da6e409daec 100644 --- a/neutron/tests/unit/agent/l3/test_ha_router.py +++ b/neutron/tests/unit/agent/l3/test_ha_router.py @@ -40,8 +40,7 @@ class TestBasicRouterOperations(base.BaseTestCase): router = mock.MagicMock() self.agent_conf = mock.Mock() self.router_id = _uuid() - return ha_router.HaRouter(mock.sentinel.enqueue_state, - mock.sentinel.agent, + return ha_router.HaRouter(mock.sentinel.agent, self.router_id, router, self.agent_conf,