Fix/keep undiscovered ips - #550
Merged
Merged
Conversation
marcinpsk
marked this pull request as draft
September 9, 2026 17:03
Both check_redfish test modules carried their own copy of the same setup: reset the inventory singleton, build a CheckRedfish through object.__new__, run the real add_necessary_base_objects(), and add a device to hang components off. Two more copies were about to arrive with the interface IP and device serial tests. conftest.py gains a check_redfish_source fixture which returns a builder. It reuses the existing inventory fixture instead of resetting the singleton again, so the reset lives in one place, and it takes the source settings as keyword arguments because each entry point of the source reads a different subset of them. It returns the source, the inventory and the device as a namespace. The primary tag is now always registered. Only the orphan tagging tests needed it, but the NetBox handler registers it in production regardless, so making it unconditional brings the fixture closer to a real run rather than further from it. No test assertion changed. Reverting either of the two fixes these modules cover still fails them, so the shared setup does not weaken what they check.
add_update_interface() removes every IP on an interface which the source did not report. The removal is gated only on skip_ip_handling, so it runs whenever a source hands over an empty IP list, treating "I found nothing" as "there is nothing". For check_redfish that assumption is wrong. Redfish reports the BMC IP and little else, while map_object_interfaces_to_current_interfaces() matches Redfish NIC ports to existing NetBox interfaces by MAC. An OS bond or bridge (pnet0, bond0, sometimes eth0) inherits the physical NIC's MAC, so a Redfish port matches the OS interface that carries the management IP. That IP is not in the Redfish data, so it was unassigned on every sync, and unassigning the device's primary IP also clears primary_ip4 on the device. permitted_subnets does not protect against this: it filters which IPs are added, not which are removed. add_update_interface() gains keep_undiscovered_ips, default False, so vmware and every other source are unchanged. When it is set, the removal loop is skipped for an interface the source discovered no IPs for. An interface which reports a different set of IPs is untouched, so a genuine IP removal is still synced. The check_redfish source passes it. The condition reads interface_ips, what the source reported, not ip_address_objects, what survived parsing and prefix matching. An address which fails to parse is dropped with a continue, so keying on the result would treat a non-empty discovery holding one bad address as "discovered nothing" and would keep stale IPs. The tests drive the real add_update_interface() removal loop and assert on the queued de-assignment, covering the flag, the default, a non-empty discovery, and a discovery whose only address is unusable.
marcinpsk
force-pushed
the
fix/keep-undiscovered-ips
branch
from
September 9, 2026 17:06
596249e to
1c1ce98
Compare
marcinpsk
marked this pull request as ready for review
September 9, 2026 17:09
This was referenced Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
add_update_interface()removes every IP on an interface which the source did not report. The removal is gated only onskip_ip_handling, so it runs whenever a source hands over an empty IP list, treating "I found nothing" as "there is nothing".For check_redfish that assumption is wrong. Redfish reports the BMC IP and little else, while
map_object_interfaces_to_current_interfaces()matches Redfish NIC ports to existing NetBox interfaces by MAC. An OS bond or bridge (pnet0,bond0, sometimeseth0) inherits the physical NIC's MAC, so a Redfish port matches the OS interface that carries the management IP. That IP is not in the Redfish data, so it was unassigned on every sync:Unassigning the device's primary IP also clears
primary_ip4, so a user reassigns it and the next sync strips it again.permitted_subnetsdoes not protect against this: it filters which IPs are added, not which are removed.Fix
add_update_interface()gainskeep_undiscovered_ips, defaultFalse, so vmware and every other source are unchanged. When it is set, the removal loop is skipped for an interface the source discovered no IPs for. An interface which reports a different set of IPs is untouched, so a genuine IP removal is still synced. The check_redfish source passes it.Relationship to the other IP-preservation PRs
Different from #525 (
preserve_primary_ips), which protects an address because it is the primary IP, wherever it is discovered. This one protects every IP on an interface the source could not see at all, primary or not, and only for a source that opts in. They don't conflict, and both touch the same removal loop.Tests
tests/test_check_redfish_interface_ips.pydrives the realadd_update_interface()removal loop against realNBInterface/NBIPAddressobjects and asserts on the queued de-assignment (unset_attribute()queues inunset_items, it does not mutatedata). Covers the flag, the unchanged default, and a non-empty discovery which must still remove a dropped IP. Two of the three fail without the change.