Skip to content

feat(provider/lldp): add LLDP leaf topology discovery - #443

Open
dmitsh wants to merge 1 commit into
mainfrom
ds-lldp
Open

feat(provider/lldp): add LLDP leaf topology discovery#443
dmitsh wants to merge 1 commit into
mainfrom
ds-lldp

Conversation

@dmitsh

@dmitsh dmitsh commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Add bare-metal and Kubernetes LLDP providers that group compute
nodes by their directly connected switch chassis ID.

Support interface filtering, reject ambiguous multi-switch
attachments, and integrate LLDP collection with the node-data-broker,
Helm chart, documentation, and Slurm update tooling.

closes #442

@dmitsh
dmitsh requested a review from ravisoundar as a code owner August 2, 2026 21:05
@greptile-apps

greptile-apps Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds bare-metal and Kubernetes LLDP providers, LLDP collection through the node-data-broker, interface filtering, topology construction, Helm integration, documentation, and Slurm tooling support.

  • Registers lldp-bm and lldp-k8s providers and maps chassis IDs into closest-first leaf topology.
  • Adds keyed LLDP JSON decoding, interface selection, and multi-switch ambiguity handling.
  • Adds broker annotation collection and deletion semantics for stale LLDP data.
  • Adds container, Helm, documentation, and script integration.

Confidence Score: 4/5

The PR is not yet safe to merge because the keyed-chassis parser fails its new regression case and several LLDP discovery failures can still preserve stale topology state.

The parser classifies a chassis keyed as id as a normalized record, dropping its nested identifier and causing the added regression assertion to fail. Separately, command, parse, and ambiguity errors still abort the broker before deleting an existing LLDP chassis annotation, leaving nodes grouped under outdated switches.

Files Needing Attention: pkg/providers/lldp/common.go, pkg/providers/lldp/provider_k8s.go, cmd/node-data-broker/main.go

Important Files Changed

Filename Overview
pkg/providers/lldp/common.go Adds LLDP JSON decoding, filtering, normalization, and switch-ID generation, but the record discriminator mishandles the keyed-chassis regression case.
pkg/providers/lldp/provider_k8s.go Adds Kubernetes annotation-backed topology and broker collection, while some discovery failures still preserve stale chassis state.
pkg/providers/lldp/provider_bm.go Adds pdsh-based bare-metal LLDP collection and leaf topology generation.
cmd/node-data-broker/main.go Wires LLDP collection into the broker and supports empty-value annotation deletion.
charts/topograph/templates/nodeDataBroker/daemonset.yaml Forwards configured LLDP interfaces to the node-data-broker.
pkg/registry/registry.go Registers both LLDP provider loaders.

Sequence Diagram

sequenceDiagram
    participant LLDPD as Host lldpd
    participant Broker as node-data-broker
    participant K8s as Kubernetes API
    participant Provider as lldp-k8s provider
    participant Graph as Topology graph
    Broker->>LLDPD: lldpctl -f json
    LLDPD-->>Broker: keyed interface/chassis JSON
    Broker->>Broker: parse and select chassis
    Broker->>K8s: update node annotations
    Provider->>K8s: list selected nodes
    K8s-->>Provider: instance and chassis annotations
    Provider->>Graph: build leaf-switch tiers
Loading

Reviews (6): Last reviewed commit: "feat(provider/lldp): add LLDP leaf topol..." | Re-trigger Greptile

Comment thread pkg/providers/lldp/common.go
Comment thread pkg/providers/lldp/provider_k8s.go
@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.93491% with 105 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.35%. Comparing base (8468abe) to head (6ad1aa8).
⚠️ Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
pkg/providers/lldp/common.go 81.67% 18 Missing and 17 partials ⚠️
pkg/providers/lldp/provider_bm.go 48.52% 24 Missing and 11 partials ⚠️
pkg/providers/lldp/provider_k8s.go 54.16% 24 Missing and 9 partials ⚠️
cmd/node-data-broker/main.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   72.15%   75.35%   +3.19%     
==========================================
  Files          89       98       +9     
  Lines        5689     6994    +1305     
==========================================
+ Hits         4105     5270    +1165     
- Misses       1382     1421      +39     
- Partials      202      303     +101     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

@dmitsh
dmitsh force-pushed the ds-lldp branch 3 times, most recently from b4315fd to 56a4a1c Compare August 3, 2026 17:45
Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
Comment on lines +241 to +247
var idFields map[string]json.RawMessage
if err := json.Unmarshal(fields["id"], &idFields); err != nil {
return false
}
_, hasType := idFields["type"]
_, hasValue := idFields["value"]
return hasType || hasValue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Keyed chassis ID is discarded

When a keyed chassis object uses id as its chassis name, isChassisRecord classifies the outer object as a normalized record. Decoding then leaves the nested chassis identifier empty, so parseNeighbors discards the neighbor and the new TestParseNeighborsHandlesNamesMatchingRecordFields regression case fails.

Knowledge Base Used: Providers registry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] LLDP provider for leaf-switch topology discovery

1 participant