From 2943d9bb0f65fb9c1a605b9c1906c25ae827a656 Mon Sep 17 00:00:00 2001 From: khramshinr Date: Thu, 8 Feb 2024 15:57:39 +0700 Subject: vrrp: T6020: vrrp health-check script not applied correctly in keepalived.conf Added health-check to sync-group in CLI Don't use instance health-check when instance in sync group member Disallow wrong healtch-check configurations New smoke test --- .../templates/high-availability/keepalived.conf.j2 | 31 +++++++++---- interface-definitions/high-availability.xml.in | 49 ++++++++++++++++++++ .../scripts/cli/test_high-availability_vrrp.py | 53 ++++++++++++++++++++++ src/conf_mode/high-availability.py | 37 +++++++++++---- 4 files changed, 152 insertions(+), 18 deletions(-) diff --git a/data/templates/high-availability/keepalived.conf.j2 b/data/templates/high-availability/keepalived.conf.j2 index d54f575b5..50b910ca3 100644 --- a/data/templates/high-availability/keepalived.conf.j2 +++ b/data/templates/high-availability/keepalived.conf.j2 @@ -33,6 +33,24 @@ global_defs { notify_fifo_script /usr/libexec/vyos/system/keepalived-fifo.py } +{# Sync group has own health-check scripts T6020 #} +{% if vrrp.sync_group is vyos_defined %} +{% for name, sync_group_config in vrrp.sync_group.items() if sync_group_config.disable is not vyos_defined %} +{% if sync_group_config.health_check is vyos_defined %} +vrrp_script healthcheck_sg_{{ name }} { +{% if sync_group_config.health_check.script is vyos_defined %} + script "{{ sync_group_config.health_check.script }}" +{% elif sync_group_config.health_check.ping is vyos_defined %} + script "/usr/bin/ping -c1 {{ sync_group_config.health_check.ping }}" +{% endif %} + interval {{ sync_group_config.health_check.interval }} + fall {{ sync_group_config.health_check.failure_count }} + rise 1 +} +{% endif %} +{% endfor %} +{% endif %} + {% if vrrp.group is vyos_defined %} {% for name, group_config in vrrp.group.items() if group_config.disable is not vyos_defined %} {% if group_config.health_check is vyos_defined %} @@ -128,7 +146,8 @@ vrrp_instance {{ name }} { {% endfor %} } {% endif %} -{% if group_config.health_check is vyos_defined %} +{# Sync group member can't use own health check script #} +{% if group_config.health_check is vyos_defined and group_config._is_sync_group_member is not vyos_defined %} track_script { healthcheck_{{ name }} } @@ -148,16 +167,12 @@ vrrp_sync_group {{ name }} { {% endif %} } -{# Health-check scripts should be in section sync-group if member is part of the sync-group T4081 #} -{% if vrrp.group is vyos_defined %} -{% for name, group_config in vrrp.group.items() if group_config.disable is not vyos_defined %} -{% if group_config.health_check.script is vyos_defined and name in sync_group_config.member %} +{% if sync_group_config.health_check is vyos_defined %} track_script { - healthcheck_{{ name }} + healthcheck_sg_{{ name }} } -{% endif %} -{% endfor %} {% endif %} + {% if conntrack_sync_group is vyos_defined(name) %} {% set vyos_helper = "/usr/libexec/vyos/vyos-vrrp-conntracksync.sh" %} notify_master "{{ vyos_helper }} master {{ name }}" diff --git a/interface-definitions/high-availability.xml.in b/interface-definitions/high-availability.xml.in index 59f0f1052..af8f1ba68 100644 --- a/interface-definitions/high-availability.xml.in +++ b/interface-definitions/high-availability.xml.in @@ -344,6 +344,55 @@ + + + Health check + + + + + Health check failure count required for transition to fault + + + + + 3 + + + + Health check execution interval in seconds + + + + + 60 + + + + ICMP ping health check + + ipv4 + IPv4 ping target address + + + ipv6 + IPv6 ping target address + + + + + + + + + Health check script file + + + + + + + #include diff --git a/smoketest/scripts/cli/test_high-availability_vrrp.py b/smoketest/scripts/cli/test_high-availability_vrrp.py index 98259d830..2b7991cc2 100755 --- a/smoketest/scripts/cli/test_high-availability_vrrp.py +++ b/smoketest/scripts/cli/test_high-availability_vrrp.py @@ -237,5 +237,58 @@ class TestVRRP(VyOSUnitTestSHIM.TestCase): self.assertIn(f'track_interface', config) self.assertIn(f' {none_vrrp_interface}', config) + def test_check_health_script(self): + sync_group = 'VyOS' + + for group in groups: + vlan_id = group.lstrip('VLAN') + vip = f'100.64.{vlan_id}.1/24' + group_base = base_path + ['vrrp', 'group', group] + + self.cli_set(['interfaces', 'ethernet', vrrp_interface, 'vif', vlan_id, 'address', inc_ip(vip, 1) + '/' + vip.split('/')[-1]]) + + self.cli_set(group_base + ['interface', f'{vrrp_interface}.{vlan_id}']) + self.cli_set(group_base + ['address', vip]) + self.cli_set(group_base + ['vrid', vlan_id]) + + self.cli_set(group_base + ['health-check', 'ping', '127.0.0.1']) + + # commit changes + self.cli_commit() + + for group in groups: + config = getConfig(f'vrrp_instance {group}') + self.assertIn(f'track_script', config) + + self.cli_set(base_path + ['vrrp', 'sync-group', sync_group, 'member', groups[0]]) + + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_delete(base_path + ['vrrp', 'group', groups[0], 'health-check']) + self.cli_commit() + + for group in groups[1:]: + config = getConfig(f'vrrp_instance {group}') + self.assertIn(f'track_script', config) + + config = getConfig(f'vrrp_instance {groups[0]}') + self.assertNotIn(f'track_script', config) + + config = getConfig(f'vrrp_sync_group {sync_group}') + self.assertNotIn(f'track_script', config) + + self.cli_set(base_path + ['vrrp', 'sync-group', sync_group, 'health-check', 'ping', '127.0.0.1']) + + # commit changes + self.cli_commit() + + config = getConfig(f'vrrp_instance {groups[0]}') + self.assertNotIn(f'track_script', config) + + config = getConfig(f'vrrp_sync_group {sync_group}') + self.assertIn(f'track_script', config) + + if __name__ == '__main__': unittest.main(verbosity=2) diff --git a/src/conf_mode/high-availability.py b/src/conf_mode/high-availability.py index b3b27b14e..7f8849de9 100755 --- a/src/conf_mode/high-availability.py +++ b/src/conf_mode/high-availability.py @@ -86,16 +86,7 @@ def verify(ha): raise ConfigError(f'Authentication requires both type and passwortd to be set in VRRP group "{group}"') if 'health_check' in group_config: - health_check_types = ["script", "ping"] - from vyos.utils.dict import check_mutually_exclusive_options - try: - check_mutually_exclusive_options(group_config["health_check"], health_check_types, required=True) - except ValueError: - Warning(f'Health check configuration for VRRP group "{group}" will remain unused ' \ - f'until it has one of the following options: {health_check_types}') - # XXX: health check has default options so we need to remove it - # to avoid generating useless config statements in keepalived.conf - del group_config["health_check"] + _validate_health_check(group, group_config) # Keepalived doesn't allow mixing IPv4 and IPv6 in one group, so we mirror that restriction # We also need to make sure VRID is not used twice on the same interface with the @@ -144,11 +135,22 @@ def verify(ha): # Check sync groups if 'vrrp' in ha and 'sync_group' in ha['vrrp']: for sync_group, sync_config in ha['vrrp']['sync_group'].items(): + if 'health_check' in sync_config: + _validate_health_check(sync_group, sync_config) + if 'member' in sync_config: for member in sync_config['member']: if member not in ha['vrrp']['group']: raise ConfigError(f'VRRP sync-group "{sync_group}" refers to VRRP group "{member}", '\ 'but it does not exist!') + else: + ha['vrrp']['group'][member]['_is_sync_group_member'] = True + if ha['vrrp']['group'][member].get('health_check') is not None: + raise ConfigError( + f'Health check configuration for VRRP group "{member}" will remain unused ' + f'while it has member of sync group "{sync_group}" ' + f'Only sync group health check will be used' + ) # Virtual-server if 'virtual_server' in ha: @@ -170,6 +172,21 @@ def verify(ha): raise ConfigError(f'Port is required but not set for virtual-server "{vs}" real-server "{rs}"') +def _validate_health_check(group, group_config): + health_check_types = ["script", "ping"] + from vyos.utils.dict import check_mutually_exclusive_options + try: + check_mutually_exclusive_options(group_config["health_check"], + health_check_types, required=True) + except ValueError: + Warning( + f'Health check configuration for VRRP group "{group}" will remain unused ' \ + f'until it has one of the following options: {health_check_types}') + # XXX: health check has default options so we need to remove it + # to avoid generating useless config statements in keepalived.conf + del group_config["health_check"] + + def generate(ha): if not ha or 'disable' in ha: if os.path.isfile(systemd_override): -- cgit v1.2.3