From 7e096d19d9d49581de52ebd10aaeb0bf4cb6a236 Mon Sep 17 00:00:00 2001 From: Gregory Thiemonge Date: Wed, 30 Oct 2024 09:29:45 +0100 Subject: [PATCH] Fix missing GARP with UDP listeners on SINGLE LB GARP was not sent when using a SINGLE topology LB with a UDP listener. Ensure that the GARP is sent when keepalived starts/reloads. Note: as the keepalivedlvs configuration is populated only when a pool is added, the GARP cannot be sent earlier (when the listener is added), this behavior differs from the behavior of ACTIVE_STANDBY or haproxy-based load balancers. Closes-Bug: #2086195 Change-Id: I0ff05a27265b32d7e37b162a3e82108253e25530 --- .../agent/api_server/keepalivedlvs.py | 9 ++ .../backends/agent/api_server/util.py | 37 +++++++- .../backends/agent/api_server/test_util.py | 90 ++++++++++++++++++- ...rp-for-udp-listeners-6bf2ec8d491d1e1b.yaml | 7 ++ 4 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/fix-garp-for-udp-listeners-6bf2ec8d491d1e1b.yaml diff --git a/octavia/amphorae/backends/agent/api_server/keepalivedlvs.py b/octavia/amphorae/backends/agent/api_server/keepalivedlvs.py index 7e797c9002..35c9d0d2ae 100644 --- a/octavia/amphorae/backends/agent/api_server/keepalivedlvs.py +++ b/octavia/amphorae/backends/agent/api_server/keepalivedlvs.py @@ -201,6 +201,15 @@ class KeepalivedLvs(lvs_listener_base.LvsListenerApiServerBase): f"{listener_id}"), 'details': e.output}, status=500) + is_vrrp = (CONF.controller_worker.loadbalancer_topology == + consts.TOPOLOGY_ACTIVE_STANDBY) + # TODO(gthiemonge) remove RESTART from the list (same as previous todo + # in this function) + if not is_vrrp and action in [consts.AMP_ACTION_START, + consts.AMP_ACTION_RESTART, + consts.AMP_ACTION_RELOAD]: + util.send_vip_advertisements(listener_id=listener_id) + return webob.Response( json={'message': 'OK', 'details': (f'keepalivedlvs listener {listener_id} ' diff --git a/octavia/amphorae/backends/agent/api_server/util.py b/octavia/amphorae/backends/agent/api_server/util.py index 93e0c87254..77d2d8b4af 100644 --- a/octavia/amphorae/backends/agent/api_server/util.py +++ b/octavia/amphorae/backends/agent/api_server/util.py @@ -347,7 +347,37 @@ def get_haproxy_vip_addresses(lb_id): return vips -def send_vip_advertisements(lb_id): +def get_lvs_vip_addresses(listener_id: str) -> list[str]: + """Get the VIP addresses for a LVS load balancer. + + :param listener_id: The listener ID to get VIP addresses from. + :returns: List of VIP addresses (IPv4 and IPv6) + """ + vips = [] + # Extract the VIP addresses from keepalived configuration + # Format is + # virtual_server_group ipv-group { + # vip_address1 port1 + # vip_address2 port2 + # } + # it can be repeated in case of dual-stack LBs + with open(keepalived_lvs_cfg_path(listener_id), encoding='utf-8') as file: + vsg_section = False + for line in file: + current_line = line.strip() + if vsg_section: + if current_line.startswith('}'): + vsg_section = False + else: + vip_address = current_line.split(' ')[0] + vips.append(vip_address) + elif line.startswith('virtual_server_group '): + vsg_section = True + return vips + + +def send_vip_advertisements(lb_id: tp.Optional[str] = None, + listener_id: tp.Optional[str] = None): """Sends address advertisements for each load balancer VIP. This method will send either GARP (IPv4) or neighbor advertisements (IPv6) @@ -357,7 +387,10 @@ def send_vip_advertisements(lb_id): :returns: None """ try: - vips = get_haproxy_vip_addresses(lb_id) + if lb_id: + vips = get_haproxy_vip_addresses(lb_id) + else: + vips = get_lvs_vip_addresses(listener_id) for vip in vips: interface = network_utils.get_interface_name( diff --git a/octavia/tests/unit/amphorae/backends/agent/api_server/test_util.py b/octavia/tests/unit/amphorae/backends/agent/api_server/test_util.py index 75e34b398f..aa5335e03c 100644 --- a/octavia/tests/unit/amphorae/backends/agent/api_server/test_util.py +++ b/octavia/tests/unit/amphorae/backends/agent/api_server/test_util.py @@ -356,13 +356,57 @@ class TestUtil(base.TestCase): self.assertEqual([], util.get_haproxy_vip_addresses(LB_ID1)) mock_cfg_path.assert_called_once_with(LB_ID1) + @mock.patch('octavia.amphorae.backends.agent.api_server.util.' + 'keepalived_lvs_cfg_path') + def test_get_lvs_vip_addresses(self, mock_cfg_path): + FAKE_PATH = 'fake_path' + mock_cfg_path.return_value = FAKE_PATH + self.useFixture( + test_utils.OpenFixture(FAKE_PATH, 'no match')).mock_open() + + # Test with no matching lines in the config file + self.assertEqual([], util.get_lvs_vip_addresses(LB_ID1)) + mock_cfg_path.assert_called_once_with(LB_ID1) + + # Test with 2 matching lines + mock_cfg_path.reset_mock() + test_data = ('virtual_server_group ipv4-group {\n' + ' 203.0.113.43 1\n' + ' 203.0.113.44 1\n' + '}\n') + self.useFixture( + test_utils.OpenFixture(FAKE_PATH, test_data)).mock_open() + expected_result = ['203.0.113.43', '203.0.113.44'] + self.assertEqual(expected_result, + util.get_lvs_vip_addresses(LB_ID1)) + mock_cfg_path.assert_called_once_with(LB_ID1) + + # Test with 2 groups + mock_cfg_path.reset_mock() + test_data = ('virtual_server_group ipv4-group {\n' + ' 203.0.113.43 1\n' + '}\n' + 'virtual_server_group ipv6-group {\n' + ' 2d01:27::1 2\n' + ' 2d01:27::2 2\n' + '}\n') + self.useFixture( + test_utils.OpenFixture(FAKE_PATH, test_data)).mock_open() + expected_result = ['203.0.113.43', '2d01:27::1', '2d01:27::2'] + self.assertEqual(expected_result, + util.get_lvs_vip_addresses(LB_ID1)) + mock_cfg_path.assert_called_once_with(LB_ID1) + @mock.patch('octavia.amphorae.backends.utils.ip_advertisement.' 'send_ip_advertisement') @mock.patch('octavia.amphorae.backends.utils.network_utils.' 'get_interface_name') @mock.patch('octavia.amphorae.backends.agent.api_server.util.' 'get_haproxy_vip_addresses') - def test_send_vip_advertisements(self, mock_get_vip_addrs, + @mock.patch('octavia.amphorae.backends.agent.api_server.util.' + 'get_lvs_vip_addresses') + def test_send_vip_advertisements(self, mock_get_lvs_vip_addrs, + mock_get_vip_addrs, mock_get_int_name, mock_send_advert): mock_get_vip_addrs.side_effect = [[], ['203.0.113.46'], Exception('boom')] @@ -371,6 +415,7 @@ class TestUtil(base.TestCase): # Test no VIPs util.send_vip_advertisements(LB_ID1) mock_get_vip_addrs.assert_called_once_with(LB_ID1) + mock_get_lvs_vip_addrs.assert_not_called() mock_get_int_name.assert_not_called() mock_send_advert.assert_not_called() @@ -380,6 +425,7 @@ class TestUtil(base.TestCase): mock_send_advert.reset_mock() util.send_vip_advertisements(LB_ID1) mock_get_vip_addrs.assert_called_once_with(LB_ID1) + mock_get_lvs_vip_addrs.assert_not_called() mock_get_int_name.assert_called_once_with( '203.0.113.46', net_ns=consts.AMPHORA_NAMESPACE) mock_send_advert.assert_called_once_with( @@ -393,6 +439,48 @@ class TestUtil(base.TestCase): mock_get_int_name.assert_not_called() mock_send_advert.assert_not_called() + @mock.patch('octavia.amphorae.backends.utils.ip_advertisement.' + 'send_ip_advertisement') + @mock.patch('octavia.amphorae.backends.utils.network_utils.' + 'get_interface_name') + @mock.patch('octavia.amphorae.backends.agent.api_server.util.' + 'get_haproxy_vip_addresses') + @mock.patch('octavia.amphorae.backends.agent.api_server.util.' + 'get_lvs_vip_addresses') + def test_send_vip_advertisements_udp(self, mock_get_lvs_vip_addrs, + mock_get_vip_addrs, + mock_get_int_name, mock_send_advert): + mock_get_lvs_vip_addrs.side_effect = [[], ['203.0.113.46'], + Exception('boom')] + mock_get_int_name.return_value = 'fake0' + + # Test no VIPs + util.send_vip_advertisements(listener_id=LISTENER_ID1) + mock_get_lvs_vip_addrs.assert_called_once_with(LISTENER_ID1) + mock_get_vip_addrs.assert_not_called() + mock_get_int_name.assert_not_called() + mock_send_advert.assert_not_called() + + # Test with a VIP + mock_get_lvs_vip_addrs.reset_mock() + mock_get_int_name.reset_mock() + mock_send_advert.reset_mock() + util.send_vip_advertisements(listener_id=LISTENER_ID1) + mock_get_lvs_vip_addrs.assert_called_once_with(LISTENER_ID1) + mock_get_vip_addrs.assert_not_called() + mock_get_int_name.assert_called_once_with( + '203.0.113.46', net_ns=consts.AMPHORA_NAMESPACE) + mock_send_advert.assert_called_once_with( + 'fake0', '203.0.113.46', net_ns=consts.AMPHORA_NAMESPACE) + + # Test with an exception (should not raise) + mock_get_lvs_vip_addrs.reset_mock() + mock_get_int_name.reset_mock() + mock_send_advert.reset_mock() + util.send_vip_advertisements(listener_id=LISTENER_ID1) + mock_get_int_name.assert_not_called() + mock_send_advert.assert_not_called() + @mock.patch('octavia.amphorae.backends.utils.ip_advertisement.' 'send_ip_advertisement') @mock.patch('octavia.amphorae.backends.utils.network_utils.' diff --git a/releasenotes/notes/fix-garp-for-udp-listeners-6bf2ec8d491d1e1b.yaml b/releasenotes/notes/fix-garp-for-udp-listeners-6bf2ec8d491d1e1b.yaml new file mode 100644 index 0000000000..cd45eaf49f --- /dev/null +++ b/releasenotes/notes/fix-garp-for-udp-listeners-6bf2ec8d491d1e1b.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixed an issue with SINGLE topology load balancer with UDP listeners, the + Amphora now sends a Gratuitous ARP packet when a UDP pool is added, it + makes the VIP address more quickly reachable after a failover or when + reusing a previously allocated IP address.