Skip to content

feat(dhcp): suppress DHCP for ignored BMC MAC addresses - #4459

Merged
kdhulipala-wq merged 1 commit into
NVIDIA:mainfrom
kdhulipala-wq:kcd-dhcp-decom-disable
Aug 1, 2026
Merged

feat(dhcp): suppress DHCP for ignored BMC MAC addresses#4459
kdhulipala-wq merged 1 commit into
NVIDIA:mainfrom
kdhulipala-wq:kcd-dhcp-decom-disable

Conversation

@kdhulipala-wq

Copy link
Copy Markdown
Contributor

When a BMC MAC has suppress_dhcp = true in ignored_bmc_macs, the DiscoverDhcp RPC returns FailedPrecondition and atomically records dhcp_discover_suppressed_at. The decommission workflow polls this timestamp to confirm the BMC DHCP client has returned to INIT state.

No changes to the DHCP server binary; suppression is handled entirely within the existing DiscoverDhcp RPC in NICo Core.

Closes #3816

@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes
    • DHCP discovery now detects when requests from a BMC MAC address are suppressed.
    • Suppressed requests are stopped with a clear failure message, while unsuppressed requests continue normally.

Walkthrough

discover_dhcp checks BMC DHCP suppression before discovery. Suppressed requests commit the transaction and return a failed-precondition error. Other requests continue unchanged.

Changes

DHCP suppression handling

Layer / File(s) Summary
Acknowledge suppressed BMC requests
crates/api-core/src/dhcp/discover.rs
The discovery path checks the requesting BMC MAC. Suppressed requests commit the transaction and return a failed-precondition error. Unsuppressed requests continue through the existing flow.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The change implements DHCPDISCOVER suppression but does not address DHCPREQUEST NAK responses or DHCP record cache invalidation required by #3816. Implement DHCPREQUEST NAK handling and provide DHCP record cache invalidation, or update the linked issue scope to match this change.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes remain within the stated scope of implementing DHCP suppression in the existing DiscoverDhcp RPC.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: suppressing DHCP for ignored BMC MAC addresses.
Description check ✅ Passed The description accurately explains the DHCP suppression behavior, timestamp recording, workflow usage, and implementation scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@kdhulipala-wq
kdhulipala-wq force-pushed the kcd-dhcp-decom-disable branch from 461129c to 750d4b2 Compare July 31, 2026 21:20
@kdhulipala-wq
kdhulipala-wq marked this pull request as ready for review July 31, 2026 21:39
@kdhulipala-wq
kdhulipala-wq requested a review from a team as a code owner July 31, 2026 21:39

@coderabbitai coderabbitai Bot left a comment

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.

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/api-core/src/dhcp/discover.rs`:
- Around line 502-504: Update the error message in the DHCP suppression path to
use lowercase wording while preserving the interpolated parsed_mac value and
omitting any trailing period. Keep the existing CarbideError::FailedPrecondition
construction unchanged.
- Around line 494-499: Update the cached-response path in
Controller::discover_dhcp to check BMC suppression before returning an unexpired
cached record, ensuring suppressed MAC addresses reach the suppression
acknowledgment flow and do not receive stale cached DHCP responses.
Alternatively, invalidate every cache key for parsed_mac when
db::bmc_suppression::acknowledge begins suppression, while preserving normal
caching for unsuppressed MAC addresses.
🪄 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: 65de907a-6810-488e-9e19-2a26ffa84941

📥 Commits

Reviewing files that changed from the base of the PR and between 3e1b818 and 750d4b2.

📒 Files selected for processing (1)
  • crates/api-core/src/dhcp/discover.rs

Comment thread crates/api-core/src/dhcp/discover.rs
Comment thread crates/api-core/src/dhcp/discover.rs

@ericpretzel ericpretzel left a comment

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.

For decommissioning, we will reset the BMC which should cause it to go back to dhcpdiscover and not request. So NAK'ing requests for this purpose isn't strictly necessary. The resets will be timed so as to not refresh the 60-second cache in case it was hit.

Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
@kdhulipala-wq
kdhulipala-wq force-pushed the kcd-dhcp-decom-disable branch from a73bc75 to dced39a Compare July 31, 2026 22:31
@ericpretzel
ericpretzel marked this pull request as draft July 31, 2026 23:00
@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

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.

@ericpretzel
ericpretzel marked this pull request as ready for review July 31, 2026 23:01
@kdhulipala-wq
kdhulipala-wq enabled auto-merge (squash) July 31, 2026 23:49
@kdhulipala-wq kdhulipala-wq added this to the v2.1 milestone Aug 1, 2026
@kdhulipala-wq
kdhulipala-wq merged commit fbe121d into NVIDIA:main Aug 1, 2026
64 checks passed
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.

DHCP server no-offer and NAK behavior for ignored MAC addresses

2 participants