diff --git a/docs/backends/snmp_discovery/supported_platforms.md b/docs/backends/snmp_discovery/supported_platforms.md index 7407c5de..9fc06f08 100644 --- a/docs/backends/snmp_discovery/supported_platforms.md +++ b/docs/backends/snmp_discovery/supported_platforms.md @@ -137,3 +137,19 @@ Any other vendor that populates `entPhysicalTable` per RFC 6933 will be discover **PID classifier.** Module rows (`entPhysicalClass = module(9)`) are split into `supervisor` / `linecard` / `transceiver` / `psu` / `fan` types by matching `entPhysicalModelName` (the vendor product ID) against a small set of prefix rules: `SUP*` / `SUPV*` / `SUP\d` → `supervisor`; optic prefixes `SFP-` / `QSFP-` / `X2-` / `GLC-` / `CFP-` / `XENPAK-` / `XFP-` → `transceiver`; `PSU-` / `PWR-` or `-PWR-` infixes → `psu`; `FAN` / `-FAN-` → `fan`; everything else inside a chassis slot defaults to `linecard`. The classifier is shared across all vendors — no Cisco-only / Arista-only branch. PSU and fan modules are recognised so they label correctly in OTLP metrics, but **never** emitted as `Module` entities (counted in `modules_dropped` instead) — the inventory surface in NetBox stays scoped to line cards, supervisors, and transceivers. +### Fixed-port optics + +Not every transceiver arrives as a `module(9)` row under a linecard. On fixed-port hardware — and in some per-port cages on modular platforms — the optic itself is published as a `container(5)` row or a `port(10)` row, with no `module(9)` level in between. The module scan widens to those two classes, but only for rows that carry a recognised optic PID (the same prefix list the PID classifier above uses); a bare cage or a port row without one is left alone, so ordinary container and port rows are never mistaken for modules. `entPhysicalClass = module(9)` remains the only class scanned unconditionally. + +**Bay naming.** A fixed-port optic's bay is named for the interface it serves rather than the cage's bare position number, which by itself does not identify the port. The name is taken from an anchored `Xcvr for ` `entPhysicalDescr` — anchored because some platforms publish a "Lane N for Xcvr for ``" row beneath the same optic, and an unanchored match would emit one bay per lane — or, failing that, from an interface-shaped `entPhysicalName`. Either candidate must contain a digit: one platform names every optic row with the literal token `port`, which would otherwise give every bay on the chassis the same name. + +**Optics without a serial.** An optic reporting no serial is still discovered and emitted, in every class. NetBox leaves a module's serial optional and matches `dcim.module` on the module bay it is installed in, never on the serial, so a serial-less module reconciles exactly as a serialled one does: the field is omitted from the payload rather than sent empty, and a later poll updates the same object instead of creating another. Vendors that omit optic serials are common rather than exceptional, and on lower-end switching a blank `entPhysicalSerialNum` is the norm across modules generally, so gating on one would discard a large amount of inventory that reconciles perfectly well. The bay name is what has to be distinct, and that comes from the interface the row names, not from the serial. + +**Interface-association limitation.** A fixed-port transceiver — one with no `module(9)` parent — is still emitted as its own `ModuleBay` and `Module`, with model, serial, and the interface-named bay all present. Its owning interface does not carry a `module=` reference, though: the `entAliasMappingTable`-based routing that attaches `Interface.module` only walks transceivers nested under another module, and a fixed-port optic never is one, so it never participates — this holds even on a device that populates `entAliasMappingTable`. The visible shape is the same as the documented `iosxr` limitation on the device-discovery side (a transceiver Module emitted without its interface backref), though the cause there is a location-string/interface-name mismatch rather than this routing gap. + +**Known false negatives.** The PID prefix list is not exhaustive. Real transceivers observed in captures whose PID it does not match, and are therefore not recognised as transceivers: `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 reach `container(5)` and `port(10)` rows as well as `module(9)` ones, so loosening them enough to catch these would risk matching a non-optic row too and manufacturing a bay that doesn't exist on the device. + +**Empty-bay harvest and nested containers.** A `container(5)` row with no `module(9)` or `port(10)` child underneath is harvested as an empty bay in `full` mode (see [Empty bays](./README.md#modules--modulebays) in the SNMP discovery README). That harvest used to mark only a bay's *nearest* `container(5)` ancestor as populated, so a container whose own children are themselves containers — never a module or port leaf directly — was reported as an empty bay even when everything beneath it was fully populated. On two Arista EOS captures this misreported three containers per device — the transceiver, fan-tray, and power-supply slot containers — as empty even though every cage beneath them held a populated module. The rule is now: a container is an empty bay only if nothing beneath it was emitted, implemented by walking up from every populated bay and marking each `container(5)` ancestor in turn. A genuinely empty slot is unaffected: it has no module beneath it at any depth, so it is still correctly reported as an empty bay. + +**Test fixtures.** With one exception, the unit fixtures behind these rules are transcribed from real SNMP simulator recordings rather than hand-authored: row class, parentage, `entPhysicalParentRelPos`, and which fields are populated all mirror the capture, because those are exactly the values the logic above reads. Two mirror separate Arista EOS fixed-port captures — one where every optic has a lane child, one where almost none do, which is the shape that would otherwise reach the empty-bay harvest. One mirrors a Cisco Catalyst 9404R capture with a `port(10)` optic nested inside a linecard's own cage. One mirrors a two-member stack whose optics are named with the literal token `port`. One mirrors a fixed-port device reporting no serial on any optic. The single exception is synthetic rather than transcribed: no captured device has been seen publishing a serial-less optic in a `container(5)` cage of its own, so that shape is built by hand to pin that the optic is emitted and claims its cage, leaving no empty bay behind for the same slot. + diff --git a/orb-discovery/snmp-discovery/mapping/module.go b/orb-discovery/snmp-discovery/mapping/module.go index 53744b62..69ffdf8f 100644 --- a/orb-discovery/snmp-discovery/mapping/module.go +++ b/orb-discovery/snmp-discovery/mapping/module.go @@ -8,6 +8,7 @@ package mapping import ( "context" "log/slog" + "regexp" "sort" "strconv" "strings" @@ -64,10 +65,11 @@ const ( ModuleTypeUnknown ModuleType = "unknown" ) -// ModuleEntry is one class=9 row from entPhysicalTable after -// classification. BayEntIndex points at the class=5 container that -// owns this module — the bay is emitted as a separate entity -// alongside the module installed in it. +// ModuleEntry is one entPhysical row after classification: a class=9 +// module, or a class=5/class=10 row whose PID identifies it as a +// transceiver (see isOpticPID). BayEntIndex points at the class=5 +// container that owns this module — the bay is emitted as a separate +// entity alongside the module installed in it. type ModuleEntry struct { EntIndex string // own entPhysicalIndex BayEntIndex string // parent class=5 container entPhysicalIndex @@ -86,9 +88,9 @@ type ModuleEntry struct { // ModuleInventory is the deduped, classified set for one target. // Modules carries top-level (chassis-rooted) modules; SubModules maps // each parent EntIndex → its transceiver children. EmptyBays carries -// class=5 rows with no class=9 child but whose parent resolves to a -// chassis or container — Aruba CX-style empty slots; emitted only in -// `full` mode. +// class=5 rows with no class=9/class=10 child of their own but whose +// parent resolves to a chassis or container — Aruba CX-style empty +// slots; emitted only in `full` mode. type ModuleInventory struct { Modules []ModuleEntry SubModules map[string][]ModuleEntry @@ -101,11 +103,123 @@ func newModuleInventory() ModuleInventory { } } -// Optic PID prefixes — pluggable transceivers across Cisco / generic -// vendors. Matched only when the row sits under a class=9 module -// parent; PID alone is insufficient (a chassis-level optic-shaped PID -// is treated as a linecard). -var opticPIDPrefixes = []string{"QSFP-", "SFP-", "X2-", "GLC-", "CFP-", "XENPAK-", "XFP-"} +// opticPIDPrefixes lists PID prefixes that identify a transceiver across +// vendors. An optic PID is sufficient on its own — a transceiver is a +// transceiver whether it sits under a linecard or directly in a fixed +// chassis port. +// +// These designators are MSA/SFF standardized, so the same set applies to every +// vendor and to every backend. Kept deliberately in step with +// device-discovery's _OPTIC_PREFIXES in custom_napalm/_modules.py: an optic +// recognised by one backend and not the other would make a device's inventory +// depend on how it happened to be discovered. QSFP-DD appears in that list but +// is omitted here because QSFP- already subsumes it; every other entry can +// match something QSFP-/SFP- cannot. +var opticPIDPrefixes = []string{ + "SFP-", "SFP+", "SFP28-", "SFP56-", + "QSFP-", "QSFP+", "QSFP28", "QSFP56-", + "QDD-", "OSFP-", + "GLC-", "X2-", "CFP-", "CFP2-", "XENPAK-", "XFP-", "CVR-", +} + +// isOpticPID reports whether a row's effective PID names a transceiver. +// Effective PID mirrors classifyModule: trimmed Model, falling back to +// trimmed VendorType. +func isOpticPID(model, vendorType string) bool { + pid := strings.TrimSpace(model) + if pid == "" { + pid = strings.TrimSpace(vendorType) + } + return hasOpticPIDPrefix(strings.ToUpper(pid)) +} + +// hasOpticPIDPrefix reports whether an already upper-cased effective PID +// carries a known optic vendor prefix. Shared by isOpticPID and +// classifyModule (which already has its own upper-cased PID in hand) so +// the prefix list is only ever walked in one place. +func hasOpticPIDPrefix(upper string) bool { + for _, p := range opticPIDPrefixes { + if strings.HasPrefix(upper, p) { + return true + } + } + return false +} + +// isOpticSubEntity reports whether a class=9 row describes part of an optic +// rather than a module. One vendor publishes such a row per lane beneath +// each transceiver: no model, no serial, an effective PID that is blank or +// the placeholder "0.0", and a parent that is itself an optic. Emitting +// these yields one bare module per lane, all sharing the optic's bay. +func isOpticSubEntity(r row, byIdx map[string]row) bool { + if strings.TrimSpace(r.Model) != "" || strings.TrimSpace(r.Serial) != "" { + return false + } + if pid := strings.TrimSpace(r.VendorType); pid != "" && pid != "0.0" { + return false + } + parent, ok := byIdx[r.ContainedIn] + return ok && isOpticPID(parent.Model, parent.VendorType) +} + +// opticDescrIfaceRe matches a transceiver row's descr where the vendor names +// the interface the optic serves. Anchored on purpose: the same platform +// publishes "Lane 0 for Xcvr for Ethernet1" beneath each optic, and an +// unanchored match would emit one bay per lane. +var opticDescrIfaceRe = regexp.MustCompile(`^Xcvr for (\S+)$`) + +// servedInterface returns the interface an optic row names, or "" when the +// row names none. An interface reference is positive evidence that the optic +// is installed in that port — a spare optic inventoried at chassis level +// does not name one. Absence of a module parent never authorises anything. +// pid is the row's own effective PID (see effectivePID) — passed through to +// ifaceShaped so a candidate that is really the optic's own part number, +// not an interface, is rejected on both the descr-derived and the +// name-derived path. +func servedInterface(name, descr, pid string) string { + if m := opticDescrIfaceRe.FindStringSubmatch(strings.TrimSpace(descr)); m != nil { + if ifaceShaped(m[1], pid) { + return m[1] + } + } + if n := strings.TrimSpace(name); ifaceShaped(n, pid) { + return n + } + return "" +} + +// ifaceShaped reports whether a token can be an interface name. A digit is +// required: one platform names every optic row with the bare word "port", +// which would otherwise name every bay on the chassis identically. A token +// equal to pid — the row's own effective PID, compared case-insensitively — +// is also rejected: a vendor that omits the "Xcvr for " descr may +// instead publish a product label such as "SFP-10G-LR" in entPhysicalName, +// and an interface is never named exactly its own transceiver part number. +func ifaceShaped(tok, pid string) bool { + if tok == "" || strings.ContainsAny(tok, " \t") { + return false + } + if !strings.ContainsAny(tok, "0123456789") { + return false + } + if pid != "" && strings.EqualFold(tok, pid) { + return false + } + return true +} + +// effectivePID returns the row's own PID for the "is this token really the +// optic's part number" check in ifaceShaped: trimmed Model, falling back to +// trimmed VendorType. Deliberately distinct from isOpticPID/classifyModule's +// upper-cased copies of the same rule — this one stays case-preserving +// because ifaceShaped's caller compares case-insensitively itself. +func effectivePID(model, vendorType string) string { + pid := strings.TrimSpace(model) + if pid == "" { + pid = strings.TrimSpace(vendorType) + } + return pid +} // classifyModule picks a ModuleType from a row's PID and its location // in the containment tree. hasModuleParent is true when an ancestor in @@ -131,16 +245,16 @@ func classifyModule(model, vendorType string, hasModuleParent bool) ModuleType { return ModuleTypeFan } - // Transceiver requires BOTH a module-class ancestor AND an optic - // PID — depth alone catches non-optic sub-modules; PID alone - // catches spare optics inventoried at chassis level. - if hasModuleParent { - for _, p := range opticPIDPrefixes { - if strings.HasPrefix(upper, p) { - return ModuleTypeTransceiver - } - } - } else if isSupervisorPID(upper) { + // An optic PID identifies a transceiver wherever it sits. Requiring a + // class=9 ancestor mistyped every optic on a fixed-port platform as a + // linecard, and a wrong type persists: Diode ingest never retracts. + // upper is already the effective PID computed above — reuse it + // instead of calling isOpticPID, which would recompute it from model + // and vendorType from scratch. + if hasOpticPIDPrefix(upper) { + return ModuleTypeTransceiver + } + if !hasModuleParent && isSupervisorPID(upper) { // Supervisor lives at chassis depth on dual-sup platforms. return ModuleTypeSupervisor } @@ -154,7 +268,23 @@ func classifyModule(model, vendorType string, hasModuleParent bool) ModuleType { return ModuleTypeLinecard } -// extractModuleInventory scans oids for class=9 entPhysical rows and +// row is one indexed entPhysical row, keyed by EntIndex in the byIdx map +// that extractModuleInventory builds. Package-scoped (rather than local to +// that function) so isOpticSubEntity can also walk it. +type row struct { + EntIndex string + ContainedIn string + Class string + ParentRel string + Name string + Serial string + Model string + Descr string + VendorType string +} + +// extractModuleInventory scans oids for class=9 entPhysical rows, plus +// class=5/class=10 rows whose PID identifies a transceiver, and // classifies each one. Drops orphans (broken containedIn chain) and // unclassifiable rows (class=1/2). Walks the containedIn chain to // determine the owning bay (class=5 ancestor) and whether the module @@ -167,17 +297,6 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn inv := newModuleInventory() // Index every entPhysical row by EntIndex so we can walk parents. - type row struct { - EntIndex string - ContainedIn string - Class string - ParentRel string - Name string - Serial string - Model string - Descr string - VendorType string - } byIdx := make(map[string]row) // trimSNMPString strips ENTITY-MIB-padded NULs and surrounding // whitespace. Applied to every string field at extraction so dedup @@ -274,32 +393,82 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn return bayIdx, parentModuleIdx, reachedChassis, true } - // Process class=9 rows in EntIndex-ascending order so dedup + // bayBelowModule reports whether the resolved bay sits beneath the given + // module. walkParents takes the nearest container and the nearest module + // independently, so a bay it returns may be the module's own slot rather + // than a cage inside the module. That distinction decides whether the bay + // already identifies a port. The same cycle guard as walkParents applies: + // a malformed containedIn chain must not loop. + bayBelowModule := func(bayIdx, moduleIdx string) bool { + if bayIdx == "" || moduleIdx == "" { + return false + } + seen := make(map[string]struct{}) + for cur := bayIdx; cur != "" && cur != "0"; { + if cur == moduleIdx { + return true + } + if _, dup := seen[cur]; dup { + return false + } + seen[cur] = struct{}{} + parent, exists := byIdx[cur] + if !exists { + return false + } + cur = parent.ContainedIn + } + return false + } + + // Process module-bay-shaped rows in EntIndex-ascending order so dedup // "first occurrence wins" is deterministic. ENTITY-MIB indexes are // numeric — a lex sort would put "10" before "9" and pick the wrong // dedup winner, so compare as integers with a lex tiebreaker for // any non-numeric edge cases. - classNineIdxs := make([]string, 0, len(byIdx)) + // + // Class 9 is the module class, but a transceiver is published as a + // container or as a port depending on vendor. Widen to those two + // classes for optic-PID rows only — a bare cage or a port row without + // an optic PID is not a module bay. + moduleIdxs := make([]string, 0, len(byIdx)) for _, r := range byIdx { - if r.Class == entPhysicalClassModule { - classNineIdxs = append(classNineIdxs, r.EntIndex) + switch r.Class { + case entPhysicalClassModule: + if isOpticSubEntity(r, byIdx) { + logger.Debug("module discovery: optic sub-entity skipped", + "ent", r.EntIndex, + "descr", r.Descr, + "reason", "optic_sub_entity") + continue + } + case entPhysicalClassContainer, entPhysicalClassPort: + if !isOpticPID(r.Model, r.VendorType) { + continue + } + default: + continue } + moduleIdxs = append(moduleIdxs, r.EntIndex) } - sort.Slice(classNineIdxs, func(i, j int) bool { - ai, errI := strconv.Atoi(classNineIdxs[i]) - aj, errJ := strconv.Atoi(classNineIdxs[j]) + sort.Slice(moduleIdxs, func(i, j int) bool { + ai, errI := strconv.Atoi(moduleIdxs[i]) + aj, errJ := strconv.Atoi(moduleIdxs[j]) if errI != nil || errJ != nil || ai == aj { - return classNineIdxs[i] < classNineIdxs[j] + return moduleIdxs[i] < moduleIdxs[j] } return ai < aj }) seenSerial := make(map[string]struct{}) - // bayHasChild tracks class=5 rows that gained at least one class=9 - // child — used by the empty-bay harvest below. + // bayHasChild tracks bay-shaped rows that gained at least one + // module child — used by the empty-bay harvest below. Usually keyed + // by the class=5 container, but a chassis-rooted module with no + // class=5 ancestor synthesizes its own EntIndex as the bay (see + // synthesizedBay below), so a key here is not always a class=5 row. bayHasChild := make(map[string]bool) - for _, idx := range classNineIdxs { + for _, idx := range moduleIdxs { r := byIdx[idx] bayIdx, parentModuleIdx, reachedChassis, chainOK := walkParents(r) if !chainOK { @@ -353,7 +522,6 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn } seenSerial[key] = struct{}{} } - bayHasChild[bayIdx] = true bay := byIdx[bayIdx] // Position is the BAY's parentRelPos (chassis slot), not the // module's own (which is almost always "1" inside its bay). @@ -384,6 +552,32 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn Type: classifyModule(r.Model, r.VendorType, parentModuleIdx != ""), ParentEntIdx: parentModuleIdx, } + // A fixed-port optic's bay is named for the interface the row + // names. A modular optic keeps the derivation from its real cage, + // which already identifies the port — but only when that cage is + // genuinely inside its module. An optic published directly under a + // linecard has no cage, and the nearest container is then the + // linecard's own slot: every optic on the card would resolve to + // that one bay name, colliding with the linecard's bay and losing + // all but the first to the duplicate-bay guard. Name those for the + // interface too. + pid := effectivePID(r.Model, r.VendorType) + if entry.Type == ModuleTypeTransceiver && + (parentModuleIdx == "" || !bayBelowModule(bayIdx, parentModuleIdx)) { + if iface := servedInterface(r.Name, r.Descr, pid); iface != "" { + entry.BayName = iface + entry.BayPosition = iface + } + } + // A blank serial is not a reason to drop an optic. dcim.module is + // matched on its module bay (unique_module_bay) and has no serial + // matcher, NetBox leaves dcim.Module.serial blank-able and in no + // constraint, and emitModule omits the field entirely when blank so + // a repoll updates the same object rather than creating another. + // See the ModuleBay note in stubs.go. Vendors that publish optics + // without a serial are common rather than exceptional, so gating on + // one would discard inventory that reconciles perfectly well. + bayHasChild[bayIdx] = true if parentModuleIdx == "" { inv.Modules = append(inv.Modules, entry) } else { @@ -396,6 +590,55 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn return inv.Modules[i].EntIndex < inv.Modules[j].EntIndex }) + // bayHasChild above only marks a module's own (nearest) class=5 bay. + // A container whose children are themselves containers — never a + // class=9/10 leaf directly — stays unmarked even when a module several + // containers below it is fully populated, and the empty-bay harvest + // below would then wrongly harvest it as empty. Propagate "has a + // child" upward through every container ancestor so the harvest's + // invariant holds: a container is an empty bay only if nothing + // beneath it was emitted. + // + // Snapshot the already-marked keys first — ranging a map while adding + // new keys to it is undefined for the keys added during the range. + markedBays := make([]string, 0, len(bayHasChild)) + for idx := range bayHasChild { + markedBays = append(markedBays, idx) + } + // Generous bound on how many container levels to climb — real + // ENTITY-MIB trees are a handful of levels deep at most; this only + // guards against a pathological/malformed capture. + const maxContainmentWalkDepth = 64 + for _, idx := range markedBays { + cur := byIdx[idx].ContainedIn + seen := make(map[string]struct{}) + for depth := 0; cur != "" && cur != "0" && depth < maxContainmentWalkDepth; depth++ { + if _, dup := seen[cur]; dup { + logger.Debug("module: containment cycle detected during bay propagation", + "from", idx, "at", cur) + break + } + seen[cur] = struct{}{} + parent, exists := byIdx[cur] + if !exists || parent.Class != entPhysicalClassContainer { + // Stop at the first non-container ancestor. A class=5 row + // that directly hosts an emitted class=9 module is + // already marked by the normal path above (that module's + // bay IS this row); the only gap this closes is a + // container whose children are all containers. + break + } + if bayHasChild[cur] { + // Already marked — its own ancestors were marked on an + // earlier pass through this same loop, so stop rather + // than re-walking ground already covered. + break + } + bayHasChild[cur] = true + cur = parent.ContainedIn + } + } + // Empty-bay harvest. A class=5 row whose parent is a chassis or // another container and which has no class=9 child is an empty // slot — emitted as a bare bay in `full` mode (Aruba CX quirk). @@ -429,6 +672,14 @@ func extractModuleInventory(oids ObjectIDValueMap, logger *slog.Logger) ModuleIn continue } r := byIdx[idx] + // An optic row is not an empty bay. It is inventory in its own + // right, emitted as a module by the scan above; harvesting it as + // well emits the same physical part twice, the second time with + // its model and serial dropped and its bay named from the optic's + // own position — which is identical on every port. + if isOpticPID(r.Model, r.VendorType) { + continue + } parent, exists := byIdx[r.ContainedIn] if !exists { continue diff --git a/orb-discovery/snmp-discovery/mapping/module_optics_fixtures_test.go b/orb-discovery/snmp-discovery/mapping/module_optics_fixtures_test.go new file mode 100644 index 00000000..f5866776 --- /dev/null +++ b/orb-discovery/snmp-discovery/mapping/module_optics_fixtures_test.go @@ -0,0 +1,265 @@ +// Copyright 2026 NetBox Labs, Inc. + +package mapping + +// Optic fixtures transcribed from SNMP simulator recordings. Row class, +// parentage, relPos and which fields are empty all mirror the capture — +// those are the values the logic reads. Optic counts are reduced to three +// per fixture for readability. + +// fixedPortLaneShapeFixture mirrors a fixed-port switch where each optic is a +// class-5 row carrying the PID and serial with an EMPTY Name, sitting in a +// per-port class-5 cage. Beneath each optic sit two DOM sensors and one +// class-9 lane row with no model, no serial and vendorType "0.0". Every +// class-9 row on this platform is a lane. +func fixedPortLaneShapeFixture() []fixtureRow { + rows := []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "JPE14030001", "DCS-7050SX-64", "64-port switch chassis", ""}, + {"1100300000", "1", "5", "3", "", "", "", "Xcvr Slot Container", ""}, + } + for _, p := range []struct{ n, cage, optic, lane, sensor1, sensor2, serial string }{ + {"1", "100301000", "100301100", "100301210", "100301201", "100301202", "G1904016438"}, + {"2", "100302000", "100302100", "100302210", "100302201", "100302202", "G1904016436"}, + {"3", "100303000", "100303100", "100303210", "100303201", "100303202", "G1904016445"}, + } { + rows = append(rows, + // Cage: class 5, no PID, empty Name, relPos is the port number. + fixtureRow{p.cage, "1100300000", "5", p.n, "", "", "", "Xcvr Slot " + p.n, ""}, + // Optic: class 5, PID and serial present, Name EMPTY, relPos 1. + fixtureRow{p.optic, p.cage, "5", "1", "", p.serial, "SFP-10GLR-31", "Xcvr for Ethernet" + p.n, ""}, + // DOM sensors: class 8, never gathered. + fixtureRow{p.sensor1, p.optic, "8", "1", "", "", "", "DOM Temperature Sensor for Ethernet" + p.n, "0.0"}, + fixtureRow{p.sensor2, p.optic, "8", "2", "", "", "", "DOM Voltage Sensor for Ethernet" + p.n, "0.0"}, + // Lane: class 9, no model, no serial, vendorType "0.0". + fixtureRow{p.lane, p.optic, "9", "0", "", "", "", "Lane 0 for Xcvr for Ethernet" + p.n, "0.0"}, + ) + } + return rows +} + +// fixedPortHarvestShapeFixture mirrors the same vendor's other capture, where +// most optics have NO lane child. Such a row has neither a class-9 nor a +// class-10 child, so it reaches the empty-bay harvest today. +func fixedPortHarvestShapeFixture() []fixtureRow { + rows := []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "JPE14030002", "DCS-7050SX-64", "64-port switch chassis", ""}, + {"1100300000", "1", "5", "3", "", "", "", "Xcvr Slot Container", ""}, + } + for _, p := range []struct{ n, cage, optic, serial string }{ + {"1", "100301000", "100301100", "XMD1447522PK"}, + {"2", "100302000", "100302100", "XMD14475233E"}, + {"3", "100303000", "100303100", "XMD14475233F"}, + } { + rows = append(rows, + fixtureRow{p.cage, "1100300000", "5", p.n, "", "", "", "Xcvr Slot " + p.n, ""}, + fixtureRow{p.optic, p.cage, "5", "1", "", p.serial, "SFP-1G-T", "Xcvr for Ethernet" + p.n, ""}, + ) + } + return rows +} + +// modularPortOpticFixture mirrors a modular chassis where the optic is a +// class-10 row inside a class-5 port cage inside a class-9 linecard. The +// second optic's PID is a real transceiver the prefix list does not match, +// pinning that documented gap. +func modularPortOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "FXS2130Q0MZ", "C9404R", "4-slot chassis", ""}, + {"6", "1", "5", "2", "Slot 2", "", "", "Slot 2 Container", ""}, + {"2000", "6", "9", "2", "Slot 2 Linecard", "JAE23140BJH", "C9400-LC-48UX", "48-port line card", ""}, + {"2061", "2000", "5", "1", "Te2/0/1 Container", "", "", "TenGigabitEthernet2/0/1 Container", "1.3.6.1.4.1.9.12.3.1.5.115"}, + {"2072", "2061", "10", "0", "TenGigabitEthernet2/0/1", "A1111111111-A", "SFP-10G-AOC2M", "10G AOC2M", ""}, + {"2062", "2000", "5", "2", "Te2/0/2 Container", "", "", "TenGigabitEthernet2/0/2 Container", "1.3.6.1.4.1.9.12.3.1.5.115"}, + {"2078", "2062", "10", "0", "TenGigabitEthernet2/0/2", "AGM11111111", "ABCU-5710RZ-CS5", "GE T", ""}, + } +} + +// stackedPortOpticFixture mirrors a two-member stack whose optics are +// class-10 rows named with the literal token "port", under a class-9 member +// module. Both members report relPos 25 and 26, so both synthesize bays +// "Slot 25" and "Slot 26" — distinct objects only because each carries its +// own Device. +func stackedPortOpticFixture() []fixtureRow { + return []fixtureRow{ + {"569", "0", "3", "1", "Chassis", "K3080012", "OS6350-P24", "24-port stackable chassis", ""}, + {"570", "0", "3", "2", "Chassis", "K3080013", "OS6450-24", "24-port stackable chassis", ""}, + {"1", "569", "9", "1", "NI-1", "K3080012", "OS6350-P24", "Network interface module 1", ""}, + {"2", "570", "9", "2", "NI-2", "K3080013", "OS6450-24", "Network interface module 2", ""}, + {"156", "1", "10", "25", "port", "19480134", "SFP-10G-T", "SFP-10G-T", ""}, + {"157", "1", "10", "26", "port", "C1404140546", "SFP-10G-C3M", "SFP-10G-C3M", ""}, + {"211", "2", "10", "25", "port", "19480215", "SFP-10G-T", "SFP-10G-T", ""}, + {"212", "2", "10", "26", "port", "GC22007671", "SFP-10G-LR", "SFP-10G-LR", ""}, + } +} + +// serialFreeCagedOpticFixture is synthetic, not transcribed from a capture — +// no captured device has shown this exact shape. It pins that a class-5 optic +// with no serial is still emitted, named for the interface its descr states, +// and that it claims its cage: the cage holds a module now, so it must not +// also surface as an empty bay. +func serialFreeCagedOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0001", "SYN-CHASSIS", "Synthetic chassis", ""}, + // Cage: class 5, no PID, no serial, directly under the chassis. + {"100", "1", "5", "1", "Cage 1", "", "", "Cage Slot 1", ""}, + // Optic: class 5, PID present, serial ABSENT. + {"101", "100", "5", "1", "", "", "SFP-10GLR-31", "Xcvr for Ethernet1", ""}, + } +} + +// serialFreeCagedPortOpticFixture mirrors the class-10-in-a-cage topology +// real captures actually show (the same cage+port nesting as +// modularPortOpticFixture) with the serial field blank. The optic is emitted +// and named for its interface; the cage produces no separate bay because +// containerHasPortChild suppresses a container whose child is a port row. +func serialFreeCagedPortOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0005", "SYN-CHASSIS5", "Synthetic chassis", ""}, + // Cage: class 5, no PID, no serial, directly under the chassis. + {"110", "1", "5", "1", "Te1/0/1 Container", "", "", "TenGigabitEthernet1/0/1 Container", ""}, + // Optic: class 10, PID present, serial ABSENT. + {"111", "110", "10", "0", "TenGigabitEthernet1/0/1", "", "SFP-10G-LR", "SFP-10GBase-LR", ""}, + } +} + +// serialFreePortOpticFixture mirrors a platform publishing class-10 optics +// directly under the chassis with NO serial and relPos -1 on every row — +// transcribed from a capture that publishes 25 of them. relPos alone would +// name every bay "Slot -1", but each row carries its own interface name and +// the transceiver bay-naming rule runs before emission, so the bays come out +// distinct. A blank serial does not prevent any of that. +func serialFreePortOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "EC2140004", "DCS203", "Fixed-port switch chassis", ""}, + {"1000000100", "1", "10", "-1", "Ethernet0", "", "SFP-10GSR-85", "SFP/SFP+/SFP28 for Eth6/1(Port1)", ""}, + {"1000000200", "1", "10", "-1", "Ethernet1", "", "SFP-10GSR-85", "SFP/SFP+/SFP28 for Eth6/2(Port1)", ""}, + {"1000000300", "1", "10", "-1", "Ethernet2", "", "SFP-10GSR-85", "SFP/SFP+/SFP28 for Eth6/3(Port1)", ""}, + } +} + +// serialFreeModuleOpticFixture is modeled on a confirmed regression rather +// than transcribed byte-for-byte from a capture: a real device in the +// corpus publishes a class-9 optic beneath a linecard with a blank +// entPhysicalSerialNum. That row was discoverable before the widened +// container/port scan — classified as a transceiver, nested under its +// linecard, and emitted with Serial left unset (emitModule already +// tolerates a blank Serial). Blank-serial optics are emitted in every class, +// and this is the shape that proves the oldest of them still is. +func serialFreeModuleOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0009", "SYN-CHASSIS9", "Synthetic chassis", ""}, + {"10", "1", "5", "1", "Slot 1", "", "", "", ""}, + {"100", "10", "9", "1", "Linecard", "SYNLC00002", "C9400-LC-48U", "", ""}, + // Port cage under the linecard. + {"101", "100", "5", "1", "Te1/0/1 Container", "", "", "", ""}, + // Optic: class 9 (module-shaped), PID present, serial ABSENT. + {"102", "101", "9", "1", "Te1/0/1 Transceiver", "", "GLC-LH-SMD", "", ""}, + } +} + +// duplicateBayNameOpticFixture is synthetic, not transcribed from a +// capture — no captured device in the corpus has shown two fixed-port +// transceivers collide on the same bay name. It exists to exercise the +// duplicate-bay-name guard: two class-10 optics sit directly under the +// chassis, each with a blank Name, a Descr that names no interface, and +// a blank relPos, so both fall all the way through servedInterface and +// emitModuleBay's fallback chain to the literal placeholder "Unknown" — +// the same effective bay name, on the same member. Each carries its own +// serial so neither is dropped by the missing- or duplicate-serial +// guards before reaching the bay-name check. +func duplicateBayNameOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0004", "SYN-CHASSIS4", "Synthetic chassis", ""}, + {"10", "1", "10", "", "", "SYNSER0001A", "SFP-10G-LR", "", ""}, + {"11", "1", "10", "", "", "SYNSER0001B", "SFP-10G-LR", "", ""}, + } +} + +// duplicateModularBayNameOpticFixture is synthetic, not transcribed from a +// capture — no captured device in the corpus has shown two modular optics +// resolve to the same cage-derived bay name. It exercises the +// duplicate-bay-name guard's submodule-loop coverage: two linecards under +// one chassis each carry a port cage that happens to be named identically +// ("Te1/0/1 Container"), each holding its own class-10 optic. Both optics +// land in inv.SubModules — keyed by their own linecard's EntIndex, never +// in inv.Modules — so this collision is unreachable through the top-level +// loop alone and would go uncaught without the guard's submodule coverage. +func duplicateModularBayNameOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0006", "SYN-CHASSIS6", "Synthetic chassis", ""}, + // Linecard A in slot 1, with a port cage "Te1/0/1 Container". + {"10", "1", "5", "1", "Slot 1", "", "", "", ""}, + {"100", "10", "9", "1", "Linecard A", "SYNLCA0001", "C9400-LC-48U", "", ""}, + {"101", "100", "5", "1", "Te1/0/1 Container", "", "", "", ""}, + {"102", "101", "10", "0", "", "SYNSERBAYA", "SFP-10G-LR", "", ""}, + // Linecard B in slot 2 — its port cage collides on the exact same + // name as Linecard A's, even though the two optics sit under + // different parent linecards. + {"20", "1", "5", "2", "Slot 2", "", "", "", ""}, + {"200", "20", "9", "2", "Linecard B", "SYNLCB0001", "C9400-LC-48U", "", ""}, + {"201", "200", "5", "1", "Te1/0/1 Container", "", "", "", ""}, + {"202", "201", "10", "0", "", "SYNSERBAYB", "SFP-10G-LR", "", ""}, + } +} + +// crossTierDuplicateBayNameOpticFixture is synthetic, not transcribed from +// a capture — no captured device in the corpus has shown a top-level +// fixed-port optic collide with a modular optic on the same device. It +// exists to prove the duplicate-bay-name guard's design point: the +// top-level loop and the submodule loop must share ONE seenTransceiverBays +// map. A fixed-port optic's bay resolves to "Ethernet1" via servedInterface +// (same shape as fixedPortLaneShapeFixture); a modular optic under a +// separate linecard sits in a port cage that is itself literally named +// "Ethernet1". The two are the same effective (device, bay name) pair even +// though one is device-rooted at the top level and the other nests under +// a linecard — a second, per-loop map would miss this case entirely. +func crossTierDuplicateBayNameOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0007", "SYN-CHASSIS7", "Synthetic chassis", ""}, + // Top-level fixed-port optic: class-5 cage directly under the + // chassis, optic names its interface via Descr. + {"10", "1", "5", "1", "Xcvr Slot 1", "", "", "", ""}, + {"11", "10", "5", "1", "", "SYNSERTOP1", "SFP-10G-LR", "Xcvr for Ethernet1", ""}, + // Modular optic under a linecard: the port cage is itself literally + // named "Ethernet1" — the same effective bay name the fixed-port + // optic above resolves to via servedInterface. + {"20", "1", "5", "2", "Slot 2", "", "", "", ""}, + {"200", "20", "9", "2", "Linecard", "SYNLC00001", "C9400-LC-48U", "", ""}, + {"201", "200", "5", "1", "Ethernet1", "", "", "", ""}, + {"202", "201", "10", "0", "", "SYNSERSUB1", "SFP-10G-LR", "", ""}, + } +} + +// opticNameEqualsOwnPIDOpticFixture is synthetic, not transcribed from a +// capture — no captured device in the corpus has shown entPhysicalName +// hold a bare product label. It pins Fix 2: without a "Xcvr for " +// descr to resolve the served interface, and with entPhysicalName holding +// the optic's own PID rather than an interface name, the bay must fall +// back to the cage-derived name rather than adopt the PID itself — an +// interface is never named exactly its own transceiver part number. +func opticNameEqualsOwnPIDOpticFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "SYN0008", "SYN-CHASSIS8", "Synthetic chassis", ""}, + {"10", "1", "5", "1", "Te1/0/1 Container", "", "", "", ""}, + {"11", "10", "10", "0", "SFP-10G-LR", "SYNSERPID1", "SFP-10G-LR", "", ""}, + } +} + +// cagelessLinecardOpticsFixture is synthetic, not transcribed from a capture — +// no captured device has shown this exact shape, and a corpus scan of the +// ENTITY-MIB recordings found none. It exists to pin an invariant that the +// class-5/10 widening otherwise leaves exposed: optics published directly +// under a slotted linecard, with no cage container between them, must not all +// resolve to the linecard's own slot. The real modular shape puts a class-5 +// cage under the card (see modularPortOpticFixture); this one omits it, which +// is what makes the nearest container the card's slot instead. +func cagelessLinecardOpticsFixture() []fixtureRow { + return []fixtureRow{ + {"1", "0", "3", "1", "Chassis", "FXS2130Q0MZ", "C9404R", "4-slot chassis", ""}, + {"6", "1", "5", "2", "Slot 2", "", "", "Slot 2 Container", ""}, + {"2000", "6", "9", "2", "Slot 2 Linecard", "JAE23140BJH", "C9400-LC-48UX", "48-port line card", ""}, + {"2072", "2000", "10", "1", "TenGigabitEthernet2/0/1", "A1111111111-A", "SFP-10G-LR", "10G LR", ""}, + {"2073", "2000", "10", "2", "TenGigabitEthernet2/0/2", "A2222222222-A", "SFP-10G-LR", "10G LR", ""}, + {"2074", "2000", "10", "3", "TenGigabitEthernet2/0/3", "A3333333333-A", "SFP-10G-LR", "10G LR", ""}, + } +} diff --git a/orb-discovery/snmp-discovery/mapping/module_optics_test.go b/orb-discovery/snmp-discovery/mapping/module_optics_test.go new file mode 100644 index 00000000..1a0b0aa4 --- /dev/null +++ b/orb-discovery/snmp-discovery/mapping/module_optics_test.go @@ -0,0 +1,621 @@ +// Copyright 2026 NetBox Labs, Inc. + +package mapping + +import ( + "bytes" + "log/slog" + "os" + "strings" + "testing" + + "github.com/netboxlabs/diode-sdk-go/diode" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testOpticLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(os.Stderr, nil)) +} + +// TestOpticDiscovery_LaneRowsNotEmittedAsModules asserts the per-lane +// sub-entities published beneath each optic are not modules or submodules. +// Every class-9 row on this platform is such a lane; the optics themselves +// are the class-5 containers, covered separately by +// TestOpticDiscovery_ContainerShapeEmitted. +func TestOpticDiscovery_LaneRowsNotEmittedAsModules(t *testing.T) { + inv := extractModuleInventory(buildOIDs(fixedPortLaneShapeFixture()), testOpticLogger()) + + laneIdxs := []string{"100301210", "100302210", "100303210"} + for _, m := range inv.Modules { + assert.NotContains(t, laneIdxs, m.EntIndex, "lane row emitted as a module") + } + for _, subs := range inv.SubModules { + for _, m := range subs { + assert.NotContains(t, laneIdxs, m.EntIndex, "lane row emitted as a submodule") + } + } +} + +// TestOpticDiscovery_OpticRowsNeverHarvestedAsEmptyBays asserts an optic row +// is never emitted as a bare bay. It is inventory in its own right, and the +// harvest drops the model and serial while naming the bay from the optic's +// own position — which is identical across every port on these platforms. +// On both fixtures every cage and the slot container above it are populated, +// so the expected outcome is that the harvest produces nothing at all — a +// stronger statement than "the optic indices specifically are absent", which +// per-index checks below also confirm. +// +// This must hold on both shapes: one has a lane child beneath every optic, +// the other beneath almost none, and suppressing lanes is what exposes the +// first shape to the harvest in the first place. +func TestOpticDiscovery_OpticRowsNeverHarvestedAsEmptyBays(t *testing.T) { + opticIdxs := []string{"100301100", "100302100", "100303100"} + + for name, fixture := range map[string][]fixtureRow{ + "lane shape": fixedPortLaneShapeFixture(), + "harvest shape": fixedPortHarvestShapeFixture(), + } { + t.Run(name, func(t *testing.T) { + inv := extractModuleInventory(buildOIDs(fixture), testOpticLogger()) + assert.Empty(t, inv.EmptyBays, + "every cage and its slot container are populated; the harvest should produce nothing") + for _, b := range inv.EmptyBays { + assert.NotContains(t, opticIdxs, b.EntIndex, + "optic row %s emitted as an empty bay", b.EntIndex) + } + }) + } +} + +// TestOpticDiscovery_ContainerOfContainersNotHarvestedAsEmptyBay asserts a +// class-5 container whose own children are themselves class-5 containers +// (the per-port cages, never a class-9/10 leaf directly) is not harvested +// as an empty bay when something beneath it was emitted. Without upward +// propagation, bayHasChild only marks the nearest bay — the cage — so the +// slot container that holds all three cages would look empty even though +// every cage beneath it holds a populated, serialed optic. Every cage on +// this fixture is populated, so the expected outcome is no empty bays at +// all, not merely the absence of the slot container specifically. +func TestOpticDiscovery_ContainerOfContainersNotHarvestedAsEmptyBay(t *testing.T) { + inv := extractModuleInventory(buildOIDs(fixedPortLaneShapeFixture()), testOpticLogger()) + + assert.Empty(t, inv.EmptyBays, + "every cage beneath the slot container is populated; the harvest should produce nothing") + for _, b := range inv.EmptyBays { + assert.NotEqual(t, "1100300000", b.EntIndex, + "the populated slot container must not be harvested as an empty bay") + } +} + +// TestOpticDiscovery_ContainerShapeEmitted asserts a class-5 row carrying an +// optic PID is discovered as a transceiver rather than ignored. +func TestOpticDiscovery_ContainerShapeEmitted(t *testing.T) { + inv := extractModuleInventory(buildOIDs(fixedPortLaneShapeFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 3, "one module per optic") + for _, m := range inv.Modules { + assert.Equal(t, ModuleTypeTransceiver, m.Type) + assert.Equal(t, "SFP-10GLR-31", m.Model) + assert.NotEmpty(t, m.Serial, "the optic's own serial must survive") + } +} + +// TestOpticDiscovery_ModularPortShapeNestsUnderLinecard asserts a class-10 +// optic inside a linecard nests as a submodule with the bay taken from its +// real cage. It must not become a device-rooted bay. +func TestOpticDiscovery_ModularPortShapeNestsUnderLinecard(t *testing.T) { + inv := extractModuleInventory(buildOIDs(modularPortOpticFixture()), testOpticLogger()) + + subs := inv.SubModules["2000"] + require.Len(t, subs, 1, "exactly one optic nests under the linecard") + assert.Equal(t, ModuleTypeTransceiver, subs[0].Type) + assert.Equal(t, "SFP-10G-AOC2M", subs[0].Model) + assert.Equal(t, "Te2/0/1 Container", subs[0].BayName, + "bay comes from the optic's real class-5 cage") + + // The prefix list does not match this PID even though it is a real + // transceiver. Pinning it makes any future prefix change a visible + // decision rather than a surprise. + for _, m := range subs { + assert.NotEqual(t, "ABCU-5710RZ-CS5", m.Model) + } + // The linecard itself is legitimately device-rooted; the optic must + // not be — no transceiver belongs in the top-level list. + for _, m := range inv.Modules { + assert.NotEqual(t, ModuleTypeTransceiver, m.Type, "no optic here is device-rooted") + } +} + +// TestOpticDiscovery_NonOpticContainersStayIgnored is the discriminator for +// the widening: PID-less class-5 cages must not become modules. +func TestOpticDiscovery_NonOpticContainersStayIgnored(t *testing.T) { + inv := extractModuleInventory(buildOIDs(chassis9404RWithTransceiversFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 2, "supervisor and linecard only") + for _, m := range inv.Modules { + assert.NotEqual(t, "Slot 1", m.Name) + assert.NotEqual(t, "Slot 2", m.Name) + assert.NotEqual(t, "TenGigabitEthernet2/0/1", m.Name) + } +} + +// TestOpticDiscovery_BayNamedForServedInterface asserts a fixed-port optic's +// bay is named for the interface the row itself names. Without it the bay is +// the cage's bare position number, which does not identify the port. +func TestOpticDiscovery_BayNamedForServedInterface(t *testing.T) { + inv := extractModuleInventory(buildOIDs(fixedPortLaneShapeFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 3) + bays := make(map[string]bool, 3) + for _, m := range inv.Modules { + bays[m.BayName] = true + } + for _, want := range []string{"Ethernet1", "Ethernet2", "Ethernet3"} { + assert.True(t, bays[want], "expected a bay named %s", want) + } +} + +// TestOpticDiscovery_LaneDescrNeverNamesABay pins the anchoring. A lane row +// descr contains "Xcvr for Ethernet1" as a substring, and an unanchored +// match would emit one bay per lane for a single physical optic. +func TestOpticDiscovery_LaneDescrNeverNamesABay(t *testing.T) { + assert.Equal(t, "Ethernet1", servedInterface("", "Xcvr for Ethernet1", "")) + assert.Empty(t, servedInterface("", "Lane 0 for Xcvr for Ethernet1", "")) + assert.Empty(t, servedInterface("", "Xcvr Slot 1", "")) +} + +// TestOpticDiscovery_NonInterfaceNameRejected pins the digit requirement. +// One platform names every optic row with the literal token "port"; +// accepting it would name every bay on the chassis identically and merge +// them into one object. +func TestOpticDiscovery_NonInterfaceNameRejected(t *testing.T) { + assert.Empty(t, servedInterface("port", "", "")) + assert.Empty(t, servedInterface("SFP cage", "", "")) + assert.Equal(t, "Ethernet0", servedInterface("Ethernet0", "", "")) +} + +// TestOpticDiscovery_NameEqualToOwnPIDRejected pins Fix 2: a name-derived +// candidate that is really the row's own transceiver part number, not an +// interface, must be rejected — compared case-insensitively, since vendors +// are not consistent about PID casing. +func TestOpticDiscovery_NameEqualToOwnPIDRejected(t *testing.T) { + assert.Empty(t, servedInterface("SFP-10G-LR", "", "SFP-10G-LR")) + assert.Empty(t, servedInterface("sfp-10g-lr", "", "SFP-10G-LR"), "case-insensitive") + assert.Equal(t, "Ethernet1", servedInterface("Ethernet1", "", "SFP-10G-LR"), + "a real interface name must still be accepted") +} + +// TestOpticDiscovery_StackedMembersKeepDistinctBays asserts that when two +// stack members each report an optic at the same position, all four bays +// survive. They are distinct NetBox objects because each carries its own +// Device; the extractor must not collapse them, and the literal token +// "port" must never become a bay name. +func TestOpticDiscovery_StackedMembersKeepDistinctBays(t *testing.T) { + inv := extractModuleInventory(buildOIDs(stackedPortOpticFixture()), testOpticLogger()) + + var count int + for _, subs := range inv.SubModules { + for _, m := range subs { + count++ + assert.NotEqual(t, "port", m.BayName, "the literal token must never name a bay") + } + } + assert.Equal(t, 4, count, "two optics on each of two members") +} + +// TestOpticDiscovery_SerialFreeOpticsEmittedWithDistinctBays covers the shape +// a real capture publishes 25 times: class-10 optics directly under the +// chassis, no serial, relPos -1 on every row. A blank serial is no reason to +// drop them — dcim.module matches on its bay, not on serial. relPos would name +// every bay "Slot -1", but the bay-naming rule runs first and each row names +// its own interface, so the bays are distinct and nothing collides. +func TestOpticDiscovery_SerialFreeOpticsEmittedWithDistinctBays(t *testing.T) { + inv := extractModuleInventory(buildOIDs(serialFreePortOpticFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 3, "a blank serial must not cost an optic its module") + + seen := make(map[string]string, len(inv.Modules)) + for _, m := range inv.Modules { + assert.Equal(t, ModuleTypeTransceiver, m.Type) + assert.Empty(t, m.Serial, "the serial really is absent in this capture") + assert.Equal(t, m.Name, m.BayName, "named for the interface the row names") + assert.NotEqual(t, "Slot -1", m.BayName, "relPos must not name the bay here") + if prev, dup := seen[m.BayName]; dup { + t.Fatalf("bay %q shared by ent %s and ent %s", m.BayName, prev, m.EntIndex) + } + seen[m.BayName] = m.EntIndex + } + assert.Len(t, seen, 3, "three optics, three distinct bays") +} + +// TestOpticDiscovery_SerialFreeCagedOpticClaimsItsCage asserts a class-5 optic +// with no serial is emitted and takes its cage with it. The cage holds a module +// once the optic is emitted, so it must not also surface as an empty bay — that +// would double-count one physical slot. +func TestOpticDiscovery_SerialFreeCagedOpticClaimsItsCage(t *testing.T) { + inv := extractModuleInventory(buildOIDs(serialFreeCagedOpticFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 1, "the serial-free optic is emitted") + assert.Equal(t, ModuleTypeTransceiver, inv.Modules[0].Type) + assert.Equal(t, "Ethernet1", inv.Modules[0].BayName, "named from the descr's served interface") + assert.Empty(t, inv.Modules[0].Serial) + assert.Empty(t, inv.EmptyBays, "the cage now holds a module, so it is not an empty bay") +} + +// TestOpticDiscovery_SerialFreeCagedPortOpticEmitted covers the class-10-in-a- +// cage shape real captures show, with the serial blank. The optic is emitted and +// named for its interface. The cage produces no bay of its own, empty or +// otherwise, because containerHasPortChild suppresses a container whose child is +// a port row — that suppression is unrelated to the serial. +func TestOpticDiscovery_SerialFreeCagedPortOpticEmitted(t *testing.T) { + inv := extractModuleInventory(buildOIDs(serialFreeCagedPortOpticFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 1, "the serial-free optic is emitted") + assert.Equal(t, ModuleTypeTransceiver, inv.Modules[0].Type) + assert.Equal(t, "TenGigabitEthernet1/0/1", inv.Modules[0].BayName) + assert.Empty(t, inv.Modules[0].Serial) + assert.Empty(t, inv.EmptyBays, "containerHasPortChild suppresses the cage") +} + +// TestOpticDiscovery_ClassNineSerialFreeOpticStillEmitted pins the oldest of +// the blank-serial shapes: a class-9 optic beneath a linecard, discoverable +// since before the container/port scan was widened. It asserts full emission +// rather than mere absence-of-drop — the transceiver surfaces as a submodule +// ModuleBay + Module with Serial left unset, which is what emitModule's +// tolerance of a blank Serial is for. +func TestOpticDiscovery_ClassNineSerialFreeOpticStillEmitted(t *testing.T) { + logger := testOpticLogger() + + oids := buildOIDs(serialFreeModuleOpticFixture()) + dev := &diode.Device{Name: strPtr("test-router")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, logger) + + var transceiverMod *diode.Module + for _, e := range entities { + m, ok := e.(*diode.Module) + if !ok || m.ModuleType == nil || m.ModuleType.Model == nil { + continue + } + if *m.ModuleType.Model == "GLC-LH-SMD" { + transceiverMod = m + } + } + require.NotNil(t, transceiverMod, "the class-9 serial-free optic must still be emitted") + require.NotNil(t, transceiverMod.ModuleBay, "the transceiver must carry its submodule bay") + assert.Nil(t, transceiverMod.Serial, "serial stays unset, matching emitModule's existing tolerance") +} + +// TestOpticDiscovery_LinecardsModeEmitsNoFixedPortOptic asserts a fixed-port +// optic stays out of linecards mode. A modular optic is already excluded by +// the full-mode gate because it lives in SubModules; a fixed-port optic is a +// top-level module and needs its own exclusion. +func TestOpticDiscovery_LinecardsModeEmitsNoFixedPortOptic(t *testing.T) { + oids := buildOIDs(fixedPortLaneShapeFixture()) + dev := &diode.Device{Name: strPtr("test-switch")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeLinecards(), nil, testOpticLogger()) + + for _, e := range entities { + mod, ok := e.(*diode.Module) + if !ok || mod.ModuleType == nil || mod.ModuleType.Model == nil { + continue + } + assert.NotEqual(t, "SFP-10GLR-31", *mod.ModuleType.Model, + "linecards mode must not emit a transceiver") + } +} + +// TestOpticDiscovery_FullModeEmitsInterfaceNamedBays asserts the emitted +// payload carries exactly one bay per optic, named for the interface it +// serves, each with its own serial. Asserting the exact set — rather than +// each wanted name plus a denylist of broken values — pins the whole +// outcome in one assertion: no container bay, no bare-number bay, no +// "Unknown" placeholder, regardless of what position value the container +// happens to report. +func TestOpticDiscovery_FullModeEmitsInterfaceNamedBays(t *testing.T) { + oids := buildOIDs(fixedPortLaneShapeFixture()) + dev := &diode.Device{Name: strPtr("test-switch")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, testOpticLogger()) + + var bayNames []string + serials := make(map[string]bool) + for _, e := range entities { + switch v := e.(type) { + case *diode.ModuleBay: + require.NotNil(t, v.Name) + bayNames = append(bayNames, *v.Name) + case *diode.Module: + if v.Serial != nil && *v.Serial != "" { + serials[*v.Serial] = true + } + } + } + + assert.ElementsMatch(t, []string{"Ethernet1", "Ethernet2", "Ethernet3"}, bayNames, + "expected exactly one bay per optic, named for its served interface") + assert.Len(t, serials, 3, "each optic contributes its own serial") +} + +// TestOpticDiscovery_DuplicateBayNameDropped asserts the defensive guard +// against two module bays merging on one device: dcim.modulebay matches on +// name+device, so two transceivers landing on the same member with the same +// effective bay name must not both be emitted. The second is skipped and +// warned about — never merged, and never given a fabricated disambiguated +// name, since inventing one would itself become a permanent wrong value. +func TestOpticDiscovery_DuplicateBayNameDropped(t *testing.T) { + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + oids := buildOIDs(duplicateBayNameOpticFixture()) + dev := &diode.Device{Name: strPtr("test-switch")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, logger) + + var bayNames []string + var serials []string + for _, e := range entities { + switch v := e.(type) { + case *diode.ModuleBay: + require.NotNil(t, v.Name) + bayNames = append(bayNames, *v.Name) + case *diode.Module: + if v.Serial != nil { + serials = append(serials, *v.Serial) + } + } + } + + assert.Equal(t, []string{"Unknown"}, bayNames, "only the first-seen bay may survive") + assert.Equal(t, []string{"SYNSER0001A"}, serials, "the colliding second optic must not be emitted") + assert.Contains(t, buf.String(), "duplicate transceiver bay name dropped") + assert.Contains(t, buf.String(), "bay=Unknown") + assert.Contains(t, buf.String(), "ent=11") + assert.Contains(t, buf.String(), "member=0") + assert.Contains(t, buf.String(), "model=SFP-10G-LR") + assert.Contains(t, buf.String(), "reason=dup_bay_name") +} + +// TestOpticDiscovery_NoDeviceMemberNeverPoisonsBayNameGuard pins the guard +// ordering fix: the device lookup must run BEFORE the duplicate-bay-name +// guard, not after. Reuses duplicateBayNameOpticFixture's two same-member, +// same-effective-bay-name ("Unknown") optics, but with an empty +// memberDevices map so BOTH lack a device — neither can ever be emitted, +// with or without the fix, because dcim.modulebay's matcher is +// (device, bay name) and the guard's key is (member, bay): two entries +// can only collide in the map if they share a member, and sharing a +// member means they share the exact same memberDevices[member] lookup, so +// one cannot have a device while the other does not. What the ordering +// DOES change is which reason each entry is dropped for. With the guard +// running first (pre-fix), the first optic still claims the bay-name key +// before its own nil-device check drops it, so the second optic — which +// was always going to be dropped for lacking a device too — instead gets +// misreported as a "duplicate transceiver bay name dropped", burying the +// real cause and polluting the guard's map with an entry nothing was ever +// emitted under. With the device check first (post-fix), neither entry +// ever touches the map, and both are reported for their real, correct +// reason. +func TestOpticDiscovery_NoDeviceMemberNeverPoisonsBayNameGuard(t *testing.T) { + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + oids := buildOIDs(duplicateBayNameOpticFixture()) + memberDevices := map[int]*diode.Device{} // member 0 has no device at all + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, logger) + + assert.Empty(t, entities, "neither optic has a device to emit under") + assert.NotContains(t, buf.String(), "duplicate transceiver bay name dropped", + "an entry that will never be emitted must not claim the bay name and misreport the next one as a duplicate") + assert.NotContains(t, buf.String(), "reason=dup_bay_name") + // Both entries must be reported for the real reason instead. + assert.Contains(t, buf.String(), "no device for member") + assert.Contains(t, buf.String(), "ent=10") + assert.Contains(t, buf.String(), "ent=11") +} + +// TestOpticDiscovery_ModularDuplicateBayNameDropped asserts the +// duplicate-bay-name guard also covers the full-mode-only submodule loop: +// two modular optics under separate linecards whose port cages happen to +// share the same name must not both be emitted. Only the first-seen +// survives; the second is skipped and warned about, exactly like the +// top-level guard. +func TestOpticDiscovery_ModularDuplicateBayNameDropped(t *testing.T) { + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + oids := buildOIDs(duplicateModularBayNameOpticFixture()) + dev := &diode.Device{Name: strPtr("test-switch")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, logger) + + var bayNames []string + var serials []string + for _, e := range entities { + switch v := e.(type) { + case *diode.ModuleBay: + require.NotNil(t, v.Name) + bayNames = append(bayNames, *v.Name) + case *diode.Module: + if v.Serial != nil { + serials = append(serials, *v.Serial) + } + } + } + + var collidingBayCount int + for _, n := range bayNames { + if n == "Te1/0/1 Container" { + collidingBayCount++ + } + } + assert.Equal(t, 1, collidingBayCount, "only the first-seen colliding bay may survive") + assert.Contains(t, serials, "SYNLCA0001", "linecard A must still be emitted") + assert.Contains(t, serials, "SYNLCB0001", "linecard B must still be emitted") + assert.Contains(t, serials, "SYNSERBAYA", "the first-seen optic must be emitted") + assert.NotContains(t, serials, "SYNSERBAYB", "the colliding second optic must not be emitted") + assert.Contains(t, buf.String(), "duplicate transceiver bay name dropped") + assert.Contains(t, buf.String(), `bay="Te1/0/1 Container"`) + assert.Contains(t, buf.String(), "ent=202") + assert.Contains(t, buf.String(), "member=0") + assert.Contains(t, buf.String(), "model=SFP-10G-LR") + assert.Contains(t, buf.String(), "reason=dup_bay_name") +} + +// TestOpticDiscovery_CrossTierDuplicateBayNameDropped is the design-point +// test: it only passes because the top-level and submodule loops share ONE +// seenTransceiverBays map. A top-level fixed-port optic's bay resolves to +// "Ethernet1" via servedInterface; a modular optic nested under a separate +// linecard sits in a port cage literally named "Ethernet1". A per-loop map +// would let both survive as two ModuleBay("Ethernet1") objects on the same +// device — the exact merge-in-NetBox scenario the guard exists to prevent. +func TestOpticDiscovery_CrossTierDuplicateBayNameDropped(t *testing.T) { + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + oids := buildOIDs(crossTierDuplicateBayNameOpticFixture()) + dev := &diode.Device{Name: strPtr("test-switch")} + memberDevices := map[int]*diode.Device{0: dev} + + entities, _ := TranslateModules(oids, nil, memberDevices, modeFull(), nil, logger) + + var bayNames []string + var serials []string + for _, e := range entities { + switch v := e.(type) { + case *diode.ModuleBay: + require.NotNil(t, v.Name) + bayNames = append(bayNames, *v.Name) + case *diode.Module: + if v.Serial != nil { + serials = append(serials, *v.Serial) + } + } + } + + var collidingBayCount int + for _, n := range bayNames { + if n == "Ethernet1" { + collidingBayCount++ + } + } + assert.Equal(t, 1, collidingBayCount, + "only one Ethernet1 bay may survive across the top-level and submodule tiers") + assert.Contains(t, serials, "SYNSERTOP1", "the top-level fixed-port optic must win (seen first)") + assert.NotContains(t, serials, "SYNSERSUB1", "the colliding modular optic must not be emitted") + assert.Contains(t, buf.String(), "duplicate transceiver bay name dropped") + assert.Contains(t, buf.String(), "bay=Ethernet1") + assert.Contains(t, buf.String(), "ent=202") + assert.Contains(t, buf.String(), "member=0") + assert.Contains(t, buf.String(), "model=SFP-10G-LR") + assert.Contains(t, buf.String(), "reason=dup_bay_name") +} + +// TestOpticDiscovery_NameEqualToOwnPIDFallsBackToCageName pins Fix 2 at the +// extraction level: entPhysicalName equal to the optic's own effective PID +// must not become an interface-shaped bay name. With the descr naming no +// interface either, the bay must fall back to the cage-derived name +// instead of adopting the PID. +func TestOpticDiscovery_NameEqualToOwnPIDFallsBackToCageName(t *testing.T) { + inv := extractModuleInventory(buildOIDs(opticNameEqualsOwnPIDOpticFixture()), testOpticLogger()) + + require.Len(t, inv.Modules, 1) + assert.Equal(t, "Te1/0/1 Container", inv.Modules[0].BayName, + "the bay must fall back to the cage-derived name") + assert.NotEqual(t, "SFP-10G-LR", inv.Modules[0].BayName, + "the optic's own PID must never become the bay name") +} + +// TestOpticDiscovery_CagelessLinecardOpticsGetDistinctBays pins the boundary +// between an optic's own cage and its module's slot. walkParents takes the +// nearest container and the nearest module independently, so for an optic with +// no cage it returns the container above the module — the linecard's slot. +// Naming every optic on the card after that one slot collides with the +// linecard's own bay and loses all but the first to the duplicate-bay guard. +func TestOpticDiscovery_CagelessLinecardOpticsGetDistinctBays(t *testing.T) { + inv := extractModuleInventory(buildOIDs(cagelessLinecardOpticsFixture()), testOpticLogger()) + + subs := inv.SubModules["2000"] + require.Len(t, subs, 3, "all three cageless optics nest under the linecard") + + var lcBay string + for _, m := range inv.Modules { + if m.Type == ModuleTypeLinecard { + lcBay = m.BayName + } + } + require.Equal(t, "Slot 2", lcBay, "the linecard keeps its slot-derived bay") + + seen := make(map[string]string, len(subs)) + for _, m := range subs { + assert.Equal(t, ModuleTypeTransceiver, m.Type) + assert.Equal(t, m.Name, m.BayName, + "a cageless optic is named for the interface its row names") + assert.NotEqual(t, lcBay, m.BayName, + "an optic must never borrow its linecard's bay") + if prev, dup := seen[m.BayName]; dup { + t.Fatalf("bay %q shared by ent %s and ent %s: the duplicate-bay guard would drop one", + m.BayName, prev, m.EntIndex) + } + seen[m.BayName] = m.EntIndex + } + assert.Len(t, seen, 3, "three optics, three distinct bays") +} + +// TestOpticDiscovery_PIDPrefixesMatchDeviceDiscovery pins the optic PID +// designators. The set is MSA/SFF standardized, so it is shared with +// device-discovery's _OPTIC_PREFIXES; an optic recognised by one backend and +// not the other would make a device's inventory depend on which backend +// discovered it. Asserting the set explicitly means widening or narrowing it +// is a deliberate edit here, not a silent drift from the other backend. +func TestOpticDiscovery_PIDPrefixesMatchDeviceDiscovery(t *testing.T) { + want := []string{ + "SFP-", "SFP+", "SFP28-", "SFP56-", + "QSFP-", "QSFP+", "QSFP28", "QSFP56-", + "QDD-", "OSFP-", + "GLC-", "X2-", "CFP-", "CFP2-", "XENPAK-", "XFP-", "CVR-", + } + assert.ElementsMatch(t, want, opticPIDPrefixes, + "keep in step with device-discovery custom_napalm/_modules.py _OPTIC_PREFIXES; "+ + "QSFP-DD is intentionally absent because QSFP- subsumes it") +} + +func TestOpticDiscovery_OpticPIDRecognition(t *testing.T) { + optics := []string{ + // Transcribed from captures that the previous seven-prefix set missed: + // iosxe_c9400x-svl and iosxe_c9500x-svl, the StackWise Virtual pairs + // this discovery path exists to serve. + "QDD-400G-CU1M", + "CVR-QSFP-SFP10G", + // One per newly added designator. + "SFP+10G-LR", "SFP28-25G-SR", "SFP56-50G-SR", + "QSFP+40G-SR4", "QSFP28-100G-LR4", "QSFP56-200G-SR4", + "OSFP-800G-DR8", "CFP2-100G-LR4", + // Already recognised before; kept so a reordering cannot lose them. + "SFP-10G-LR", "QSFP-100G-SR4", "GLC-LH-SMD", + "X2-10GB-LR", "CFP-100G-LR4", "XENPAK-10GB-LR", "XFP-10G-MM-SR", + } + for _, pid := range optics { + assert.True(t, isOpticPID(pid, ""), "%s should be recognised as an optic", pid) + // vendorType carries the PID when model is blank. + assert.True(t, isOpticPID("", pid), "%s should be recognised via vendorType", pid) + assert.True(t, isOpticPID(strings.ToLower(pid), ""), "%s should match case-insensitively", pid) + } + + notOptics := []string{ + "C9400-LC-48UX", "C9400-SUP-1", "C9404R", "PWR-C4-950WAC-R", + "FAN-T1-R", "OS6350-P24", "STACK-T1-50CM", "", + // Near-misses: a shared leading substring must not be enough. + "SFPX-99", "QSFPX-99", "GLCX-1", "CVRX-1", + } + for _, pid := range notOptics { + assert.False(t, isOpticPID(pid, ""), "%q must not be taken for an optic", pid) + } +} diff --git a/orb-discovery/snmp-discovery/mapping/module_test.go b/orb-discovery/snmp-discovery/mapping/module_test.go index c8513c5f..8b5ce7f3 100644 --- a/orb-discovery/snmp-discovery/mapping/module_test.go +++ b/orb-discovery/snmp-discovery/mapping/module_test.go @@ -92,8 +92,9 @@ func TestClassifyModule(t *testing.T) { // Edge: under module parent but non-optic → linecard (depth alone insufficient). {"non-optic under module parent", "WS-X45-FOO", "", true, ModuleTypeLinecard}, - // Edge: optic-shaped PID at chassis level → linecard (PID alone insufficient). - {"optic PID at chassis depth", "QSFP-100G-SR4", "", false, ModuleTypeLinecard}, + // An optic PID is sufficient on its own: a fixed-port chassis holds + // its transceivers directly, with no intervening module. + {"optic PID at chassis depth", "QSFP-100G-SR4", "", false, ModuleTypeTransceiver}, // VendorType fallback when Model is blank. {"vendortype fallback optic", "", "QSFP-100G-LR4", true, ModuleTypeTransceiver}, diff --git a/orb-discovery/snmp-discovery/mapping/module_translate.go b/orb-discovery/snmp-discovery/mapping/module_translate.go index 6d7d988a..efdf991c 100644 --- a/orb-discovery/snmp-discovery/mapping/module_translate.go +++ b/orb-discovery/snmp-discovery/mapping/module_translate.go @@ -78,6 +78,12 @@ func TranslateModulesWithAlias( var entities []diode.Entity emittedModules := make(map[string]*diode.Module, len(inv.Modules)) + // dcim.modulebay's matcher is name+device: two bays sharing a name on + // one member would merge into one NetBox object, taking their modules + // with them. Keyed by (MemberID, effective bay name) — MemberID is + // only populated by assignMemberID above, which is why this guard + // lives here and not in extractModuleInventory. + seenTransceiverBays := make(map[transceiverBayKey]string, len(inv.Modules)) for _, m := range inv.Modules { // PSU / Fan are classified for labelling only — never emitted as @@ -85,17 +91,52 @@ func TranslateModulesWithAlias( if m.Type == ModuleTypePSU || m.Type == ModuleTypeFan { continue } + // A transceiver is full-mode-only. Modular optics are already + // excluded by the mode gate below because they live in SubModules; + // a fixed-port optic is a top-level module and needs this. + if m.Type == ModuleTypeTransceiver && mode != config.DiscoverModulesFull { + continue + } // assignMemberID stamps MemberID=-1 on entries whose chassis // ancestor isn't in the VC member set. Skip — already warn-logged. if m.MemberID < 0 { continue } + // Resolve the device — and skip entries with no device — BEFORE + // consulting the duplicate-bay-name guard below. An entry with no + // device is never emitted regardless of the guard, so it must not + // claim a bay name: doing so would make a later, legitimate optic + // with that same effective bay name look like a duplicate and get + // dropped, even though nothing was ever emitted under that name. device := memberDevices[m.MemberID] if device == nil { logger.Warn("module discovery: no device for member", "member", m.MemberID, "ent", m.EntIndex, "model", m.Model) continue } + // No capture in the corpus has shown two fixed-port transceivers + // collide on the same member's bay name, but the merge this + // guards against is silent and Diode never retracts a wrong + // value, so refuse rather than risk it. Scoped to transceivers — + // bay-name collisions among other module types are pre-existing + // and out of scope here. Skip and warn rather than invent a + // disambiguated name: a fabricated value would itself become a + // permanent wrong value. + if m.Type == ModuleTypeTransceiver { + key := transceiverBayKey{member: m.MemberID, bay: effectiveBayName(m)} + if _, dup := seenTransceiverBays[key]; dup { + logger.Warn("module discovery: duplicate transceiver bay name dropped", + "bay", key.bay, "ent", m.EntIndex, "member", m.MemberID, "model", m.Model, + "reason", "dup_bay_name") + if c := metrics.GetModulesDropped(); c != nil { + c.Add(context.Background(), 1, metric.WithAttributes( + attribute.String("reason", "dup_bay_name"), + )) + } + continue + } + seenTransceiverBays[key] = m.EntIndex + } bay := emitModuleBay(device, m) entities = append(entities, bay) if c := metrics.GetModuleBaysEmitted(); c != nil { @@ -148,12 +189,38 @@ func TranslateModulesWithAlias( if _, dup := emittedModules[tr.EntIndex]; dup { continue } + // Resolve the device — and skip entries with no device — + // BEFORE consulting the duplicate-bay-name guard below, same + // as the top-level loop above and for the same reason: an + // entry that is never emitted must not claim a bay name. device := memberDevices[tr.MemberID] if device == nil { logger.Warn("module discovery: no device for transceiver member", "member", tr.MemberID, "ent", tr.EntIndex, "model", tr.Model) continue } + // Same guard as the top-level loop above, against the SAME + // seenTransceiverBays map — deliberately shared rather than a + // second map. The collision domain is (device, bay name) + // regardless of whether the module is top-level or nested, so + // a top-level fixed-port bay and a submodule bay sharing a + // name on the same device collide in NetBox exactly as two + // submodule bays would; a separate map would miss that + // cross-tier case. See the top-level guard's comment for why + // this refuses rather than invents a disambiguated name. + key := transceiverBayKey{member: tr.MemberID, bay: effectiveBayName(tr)} + if _, dup := seenTransceiverBays[key]; dup { + logger.Warn("module discovery: duplicate transceiver bay name dropped", + "bay", key.bay, "ent", tr.EntIndex, "member", tr.MemberID, "model", tr.Model, + "reason", "dup_bay_name") + if c := metrics.GetModulesDropped(); c != nil { + c.Add(context.Background(), 1, metric.WithAttributes( + attribute.String("reason", "dup_bay_name"), + )) + } + continue + } + seenTransceiverBays[key] = tr.EntIndex // Sub-bay reconciler workaround (spec §Sub-bay emission // workaround): emit transceiver sub-bays DEVICE-ROOTED // (no Module=parent_linecard link). Linking the sub-bay to @@ -182,8 +249,9 @@ func TranslateModulesWithAlias( } } - // Empty bays — class=5 rows with no class=9 child. Bare ModuleBay - // only; no Module entity. + // Empty bays — class=5 rows with no module or port child and no + // optic PID of their own (extractModuleInventory's harvest). Bare + // ModuleBay only; no Module entity. for _, b := range inv.EmptyBays { if b.MemberID < 0 { continue @@ -206,20 +274,36 @@ func TranslateModulesWithAlias( return entities, ifaceMap } +// transceiverBayKey is the (member, effective bay name) dedup key used +// by the duplicate-bay-name guard in TranslateModulesWithAlias. +type transceiverBayKey struct { + member int + bay string +} + +// effectiveBayName returns the name a ModuleBay will actually carry — +// BayName, falling back to BayPosition, falling back to "Unknown". This +// is the same fallback chain emitModuleBay applies, factored out so the +// duplicate-bay-name guard compares the value Diode will actually see +// rather than the raw (possibly blank) ModuleEntry field. +func effectiveBayName(m ModuleEntry) string { + if m.BayName != "" { + return m.BayName + } + if m.BayPosition != "" { + return m.BayPosition + } + return "Unknown" +} + // emitModuleBay constructs a ModuleBay entity for a top-level // (chassis-slot) module. Always carries Device — the chassis device is // the matching scope for both ModuleBay and Module per Diode docs. func emitModuleBay(device *diode.Device, m ModuleEntry) *diode.ModuleBay { - name := m.BayName - if name == "" { - // Bay rows occasionally arrive without a name; fall back to - // position so we never ship an empty-string required field - // (Diode rejects ""). - name = m.BayPosition - } - if name == "" { - name = "Unknown" - } + // Bay rows occasionally arrive without a name; effectiveBayName + // falls back to position, then "Unknown", so we never ship an + // empty-string required field (Diode rejects ""). + name := effectiveBayName(m) bay := &diode.ModuleBay{ Device: device, Name: &name,