Skip to content

Option to prevent FHRP group assignment from being overwritten (#445) - #476

Merged
semx merged 22 commits into
bb-Ricardo:developmentfrom
sol1:445-review
Sep 7, 2026
Merged

semx merged 22 commits into
bb-Ricardo:developmentfrom
sol1:445-review

Conversation

@afoster

@afoster afoster commented Sep 1, 2025

Copy link
Copy Markdown

Added a per source config option (skip_fhrp_group_ips) to prevent FHRP group IP addresses in Netbox from being overridden by their members, as described in issue #445.

Noah418 and others added 22 commits August 25, 2025 12:17
…assignment' into 445-sync-overwriting-FHRP-Group-assignment
…ured to prevent a duplicate ip address from being created, causing errors.
…nment

Option to prevent FHRP group assignment from being overwritten (bb-Ricardo#445)
@bb-Ricardo

Copy link
Copy Markdown
Owner

Hi, and thank you very much for this PR.

I'm still a bit busy but soon will have some time to look at this. Don't worry, just need some more time.

@bb-Ricardo
bb-Ricardo force-pushed the development branch 2 times, most recently from 0655b11 to 499862a Compare October 24, 2025 21:26
@bb-Ricardo
bb-Ricardo force-pushed the development branch 2 times, most recently from 8adc4cd to 4c5fbc8 Compare March 9, 2026 09:09
@bb-Ricardo bb-Ricardo modified the milestone: v1.9.0 Mar 19, 2026
@bb-Ricardo
bb-Ricardo self-requested a review as a code owner May 13, 2026 22:13
@ionoci

ionoci commented May 18, 2026

Copy link
Copy Markdown

Hi @Noah418 or @afoster , thanks a lot for implementing this feature.
I wanted to try it with the development branch, but I get following error:

Traceback (most recent call last):
  File "/srv/netboxstuff/nbsync-devel/./netbox-sync.py", line 23, in <module>
    from module.sources import instantiate_sources
  File "/srv/netboxstuff/nbsync-devel/module/sources/__init__.py", line 11, in <module>
    from module.sources.vmware.connection import VMWareHandler
  File "/srv/netboxstuff/nbsync-devel/module/sources/vmware/connection.py", line 29, in <module>
    from module.sources.common.source_base import SourceBase
  File "/srv/netboxstuff/nbsync-devel/module/sources/common/source_base.py", line 449
    log.info(f"Ip address {grab(ip, "data.address")} is assigned to an FHRP Group and skip_fhrp_group_ips is set to {self.settings.skip_fhrp_group_ips}, skipping.")
                                     ^^^^
SyntaxError: f-string: unmatched '('

I fixed it by setting single ticks as quotation marks:
log.info(f"Ip address '{grab(ip, 'data.address')}' is assigned to an FHRP Group and skip_fhrp_group_ips is set to {self.settings.skip_fhrp_group_ips}, skipping.")

If I enable skip_fhrp_group_ips the synchronisation works and leaves FHRP group assignment as it is, BUT all IPs bound to VM interface will be unbound :(

how may I help debugging this?

Cheers
Ionoci

@semx

semx commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

I've opened #538 which builds on this PR (your commits included) and adds two fixes @ionoci reported: the py<3.12 SyntaxError that broke import, and the regression where skip_fhrp_group_ips unbound every interface IP (the FHRP check now runs before the matching loop, scoped to the same address+VRF, so it's order-independent). It also adds the first unit tests in the repo. Credited you and @afoster as co-authors.

semx pushed a commit that referenced this pull request Sep 7, 2026
Fix skip_fhrp_group_ips unbinding all interface IPs and py<3.12 import (continues #476)
@semx
semx merged commit c04587c into bb-Ricardo:development Sep 7, 2026
marcinpsk pushed a commit to marcinpsk/netbox-sync that referenced this pull request Sep 7, 2026
The FHRP-group check ran inside the address-matching loop, before the
candidate address was compared to the IP being processed, so any FHRP
group IP in the inventory raised skip_this_ip and the IP being handled
was left unbound (issue bb-Ricardo#476). It also depended on inventory order and
ignored the VRF. Move the check to a dedicated pass before the matching
loop that scans for an FHRP-group assignment on this exact address in
the same VRF, so unrelated FHRP IPs no longer suppress a regular IP and
the outcome is order-independent, while the bb-Ricardo#445 behaviour (not
rebinding a real FHRP-group IP) is preserved.

Also fix an f-string with nested double quotes that is a SyntaxError
before Python 3.12 and broke import on the supported Python 3.6+.

Adds the first unit tests in the repo: an in-memory reproduction of the
unbinding regression in both inventory orders, the VRF scoping, the
bb-Ricardo#445 feature itself, and a control case. Verified red on the original
logic and green with the fix on Python 3.11 and 3.13.

Co-authored-by: Noah <noah@sol1.com.au>
Co-authored-by: Andrew Foster <andy@sol1.com.au>
marcinpsk pushed a commit to marcinpsk/netbox-sync that referenced this pull request Sep 7, 2026
The VMware source could only be exercised against a live vCenter, which
is why bugs like bb-Ricardo#476 were found by users rather than by a test. vcsim,
the simulator from the govmomi project, replays an inventory captured
with `govc object.save`, so the real pyVmomi code path can run in CI.

Adds:

- tests/fixtures/vcsim: the two captures contributed in bb-Ricardo#474, a 6.7.0
  and an 8.0.3 vCenter. The tests are parametrized over every archive in
  that directory, so adding a capture needs no test changes.
- tests/conftest.py: starts one vcsim per capture on a free port, gives
  each test a clean in-memory NetBoxInventory, and feeds the source a
  generated settings.ini. A second connection through pyVmomi provides
  an independent view of the same simulator to compare against.
- tests/test_vmware_source.py: every VM and host is synced, host serial,
  model and manufacturer match the hardware, VM cpu and memory match the
  hypervisor, interface MAC addresses match, guest IPs of VMs with
  running tools are synced and bound to an interface, two runs produce
  the same inventory, and a run against objects that already exist in
  NetBox creates nothing new.
- .github/workflows/test.yml: installs vcsim and runs pytest on pushes
  and pull requests.

The suite needs no vCenter and no NetBox. Without vcsim installed the
vcsim backed tests skip rather than fail, so `pytest` still works on a
machine that only has the Python dependencies.

Verified on Python 3.13 (the CI version) and 3.14: 16 passed. To check
the tests actually catch regressions, four faults were injected into the
source one at a time: memory reported in GB, guest IPs never collected
(the shape of bb-Ricardo#476), a VM silently dropped, and the MAC address dropped
from interface data. Each one turned the suite red, and it went green
again after reverting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants