Enable LLDP-MED for DPUs#3692
Draft
stoo-davies wants to merge 2 commits into
Draft
Conversation
Signed-off-by: Stoo Davies <189670495+stoo-davies@users.noreply.github.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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughLLDP preparation now updates lldpd configuration, verifies LLDP-MED inventory settings with retryable service restarts, and runs during DPU-OS startup before setting the LLDP system description. ChangesLLDP preparation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant setup_and_run
participant prepare_lldp
participant lldpcli
participant systemctl
setup_and_run->>prepare_lldp: prepare_lldp()
prepare_lldp->>lldpcli: Query LLDP-MED configuration
lldpcli-->>prepare_lldp: Configuration JSON
prepare_lldp->>systemctl: Restart lldpd.service when required
systemctl-->>prepare_lldp: Restart result
prepare_lldp-->>setup_and_run: Preparation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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/agent/src/lldp.rs`:
- Around line 81-103: Extract the configuration line-rewrite logic into a pure
helper and the inventory JSON value parsing into another pure helper, replacing
direct filesystem and command coupling in the tested paths. Add table-driven
tests using value_scenarios! and scenarios! covering absent and existing
DAEMON_ARGS, LF, CRLF, and missing-final-newline inputs, plus yes, no, missing,
and unexpected JSON values; preserve the current transformation behavior.
- Around line 119-122: Bound all LLDP-related child processes with deadlines:
update the lldpcli call in the LLDP configuration read path, the systemctl
restart path, and LldpdConfigFileWriter::daemon_read() to use timeout-capable
process handling. Ensure expired commands are terminated and reported as
retryable failures so agent startup and config-change handling cannot block
indefinitely.
🪄 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: f2e83359-f23d-45d5-af93-f2b125c2bd71
📒 Files selected for processing (2)
crates/agent/src/lldp.rscrates/agent/src/main_loop.rs
Signed-off-by: Stoo Davies <189670495+stoo-davies@users.noreply.github.com>
Contributor
|
Is there a plan to support this when DPF is used? (or does it do that already?) |
wminckler
reviewed
Jul 17, 2026
|
|
||
| fn ensure_lldpd_daemon_args() -> eyre::Result<bool> { | ||
| let current_contents = | ||
| fs::read_to_string(LLDPD_DEFAULT_CONFIG).wrap_err("couldn't read lldpd default config")?; |
Contributor
There was a problem hiding this comment.
nit: this should use a bounded buffer size
wminckler
approved these changes
Jul 17, 2026
stoo-davies
marked this pull request as draft
July 17, 2026 22:19
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
Converting this back to draft until I can address the DPF question from @wminckler The new code won't break anything since it's guarded by the is_dpu_os() check, however I need to get the same config applied under DPF provisioning. |
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.
Adds code to forge-dpu-agent to do two things..
These changes are for automated cable validation.
Related issues
#3204
Type of Change
Breaking Changes
Testing
Additional Notes
Tested on a BF3.