feat(snmp-discovery): support VirtualMachine targets (OBS-1380)#429
feat(snmp-discovery): support VirtualMachine targets (OBS-1380)#429leoparente wants to merge 11 commits into
Conversation
Add TargetTypeDevice/TargetTypeVirtualMachine constants and two new Defaults fields (Type, Cluster) so a policy can declare its targets as VirtualMachines instead of Devices. Type is normalized to lowercase at parse time, validated against the two-element enum, and defaults to "device" for backward compatibility. Cluster is optional and only meaningful when Type == TargetTypeVirtualMachine. MergeDefaults gains per-field merge lines so override_defaults can flip Type/Cluster per target. This commit only wires the config; the runner does not consume the new fields yet. Refs: OBS-1380 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three-phase []diode.Entity -> []diode.Entity transform that rewrites a Device-rooted entity graph into a VirtualMachine-rooted one for targets the policy declares as type=VirtualMachine. Phase 1 — harvest every distinct *Interface referenced anywhere in the graph (top-level + IPAddress.AssignedObject + Device.PrimaryIp4/6 + fixed-point Parent/Bridge chains). The mapper intentionally suppresses top-level emission of IP-assigned interfaces, so a top- level-only walk would miss those and the IP would keep an *Interface ref (Codex round-1 BLOCKER). Phase 2 — convert each *Device -> *VirtualMachine (carry Name, Status, Serial, Role, Platform, Site, Tenant, PrimaryIp4/6, Description, Comments, Tags, CustomFields, Metadata, Owner; set Cluster from defaults; drop DeviceType, Location, Rack, Position/ Face, Latitude/Longitude, Airflow, AssetTag, OobIp, VirtualChassis, VcPosition, VcPriority) and *Interface -> *VMInterface (carry Name, Enabled, Mtu, PrimaryMacAddress, Description, Mode, UntaggedVlan, QinqSvlan, VlanTranslationPolicy, Vrf, Tags, CustomFields, TaggedVlans, Metadata, Owner; drop Speed, Type, Duplex, Wwn, MgmtOnly, Lag, RF*, PoE*, MarkConnected, Vdcs, WirelessLans, Label, Module, Device). Parent/Bridge remap by identity with name fallback. Phase 2b — rebuild VM.PrimaryIp4/PrimaryIp6 so their AssignedObject points at a matcher-only VMInterface stub (Name + Name-only VM stub + MAC matcher via newMACMatchStub) that breaks the VM<->VMInterface cycle that would otherwise crash the runner's debug-log proto conversion (Codex round-2 BLOCKER; mirrors how the Device path's detachForPrimaryIP solves the analogous issue). Phase 3 — rewrite top-level IPAddress.AssignedObject in place from *Interface to *VMInterface (identity-first, name-fallback); non-*Interface refs (*FHRPGroup, etc.) pass through unchanged. VLANs pass through unchanged. Chassis members (VcPosition != nil), VirtualChassis, Module, ModuleBay are dropped. Output order: VM, then VMInterfaces in top-level source order, then nested-only-harvested VMInterfaces in deterministic first-encounter order, then IPs, then VLANs. 21 unit tests cover the field carry/drop sets, IP-only nested interface, distinct-pointer-same-name IP, FHRPGroup passthrough, deep Parent chain (fixed-point harvest), PrimaryIP rebuild + harvest + nil passthrough, MAC matcher-only stub, cycle-break, output order, modules drop, VLAN passthrough, no-Device VLAN-only no-op. Caller is responsible for guarding the call site on defaults.Type == TargetTypeVirtualMachine. Transform is not yet wired into the runner. Refs: OBS-1380 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ine graph
annotateDeviceWithSourceMatch previously stamped netbox_id on the
top-level *diode.Device only. annotateEntitiesWithRunID previously
walked Device/Interface/IPAddress/VLAN/VirtualChassis only —
*diode.VirtualMachine and top-level *diode.VMInterface (without an
attached IP) were invisible to both walkers. With the upcoming
VM-target path, the top-level entity is the VM and the annotators
must reach it.
Adds:
- *diode.VirtualMachine case + setVMSourceMatch sibling helper in
annotateDeviceWithSourceMatch.
- *diode.VirtualMachine and top-level *diode.VMInterface cases +
annotateVirtualMachine / annotateVMInterface sibling walkers in
annotateEntitiesWithRunID. The existing annotateIPAddress
*VMInterface case is upgraded to a full annotateVMInterface call
so the nested VM/Parent/VLAN refs reachable from an IP-assigned
VMInterface also get stamped.
Refs: OBS-1380
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lity
Sibling helpers to CurrentDeviceFrom / PruneNestedRefs for the
VM-rooted entity graph that TransformToVirtualMachine produces.
Mirrors the Device-prune fidelity bar:
- newVMMatchStub keeps matcher fields (Name, Site, Cluster, Tenant,
Role + PrimaryIp4/6 matcher stubs) and preserves
metadata.source_match when present, so Diode's resolver finds the
same NetBox VM row as the rich top-level entity. Cluster is
matcher-critical: the NetBox plugin VM matcher uses (name+cluster)
and (name+cluster+tenant).
- newVMInterfaceStub keeps Name + a stubbed VirtualMachine ref + the
PrimaryMacAddress matcher stub, so VMInterface stubs land on the
same NetBox row as the rich top-level entity.
- PruneNestedRefsVM walks entities once: stubs
VMInterface.VirtualMachine + Parent + Bridge, and rewrites
IPAddress.AssignedObject (when it points at a VMInterface) to a
matcher-only VMInterface stub. Top-level VirtualMachine and
VMInterface entities are left rich.
The Device path is unchanged.
Refs: OBS-1380
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the resolved target defaults.Type == "virtualmachine", the runner calls mapping.TransformToVirtualMachine on the entity slice returned by queryTarget — rewriting the Device-rooted graph into a VirtualMachine-rooted graph — and threads the VM-aware CurrentVirtualMachineFrom / PruneNestedRefsVM helpers instead of the Device ones for nested-ref stubbing. annotateDeviceWithSourceMatch and annotateEntitiesWithRunID gained VM/VMInterface branches in the prior commit, so their call sites are unchanged. Default behaviour (Type unset or "device") is unchanged: the Device graph flows through annotateDeviceWithSourceMatch + annotateEntitiesWithRunID + PruneNestedRefs + Ingest exactly as before, pinned by the existing TestRunner_* regression set. Closes OBS-1380 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eate the Cluster
E2E against the local Diode -> NetBox stack surfaced a downstream
constraint that the design spec missed: NetBox's virtualization.cluster
model requires a ClusterType foreign key, so a defaults.cluster value
on its own was rejected by the reconciler with:
generate diff failed - ERR_OPS_GENERATE_DIFF -
{"virtualization.cluster":{"type":["Field type is required"]}}
Add a sibling defaults.cluster_type field. When both cluster and
cluster_type are set, the transform emits Cluster{Name, Type:
ClusterType{Name}} — letting the Diode reconciler auto-create both
the Cluster and its ClusterType on the fly. When cluster is set but
cluster_type is empty, the Cluster is emitted reference-only (works
when the operator has pre-created the cluster in NetBox).
MergeDefaults gains an override line for ClusterType. Two new
transform tests pin the Type-set-when-configured and
Type-omitted-when-unset behaviours; two new config-merge cases pin
the override-wins / empty-doesn't-clobber rules.
Verified end-to-end: with defaults.cluster_type set, the Dell
PowerConnect recording ingests as a VirtualMachine inside a
freshly-created Cluster, with zero reconciler errors.
Refs: OBS-1380
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NetBox's dcim.Interface.mtu and virtualization.VMInterface.mtu both use PositiveIntegerField(MaxValueValidator(65536)). Anything outside that range is rejected by the reconciler at the model layer, which drops the entire Interface / VMInterface object — and its child IPAddress / MAC entities in the same change-set — from the ingest. The previous bound (1 to 2 147 483 647, int32 max) was sized to "not overflow Go's int32", not to "stay within NetBox's accepted range". On Cumulus Linux this surfaced on the mgmt interface (ifIndex 69) which reports MTU = 65575: the whole VMInterface was rejected even though every other field was valid. Clamp the upper bound to 65536 and update the warning message. The field is skipped (left unset) when out of range so the rest of the interface still ingests cleanly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rd ambiguous VMInterface name rebind
Two findings from Codex review of the OBS-1380 branch:
1. virtualmachine.go rebuildPrimaryIP emitted a name-only VirtualMachine
stub (`&diode.VirtualMachine{Name: vm.Name}`) for the cycle-break
path. NetBox's VM matcher uses (name, cluster) and (name, cluster,
tenant); a name-only stub fails to resolve when VM names are unique
only within a cluster. Replace with newVMInterfaceStub +
newVMMatchStub so the cycle-break stub carries the same matcher
fields the rich VM has (Cluster, Site, Tenant, Role, source_match).
Cycle still terminates because newVMMatchStub builds PrimaryIp4 via
newIPMatchStub, which clears AssignedObject. Mirrors how the Device
path's detachForPrimaryIP preserves a full Device snapshot with only
PrimaryIp4/6 nulled.
2. PruneNestedRefsVM's stubForIface unconditionally rebound a nested
VMInterface ref to whichever top-level VMInterface had matched its
name first. Mirror PruneNestedRefs's defensive pattern: change the
index to map[string][]*diode.VMInterface and only rewrite owner when
the by-name lookup is unambiguous (len == 1). Single-VM walks have
unique interface names so the common path is unchanged; duplicate
names (config errors, partial walks) no longer silently re-point.
Tests:
- TestTransformToVirtualMachine_PrimaryIPRebuilt updated to assert the
new stub shape: stubVM.Cluster set, stubVM.PrimaryIp4 may be a
matcher IP but its AssignedObject must be nil for cycle termination.
- TestPruneNestedRefsVM_AmbiguousNameLeavesOwnerUnchanged: regression
test for the duplicate-name guard.
E2E re-ingest against orb-test-lab (cumulus_bgp) confirms no
proto-conversion stack overflow and 148 entities ingest cleanly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…le-break stub captures it Codex round-2 BLOCKER. The runner ran annotateDeviceWithSourceMatch AFTER TransformToVirtualMachine, but rebuildPrimaryIP builds the cycle-break VM stub (via newVMMatchStub) at transform time and captures Metadata["source_match"] by value at construction. Stamping source_match later only mutated the rich top-level VM; the already-frozen cycle-break stub at vm.PrimaryIp4.AssignedObject.VirtualMachine was left without it — weakening the Diode NetBox-plugin matcher path under target.netbox_id rediscovery. Fix: move annotateDeviceWithSourceMatch to BEFORE the transform in runner.go. deviceToVM (mapping/virtualmachine.go) already does `Metadata: d.Metadata`, so the stamp propagates Device → VM → stub without any further plumbing. annotateEntitiesWithRunID still runs AFTER the transform — run_id is annotation metadata that intentionally does not belong on stubs. Tests: - New TestVMTargetSourceMatchPropagatesThroughTransform exercises the full runner sequence (annotate → transform) and asserts source_match lands on both the rich VM and the cycle-break stub. - New TestTransformToVirtualMachine_PrimaryIPStubCarriesSourceMatchWhenStampedPreTransform covers the same invariant at the mapping-package level. - Round-2 NIT: TestPruneNestedRefsVM_AmbiguousNameLeavesOwnerUnchanged now uses distinct PrimaryMacAddress values on dupA / dupB so the guard's effect is observable — a first-wins map would yield dupA's MAC; the guard yields dupB's. - Round-2 NIT: TestTransformToVirtualMachine_PrimaryIPRebuilt now asserts stubVM.Cluster.Name == "vyos-cluster" (not just non-nil). Full go test ./... passes. E2E re-ingest against orb-test-lab (cumulus_bgp) confirms clean ingest, 148 entities, no panics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…larify stubs run_id doc NIT 1 from review: the inline post-queryTarget pipeline in runWithMetadata (source_match → transform → run_id → log → prune) encoded ordering invariants that no test could catch a regression on. A future refactor that moved annotateDeviceWithSourceMatch after TransformToVirtualMachine would have broken the NetBox VM-matcher path under target.netbox_id rediscovery, but every existing test that exercised the four steps invoked them itself in test code — none invoked the runner's actual orchestration. Extract the four-step pipeline into prepareEntitiesForIngest in a new policy/entities_pipeline.go. The Runner method now calls the helper. New entities_pipeline_test.go covers: - TestPrepareEntitiesForIngest_SourceMatchReachesCycleBreakStub: runner-level guard for the round-2 BLOCKER. With VM target + NetboxID set, source_match must land on both the rich VM AND vm.PrimaryIp4.AssignedObject.VirtualMachine. - TestPrepareEntitiesForIngest_NoNetboxID_NoSourceMatch: no NetboxID means no source_match stamp. - TestPrepareEntitiesForIngest_DeviceTargetPathUnchanged: Type unset still routes through PruneNestedRefs (not …VM). - TestPrepareEntitiesForIngest_RunIDStampedOnPostTransformEntities: run_id lands on VM/VMInterface (would land on dropped Device if ordering broke). - TestPrepareEntitiesForIngest_LogEntitiesCalledBeforePrune: the debug-log callback sees the rich shared graph, not stubs. NIT 2 from review: the stubs.go doc on newDeviceStub claimed "Annotation metadata such as run_id is intentionally NOT copied — stubs are matcher-only", but the run_id annotation walker reaches embedded cycle-break stubs via rich Device.PrimaryIp4.AssignedObject. Diode ignores run_id during matching, so the leak is inert payload not a correctness issue; the doc now reflects what actually happens instead of an aspirational invariant. Full go test ./... passes. E2E re-ingest against orb-test-lab (cumulus_bgp) confirms behavior unchanged: 148 entities, no panic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Vulnerability Scan: Passed — snmp-discoveryImage: No vulnerabilities found. Commit: 3f078ff |
Vulnerability Scan: Passed — network-discoveryImage: No vulnerabilities found. Commit: 3f078ff |
|
Go test coverage
|
- gofumpt-style import grouping (collapse blank line between import groups in mapping/virtualmachine.go, mapping/virtualmachine_test.go, policy/entities_pipeline.go, policy/entities_pipeline_test.go) - gofumpt comment-indentation cleanup in the TransformToVirtualMachine doc block and in two existing comment-aligned struct literals - revive: wrap PruneNestedRefsVM_NoOpOnEmptyOrNil's panic-free calls in assert.NotPanics so the *testing.T parameter is used (the previous shape relied on a "must not panic" comment with no assertion, which revive flagged as an unused parameter) go test ./... still green; make fix-lint reports 0 issues. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds support for ingesting SNMP discovery targets as NetBox virtualization.VirtualMachine entities (instead of dcim.Device) via a new defaults.type: VirtualMachine policy option. Introduces a Device→VM graph transform, VM-rooted prune/stub helpers, source_match/run_id annotation paths for VM types, and a small drive-by fix clamping interface MTU to NetBox's documented [1, 65536] range.
Changes:
- New
mapping.TransformToVirtualMachine,PruneNestedRefsVM,newVMMatchStub,newVMInterfaceStub,CurrentVirtualMachineFromto convert and prune the VM-rooted graph (with cycle-break stub atvm.PrimaryIp4.AssignedObject.VirtualMachine). - New
Defaults.Type/Cluster/ClusterTypeconfig fields with case-insensitive validation/normalization inmanager.applyDefaultsandvalidatePolicy; extendedMergeDefaultsand source_match/run_id annotators to handle VirtualMachine and VMInterface entities. - New
prepareEntitiesForIngesthelper inpolicy/entities_pipeline.goconsolidating the source_match → transform → run_id → log → prune pipeline with explicit ordering documented and guarded by tests; MTU upper bound lowered from int32-max to 65536.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| snmp-discovery/config/config.go | Adds TargetTypeDevice/TargetTypeVirtualMachine constants and Type/Cluster/ClusterType fields; extends MergeDefaults. |
| snmp-discovery/config/config_test.go | Unit tests for new merge behavior across Type/Cluster/ClusterType. |
| snmp-discovery/policy/manager.go | Lowercase normalization and validation of defaults.type and per-target overrides. |
| snmp-discovery/policy/manager_test.go | Tests for type normalization, default-to-device, and invalid type/override rejection. |
| snmp-discovery/policy/runner.go | Replaces inlined post-query steps with a call to prepareEntitiesForIngest. |
| snmp-discovery/policy/entities_pipeline.go | New helper enforcing source_match → optional VM transform → run_id → log → prune order. |
| snmp-discovery/policy/entities_pipeline_test.go | Tests guarding the ordering invariants and Device-path no-regression. |
| snmp-discovery/policy/entity_run_metadata.go | Extends source_match/run_id annotators to VirtualMachine and VMInterface, including IP-assigned VMInterface recursion. |
| snmp-discovery/policy/entity_run_metadata_test.go | Tests for VM annotation paths and pre-transform source_match propagation. |
| snmp-discovery/mapping/virtualmachine.go | New Device→VM graph transform, including primary IP rebuild with cycle-break stub. |
| snmp-discovery/mapping/virtualmachine_test.go | Extensive coverage for the transform (fields, IP re-anchoring, primary-IP cycle, parent/bridge remap, output order, drops). |
| snmp-discovery/mapping/stubs.go | Adds CurrentVirtualMachineFrom, newVMMatchStub, newVMInterfaceStub, PruneNestedRefsVM; clarifies stub run_id doc. |
| snmp-discovery/mapping/stubs_test.go | Tests for VM/VMInterface stub helpers, prune behavior, and ambiguous-name guard. |
| snmp-discovery/mapping/mappers.go | Lowers maxInterfaceMTU to 65536 with documentation. |
| snmp-discovery/mapping/mappers_test.go | Updates MTU boundary tests to the new 65536 limit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Adds a
defaults.type: VirtualMachinepolicy option to snmp-discovery so operators can ingest VMs (e.g. VyOS VMs used as virtual routers) as NetBoxvirtualization.VirtualMachineentities instead ofdcim.Device. Driven by OBS-1380 — community feedback (netdev Slack) from an operator running SNMP discovery against VyOS VMs.When a target's effective defaults set
type: VirtualMachine, the runner transforms the standard Device-rooted entity graph emitted by the SNMP mapper into a VirtualMachine-rooted one before ingest:*diode.Device→*diode.VirtualMachine*diode.Interface→*diode.VMInterface(Parent/Bridge remapped by pointer-then-name)*diode.IPAddress.AssignedObjectre-anchored from*Interfaceto*VMInterface*diode.VirtualChassis, member*Device(VcPosition != nil),*Module,*ModuleBaydropped (no VM analog)*diode.VLANpassthroughdefaults.cluster/defaults.cluster_typematerialize aCluster{Name, Type: ClusterType{Name}}on the VM (ClusterType is required for NetBox to auto-create a Cluster on first ingest)A new
mapping.PruneNestedRefsVMmirrors the existingPruneNestedRefsfor the VM-rooted graph. A newprepareEntitiesForIngesthelper consolidates the four-step post-queryTarget pipeline (source_match annotation → optional VM transform → run_id annotation → prune) with explicit ordering invariants documented at the helper and asserted by a dedicated test file.The Device-target path (
typeunset ordevice) is unchanged.Also drive-by: clamp interface MTU to NetBox's documented
[1, 65536]range. The previous bound was Go's int32 max which caused real Linux interfaces (Cumulusmgmtreports MTU=65575) to be rejected by NetBox's model validator — the reconciler then dropped the entire VMInterface (and its child IP/MAC entities). Now the MTU field is skipped when out of range; the rest of the interface still ingests.Commits
feat: add defaults.type and defaults.clusterfeat: add TransformToVirtualMachine post-processing passfeat: annotate source_match and run_id on VirtualMachine graphfeat: add VM-rooted prune helpers with full stub fidelityfeat: run VM transform + VM-aware prune for VM targetsfeat: add defaults.cluster_type so NetBox can auto-create the Clusterfix: clamp interface MTU to NetBox's [1, 65536] rangefix: carry VM matcher fields on cycle-break stub; guard ambiguous VMInterface name rebind(Codex round 1)fix: annotate source_match before VM transform so cycle-break stub captures it(Codex round 2)refactor: extract prepareEntitiesForIngest helper + clarify stubs run_id doc(manual review NITs)Notable design choices
Cycle-break stub.
VirtualMachine.PrimaryIp4.AssignedObjectwould naturally point at a richVMInterfacewhoseVirtualMachineref points back at the rich VM — a cycle that crashes Diode's proto-conversion in debug-log paths before prune runs.mapping/virtualmachine.go::rebuildPrimaryIPemits a matcher-only stub vianewVMInterfaceStub(vmIfRef, newVMMatchStub(vm))instead. The inner VM stub'sPrimaryIp4runs throughnewIPMatchStubwhich clearsAssignedObject, terminating the chain.Ordering: source_match BEFORE transform.
newVMMatchStubcapturesMetadata["source_match"]by value at construction time. IfannotateDeviceWithSourceMatchruns after the transform, the rich top-level VM gets stamped but the already-frozen cycle-break stub does not — breaking the NetBox plugin matcher path undertarget.netbox_idrediscovery.deviceToVM'sMetadata: d.Metadatapointer-share carries the Device's stamp forward into the VM, so annotating the Device first works through the chain. TheprepareEntitiesForIngesthelper enforces this ordering and the new test file guards it.Ambiguous VMInterface name guard.
PruneNestedRefsVM.stubForIfacemirrorsPruneNestedRefs's defensivelen(tops) == 1check before rebinding owner by name. Single-VM walks have unique interface names so this is the common path; the guard prevents silent mis-rebinding under unexpected input shapes (config errors, partial walks).Cluster auto-creation requires ClusterType. Discovered during e2e — NetBox rejected every change-set with "Field type is required" until we threaded
defaults.cluster_typethroughMergeDefaultsanddeviceToVM.Review history
ddbb7319.483c98cd.4cccb556:prepareEntitiesForIngestso the pipeline ordering is testable at runner level (previously the only ordering protection was a comment block).stubs.godoc —run_iddoes reach cycle-break stubs via the annotation walker, but Diode ignores it during matching, so the leak is inert payload not a correctness issue.Test plan
go test ./...greencumulus_bgpsnmpsim (98 interfaces, 16 IPs):object_id=11) across re-ingests — idempotent, pure noop on the VM/Cluster/Site/Role/VLAN entitiesipam.ipaddresscreates withassigned_object_type: virtualization.vminterface(re-anchoring verified)vm.primary_ip4populated end-to-end via the cycle-break pathmgmtinterface (MTU=65575) now ingests withmtufield unset, rest of the interface intactvirtualization.clustermodel — relies onTypeFK being the only auto-create requirement🤖 Generated with Claude Code