Fall back to LLDP-V2-MIB when the classic remote table yields nothing (#688) - #694
Open
roc-ops wants to merge 1 commit into
Open
Fall back to LLDP-V2-MIB when the classic remote table yields nothing (#688)#694roc-ops wants to merge 1 commit into
roc-ops wants to merge 1 commit into
Conversation
Some agents implement only the 802.1AB-2009 revision of the MIB, rooted at 1.3.111.2.802.1.1.13 -- IP Infusion OcNOS among them. Every classic 1.0.8802.1.1.2 remote-table column on such a device walks straight into the adjacent lldpExtensions subtree (EndOfSubtree, indistinguishable from implemented-and-empty, so `unsupported` stays false), while the same neighbours sit fully populated in lldpV2RemTable. The device contributed no L2 edges at all. The V2 MIB is a second LldpMibProfile, walked by query_lldp_neighbors as a fallback and never alongside the classic one: only when the classic walk finished and read nothing. An incomplete classic walk is a failed read, not a device with nothing, and does not fall back. An equally empty V2 attempt keeps the classic verdict, so an agent with no LLDP stays `unsupported` rather than becoming authority to clear what the server holds. Its remote columns sit one above their classic numbers (lldpV2RemLocalIfIndex is inserted as column 2), and its row index is timeMark.localIfIndex.localDestMACAddress.remIndex -- four sub-ids, read from the front and required whole by a separate splitter. The classic end-relative splitter, applied to that index, keys every neighbour on the destination-address index and discards all but one as duplicates. lldpV2RemLocalIfIndex is already an ifIndex, so the result carries `local_port_is_if_index` and the collection skips the lldpLocPortTable walk and remap_lldp_local_ports on that path; the placement check against the interface table still runs. The management-address index is read with or without its address-length sub-id (OcNOS serves none), and the device's own chassis identity gets the same fallback so a V2-only device can resolve as itself. The simulated lab gains switch-ocnos-01 (192.168.7.252), built from an snmpwalk of a UfiSpace S9600-32X running OcNOS 7.0.1 with identifiers rewritten: eth0 at ifIndex 3, ce0-ce31 at 10001..10125 step 4, three V2 neighbours keyed by those ifIndex values, and no classic LLDP tables. Its regression test drives the real collection path and asserts the neighbours land on 10009 and 10073 directly. Along the way, RemoteNeighbour::wire_rows now serves the chassis-subtype column its own MIB names rather than the classic constant, and the own-MIB test runs against both profiles. Fixes scanopy#688
roc-ops
added a commit
to roc-ops/scanopy
that referenced
this pull request
Aug 31, 2026
Three points from the independent verification of 5af3ef0. The seam itself was confirmed real -- the reviewer added a hypothetical third vendor differing on every axis (non-empty origin, two-element root, different state container) and changed nothing but the table, and it worked end to end -- so this only corrects the record around it. CORRECTS 5af3ef0's MESSAGE, which is the part a maintainer reads on the PR. It said behaviour is unchanged for ArcOS, cEOS and SR Linux "and the fixtures captured from those devices assert the same rows they did". There are no cEOS or SR Linux fixtures. The captured devices are ArcOS 8.5 (netlab-leaf1) and DriveNets (cDNOS 26.2 and a 72XC); the only `srl_nokia` in the tree is a hypothetical origin string in a path-rendering test. ArcOS is genuinely fixture-covered. The other two are unchanged BY CONSTRUCTION, which is a real argument and a different one: for OPENCONFIG_LLDP the normalisation is provably the identity on every input -- the root is empty so the prefix check always holds, the guard then requires the first element to be `lldp` and returns the input untouched otherwise, and the state-container rewrite maps `state` to `state`. The claim was inherited from the original doc; the code no longer asserts it anywhere, so this is the last place it lived. RECORDS AN UNCOVERED WIRE. Nothing tests that `probe` populates `GnmiProbeHandle.models`. Replacing that assignment with `Vec::new()` leaves the suite green while every device becomes `NoneAdvertised` and DriveNets returns zero neighbours -- the exact defect this line of work exists to fix. Coverage regressed here: `collect` used to call `capabilities()` itself, so the fake's list flowed through the production call; the new test helper read the field directly and left `ScriptedDevice::capabilities()` dead. The helper now goes through the transport, which is right but does NOT close it -- the assignment is inside `probe`, and no test reaches `probe` at all because it builds a `TonicTransport` directly with no seam to inject a fake. Making that injectable is the real fix; until then the gap is named on the handle rather than left for someone to assume the tests cover it. NAMES THE SEAM'S BOUNDARY. `LldpModelProfile` said a further vendor is a static rather than a branch. True for openconfig-SHAPED vendors; the list key names (`interface[name]`, `neighbor[id]`) and the literal `lldp` element are still fixed in code. Said so in the profile doc, the same courtesy `LldpMibProfile` pays about subtype enums. Also verified by the review and worth recording: clippy is clean, the branch is based on `feat/gnmi-lldp-collector` off `upstream/dev` with neither scanopy#694 nor scanopy#697 contained, so it can reach PR scanopy#696 on its own, and six of seven sabotages were caught (the seventh is the probe wire above).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #688.
What
Some agents implement only the 802.1AB-2009 LLDP-V2-MIB (
1.3.111.2.802.1.1.13) — IP Infusion OcNOS among them — and contribute no L2 edges because only the classic1.0.8802.1.1.2remote table is walked. This adds the V2 MIB as a secondLldpMibProfileand walks it as a fallback, never alongside the classic one.Per the review notes on the issue:
.5chassis-id-subtype ….11sysDesc); local tables keep classic numbering.lldpV2RemEntry(timeMark.localIfIndex.localDestMACAddress.remIndex), requiring exactly four sub-ids.split_lldp_rem_indexis untouched; a test shows what it would have done to a V2 index (every neighbour keyed on the destination-address index).remap_lldp_local_portsand thelldpLocPortTablewalk are skipped on the V2 path —lldpV2RemLocalIfIndexis already an ifIndex. The result carrieslocal_port_is_if_indexfor that; the placement check against the interface table still runs.LldpChassisId/LldpPortId/from_snmpare reused as-is; no schema change.One deliberate deviation — the gate
The issue suggested gating on
unsupported. On the device this exists for, that flag is false: OcNOS serves thelldpExtensionssubtree under the classic root, so every classic remote column endsEndOfSubtree("implemented and empty"), neverEndOfMibView— the walks in the issue body show it. The gate is therefore classic walk finished with zero rows, which is wider thanunsupportedand is paid for by every host/printer/empty switch with an implemented-but-empty classic table: eight single-page walks that find nothing. An incomplete classic walk never falls back, and an equally empty V2 attempt keeps the classic verdict so a no-LLDP agent staysunsupportedrather than gaining authority to clear another integration's neighbours. An incomplete V2 walk is likewise returned as non-authoritative rather than laundered into "no neighbours". If you'd rather gate onunsupported || zero rowswith a narrower cost, it's a one-line change — but on OcNOS it has to include zero rows.Simulator
SimLldpMib::V2besideCLASSIC(own root,lldpv2file suffix, shifted columns, four-sub-idrem_suffix) andswitch-ocnos-01(192.168.7.252,Purpose::Regression { issue: "#688" }), built from ansnmpwalk -Onof a UfiSpace S9600-32X on OcNOS 7.0.1.60 with identifiers rewritten:eth0at ifIndex 3,ce0..ce31at 10001..10125 step 4 with ifXTable names, the full 33-rowlldpV2LocPortTable, three V2 neighbours on 3/10009/10073 (remIndex 4/6/2, time mark 0, dest-address index 1, subtypes 4 and 7/5/5, the blank" "port descriptions the box really sends), and no classic LLDP tables. Its regression test drives the real collection path and asserts the neighbours land on 10009/10073 directly. While wiring V2 through the simulator,RemoteNeighbour::wire_rowsturned out to hard-code the classic chassis-subtype OID instead ofmib.remote.chassis_id_subtype; fixed, and the own-MIB test now runs against both profiles with aSubtypeWrongTypeneighbour that would have caught it. Classic fixture output is unchanged.Tests
cargo test --lib -- discovery::integration::snmp→ 166 passed (new: V2-only agent keyed by ifIndex, classic agent never reaches V2, neither-MIB stays unsupported, incomplete classic does not fall back, incomplete V2 is not authoritative, splitter rejects 0/3/5 sub-ids, classic splitter mis-keys V2, man-addr index with/without length;switch-ocnos-01device tests; simlldpown-MIB test for both profiles)cargo test --lib -- tests::snmp_sim_resolution→ 7 passedcargo check --features snmp-sim --bin generate-snmp-fixturesOK; clippy clean in touched files; fmt clean