fix(snmp-discovery): discover optics published as container or port rows - #528
fix(snmp-discovery): discover optics published as container or port rows#528leoparente wants to merge 19 commits into
Conversation
aristaLaneShapeFixture/aristaHarvestShapeFixture embedded a vendor brand in a Go identifier. Renamed to fixedPortLaneShapeFixture/ fixedPortHarvestShapeFixture; doc-comment leading words and call sites updated to match. Fixture data, other fixture names, and test behaviour are unchanged.
bayHasChild was set before the missing-serial guard's continue, so the optic's cage never surfaced in EmptyBays either. Moving the flag past both new guards restores parity with the existing duplicate-serial drop, which already left its bay harvestable.
linecards mode is documented to emit no transceivers, but a fixed-port
optic is a top-level entry in inv.Modules and the loop emitting that
list ran in both modes, filtering only PSU/Fan. Add a mode gate beside
that filter so a transceiver only emits in full mode.
Assert on the actual emitted Diode payload, not just the inventory
struct: linecards mode must not contain the transceiver, and full mode
must name each optic's bay for the interface it serves with its own
serial, asserted as an exact set.
That payload-level assertion surfaced a second, independent defect in
extractModuleInventory's empty-bay harvest: bayHasChild only marked a
module's own nearest class=5 bay, so a container whose children are
themselves containers (never a class=9/10 leaf directly) looked empty
even when fully populated several levels down. Propagate "has a child"
upward through container ancestors after the extraction loop so the
harvest's invariant holds: a container is an empty bay only if nothing
beneath it was emitted. The propagation is type-agnostic — it walks
Class/ContainedIn only, so it also closes the same gap for the fan-tray
and power-supply slot containers, not just the transceiver one, without
changing how PSU/Fan cages themselves participate in bayHasChild.
Corrected both fixed-port optic fixtures' container relPos ("1" -> "3")
to match the real capture.
Documents the fixed-port optic widening: which entPhysicalClass rows are scanned, how a fixed-port bay is named, the serial gate and its aggregated warning, the known PID false negatives, and the empty-bay harvest fix for containers whose children are themselves containers. Also notes that the interface-association gap is unconditional for fixed-port transceivers (the ifIndex routing only walks nested submodules), not merely a symptom of a missing entAliasMappingTable, and states which optic test fixtures are capture-derived versus the one synthetic exception.
classifyModule already computes the upper-cased effective PID before calling isOpticPID, which recomputed the same trimmed-Model-falling- back-to-VendorType value from scratch. Extract the prefix match into hasOpticPIDPrefix and have both callers share it, so the two copies of that rule no longer have to be kept in lockstep by hand. Pure refactor, no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch widened module discovery to class=5/class=10 rows whose PID identifies a transceiver, but several comments still describe the scan as class=9-only. Update ModuleEntry, ModuleInventory and extractModuleInventory's doc comments, and note that bayHasChild's key is not always a class=5 row: a chassis-rooted module with no container ancestor synthesizes its own EntIndex as the bay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dcim.modulebay matches on name+device, so two bays sharing a name on one device silently merge into one NetBox object, taking their modules with them — and Diode never retracts a wrong value once applied. No capture in the 204-device corpus shows two fixed-port transceivers collide on the same member's bay name, but the failure mode is silent and permanent, so guard against it anyway. Keyed on (MemberID, effective bay name) inside TranslateModules- WithAlias's module loop, where MemberID is already resolved — not in extractModuleInventory, where it isn't. Scoped to transceivers; other module types' bay-name collisions are pre-existing and out of scope. On a collision, skip and warn rather than invent a disambiguated name, since a fabricated value would itself become a permanent wrong one. Factor effectiveBayName out of emitModuleBay so the guard compares the name Diode will actually see, not the raw (possibly blank) ModuleEntry field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestOpticDiscovery_OpticRowsNeverHarvestedAsEmptyBays and the container-of-containers test both looped over inv.EmptyBays filtering for indices that are never present on these fixtures, so their loop bodies never ran. Assert the actual outcome — EmptyBays is empty — directly, and keep the per-index checks alongside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The class-5 and class-10 serial-free optic shapes behave differently: a class-5 optic in a cage leaves the cage harvested into EmptyBays, while a class-10 optic's cage is suppressed by the pre-existing containerHasPortChild skip and produces no bay at all. Only the class-5 variant was pinned, and only by a synthetic fixture, even though the class-10 cage+port topology is the one real captures show. Add the missing coverage for the class-10 shape as it actually behaves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Go test coverage
Total coverage: 88.7% |
Vulnerability Scan: Failed — blocking vulnerabilities detectedImage:
Commit: b828065 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cbd5552f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e optics TranslateModulesWithAlias guarded only the top-level module loop against two transceivers merging into one dcim.modulebay object on the same device. The full-mode-only submodule loop (modular optics nested under a linecard) emitted its bays without consulting that guard, so two modular optics whose cages resolve to the same bay name could still silently merge. Extend the existing seenTransceiverBays map — not a second map — to the submodule loop, since the collision domain is (device, bay name) regardless of nesting depth: a top-level fixed-port bay and a submodule bay sharing a name on one device collide in NetBox too, and only a shared map catches that cross-tier case. No captured device in the corpus exhibits either shape; the added fixtures are synthetic hardening, documented as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ifaceShaped accepted any whitespace-free, digit-bearing token as an interface name, including the row's own transceiver PID. A vendor that omits the "Xcvr for <iface>" descr but publishes a product label such as "SFP-10G-LR" in entPhysicalName would have that label become the bay name and position, displacing the real cage-derived name and colliding across every optic of the same model. Reject a candidate that equals the row's effective PID (trimmed Model, falling back to trimmed VendorType), compared case-insensitively, on both the descr-derived and name-derived path. Deliberately narrow: an interface is never named exactly its own transceiver part number, so this cannot reject a legitimate interface name on an unseen platform, unlike a broader pattern would. servedInterface's signature grew a pid parameter; both extraction call sites (including the missing-serial warning label) are updated. No captured device in the corpus exhibits this shape; the added fixture is synthetic hardening, documented as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
In both the top-level and submodule loops, the duplicate-bay-name guard
ran before the memberDevices nil check, so an entry whose member had no
device still claimed its bay-name key before being dropped. A later
entry landing on that same key was then misreported as a "duplicate
transceiver bay name dropped" instead of its real cause.
Move the device lookup (and its nil skip) ahead of the guard in both
loops, so only an entry that is actually about to be emitted can claim a
key. Confirmed MemberID<0 was already positioned before the guard in
both loops, so that path is unaffected.
Added a test reusing duplicateBayNameOpticFixture with an empty
memberDevices map: both optics share a member with no device, so
neither can ever be emitted either way (the guard's key is
(member, bay) — two entries can only collide if they share a member,
and sharing a member means sharing the exact same device-lookup
result). What the fix changes is that both are now reported for their
real reason ("no device for member") instead of the second being
misattributed as a duplicate and the guard's map being polluted with an
entry nothing was ever emitted under.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e066ca96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rows The missing-serial gate applied to every ModuleTypeTransceiver, but a class=9 optic beneath a linecard with a blank entPhysicalSerialNum was already discoverable before the widened container/port scan: it was classified as a transceiver, nested under its linecard, and emitted by emitModule with Serial left unset (emitModule tolerates a blank Serial unconditionally — no error, no drop). The unconditional gate turned that pre-existing behaviour into a regression, confirmed against the capture corpus: 25 of the corpus's 26 serial-less optic-PID rows are class-10 with no module parent (newly discovered by this branch, so skipping them loses nothing), but one is class-9 with a module parent and was already found before this branch. Add a class test to the gate condition so it fires only for the container/port classes the widened scan added, leaving class=9 behaviour byte-identical to pre-branch. The append-then-continue for the aggregated warning stays inside the same (now narrower) condition, so its count only ever reflects rows the gate actually skips. Verified emitModule's tolerance directly before making this change: it builds Device/ModuleBay/ModuleType unconditionally and only conditionally sets Serial, never erroring or dropping the Module for a blank one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1095bd7e42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every pre-existing drop path (orphan_containment, dup_serial, orphan_member) increments metrics.GetModulesDropped() with a reason attribute, but the three drop paths this branch introduced only logged a warning, leaving the counter at zero while optics were being discarded. Instrument missing_serial (module.go) once per dropped row, at the continue site inside the extraction loop, not next to the aggregated per-device warning — the metric must reflect the number of rows dropped, not the number of log lines. Instrument dup_bay_name (module_translate.go) at both the top-level and submodule guard sites, using the same GetModulesDropped() counter the rest of the package uses for drops (this file's own GetModuleBaysEmitted/GetModulesEmitted calls are for successful emission, a different signal). Deliberately left optic_sub_entity uninstrumented: a lane row is not a dropped module, it's a row correctly identified as not being a module at all. Counting it would add per-poll noise on every Arista device during normal operation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1095bd7 scoped the missing-serial gate to container(5)/port(10) rows, so a module(9) transceiver with a blank serial is now emitted (Serial left unset) rather than dropped. The "Optics without a serial" paragraph in supported_platforms.md still claimed the rule applied to every optic unconditionally, which that fix made false. Limit the claim to container(5)/port(10) optics and state the module(9) exception plainly: that shape was already discoverable before the widened scan, so gating it too would have removed working behaviour rather than tightened new behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 879bd7c394
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // names. A modular optic keeps the derivation from its real cage, | ||
| // which already identifies the port. | ||
| pid := effectivePID(r.Model, r.VendorType) | ||
| if entry.Type == ModuleTypeTransceiver && parentModuleIdx == "" { |
There was a problem hiding this comment.
Handle slotted linecard optics as separate bays
When a newly scanned class-5 or class-10 optic is directly beneath a class-9 linecard that is itself installed in a class-5 chassis slot, walkParents crosses the module boundary and selects the linecard's slot as bayIdx. This condition then prevents the optic's interface name from replacing that slot-derived bay, so the first optic reuses the linecard's own ModuleBay and subsequent optics under the card resolve to the same name and are dropped by the duplicate-bay guard. Treat only containers below the parent module as optic cages, or use the served interface when no such cage exists.
Useful? React with 👍 / 👎.
Summary
ENTITY-MIB publishes a transceiver in three different shapes depending on vendor: as a
module(9)row under a linecard, as acontainer(5)row, or as aport(10)row.extractModuleInventoryscannedmodule(9)only, so most optics were never examined, and the ones that were visible on fixed-port hardware were emitted as the wrong object.This is the snmp-discovery counterpart to #525, which fixed the same class of gap on device-discovery. The two backends differ in an important way: CLI inventory never states whether an optic has a parent module, so #525 had to infer it from naming conventions. ENTITY-MIB states the relationship outright, and the optic row itself names the interface it serves, so the signal here is asserted by the device rather than deduced. Every behavioural claim below was measured against 204 ENTITY-MIB recordings from an SNMP simulator corpus, and the unit fixtures are transcribed from those same recordings.
What was broken
Three defects, all measured rather than assumed.
Most optic rows were never examined. Across the corpus, optic-PID rows are classed 139
port(10), 80module(9), 34container(5). So 173 of 253 were invisible to module discovery regardless of parentage, and both shapes real fixed-port hardware publishes are in that 173.Per-lane sub-entities were emitted as linecards. One vendor publishes a
module(9)row per lane beneath each optic, carrying no model, no serial, and the placeholder PID0.0. On one captured 48-port switch, everymodule(9)row on the device is one of these, so the device's entire module inventory was lanes. Each one resolved its bay to its parent optic row, whoseentPhysicalNameis empty, so all twelve fell back to the same bay position and merged into a single object. None of the twelve real optics, each with a serial, was emitted at all.An optic with no lane child was emitted as an empty bay with its identity discarded. The empty-bay harvest gathers a
container(5)row that has nomodule(9)orport(10)child, and sets neither Model nor Serial on the entry. On the second capture, 20 of 22 optics took this path, so their real serials were dropped on the floor and all 20 collapsed into one bay.The two mechanisms are coupled, which is why they are fixed together: a lane's bay is the optic row, so suppressing lanes stops
bayHasChildbeing set on the optic, which drops it straight into the harvest. Fixing the first alone converts it into the second.What changed
container(5)andport(10)rows that carry a recognised optic PID.module(9)remains the only class scanned unconditionally; a bare cage or a port row without an optic PID is still ignored.module(9)ancestor, which mistyped every fixed-port optic as a linecard. A wrong type persists, since Diode applies updates withpartial=Trueand never retracts.Xcvr for <iface>descr or an interface-shapedentPhysicalName. Anchoring is required: one platform publishesLane N for Xcvr for <iface>beneath the same optic, and an unanchored match would emit one bay per lane. An accepted token must contain a digit, because one platform names every optic row with the literal wordport.linecardsmode. That mode is documented to emit none, and enforced it by returning before the sub-module loop; a fixed-port optic is a top-level entry, so without an explicit exclusion the widening would have started delivering transceivers tolinecardsusers.bayHasChildmarked only a module's nearestcontainer(5)ancestor, so a container whose children are themselves containers was reported as empty even when fully populated. The rule is now that a container is an empty bay only if nothing beneath it was emitted. A genuinely empty slot has no module beneath it and is still reported.Measured impact
Corpus-wide, 148 rows begin emitting across 12 of the 204 ENTITY-MIB recordings: 34 fixed-port on two captures, 114 modular
port(10)rows on ten others. Modular hardware is deliberately in scope rather than incidentally affected, and each distinct shape has a fixture.Per capture, before to after:
Also: 14 lane rows suppressed across two captures; 25 serial-less optics skipped on one capture with a single aggregated warning; no bay-name collision anywhere in the corpus.
Known limitations
Interface.modulebackref.buildIfaceModuleMaproutes only sub-modules, and a fixed-port optic is a top-level module, so it never participates. This holds even on a device that publishesentAliasMappingTable, so it is unconditional rather than data-dependent. Neither capture with fixed-port optics publishes that table, so closing it would change nothing measurable today; left as a follow-up.CAB-SFP-SFP-1M,SFPP-PC005,ABCU-5710RZ-CS5,FN-TRAN-SFP+GC,CVR-QSFP-SFP10G,10GE SR 300m SFP+,10GE LR 10km SFP+. Broadening the list is deliberately deferred: those prefixes now reachcontainer(5)andport(10)rows, where a false positive would manufacture a bay that does not exist on the device.fan(7)and PSUs arepowerSupply(6), so nothing beneath them is ever emitted as a module. Pre-existing and unchanged here; noted so the harvest fix is not mistaken for complete.dcim.moduleidentity leans on the bay it occupies, which is a separate decision.Testing
Unit fixtures are transcribed from real recordings rather than hand-shaped, because an earlier hand-shaped fixture concealed a wrong assumption about current behaviour: it placed a lane row at
container(5)where the capture has it atmodule(9). Row class, parentage,entPhysicalParentRelPos, and which fields are blank all mirror the capture, since those are exactly the values the logic reads.Five fixtures mirror captures: two fixed-port shapes (one with a lane child under every optic, one with almost none), a modular chassis with a
port(10)optic inside a linecard's cage, a two-member stack whose optics are namedport, and a device reporting no serial on any optic. Three are synthetic and say so in their doc comments, covering invariants no captured device exhibits.Assertions run at both layers: on the inventory struct, and on the emitted payload through
TranslateModules, since the bay-name fallback and per-device stamping live there and that is where the merge would actually happen.Gates:
go test ./...green for the module;make lint-allreports0 issuesfor all four Go modules.Notes for review
Build & Scancheck fails on standingmsgpackandsetuptoolsCVEs. It fails identically on the branch point and on fix(device-discovery): discover transceivers with no parent module #525, and this branch changes no dependency, lock, or image file.modules_droppedthe way the orphan and duplicate-serial drop paths do. Worth deciding whether it should.BayPositionis set to the interface string alongsideBayName, for parity with device-discovery's equivalent path. A reviewer argued for keeping the cage's numeric position instead, since every other bay in this backend carries a number. Position is not a matcher field, so neither choice risks a merge. Flagging both sides.🤖 Generated with Claude Code