T6820: Add HA/VRRP module by omnom62 · Pull Request #438 · vyos/vyos.vyos

omnom62 · 2025-10-20T10:44:04Z

Change Summary

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

https://vyos.dev/T6820

Related PR(s)

Component(s) name

Proposed changes

How to test

Test results

Sanity tests passed
Unit tests passed

Tested against VyOS versions:

1.4.2
1.5q2

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the ansible sanity and unit tests
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly
I have added unit tests to cover my changes
I have added a file to changelogs/fragments to describe the changes

gaige · 2025-10-21T08:53:46Z

@omnom62 Can you update the PR template, it seems rather empty.

gaige · 2025-10-21T08:58:53Z

RST file is missing as well, which is odd because that should be auto-created. Sanity checks are failing as well. This seem s not quite ready, should it be in Draft?

omnom62 · 2025-10-21T09:01:16Z

RST file is missing as well, which is odd because that should be auto-created. Sanity checks are failing as well. This seem s not quite ready, should it be in Draft?

My bad, @gaige , yes, indeed, this is still in Draft - finishing rm_templates section

gaige

I'm mostly OK with this. I do think we should consider breaking out the load balancer components, since although they are HA, they're not strictly VRRP. A few other small items and I noticed we're now failing some tests.

omnom62 · 2026-03-09T20:13:24Z

Speaking about the linter and spaces around '|' - I have found a number of such expressions in tests and modules.
Attn @gaige

Copilot

Pull request overview

Adds a new vyos_ha network resource module to manage High Availability configuration (VRRP + LVS virtual servers) on VyOS, including facts support and full test coverage.

Changes:

Introduces vyos_ha module plus argspec/config/facts/template plumbing for render/parse/gather/merge/replace/override/delete/purge.
Adds unit + integration (CLI) test suites and fixtures for HA/VRRP workflows.
Wires module into collection routing, changelog, README, sanity ignores, and repo housekeeping.

Reviewed changes

Copilot reviewed 41 out of 46 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tests/unit/modules/network/vyos/test_vyos_ha.py	New unit tests for `vyos_ha` behaviors across all supported states.
tests/unit/modules/network/vyos/fixtures/vyos_ha_config.cfg	Fixture running-config used by unit tests for HA parsing/gathering.
tests/sanity/ignore-2.22.txt	Adds action plugin docs ignore entry for new `ha` action plugin.
tests/sanity/ignore-2.21.txt	Same as above for Ansible 2.21 sanity profile.
tests/sanity/ignore-2.20.txt	Same as above for Ansible 2.20 sanity profile.
tests/sanity/ignore-2.19.txt	Same as above for Ansible 2.19 sanity profile.
tests/sanity/ignore-2.18.txt	Same as above for Ansible 2.18 sanity profile.
tests/sanity/ignore-2.17.txt	Same as above for Ansible 2.17 sanity profile.
tests/sanity/ignore-2.16.txt	Same as above for Ansible 2.16 sanity profile.
tests/sanity/ignore-2.15.txt	Same as above for Ansible 2.15 sanity profile.
tests/integration/targets/vyos_ha/vars/main.yaml	Integration test data: before/after configs and expected command sets for each state.
tests/integration/targets/vyos_ha/tests/cli/rtt.yaml	Round-trip (facts → config → revert) integration test.
tests/integration/targets/vyos_ha/tests/cli/replaced.yaml	Integration test coverage for `state: replaced` + idempotency.
tests/integration/targets/vyos_ha/tests/cli/rendered.yaml	Integration test for offline rendering (`state: rendered`).
tests/integration/targets/vyos_ha/tests/cli/purged.yaml	Integration test for `state: purged` + idempotency.
tests/integration/targets/vyos_ha/tests/cli/parsed.yaml	Integration test for parsing a provided running config (`state: parsed`).
tests/integration/targets/vyos_ha/tests/cli/overridden.yaml	Integration test for `state: overridden` + idempotency.
tests/integration/targets/vyos_ha/tests/cli/merged.yaml	Integration test for `state: merged` + idempotency.
tests/integration/targets/vyos_ha/tests/cli/gathered.yaml	Integration test for `state: gathered` vs facts output.
tests/integration/targets/vyos_ha/tests/cli/empty_config.yaml	Integration tests for expected error messages on empty config inputs.
tests/integration/targets/vyos_ha/tests/cli/deleted_vrrp_groups.yaml	Integration test deleting VRRP groups/sync-groups by name.
tests/integration/targets/vyos_ha/tests/cli/deleted_bulk.yaml	Integration test deleting “stub”/bulk objects and verifying after-state.
tests/integration/targets/vyos_ha/tests/cli/deleted_all.yaml	Integration test for `state: deleted` with empty config (delete everything) + idempotency.
tests/integration/targets/vyos_ha/tests/cli/_remove_config.yaml	Helper to purge HA config and clean up dummy scripts before/after tests.
tests/integration/targets/vyos_ha/tests/cli/_populate_config.yaml	Helper to pre-populate device config for scenario tests.
tests/integration/targets/vyos_ha/tests/cli/_parsed.cfg	Input config used by parsed-state integration test.
tests/integration/targets/vyos_ha/tests/cli/_add_dummy_scripts.yaml	Helper to create dummy scripts referenced by HA config.
tests/integration/targets/vyos_ha/tasks/main.yaml	Integration test entrypoint for `vyos_ha` target.
tests/integration/targets/vyos_ha/tasks/cli.yaml	Test harness to discover/run per-scenario CLI testcases.
tests/integration/targets/vyos_ha/defaults/main.yaml	Default vars for testcase selection/discovery.
plugins/modules/vyos_ha.py	New public module with DOCUMENTATION/EXAMPLES/RETURN and module entrypoint.
plugins/module_utils/network/vyos/utils/utils.py	Extends `combine()` to support multiple list-merge modes used by HA merge behavior.
plugins/module_utils/network/vyos/rm_templates/ha.py	New resource template: parsers + command generation for HA/VRRP/LVS.
plugins/module_utils/network/vyos/facts/ha/ha.py	New facts gatherer/parser/normalizer for HA resource into `ansible_network_resources`.
plugins/module_utils/network/vyos/facts/ha/init.py	Initializes HA facts package.
plugins/module_utils/network/vyos/facts/facts.py	Registers `ha` facts provider in the facts dispatcher map.
plugins/module_utils/network/vyos/config/ha/ha.py	New ResourceModule implementation for HA state machine + diff/compare logic.
plugins/module_utils/network/vyos/config/ha/init.py	Initializes HA config package.
plugins/module_utils/network/vyos/argspec/ha/ha.py	Auto-generated argspec for HA module options/structure.
plugins/module_utils/network/vyos/argspec/ha/init.py	Initializes HA argspec package.
meta/runtime.yml	Adds plugin routing redirect for `ha` → `vyos_ha`.
changelogs/fragments/t6820_vrrp.yml	Changelog fragment announcing HA/VRRP support.
README.md	Adds `vyos_ha` to the module listing table.
.gitignore	Ignores `*.bak` files.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T12:11:53Z

+        ]
+        self.execute_module(changed=True, commands=commands)
+
+    def test_ntp_replaced_idempotent(self):


Test name looks copy/pasted (test_ntp_replaced_idempotent) but the content is HA/VRRP. Rename to reflect the HA module/state being tested so failures are discoverable in CI output.

Copilot · 2026-04-17T12:11:53Z

+            vrrp_snmp = re.search(r"set high-availability vrrp snmp", config_line)
+
+            if vrrp_disable:
+                config_dict["disable"] = [config_dict.get("disable", "") + config_line]


The disable bucket is built with config_dict.get("disable", "") + config_line and then wrapped in a list; if the input ever contains multiple matching lines this will throw (list + str) and it also drops line separation. Use the same pattern as the other sections (e.g., setdefault("disable", []).append(config_line)).

Suggested change

config_dict["disable"] = [config_dict.get("disable", "") + config_line]

config_dict.setdefault("disable", []).append(config_line)

Copilot · 2026-04-17T12:11:53Z

+                        existing = vrrp_facts[section].get(name, {})
+                        vrrp_facts[section][name] = self.deep_merge(existing, data)
+
+        ansible_facts["ansible_network_resources"].pop("vrrp", None)


This facts module updates ansible_network_resources['ha'], but it pops the unrelated key vrrp. This should likely be pop('ha', None) (or whatever key this facts module uses) to avoid leaving stale HA facts around across runs.

Suggested change

ansible_facts["ansible_network_resources"].pop("vrrp", None)

ansible_facts["ansible_network_resources"].pop("ha", None)

Copilot · 2026-04-17T12:11:54Z

+        return connection.get('show configuration commands |  match "set high-availability"')
+
+    def get_config_set(self, data, connection):
+        """To classify the configurations beased on high availability sections"""


Typo in docstring: "beased" → "based".

Suggested change

"""To classify the configurations beased on high availability sections"""

"""To classify the configurations based on high availability sections"""

Copilot · 2026-04-17T12:11:54Z

+#     """
+#     Merge two dictionaries (shallow or deep).
+#     :param a: dict
+#     :param b: dict
+#     :param recursive: bool, deep merge
+#     :param list_merge: str, only 'replace' is supported (default Ansible behavior)
+#     """
+#     if not isinstance(a, dict) or not isinstance(b, dict):
+#         raise ValueError("combine expects two dictionaries")
+
+#     result = a.copy()
+
+#     for k, v in b.items():
+#         if recursive and k in result and isinstance(result[k], dict) and isinstance(v, dict):
+#             result[k] = combine(result[k], v, recursive=True, list_merge=list_merge)
+#         else:
+#             result[k] = v
+
+#     return result
+
+


Remove the large commented-out previous implementation of combine(); keeping dead code in-tree makes future maintenance harder and obscures the current behavior.

Suggested change

# """

# Merge two dictionaries (shallow or deep).

# :param a: dict

# :param b: dict

# :param recursive: bool, deep merge

# :param list_merge: str, only 'replace' is supported (default Ansible behavior)

# """

# if not isinstance(a, dict) or not isinstance(b, dict):

# raise ValueError("combine expects two dictionaries")

# result = a.copy()

# for k, v in b.items():

# if recursive and k in result and isinstance(result[k], dict) and isinstance(v, dict):

# result[k] = combine(result[k], v, recursive=True, list_merge=list_merge)

# else:

# result[k] = v

# return result

Copilot · 2026-04-17T12:11:54Z

+        if isinstance(vss, dict):
+            for vs in vss.items():
+                rs = vs.get("real_server")
+                if isinstance(rs, list):
+                    vs["real_server"] = {
+                        item["address"]: item
+                        for item in rs
+                        if isinstance(item, dict) and item.get("address")
+                    }
+            return data


_virtual_servers_list_to_dict() incorrectly iterates over a dict with for vs in vss.items(); vs will be a (key, value) tuple, so vs.get(...) / vs[...] will raise. Iterate over vss.values() (or for _, vs in vss.items():) and update the dict values instead.

Copilot · 2026-04-17T12:11:54Z

+                ],
+            },
+        }
+        self.assertEqual(sorted(parsed_list), sorted(result["parsed"]))


These assertions only compare sorted dict keys (since iterating a dict yields keys), so the test can pass even when the parsed structure is wrong. Compare the full dicts (or use assertDictEqual) after normalizing ordering as needed.

Copilot · 2026-04-17T12:11:55Z

+            },
+        }
+
+        self.assertEqual(sorted(gathered_list), sorted(result["gathered"]))


Same issue here: sorting a dict compares only keys, not values. This should assert equality of the full gathered structure (optionally after normalizing list ordering).

Init VRRP

17bf622

omnom62 requested a review from a team as a code owner October 20, 2025 10:44

omnom62 requested review from andamasov, dmbaturin and gaige October 20, 2025 10:44

Merge branch 'main' into t6820_vrrp

b896b0a

omnom62 changed the title ~~T6820 VRRP support~~ T6820 VRRP module Oct 20, 2025

Merge branch 'main' into t6820_vrrp

9c68abb

omnom62 marked this pull request as draft October 21, 2025 09:00

omnom62 removed request for andamasov, dmbaturin and gaige October 21, 2025 09:02

omnom62 added 7 commits October 23, 2025 14:36

RM Templates

39192ba

rm_templates for global_parameters

2786c32

virtual servers rm_templates

876babe

update

60f44fc

sync-group rm_tempalte

42cfefc

VRRP GP GARP template rework

60e5bcb

RM template and new facts logic

b4fd7df

gaige reviewed Oct 29, 2025

View reviewed changes

Comment thread plugins/module_utils/network/vyos/facts/vrrp/vrrp.py Outdated

omnom62 changed the title ~~T6820 VRRP module~~ WIP T6820 VRRP module Oct 29, 2025

omnom62 and others added 6 commits October 29, 2025 20:52

Merge branch 'main' into t6820_vrrp

1c5c271

Merge branch 'main' into t6820_vrrp

bdeaa32

vrrp argspec

288c59c

Suppressing backup versions of files

e6ea3b1

Facts for vrrp

57ede2e

VRRP facts updates

888e3f9

Merge branch 'main' into t6820_vrrp

4a9826c

gaige reviewed Mar 9, 2026

View reviewed changes

Comment thread plugins/module_utils/network/vyos/config/vrrp/vrrp.old

gaige requested changes Mar 9, 2026

View reviewed changes

omnom62 added 3 commits March 9, 2026 21:00

Misc lint fixes

f82c51b

Misc Lint fixes

c372a87

remove old files

3aa532c

omnom62 added 2 commits March 18, 2026 05:48

Rename vyos_vrrp -> vyos_ha

c7ee93b

Rename vyos_vrrp to vyos_ha

acc8090

omnom62 requested a review from gaige March 17, 2026 19:59

omnom62 changed the title ~~T6820: Add VRRP module~~ T6820: Add HA/VRRP module Mar 17, 2026

doc fixes

89930bb

omnom62 marked this pull request as draft March 22, 2026 21:47

auto-merge was automatically disabled March 22, 2026 21:47
Pull request was converted to draft

omnom62 changed the title ~~T6820: Add HA/VRRP module~~ WIP T6820: Add HA/VRRP module Mar 22, 2026

omnom62 added 2 commits April 1, 2026 19:03

Changes to HA module

7c5244d

HA module fixes

e3d857c

omnom62 changed the title ~~WIP T6820: Add HA/VRRP module~~ T6820: Add HA/VRRP module Apr 2, 2026

omnom62 marked this pull request as ready for review April 2, 2026 20:36

omnom62 and others added 7 commits April 4, 2026 05:30

Merge branch 'main' into t6820_vrrp

d500514

Sanity test fixes

c19d55b

changelog

271c942

Merge branch 'main' into t6820_vrrp

3cf891e

Merge branch 'main' into t6820_vrrp

aea3f48

ignore for HA/VRRP and 2.22

aca5bcb

Merge branch 'main' into t6820_vrrp

359a3c0

Copilot AI review requested due to automatic review settings April 17, 2026 12:06

Copilot started reviewing on behalf of omnom62 April 17, 2026 12:06 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Merge branch 'main' into t6820_vrrp

79daad8

	config_dict["disable"] = [config_dict.get("disable", "") + config_line]
	config_dict.setdefault("disable", []).append(config_line)

	ansible_facts["ansible_network_resources"].pop("vrrp", None)
	ansible_facts["ansible_network_resources"].pop("ha", None)

	"""To classify the configurations beased on high availability sections"""
	"""To classify the configurations based on high availability sections"""

Conversation

omnom62 commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Test results

Checklist:

Uh oh!

gaige commented Oct 21, 2025

Uh oh!

gaige commented Oct 21, 2025

Uh oh!

omnom62 commented Oct 21, 2025

Uh oh!

Uh oh!

Uh oh!

gaige left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

omnom62 commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

omnom62 commented Oct 20, 2025 •

edited

Loading