feat: collect LLDP data on DPU and host (1/3)#3418
Open
kurotych wants to merge 8 commits into
Open
Conversation
Move LLDP parsing (get_port_lldp_info and friends) out of the DPU-specific enumeration into a shared lldp_collector module. Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
… port Rework lldp collector to consume `lldpcli -f json0` instead of `-f json`. json0 emits a stable, uniform structure — every field is an array with fixed keys — whereas `-f json` makes the output structure data-dependent and fragile to parse. Add split id_type/id_value and remote_port_type/remote_port_value fields to LldpSwitchData; the combined id/remote_port strings are kept and marked deprecated for backward compatibility Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
…oting Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis change introduces a dedicated LLDP collector, updates RPC fields, removes embedded DPU LLDP logic, adds agent and scout CLI commands, and installs and enables ChangesLLDP discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ScoutOrAgent
participant LldpCollector
participant UdevAndSysfs
participant Lldpcli
Operator->>ScoutOrAgent: invoke LldpNeighbors
ScoutOrAgent->>LldpCollector: print_lldp_neighbors()
LldpCollector->>UdevAndSysfs: enumerate interfaces and read MACs
UdevAndSysfs-->>LldpCollector: Ethernet interfaces and MACs
LldpCollector->>Lldpcli: query neighbors and local chassis as JSON
Lldpcli-->>LldpCollector: LLDP responses
LldpCollector-->>ScoutOrAgent: formatted neighbor output
ScoutOrAgent-->>Operator: exit without normal startup
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/host-support/src/hardware_enumeration.rs (1)
157-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow visibility to
pub(crate)for consistency and minimal surface.The only new consumer (
lldp_collector) references this throughcrate::hardware_enumeration::convert_property_to_string, sopub(crate)is sufficient. Since the helper was previously private, no external crate depends on it. This also matches thepub(crate)you correctly chose forPCI_SUBCLASSon Line 45.♻️ Proposed change
-pub fn convert_property_to_string<'a>( +pub(crate) fn convert_property_to_string<'a>(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/host-support/src/hardware_enumeration.rs` at line 157, Change the visibility of convert_property_to_string from pub to pub(crate), keeping its existing signature and behavior unchanged. Ensure the lldp_collector crate-local reference continues to resolve through crate::hardware_enumeration.crates/host-support/src/lldp_collector.rs (1)
177-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit errors as structured fields, not string interpolation. These log sites interpolate the error (and raw JSON payloads) into the message string, which defeats logfmt key/value search. As per coding guidelines, prefer
error = %eand named fields over interpolation (tracing::error!(%machine_id, error=%e, "...")).
crates/host-support/src/lldp_collector.rs#L177-L177: replacewarn!("Could not deserialize local LLDP chassis {json}, {e}")withwarn!(error = %e, "Could not deserialize local LLDP chassis")(drop or demote the fulljsonpayload to adebugfield).crates/host-support/src/lldp_collector.rs#L158-L158: replace withwarn!(error = %e, "Could not read local LLDP chassis").crates/host-support/src/lldp_collector.rs#L224-L224: replacetracing::debug!(ifname, "no LLDP neighbor: {e}")withtracing::debug!(ifname, error = %e, "no LLDP neighbor").crates/host-support/src/lldp_collector.rs#L237-L237: replace withwarn!(port, error = %e, "Could not discover LLDP peer").crates/host-support/src/lldp_collector.rs#L255-L255: replace withwarn!(error = %e, "Could not deserialize LLDP response")(payload to adebugfield).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/host-support/src/lldp_collector.rs` at line 177, Update all five LLDP logging sites in lldp_collector.rs (lines 158, 177, 224, 237, and 255) to emit errors as structured tracing fields using error = %e and concise static messages; preserve ifname and port fields where present, and drop or demote raw JSON payloads to debug fields at the deserialization sites.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host-support/src/lldp_collector.rs`:
- Around line 231-240: Update get_lldp_port_info to invoke lldpcli directly with
separate arguments for JSON output, neighbor display, and port, removing the
constructed bash -c command. Apply the same direct-argument approach to
get_local_chassis_id, preserving both commands’ existing behavior and error
handling.
---
Nitpick comments:
In `@crates/host-support/src/hardware_enumeration.rs`:
- Line 157: Change the visibility of convert_property_to_string from pub to
pub(crate), keeping its existing signature and behavior unchanged. Ensure the
lldp_collector crate-local reference continues to resolve through
crate::hardware_enumeration.
In `@crates/host-support/src/lldp_collector.rs`:
- Line 177: Update all five LLDP logging sites in lldp_collector.rs (lines 158,
177, 224, 237, and 255) to emit errors as structured tracing fields using error
= %e and concise static messages; preserve ifname and port fields where present,
and drop or demote raw JSON payloads to debug fields at the deserialization
sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95d41ba3-c744-4d45-a312-0454ce9d3d75
📒 Files selected for processing (16)
crates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/main.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/lib.rscrates/host-support/src/lldp_collector.rscrates/host-support/test/lldp_query.jsoncrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rscrates/scout/src/cfg/command_line.rscrates/scout/src/main.rspxe/mkosi.profiles/scout-oss-aarch64/mkosi.confpxe/mkosi.profiles/scout-oss-aarch64/mkosi.postinst.chrootpxe/mkosi.profiles/scout-oss-x86_64/mkosi.confpxe/mkosi.profiles/scout-oss-x86_64/mkosi.postinst.chroot
💤 Files with no reviewable changes (1)
- crates/host-support/test/lldp_query.json
Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
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
1 participant
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.
This is the first of three parts implementing the LLDP collection feature described in #3204.
lldp_collector.rsmodule, which collects the machine's LLDP neighbor data.lldp-neighborsCLI command to thescoutandagentservices.lldpdpacket to scout osDetails
Ported and fixed abandoned code
dpu.rs contained leftovers from an earlier attempt at LLDP data collection. That code has been fixed and ported into
lldp_collector.rs.Unlike the old code, which lived in the DPU enumeration path,
lldp_collector.rsis machine-agnostic: it enumerates the Ethernet interfaces on any host (DPU, scout, etc.) and collects their LLDP neighbors uniformly. It also filters out self-loopback neighbors.Concerns multiple neighbors on a single port: the old code assumed this was possible but handled it in a strange manner: only the last neighbor survived (
get_port_lldp_infooverwrote the result on each iteration).The other change is switching
lldpclito the-f json0output format, which is more stable to parse programmatically. The map-keyed-f jsonformat uses dynamic keys (port names, neighbor names) as JSON object keys, whereasjson0renders everything as arrays of objects with fixed keysManual test results
The real values were changed
To be done in later PRs
Related issues
#3204
Type of Change
Breaking Changes
Testing
Additional Notes