docs: document expected machine interfaces - #4418
Open
chet wants to merge 1 commit into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughDocuments the ChangesExpected Machine Interfaces
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4418.docs.buildwithfern.com/infra-controller |
![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: 3
🤖 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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md`:
- Around line 67-78: Document the supported legacy --host_nics alias alongside
--interfaces in both
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 67-78 and
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 164-173, keeping the existing interface option description consistent
across both pages.
In `@docs/provisioning/expected-machine-interfaces.md`:
- Around line 179-181: Replace every copyable BMC password in the listed
Markdown examples with a clearly non-secret placeholder; update
docs/provisioning/expected-machine-interfaces.md at lines 179-181 and 226-232,
docs/provisioning/ip-and-network-configuration.md at lines 124-126,
docs/provisioning/ingesting-hosts.md at lines 246-248, and
docs/provisioning/boot-interfaces-and-dpu-modes.md at lines 110-112 and 129-131.
For the command-argument examples at expected-machine-interfaces.md lines
226-232 and boot-interfaces-and-dpu-modes.md lines 129-131, explicitly document
the shell-history and process-list exposure risk while keeping the examples
realistic and safe.
In `@docs/provisioning/ingesting-hosts.md`:
- Around line 228-233: Update the documentation for bmc_ip_address and
bmc_ip_allocation to state that Auto infers Fixed when an address is present,
while an explicit top-level allocation takes precedence over both the inferred
value and nested policy; clarify that an address is not necessarily reserved
when a different allocation is explicitly selected.
🪄 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: 6623cdbb-25a5-4cc6-9544-7f66ed9ccd0c
📒 Files selected for processing (7)
docs/index.ymldocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/expected-machine-interfaces.mddocs/provisioning/ingesting-hosts.mddocs/provisioning/ip-and-network-configuration.md
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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md`:
- Around line 199-201: Update the three expected-machine add examples to replace
the literal BMC password “mypassword” with a clearly non-secret placeholder, and
add an explicit warning that command-line passwords may be exposed through shell
history and process listings.
- Around line 145-152: Update the retained-address description in
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 145-152 to say it remains static for the lifetime of the interface record
instead of “never expires.” Apply the same wording correction in
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 142-149 and link it to the retained-address lifetime section.
🪄 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: 652d4fd9-0949-4700-9a7b-a60b3a833787
📒 Files selected for processing (7)
docs/index.ymldocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/expected-machine-interfaces.mddocs/provisioning/ingesting-hosts.mddocs/provisioning/ip-and-network-configuration.md
Explain the interface roles and IP allocation policies available during Expected Machine ingestion. Cover segment selection, Host BMC compatibility, legacy aliases, update behavior, and reservation-only network segments. This supports NVIDIA#4151 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
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 `@docs/provisioning/expected-machine-interfaces.md`:
- Around line 132-136: Clarify the segment-guard wording in the explicit
allocation policy or DPU role section so rejection occurs only when a
network_segment_type guard is supplied and the selected segment has a different
type. State that the role alone does not impose a segment constraint, while
preserving the existing DHCP or fixed-address selection behavior and example.
- Around line 148-166: Update the “Allow Only Reserved Addresses” documentation
to explicitly describe how retained Expected Machine reservations behave on
reserved segments: state whether retained is unsupported or requires a
pre-seeded fixed reservation, and clarify the resulting interface
address-acquisition behavior so operators avoid an unusable configuration.
🪄 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: 11f7469a-3c68-4bb0-9ac6-06627d0e0f73
📒 Files selected for processing (7)
docs/index.ymldocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/expected-machine-interfaces.mddocs/provisioning/ingesting-hosts.mddocs/provisioning/ip-and-network-configuration.md
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/provisioning/ingesting-hosts.md
- docs/index.yml
- docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
- docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
- docs/provisioning/boot-interfaces-and-dpu-modes.md
- docs/provisioning/ip-and-network-configuration.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md`:
- Around line 145-166: The BMC allocation documentation omits the meaning of
unspecified and the invalid bmc_ip_address combinations. Update the
bmc-ip-allocation sections in
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 145-166 and
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 142-163 to define unspecified according to the server contract and state
that supplying bmc_ip_address with dynamic or retained allocation is rejected;
keep both pages consistent.
- Around line 67-78: Update the shared CLI source that generates the
expected-machine interface option documentation to identify --host_nics as the
legacy alias for --interfaces, then regenerate both pages:
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 67-78 and
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 165-175. Ensure both generated descriptions consistently document the
alias.
In
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md`:
- Around line 171-175: Update the patch description covering stored MAC matching
to explicitly state that omitting network_segment_type clears the stored segment
guard, distinguishing this from the preservation behavior documented for role
and ip_allocation.
In `@docs/provisioning/expected-machine-interfaces.md`:
- Around line 163-166: Clarify the Retained address behavior for
reservation-only segments in both
docs/provisioning/expected-machine-interfaces.md lines 163-166 and
docs/provisioning/ip-and-network-configuration.md lines 213-217: explicitly
state whether Retained is unsupported or requires a pre-seeded reservation, and
apply the same rule consistently in both procedures.
🪄 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: 5dde8219-53a5-4824-882e-3921ab392c32
📒 Files selected for processing (7)
docs/index.ymldocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.mddocs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.mddocs/provisioning/boot-interfaces-and-dpu-modes.mddocs/provisioning/expected-machine-interfaces.mddocs/provisioning/ingesting-hosts.mddocs/provisioning/ip-and-network-configuration.md
Comment on lines
+67
to
+78
| **--interfaces** *\<INTERFACES\>*\ | ||
| Interfaces as a JSON array of ExpectedInterface objects (fields: | ||
| mac_address, role, ip_allocation, network_segment_type, fixed_ip, | ||
| fixed_mask, fixed_gateway, primary; legacy: nic_type). Accepted values: | ||
| role=host\|dpu_os\|dpu_bmc\|host_bmc and | ||
| ip_allocation=dynamic\|fixed\|retained. An omitted role defaults to host. | ||
| When ip_allocation is omitted, fixed_ip implies fixed; without fixed_ip, | ||
| host_bmc defaults to retained and every other role defaults to dynamic. | ||
| Explicit fixed policies, DPU fixed addresses, and inferred host_bmc fixed | ||
| addresses with a segment guard must fall within a configured managed | ||
| prefix. Legacy host entries with an omitted policy and unguarded inferred | ||
| host_bmc fixed addresses keep the static-assignments fallback. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the legacy interface-option alias consistently.
Both generated pages advertise --interfaces but omit the supported --host_nics alias.
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md#L67-L78: document--host_nicsas the legacy alias for--interfaces.docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md#L165-L175: document the same alias and regenerate both pages from the shared CLI source.
📍 Affects 2 files
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md#L67-L78(this comment)docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md#L165-L175
🤖 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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md`
around lines 67 - 78, Update the shared CLI source that generates the
expected-machine interface option documentation to identify --host_nics as the
legacy alias for --interfaces, then regenerate both pages:
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 67-78 and
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 165-175. Ensure both generated descriptions consistently document the
alias.
Source: Learnings
Comment on lines
+145
to
+166
| **--bmc-ip-allocation** *\<BMC_IP_ALLOCATION\>*\ | ||
| Per-host control over how this BMC's IP is assigned and retained. | ||
| \`auto\` (default): infer from \`--bmc-ip-address\` -- a configured | ||
| address is \`fixed\`, no address is \`retained\`; \`dynamic\`: a normal | ||
| DHCP lease that may expire and change; \`fixed\`: the operator-specified | ||
| \`--bmc-ip-address\` (static); \`retained\`: an auto-allocated address | ||
| kept Static for the lifetime of its interface record. Unset defers to | ||
| the server default (\`auto\`). See | ||
| [Retained Address Lifetime](../../../../provisioning/expected-machine-interfaces.md#retained-address-lifetime).\ | ||
|
|
||
| \ | ||
| *Possible values:* | ||
|
|
||
| - unspecified | ||
|
|
||
| - auto | ||
|
|
||
| - dynamic | ||
|
|
||
| - fixed | ||
|
|
||
| - retained |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align BMC allocation documentation with the server contract.
Both pages list unspecified without defining it and omit the invalid combination rules for bmc_ip_address with Dynamic or Retained allocation.
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md#L145-L166: defineunspecifiedand state the rejected combinations for add.docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md#L142-L163: defineunspecifiedand state the same rejected combinations for patch.
📍 Affects 2 files
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md#L145-L166(this comment)docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md#L142-L163
🤖 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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md`
around lines 145 - 166, The BMC allocation documentation omits the meaning of
unspecified and the invalid bmc_ip_address combinations. Update the
bmc-ip-allocation sections in
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-add.md
lines 145-166 and
docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md
lines 142-163 to define unspecified according to the server contract and state
that supplying bmc_ip_address with dynamic or retained allocation is rejected;
keep both pages consistent.
Comment on lines
+171
to
+175
| machine's full interface list. For a matching stored MAC, omitting role | ||
| preserves the stored role; role=unspecified resets it to host. Omitting | ||
| ip_allocation preserves the stored policy when the presence of fixed_ip | ||
| is unchanged; ip_allocation=unspecified resets it to fixed_ip inference. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document that omitted network_segment_type clears the stored guard.
The patch description explains preservation/reset behavior for role and ip_allocation, but omits the distinct behavior for network_segment_type: when omitted, the stored segment guard is cleared. State this explicitly to prevent operators from assuming the guard is preserved.
🤖 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
`@docs/manuals/nico-admin-cli/commands/expected-machine/expected-machine-patch.md`
around lines 171 - 175, Update the patch description covering stored MAC
matching to explicitly state that omitting network_segment_type clears the
stored segment guard, distinguishing this from the preservation behavior
documented for role and ip_allocation.
Comment on lines
+163
to
+166
| On a reserved segment, configure a `fixed` Expected Machine reservation before | ||
| the interface sends DHCP. NICo does not create a dynamic address for an | ||
| interface that has no existing reservation. The default segment allocation | ||
| strategy is `dynamic`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define Retained behavior for reservation-only segments.
Both pages explain Fixed reservations but leave Retained address acquisition ambiguous on allocation_strategy = "reserved" segments.
docs/provisioning/expected-machine-interfaces.md#L163-L166: state whether Retained is unsupported or requires a pre-seeded reservation.docs/provisioning/ip-and-network-configuration.md#L213-L217: apply the same rule to the reservation-only configuration procedure.
📍 Affects 2 files
docs/provisioning/expected-machine-interfaces.md#L163-L166(this comment)docs/provisioning/ip-and-network-configuration.md#L213-L217
🤖 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 `@docs/provisioning/expected-machine-interfaces.md` around lines 163 - 166,
Clarify the Retained address behavior for reservation-only segments in both
docs/provisioning/expected-machine-interfaces.md lines 163-166 and
docs/provisioning/ip-and-network-configuration.md lines 213-217: explicitly
state whether Retained is unsupported or requires a pre-seeded reservation, and
apply the same rule consistently in both procedures.
Source: Path instructions
10 tasks
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.
Explain the interface roles and IP allocation policies available during Expected Machine ingestion.
Cover segment selection, Host BMC compatibility, legacy aliases, update behavior, and reservation-only network segments.
Related issues
Type of Change
Breaking Changes
Testing
Validated with:
cargo test -p nico-admin-cli cfg::cli_options::testscargo make format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
This is a docs-only PR so the documentation review can proceed independently from the implementation changes.
The generated CLI source help still says a
Retainedaddress never expires. These docs use the current behavior more precisely: the address remains Static for the lifetime of the interface record. A small code follow-up can align the source help text.