From 58ddc4e20f7cead1f2594241450f4beb5230c746 Mon Sep 17 00:00:00 2001 From: Adam King Date: Tue, 1 Aug 2023 16:32:06 -0400 Subject: [PATCH] mgr/cephadm: select IPs/interface based on VIP for keepalive conf We need to make sure the keepalive conf sets the unicast src and peer IPs to be the ones in the same subnet as the VIP we're setting up, as well as specify the correct interface. Otherwise, the keepalive daemons don't speak to each other properly and all end up going into MASTER state. Fixes: https://tracker.ceph.com/issues/62276 Signed-off-by: Adam King --- src/pybind/mgr/cephadm/services/ingress.py | 98 ++++++++------- .../services/ingress/keepalived.conf.j2 | 8 +- src/pybind/mgr/cephadm/tests/test_services.py | 114 ++++++++++++++++-- 3 files changed, 163 insertions(+), 57 deletions(-) diff --git a/src/pybind/mgr/cephadm/services/ingress.py b/src/pybind/mgr/cephadm/services/ingress.py index 134c9ce9a39d9..39fb5c27fcab3 100644 --- a/src/pybind/mgr/cephadm/services/ingress.py +++ b/src/pybind/mgr/cephadm/services/ingress.py @@ -247,56 +247,35 @@ class IngressService(CephService): host = daemon_spec.host hosts = sorted(list(set([host] + [str(d.hostname) for d in daemons]))) - # interface - bare_ips = [] - if spec.virtual_ip: - bare_ips.append(str(spec.virtual_ip).split('/')[0]) - elif spec.virtual_ips_list: - bare_ips = [str(vip).split('/')[0] for vip in spec.virtual_ips_list] - interface = None - for bare_ip in bare_ips: + def _get_valid_interface_and_ip(vip: str, host: str) -> Tuple[str, str]: + # interface + bare_ip = ipaddress.ip_interface(vip).ip + host_ip = '' + interface = None for subnet, ifaces in self.mgr.cache.networks.get(host, {}).items(): if ifaces and ipaddress.ip_address(bare_ip) in ipaddress.ip_network(subnet): interface = list(ifaces.keys())[0] + host_ip = ifaces[interface][0] logger.info( f'{bare_ip} is in {subnet} on {host} interface {interface}' ) break - else: # nobreak - continue - break - # try to find interface by matching spec.virtual_interface_networks - if not interface and spec.virtual_interface_networks: - for subnet, ifaces in self.mgr.cache.networks.get(host, {}).items(): - if subnet in spec.virtual_interface_networks: - interface = list(ifaces.keys())[0] - logger.info( - f'{spec.virtual_ip} will be configured on {host} interface ' - f'{interface} (which has guiding subnet {subnet})' - ) - break - if not interface: - raise OrchestratorError( - f"Unable to identify interface for {spec.virtual_ip} on {host}" - ) - - # Use interface as vrrp_interface for vrrp traffic if vrrp_interface_network not set on the spec - vrrp_interface = None - if not spec.vrrp_interface_network: - vrrp_interface = interface - else: - for subnet, ifaces in self.mgr.cache.networks.get(host, {}).items(): - if subnet == spec.vrrp_interface_network: - vrrp_interface = list(ifaces.keys())[0] - logger.info( - f'vrrp will be configured on {host} interface ' - f'{vrrp_interface} (which has guiding subnet {subnet})' - ) - break - else: + # try to find interface by matching spec.virtual_interface_networks + if not interface and spec.virtual_interface_networks: + for subnet, ifaces in self.mgr.cache.networks.get(host, {}).items(): + if subnet in spec.virtual_interface_networks: + interface = list(ifaces.keys())[0] + host_ip = ifaces[interface][0] + logger.info( + f'{spec.virtual_ip} will be configured on {host} interface ' + f'{interface} (which is in subnet {subnet})' + ) + break + if not interface: raise OrchestratorError( - f"Unable to identify vrrp interface for {spec.vrrp_interface_network} on {host}" + f"Unable to identify interface for {spec.virtual_ip} on {host}" ) + return interface, host_ip # script to monitor health script = '/usr/bin/false' @@ -341,7 +320,36 @@ class IngressService(CephService): # other_ips in conf file and converter to ips if host in hosts: hosts.remove(host) - other_ips = [utils.resolve_ip(self.mgr.inventory.get_addr(h)) for h in hosts] + host_ips: List[str] = [] + other_ips: List[List[str]] = [] + interfaces: List[str] = [] + for vip in virtual_ips: + interface, ip = _get_valid_interface_and_ip(vip, host) + host_ips.append(ip) + interfaces.append(interface) + ips: List[str] = [] + for h in hosts: + _, ip = _get_valid_interface_and_ip(vip, h) + ips.append(ip) + other_ips.append(ips) + + # Use interface as vrrp_interface for vrrp traffic if vrrp_interface_network not set on the spec + vrrp_interfaces: List[str] = [] + if not spec.vrrp_interface_network: + vrrp_interfaces = interfaces + else: + for subnet, ifaces in self.mgr.cache.networks.get(host, {}).items(): + if subnet == spec.vrrp_interface_network: + vrrp_interface = [list(ifaces.keys())[0]] * len(interfaces) + logger.info( + f'vrrp will be configured on {host} interface ' + f'{vrrp_interface} (which is in subnet {subnet})' + ) + break + else: + raise OrchestratorError( + f"Unable to identify vrrp interface for {spec.vrrp_interface_network} on {host}" + ) keepalived_conf = self.mgr.template.render( 'services/ingress/keepalived.conf.j2', @@ -349,14 +357,14 @@ class IngressService(CephService): 'spec': spec, 'script': script, 'password': password, - 'interface': interface, - 'vrrp_interface': vrrp_interface, + 'interfaces': interfaces, + 'vrrp_interfaces': vrrp_interfaces, 'virtual_ips': virtual_ips, 'first_virtual_router_id': spec.first_virtual_router_id, 'states': states, 'priorities': priorities, 'other_ips': other_ips, - 'host_ip': utils.resolve_ip(self.mgr.inventory.get_addr(host)), + 'host_ips': host_ips, } ) diff --git a/src/pybind/mgr/cephadm/templates/services/ingress/keepalived.conf.j2 b/src/pybind/mgr/cephadm/templates/services/ingress/keepalived.conf.j2 index dfab7e342451d..e19f556c6f427 100644 --- a/src/pybind/mgr/cephadm/templates/services/ingress/keepalived.conf.j2 +++ b/src/pybind/mgr/cephadm/templates/services/ingress/keepalived.conf.j2 @@ -11,7 +11,7 @@ vrrp_script check_backend { vrrp_instance VI_{{ x }} { state {{ states[x] }} priority {{ priorities[x] }} - interface {{ vrrp_interface }} + interface {{ vrrp_interfaces[x] }} virtual_router_id {{ first_virtual_router_id + x }} advert_int 1 authentication { @@ -19,15 +19,15 @@ vrrp_instance VI_{{ x }} { auth_pass {{ password }} } {% if not spec.use_keepalived_multicast %} - unicast_src_ip {{ host_ip }} + unicast_src_ip {{ host_ips[x] }} unicast_peer { - {% for ip in other_ips %} + {% for ip in other_ips[x] %} {{ ip }} {% endfor %} } {% endif %} virtual_ipaddress { - {{ virtual_ips[x] }} dev {{ interface }} + {{ virtual_ips[x] }} dev {{ interfaces[x] }} } track_script { check_backend diff --git a/src/pybind/mgr/cephadm/tests/test_services.py b/src/pybind/mgr/cephadm/tests/test_services.py index 25e557631e118..8ab71c2bbdaae 100644 --- a/src/pybind/mgr/cephadm/tests/test_services.py +++ b/src/pybind/mgr/cephadm/tests/test_services.py @@ -1657,7 +1657,7 @@ class TestIngressService: with with_host(cephadm_module, 'test', addr='1.2.3.7'): cephadm_module.cache.update_host_networks('test', { '1.2.3.0/24': { - 'if0': ['1.2.3.4/32'] + 'if0': ['1.2.3.4'] } }) @@ -1701,7 +1701,7 @@ class TestIngressService: 'auth_type PASS\n ' 'auth_pass 12345\n ' '}\n ' - 'unicast_src_ip 1.2.3.7\n ' + 'unicast_src_ip 1.2.3.4\n ' 'unicast_peer {\n ' '}\n ' 'virtual_ipaddress {\n ' @@ -1780,7 +1780,7 @@ class TestIngressService: with with_host(cephadm_module, 'test'): cephadm_module.cache.update_host_networks('test', { '1.2.3.0/24': { - 'if0': ['1.2.3.4/32'] + 'if0': ['1.2.3.1'] } }) @@ -1824,7 +1824,7 @@ class TestIngressService: 'auth_type PASS\n ' 'auth_pass 12345\n ' '}\n ' - 'unicast_src_ip 1::4\n ' + 'unicast_src_ip 1.2.3.1\n ' 'unicast_peer {\n ' '}\n ' 'virtual_ipaddress {\n ' @@ -1905,7 +1905,7 @@ class TestIngressService: with with_host(cephadm_module, 'test', addr='1.2.3.7'): cephadm_module.cache.update_host_networks('test', { '1.2.3.0/24': { - 'if0': ['1.2.3.4/32'] + 'if0': ['1.2.3.1'] } }) @@ -1950,7 +1950,7 @@ class TestIngressService: 'auth_type PASS\n ' 'auth_pass 12345\n ' '}\n ' - 'unicast_src_ip 1.2.3.7\n ' + 'unicast_src_ip 1.2.3.1\n ' 'unicast_peer {\n ' '}\n ' 'virtual_ipaddress {\n ' @@ -2022,6 +2022,104 @@ class TestIngressService: assert haproxy_generated_conf[0] == haproxy_expected_conf + @patch("cephadm.serve.CephadmServe._run_cephadm") + def test_keepalive_config_multi_interface_vips(self, _run_cephadm, cephadm_module: CephadmOrchestrator): + _run_cephadm.side_effect = async_side_effect(('{}', '', 0)) + + with with_host(cephadm_module, 'test', addr='1.2.3.1'): + with with_host(cephadm_module, 'test2', addr='1.2.3.2'): + cephadm_module.cache.update_host_networks('test', { + '1.2.3.0/24': { + 'if0': ['1.2.3.1'] + }, + '100.100.100.0/24': { + 'if1': ['100.100.100.1'] + } + }) + cephadm_module.cache.update_host_networks('test2', { + '1.2.3.0/24': { + 'if0': ['1.2.3.2'] + }, + '100.100.100.0/24': { + 'if1': ['100.100.100.2'] + } + }) + + # Check the ingress with multiple VIPs + s = RGWSpec(service_id="foo", placement=PlacementSpec(count=1), + rgw_frontend_type='beast') + + ispec = IngressSpec(service_type='ingress', + service_id='test', + placement=PlacementSpec(hosts=['test', 'test2']), + backend_service='rgw.foo', + frontend_port=8089, + monitor_port=8999, + monitor_user='admin', + monitor_password='12345', + keepalived_password='12345', + virtual_ips_list=["1.2.3.100/24", "100.100.100.100/24"]) + with with_service(cephadm_module, s) as _, with_service(cephadm_module, ispec) as _: + keepalived_generated_conf = cephadm_module.cephadm_services['ingress'].keepalived_generate_config( + CephadmDaemonDeploySpec(host='test', daemon_id='ingress', service_name=ispec.service_name())) + + keepalived_expected_conf = { + 'files': + { + 'keepalived.conf': + '# This file is generated by cephadm.\n' + 'vrrp_script check_backend {\n ' + 'script "/usr/bin/curl http://1.2.3.1:8999/health"\n ' + 'weight -20\n ' + 'interval 2\n ' + 'rise 2\n ' + 'fall 2\n}\n\n' + 'vrrp_instance VI_0 {\n ' + 'state MASTER\n ' + 'priority 100\n ' + 'interface if0\n ' + 'virtual_router_id 50\n ' + 'advert_int 1\n ' + 'authentication {\n ' + 'auth_type PASS\n ' + 'auth_pass 12345\n ' + '}\n ' + 'unicast_src_ip 1.2.3.1\n ' + 'unicast_peer {\n ' + '1.2.3.2\n ' + '}\n ' + 'virtual_ipaddress {\n ' + '1.2.3.100/24 dev if0\n ' + '}\n ' + 'track_script {\n ' + 'check_backend\n }\n' + '}\n' + 'vrrp_instance VI_1 {\n ' + 'state BACKUP\n ' + 'priority 90\n ' + 'interface if1\n ' + 'virtual_router_id 51\n ' + 'advert_int 1\n ' + 'authentication {\n ' + 'auth_type PASS\n ' + 'auth_pass 12345\n ' + '}\n ' + 'unicast_src_ip 100.100.100.1\n ' + 'unicast_peer {\n ' + '100.100.100.2\n ' + '}\n ' + 'virtual_ipaddress {\n ' + '100.100.100.100/24 dev if1\n ' + '}\n ' + 'track_script {\n ' + 'check_backend\n }\n' + '}\n' + } + } + + # check keepalived config + assert keepalived_generated_conf[0] == keepalived_expected_conf + @patch("cephadm.serve.CephadmServe._run_cephadm") @patch("cephadm.services.nfs.NFSService.fence_old_ranks", MagicMock()) @patch("cephadm.services.nfs.NFSService.run_grace_tool", MagicMock()) @@ -2033,7 +2131,7 @@ class TestIngressService: with with_host(cephadm_module, 'test', addr='1.2.3.7'): cephadm_module.cache.update_host_networks('test', { '1.2.3.0/24': { - 'if0': ['1.2.3.4/32'] + 'if0': ['1.2.3.1'] } }) @@ -2080,7 +2178,7 @@ class TestIngressService: 'auth_type PASS\n ' 'auth_pass 12345\n ' '}\n ' - 'unicast_src_ip 1.2.3.7\n ' + 'unicast_src_ip 1.2.3.1\n ' 'unicast_peer {\n ' '}\n ' 'virtual_ipaddress {\n ' -- 2.39.5