refactor(machine): separate Machine fields into config/status#2847
Open
ianderson-nvidia wants to merge 1 commit into
Open
refactor(machine): separate Machine fields into config/status#2847ianderson-nvidia wants to merge 1 commit into
ianderson-nvidia wants to merge 1 commit into
Conversation
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
June 24, 2026 17:03
bbb991e to
56875b9
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
Action performedFull review triggered. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR separates machine configuration from observed status in protobuf and Rust models, populates both structured and deprecated RPC fields, migrates runtime consumers to nested paths, and updates compatibility allowances plus broad regression coverage. ChangesMachine model and RPC contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2847.docs.buildwithfern.com/infra-controller |
rwthompsonii
approved these changes
Jun 24, 2026
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rpc/src/model/machine/mod.rs (1)
627-650: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMirror compatibility overrides into
status.*as well.
From<Machine>initializesstatus.healthas empty for hosts andstatus.associated_host_machine_idasNone; this method then patches only the deprecated flat fields. New clients reading the nestedstatusmessage will miss aggregate host health and DPU host association.Proposed fix
let mut rpc_machine: rpc::forge::Machine = self.host_snapshot.clone().into(); let state = &self.host_snapshot.state.value; let version = &self.host_snapshot.state.version; - rpc_machine.health = Some(self.aggregate_health.clone().into()); + let aggregate_health = self.aggregate_health.clone().into(); + rpc_machine.health = Some(aggregate_health.clone()); + if let Some(status) = rpc_machine.status.as_mut() { + status.health = Some(aggregate_health); + } @@ let mut rpc_machine: rpc::forge::Machine = dpu_snapshot.clone().into(); // In case the DPU does not know the associated Host - we can backfill the data here - rpc_machine.associated_host_machine_id = Some(self.host_snapshot.id); + let host_machine_id = self.host_snapshot.id; + rpc_machine.associated_host_machine_id = Some(host_machine_id); + if let Some(status) = rpc_machine.status.as_mut() { + status.associated_host_machine_id = Some(host_machine_id); + }🤖 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/rpc/src/model/machine/mod.rs` around lines 627 - 650, Mirror the host/DPU compatibility overrides into the nested status object in the machine conversion path: in the branch that builds rpc::forge::Machine from host_snapshot or dpu_snapshot, update both the deprecated flat fields and the corresponding rpc_machine.status fields so new clients see the same health and associated_host_machine_id values; use the existing conversion logic in the machine builder around state_sla and associated_host_machine_id as the place to patch status.health and status.associated_host_machine_id too.
🧹 Nitpick comments (1)
crates/api-web/src/lib.rs (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow deprecated allowance scope.
Line 20 uses a file-wide
#![allow(deprecated)]; please scope this to the smallest item(s) that still need legacy field access so unrelated deprecation regressions remain visible.
As per coding guidelines, “Avoid using#[allow(...)]unless you have a strong reason to do so,” and as per path instructions, prefer clippy-clean changes without broad allows.🤖 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/api-web/src/lib.rs` around lines 18 - 21, The deprecated allowance is too broad because `#![allow(deprecated)]` suppresses warnings for the entire crate. Move the deprecation allowance to the smallest relevant item that reads legacy `rpc::forge::Machine` fields in `crates/api-web/src/lib.rs`, and keep the rest of the crate deprecation-clean so unrelated regressions still surface.Sources: Coding guidelines, Path instructions
🤖 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/instance/mod.rs`:
- Around line 1155-1161: The validation in `updated_network_config.interfaces`
is reporting the wrong field path in its `ConfigValidationError::InvalidValue`
message. Update the error text in `crates/api-core/src/instance/mod.rs` within
the `InstanceNetworkConfig` validation branch so it references the actual field
being checked, not `InstanceNetworkConfig.status.interfaces`, and keep the
machine id context intact.
In `@crates/rpc/proto/forge.proto`:
- Around line 3512-3514: The comment on Machine.interfaces is stale and
contradicts the actual deprecation of field 9. Update the note near the Machine
definition to match the [deprecated = true] annotation and the use
status.interfaces directive, or remove the misleading guidance entirely. Make
sure the wording around Machine and MachineInterface clearly reflects the
current wire contract so there is no ambiguity about whether interfaces remains
supported or reserved.
In `@crates/rpc/src/model/machine/mod.rs`:
- Around line 335-395: Clone the shared RPC values before they are reused in
`rpc::Machine` construction: `dpf` and `placement_in_rack` are moved into
`rpc::forge::MachineConfig` and `rpc::forge::MachineStatus`, so update the
`Machine` builder in `crates/rpc/src/model/machine/mod.rs` to pass clones there
and keep the original values available for the flat fields. Leave
`last_observation_time` unchanged since it is `Copy`.
---
Outside diff comments:
In `@crates/rpc/src/model/machine/mod.rs`:
- Around line 627-650: Mirror the host/DPU compatibility overrides into the
nested status object in the machine conversion path: in the branch that builds
rpc::forge::Machine from host_snapshot or dpu_snapshot, update both the
deprecated flat fields and the corresponding rpc_machine.status fields so new
clients see the same health and associated_host_machine_id values; use the
existing conversion logic in the machine builder around state_sla and
associated_host_machine_id as the place to patch status.health and
status.associated_host_machine_id too.
---
Nitpick comments:
In `@crates/api-web/src/lib.rs`:
- Around line 18-21: The deprecated allowance is too broad because
`#![allow(deprecated)]` suppresses warnings for the entire crate. Move the
deprecation allowance to the smallest relevant item that reads legacy
`rpc::forge::Machine` fields in `crates/api-web/src/lib.rs`, and keep the rest
of the crate deprecation-clean so unrelated regressions still surface.
🪄 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: 46e9ffb3-783e-4c80-a612-54a3bf7f2f24
📒 Files selected for processing (86)
crates/admin-cli/src/main.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/connected_device.rscrates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpu_agent_upgrade.rscrates/api-core/src/tests/dpu_machine_inventory.rscrates/api-core/src/tests/dpu_nic_firmware.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/explored_endpoint_find.rscrates/api-core/src/tests/finder.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/ib_fabric_monitor.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/ib_machine.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_bmc_metadata.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/machine_health.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/machine_topology.rscrates/api-core/src/tests/machine_validation.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/nvl_instance.rscrates/api-core/src/tests/redfish_actions.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/sku.rscrates/api-db/src/dpu_machine_update.rscrates/api-db/src/instance_address.rscrates/api-db/src/machine.rscrates/api-db/src/sku.rscrates/api-model/src/dpu_machine_update.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/machine/config.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/status.rscrates/api-web/src/lib.rscrates/api-web/src/managed_host.rscrates/dpa-manager/src/card_handler/svpc.rscrates/health/src/api_client.rscrates/ib-fabric/src/lib.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/nvlink-manager/src/lib.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/rvs/src/client/mod.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/health_report.rscrates/site-explorer/tests/machine_creator.rscrates/site-explorer/tests/site_explorer.rscrates/ssh-console-mock-api-server/src/lib.rs
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/machine-controller/src/handler/sku.rs (1)
75-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat a missing
last_match_attemptas eligible for matching.Line 75 only retries when
hw_sku_statusis absent or carries an old timestamp. If the status row exists butlast_match_attemptis stillNone—for example after anotherhw_sku_statusfield was populated first—this returnsOk(None)forever and BOM validation stops trying to discover a SKU.Suggested fix
- if sku_status.is_none() - || sku_status.is_some_and(|ss| { - ss.last_match_attempt.is_some_and(|t| { - t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) - }) - }) + if sku_status.is_none() + || sku_status.is_some_and(|ss| { + ss.last_match_attempt.map_or(true, |t| { + t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) + }) + })
🧹 Nitpick comments (1)
crates/api-web/src/lib.rs (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer a narrowly-scoped suppression over a crate-wide
#![allow(deprecated)].Because this attribute resides in
lib.rs, the inner attribute suppresses deprecation diagnostics across the entire crate, not merely the specific reads of the deprecatedrpc::forge::Machineflat fields. This is a blunt instrument: it will silently mask any unrelated deprecation introduced into this crate in the future, eroding the value of the lint. The upstream contract site (crates/rpc/src/model/machine/mod.rs) models the preferred approach by attaching#[allow(deprecated)]to the specificimpl. Consider applying the allowance to the smallest enclosing item(s) that actually read the deprecated fields instead.As per path instructions (STYLE_GUIDE.md): "only suppress deprecation warnings when deprecated fields must still be read for backward compatibility, and keep the suppression as narrow/specific as possible."
🤖 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/api-web/src/lib.rs` around lines 18 - 20, The crate-wide deprecated suppression in lib.rs is too broad and should be narrowed to only the code that actually reads the deprecated rpc::forge::Machine flat fields. Move the allowance from the inner crate attribute to the smallest enclosing item(s) in the relevant read path, following the pattern used in rpc::model::machine::mod.rs with a scoped #[allow(deprecated)] on the specific impl or function. This keeps backward-compat reads working without masking unrelated deprecations across the whole crate.Source: Path instructions
🤖 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/admin-cli/src/main.rs`:
- Around line 20-22: The crate-level deprecation allow in main is too broad;
narrow it to only the compatibility code that still reads deprecated
rpc::forge::Machine fields. Move the suppression onto the specific
impls/functions that perform those reads, keeping the rest of the CLI entrypoint
under normal deprecated-warning checks. Use the existing Machine compatibility
read sites as the target for the scoped allow so accidental new deprecated usage
still fails the build.
In `@crates/api-core/src/tests/finder.rs`:
- Around line 76-83: The current assertions in the `finder.rs` tests only cover
the nested `status`/`config` paths and no longer verify the deprecated flat
`forge::Machine` compatibility fields. Update one RPC-facing test that loads a
`forge::Machine` from the same fixture and assert both the nested
`status/config` values and the flat aliases are populated consistently, using
the existing `host_machine`/`machine` mapping checks in the affected test cases.
In `@crates/api-core/src/tests/machine_health.rs`:
- Around line 961-964: The host aggregate-health test helper currently reads
from status.health, but find_machines_by_ids still populates the real aggregated
host health on the deprecated top-level health field. Update the aggregate
helper in machine_health tests to assert against machine.health for host-path
cases, or alternatively adjust ManagedHostStateSnapshotRpc::rpc_machine_state
and the From<Machine> flow so status.health is also overwritten with the
aggregated report; keep the test target aligned with the field that actually
carries the aggregate.
In `@crates/api-db/src/machine.rs`:
- Line 2753: The test in machine.rs only checks that firmware_autoupdate is
present, so it can pass even if set_firmware_autoupdate writes the wrong value.
Update the assertion in the affected test to verify the actual value returned in
host.config.firmware_autoupdate, comparing it directly against Some(true) after
calling set_firmware_autoupdate(..., Some(true)).
In `@crates/api-model/src/machine/config.rs`:
- Around line 27-43: MachineConfig is missing the new maintenance fields, so the
Rust model no longer matches the nested config contract. Add
maintenance_reference and maintenance_start_time to MachineConfig and make sure
the JSON/persistence loading and RPC serialization paths carry them through
consistently, rather than reconstructing them from health_reports. If these
values are meant to be system-generated, keep them out of the user-controlled
config model and align the contract in the MachineConfig/loader code
accordingly.
In `@crates/rpc/src/model/machine/mod.rs`:
- Around line 616-619: Backfill the canonical nested status fields in the
`From<Machine>` / `rpc_machine_state()` path, not just the deprecated flat
aliases. Update the `Machine` -> RPC conversion so
`status.associated_host_machine_id` is populated consistently and host
`status.health` reflects the real machine health instead of a placeholder, while
still writing the deprecated `health` and `associated_host_machine_id` aliases
for backwards compatibility. Use the existing `rpc_machine_state` and
`From<Machine>` symbols to locate the conversion logic and keep both nested and
flat fields in sync.
In `@crates/ssh-console-mock-api-server/src/lib.rs`:
- Around line 49-51: The mock `forge::Machine` conversion is only setting the
deprecated flat `discovery_info` alias, but production now populates
`Machine.status.discovery_info` as the canonical source. Update the
machine-building logic in `ssh-console-mock-api-server` to fill the nested
`status` sub-message first, then mirror the same discovery info into the
deprecated flat field for compatibility. Use the `forge::Machine` construction
path in this module to ensure migrated callers see the same shape as the real
converter.
---
Nitpick comments:
In `@crates/api-web/src/lib.rs`:
- Around line 18-20: The crate-wide deprecated suppression in lib.rs is too
broad and should be narrowed to only the code that actually reads the deprecated
rpc::forge::Machine flat fields. Move the allowance from the inner crate
attribute to the smallest enclosing item(s) in the relevant read path, following
the pattern used in rpc::model::machine::mod.rs with a scoped
#[allow(deprecated)] on the specific impl or function. This keeps
backward-compat reads working without masking unrelated deprecations across the
whole crate.
🪄 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: c04e057f-d5ab-4b8f-8064-00c890dc5471
📒 Files selected for processing (86)
crates/admin-cli/src/main.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/connected_device.rscrates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpu_agent_upgrade.rscrates/api-core/src/tests/dpu_machine_inventory.rscrates/api-core/src/tests/dpu_nic_firmware.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/explored_endpoint_find.rscrates/api-core/src/tests/finder.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/ib_fabric_monitor.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/ib_machine.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_bmc_metadata.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/machine_health.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/machine_topology.rscrates/api-core/src/tests/machine_validation.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/nvl_instance.rscrates/api-core/src/tests/redfish_actions.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/sku.rscrates/api-db/src/dpu_machine_update.rscrates/api-db/src/instance_address.rscrates/api-db/src/machine.rscrates/api-db/src/sku.rscrates/api-model/src/dpu_machine_update.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/machine/config.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/status.rscrates/api-web/src/lib.rscrates/api-web/src/managed_host.rscrates/dpa-manager/src/card_handler/svpc.rscrates/health/src/api_client.rscrates/ib-fabric/src/lib.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/nvlink-manager/src/lib.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/rvs/src/client/mod.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/health_report.rscrates/site-explorer/tests/machine_creator.rscrates/site-explorer/tests/site_explorer.rscrates/ssh-console-mock-api-server/src/lib.rs
Matthias247
reviewed
Jun 24, 2026
Matthias247
left a comment
Matthias247
left a comment
Contributor
There was a problem hiding this comment.
Thanks for taking this big work item on!
It's definitely great to clear this up. I'm not sure yet whether we really want to do it in one go, or migrate certain fields for which we are more sure where they should go.
Added some more detailed question on this on the PR. I think we might need to document some concrete rules of "what goes where" to get the answers.
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
June 24, 2026 18:10
56875b9 to
b8a2a82
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/common/api_fixtures/test_managed_host.rs`:
- Around line 55-68: `TestManagedHost::from_rpc_machine` is dropping legacy DPU
information when `m.status` is absent, which leaves `dpu_ids` empty and can
break existing fixture behavior. Update the `dpu_ids` construction to preserve
the current `status.interfaces` path but fall back to the legacy top-level
`m.interfaces` when `status` is `None` (or otherwise unavailable), so
compatibility is maintained during the transition window.
🪄 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: 17a73e76-543f-45a5-a3b4-026175870f37
📒 Files selected for processing (87)
crates/admin-cli/src/main.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_managed_host.rscrates/api-core/src/tests/connected_device.rscrates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpu_agent_upgrade.rscrates/api-core/src/tests/dpu_machine_inventory.rscrates/api-core/src/tests/dpu_nic_firmware.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/expected_machine.rscrates/api-core/src/tests/explored_endpoint_find.rscrates/api-core/src/tests/finder.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/ib_fabric_monitor.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/ib_machine.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_bmc_metadata.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/machine_health.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/machine_topology.rscrates/api-core/src/tests/machine_validation.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/nvl_instance.rscrates/api-core/src/tests/redfish_actions.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/sku.rscrates/api-db/src/dpu_machine_update.rscrates/api-db/src/instance_address.rscrates/api-db/src/machine.rscrates/api-db/src/sku.rscrates/api-model/src/dpu_machine_update.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/machine/config.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/status.rscrates/api-web/src/lib.rscrates/api-web/src/managed_host.rscrates/dpa-manager/src/card_handler/svpc.rscrates/health/src/api_client.rscrates/ib-fabric/src/lib.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/nvlink-manager/src/lib.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/rvs/src/client/mod.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/health_report.rscrates/site-explorer/tests/machine_creator.rscrates/site-explorer/tests/site_explorer.rscrates/ssh-console-mock-api-server/src/lib.rs
✅ Files skipped from review due to trivial changes (13)
- crates/site-explorer/tests/health_report.rs
- crates/api-core/src/tests/dpf/happy_path.rs
- crates/machine-controller/src/handler/power.rs
- crates/rvs/src/client/mod.rs
- crates/admin-cli/src/main.rs
- crates/rpc-utils/src/managed_host_display.rs
- crates/dpa-manager/src/card_handler/svpc.rs
- crates/api-core/src/tests/machine_bmc_metadata.rs
- crates/api-db/src/machine.rs
- crates/api-model/src/machine/capabilities.rs
- crates/ssh-console-mock-api-server/src/lib.rs
- crates/api-web/src/lib.rs
- crates/health/src/api_client.rs
🚧 Files skipped from review as they are similar to previous changes (73)
- crates/api-core/src/tests/dpu_agent_upgrade.rs
- crates/api-model/src/dpu_machine_update.rs
- crates/api-core/src/tests/explored_endpoint_find.rs
- crates/api-core/src/handlers/sku.rs
- crates/api-core/src/handlers/dpf.rs
- crates/api-core/src/tests/connected_device.rs
- crates/machine-controller/src/handler/machine_validation.rs
- crates/api-core/src/tests/common/api_fixtures/host.rs
- crates/api-core/src/tests/redfish_actions.rs
- crates/api-model/src/machine/config.rs
- crates/api-core/src/handlers/instance_type.rs
- crates/machine-controller/src/handler/attestation.rs
- crates/api-core/src/tests/ib_machine.rs
- crates/api-core/src/dpa/handler.rs
- crates/api-core/src/tests/dpu_machine_inventory.rs
- crates/api-core/src/handlers/attestation.rs
- crates/api-db/src/sku.rs
- crates/api-core/src/tests/machine_discovery.rs
- crates/api-core/src/tests/finder.rs
- crates/api-core/src/tests/machine_network.rs
- crates/api-core/src/tests/expected_machine.rs
- crates/api-core/src/tests/ib_fabric_monitor.rs
- crates/api-core/src/handlers/instance.rs
- crates/api-core/src/ipxe.rs
- crates/api-core/src/attestation/mod.rs
- crates/api-core/src/tests/machine_topology.rs
- crates/api-core/src/tests/machine_admin_force_delete.rs
- crates/api-model/src/machine/status.rs
- crates/api-core/src/ethernet_virtualization.rs
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/api-core/src/tests/instance.rs
- crates/api-core/src/tests/maintenance.rs
- crates/machine-controller/src/handler/bios_config.rs
- crates/machine-controller/src/handler/dpf.rs
- crates/api-core/src/handlers/managed_host.rs
- crates/api-core/src/tests/machine_validation.rs
- crates/site-explorer/tests/site_explorer.rs
- crates/api-core/src/tests/instance_type.rs
- crates/api-web/src/managed_host.rs
- crates/api-db/src/dpu_machine_update.rs
- crates/api-core/src/tests/ib_instance.rs
- crates/rpc/build.rs
- crates/api-core/src/handlers/machine_scout.rs
- crates/api-db/src/instance_address.rs
- crates/api-core/src/tests/machine_find.rs
- crates/api-core/src/tests/ipxe.rs
- crates/api-core/src/tests/machine_dhcp.rs
- crates/api-core/src/handlers/component_manager.rs
- crates/api-core/src/tests/machine_health.rs
- crates/site-explorer/src/machine_creator.rs
- crates/api-core/src/handlers/machine.rs
- crates/api-core/src/tests/common/api_fixtures/site_explorer.rs
- crates/api-core/src/tests/dpu_nic_firmware.rs
- crates/api-core/src/tests/instance_allocate.rs
- crates/api-core/src/tests/host_bmc_firmware_test.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-core/src/handlers/dpu.rs
- crates/api-core/src/instance/mod.rs
- crates/nvlink-manager/src/lib.rs
- crates/api-core/src/tests/site_explorer.rs
- crates/ib-fabric/src/lib.rs
- crates/api-core/src/tests/sku.rs
- crates/api-core/src/tests/nvl_instance.rs
- crates/api-model/src/machine/json.rs
- crates/machine-controller/src/handler/sku.rs
- crates/api-core/src/tests/machine_states.rs
- crates/api-core/src/tests/machine_interfaces.rs
- crates/api-core/src/tests/dpu_reprovisioning.rs
- crates/site-explorer/tests/machine_creator.rs
- crates/api-model/src/machine/mod.rs
- crates/rpc/src/model/machine/mod.rs
- crates/rpc/proto/forge.proto
- crates/machine-controller/src/handler.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
5 times, most recently
from
June 24, 2026 23:34
7f24a5e to
af8b56f
Compare
ianderson-nvidia
changed the title
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/handlers/sku.rs (1)
121-129: 🎯 Functional Correctness | 🟠 MajorAvoid moving
machine.config.hw_skuhere
machine.config.hw_skuis anOption<String>, so this pattern moves the SKU out ofmachineand the latermachine.current_state()borrow will fail. Usemachine.config.hw_sku.as_ref()here, or clone only if ownership is required.🤖 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/api-core/src/handlers/sku.rs` around lines 121 - 129, The check in the `sku` handler is moving `machine.config.hw_sku` out of `machine`, which breaks the later borrow used by `machine.current_state()`. Update the `if let Some(...)` guard in this block to borrow the SKU from `machine.config.hw_sku` instead of taking ownership, using `as_ref()` (or cloning only if an owned value is truly needed) so `machine` remains usable for the subsequent state check.
♻️ Duplicate comments (2)
crates/rpc/src/model/machine/mod.rs (1)
347-357: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winClone generated RPC messages before reusing them.
dpfis moved intoconfig_msgand then reused for the flatMachine.dpf;slais moved intostate_slaand then reused forstatus.lifecycle.sla. Generated prost messages areClone, notCopy, unless explicitly configured, so this will fail to compile.Proposed fix
let config_msg = rpc::forge::MachineConfig { maintenance_reference: maintenance_reference.clone(), maintenance_start_time: maintenance_start_time.map(rpc::Timestamp::from), firmware_autoupdate: machine.config.firmware_autoupdate, instance_type_id: machine .config .instance_type_id .as_ref() .map(|i| i.to_string()), - dpf, + dpf: dpf.clone(), }; @@ - rpc_machine.state_sla = Some(sla); + rpc_machine.state_sla = Some(sla.clone()); if let Some(status) = rpc_machine.status.as_mut() { status.health = Some(self.aggregate_health.clone().into()); if let Some(lifecycle) = status.lifecycle.as_mut() { lifecycle.sla = Some(sla); @@ - rpc_machine.state_sla = Some(sla); + rpc_machine.state_sla = Some(sla.clone()); if let Some(status) = rpc_machine.status.as_mut() { status.associated_host_machine_id = Some(self.host_snapshot.id); if let Some(lifecycle) = status.lifecycle.as_mut() { lifecycle.sla = Some(sla);#!/bin/bash set -euo pipefail # Verify the values are assigned in multiple places and that build.rs does not add Copy derives. sed -n '340,480p' crates/rpc/src/model/machine/mod.rs | nl -ba sed -n '632,678p' crates/rpc/src/model/machine/mod.rs | nl -ba rg -n 'DpfMachineState|StateSla|Copy' crates/rpc/build.rsAlso applies to: 473-477, 637-652, 661-677
🤖 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/rpc/src/model/machine/mod.rs` around lines 347 - 357, The generated prost messages used in `Machine::from`/related mapping code are being moved and then reused, which will not compile because they are Clone but not Copy. Update the assignments for `dpf` and `sla` so each place that needs the value gets its own clone before reuse, especially where `config_msg`, the flat `Machine.dpf`, `state_sla`, and `status.lifecycle.sla` are populated. Use the existing symbols like `MachineConfig`, `Machine`, and `status.lifecycle` to locate all reuse sites and clone the RPC message values before the second assignment.crates/ssh-console-mock-api-server/src/lib.rs (1)
49-64: 🗄️ Data Integrity & Integration | 🟠 MajorPopulate the nested
status.discovery_infoin the mock response too.This mock still emits only the deprecated flat
forge::Machine.discovery_info. Production now fillsMachine.status.discovery_infoand mirrors it into the flat alias, so migrated callers will observe a different payload shape here than they do against the real converter.Proposed fix
#[allow(deprecated)] impl From<MockHost> for forge::Machine { fn from(value: MockHost) -> Self { + let discovery_info = machine_discovery::DiscoveryInfo { + dmi_data: Some(machine_discovery::DmiData { + sys_vendor: value.sys_vendor.to_string(), + ..Default::default() + }), + ..Default::default() + }; + Self { id: Some(value.machine_id), - discovery_info: Some(machine_discovery::DiscoveryInfo { - dmi_data: Some(machine_discovery::DmiData { - sys_vendor: value.sys_vendor.to_string(), - ..Default::default() - }), - ..Default::default() - }), + discovery_info: Some(discovery_info.clone()), + status: Some(forge::MachineStatus { + discovery_info: Some(discovery_info), + ..Default::default() + }), ..Default::default() } } }🤖 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/ssh-console-mock-api-server/src/lib.rs` around lines 49 - 64, The MockHost to forge::Machine conversion currently sets only the deprecated flat discovery_info field, so update the From<MockHost> implementation to also populate Machine.status.discovery_info with the same DiscoveryInfo payload and keep the flat alias mirrored from it. Use the existing MockHost, forge::Machine, and machine_discovery::DiscoveryInfo construction in this conversion so the mock response matches production’s nested status shape.
🧹 Nitpick comments (4)
crates/api-model/src/machine/status.rs (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that this is the internal status model, not a 1:1 proto mirror.
This struct contains internal fields that are not present in
forge.MachineStatus, so “Corresponds toMachineStatusin the protobuf” is misleading during this migration. Please make the comment describe it as the internal status container that feeds both nested RPC status and legacy flat RPC fields.Also applies to: 32-55
🤖 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/api-model/src/machine/status.rs` around lines 28 - 30, Update the doc comment on MachineStatus to describe it as the internal status container rather than a direct protobuf mirror; the current wording in the MachineStatus type comment is misleading because this struct has extra internal fields and feeds both nested RPC status and legacy flat RPC fields. Adjust the comment near MachineStatus and any related field docs in this block so it clearly says the model is internal and used for both response shapes, not a 1:1 forge.MachineStatus mapping.crates/api-model/src/machine/config.rs (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
MachineConfigdocs with the actual split.The struct-level comment says this mirrors proto
MachineConfigand only contains operator-mutated fields, butrack_idandhw_skuare intentionally internal-only and not in the proto config. Please reword this to avoid implying a 1:1 proto/config contract. As per path instructions, API/model changes should keep config/status contracts explicit and unambiguous.Also applies to: 37-44
🤖 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/api-model/src/machine/config.rs` around lines 22 - 28, The struct-level documentation for MachineConfig is misleading because it implies a direct 1:1 match with the proto config while the actual model includes internal-only fields like rack_id and hw_sku. Reword the comments on MachineConfig and its field docs to clearly describe the split between operator-mutated config, internal ingestion-only fields on Machine, and any proto-aligned settings, using the MachineConfig type and related field comments as the location to update. Keep the contract explicit and unambiguous without claiming that every field maps directly to the forge proto.Source: Path instructions
crates/api-web/src/network_status.rs (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow deprecated suppression scope.
Line 20 uses a module-wide
#![allow(deprecated)], which can hide new deprecated usages unrelated to this migration. Prefer function/block-level#[allow(deprecated)]around only the legacy flat-field reads.As per path instructions, "Avoid
#[allow(deprecated)]for new code unless there is a strong compatibility requirement; suppress only where deprecated flat fields must still be read for the migration period."🤖 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/api-web/src/network_status.rs` around lines 17 - 20, Narrow the deprecated suppression in network_status by removing the module-wide `#![allow(deprecated)]` and applying `#[allow(deprecated)]` only around the legacy flat-field reads that still access `rpc::forge::Machine` deprecated fields. Keep the compatibility exception localized to the specific code paths in this module that must read the old fields during the migration, so new code in the module still surfaces deprecated usages.Source: Path instructions
crates/api-web/src/machine.rs (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the deprecation suppression to the specific compatibility reads.
A file-level
#![allow(deprecated)]also suppresses unrelated deprecated usage added later in this module. Please scope the allow to the exact helper or block still reading the flatrpc::forge::Machinefields so the rest of the handler keeps normal lint coverage.As per path instructions,
crates/**/*.rsshould remain “clippy-clean changes without broad allows”, and STYLE_GUIDE says to “suppress only where deprecated flat fields must still be read for the migration period.”🤖 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/api-web/src/machine.rs` around lines 17 - 20, Narrow the deprecation suppression in machine.rs so only the compatibility code that still reads the deprecated flat rpc::forge::Machine fields is allowed, instead of applying a file-level allow. Move the allow onto the specific helper or smallest block that performs those reads, keeping the rest of the module under normal deprecation lint coverage. Use the existing compatibility read path for status/config migration as the location to scope the allowance, and remove the broad crate-level attribute from the module header.Source: Path instructions
🤖 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/rvs/src/client/mod.rs`:
- Around line 1-3: The crate-level deprecated allowance is too broad and
suppresses warnings across all of `crates/rvs` instead of only for the legacy
compatibility read. Remove the root `#![allow(deprecated)]` from the client
crate and move the suppression to the smallest enclosing item that performs the
flat-field read in `rack/io.rs`, so only that compatibility path is exempt. Keep
the scope limited to the specific read of the deprecated `rpc::forge::Machine`
fields and leave all other deprecated uses warning normally.
---
Outside diff comments:
In `@crates/api-core/src/handlers/sku.rs`:
- Around line 121-129: The check in the `sku` handler is moving
`machine.config.hw_sku` out of `machine`, which breaks the later borrow used by
`machine.current_state()`. Update the `if let Some(...)` guard in this block to
borrow the SKU from `machine.config.hw_sku` instead of taking ownership, using
`as_ref()` (or cloning only if an owned value is truly needed) so `machine`
remains usable for the subsequent state check.
---
Duplicate comments:
In `@crates/rpc/src/model/machine/mod.rs`:
- Around line 347-357: The generated prost messages used in
`Machine::from`/related mapping code are being moved and then reused, which will
not compile because they are Clone but not Copy. Update the assignments for
`dpf` and `sla` so each place that needs the value gets its own clone before
reuse, especially where `config_msg`, the flat `Machine.dpf`, `state_sla`, and
`status.lifecycle.sla` are populated. Use the existing symbols like
`MachineConfig`, `Machine`, and `status.lifecycle` to locate all reuse sites and
clone the RPC message values before the second assignment.
In `@crates/ssh-console-mock-api-server/src/lib.rs`:
- Around line 49-64: The MockHost to forge::Machine conversion currently sets
only the deprecated flat discovery_info field, so update the From<MockHost>
implementation to also populate Machine.status.discovery_info with the same
DiscoveryInfo payload and keep the flat alias mirrored from it. Use the existing
MockHost, forge::Machine, and machine_discovery::DiscoveryInfo construction in
this conversion so the mock response matches production’s nested status shape.
---
Nitpick comments:
In `@crates/api-model/src/machine/config.rs`:
- Around line 22-28: The struct-level documentation for MachineConfig is
misleading because it implies a direct 1:1 match with the proto config while the
actual model includes internal-only fields like rack_id and hw_sku. Reword the
comments on MachineConfig and its field docs to clearly describe the split
between operator-mutated config, internal ingestion-only fields on Machine, and
any proto-aligned settings, using the MachineConfig type and related field
comments as the location to update. Keep the contract explicit and unambiguous
without claiming that every field maps directly to the forge proto.
In `@crates/api-model/src/machine/status.rs`:
- Around line 28-30: Update the doc comment on MachineStatus to describe it as
the internal status container rather than a direct protobuf mirror; the current
wording in the MachineStatus type comment is misleading because this struct has
extra internal fields and feeds both nested RPC status and legacy flat RPC
fields. Adjust the comment near MachineStatus and any related field docs in this
block so it clearly says the model is internal and used for both response
shapes, not a 1:1 forge.MachineStatus mapping.
In `@crates/api-web/src/machine.rs`:
- Around line 17-20: Narrow the deprecation suppression in machine.rs so only
the compatibility code that still reads the deprecated flat rpc::forge::Machine
fields is allowed, instead of applying a file-level allow. Move the allow onto
the specific helper or smallest block that performs those reads, keeping the
rest of the module under normal deprecation lint coverage. Use the existing
compatibility read path for status/config migration as the location to scope the
allowance, and remove the broad crate-level attribute from the module header.
In `@crates/api-web/src/network_status.rs`:
- Around line 17-20: Narrow the deprecated suppression in network_status by
removing the module-wide `#![allow(deprecated)]` and applying
`#[allow(deprecated)]` only around the legacy flat-field reads that still access
`rpc::forge::Machine` deprecated fields. Keep the compatibility exception
localized to the specific code paths in this module that must read the old
fields during the migration, so new code in the module still surfaces deprecated
usages.
🪄 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: cae1aeb8-7928-442f-a2ad-a527e32e9e37
📒 Files selected for processing (99)
crates/admin-cli/src/debug_bundle/cmds.rscrates/admin-cli/src/dpu/network/cmd.rscrates/admin-cli/src/dpu/reprovision/cmd.rscrates/admin-cli/src/dpu/status/cmd.rscrates/admin-cli/src/dpu/versions/cmd.rscrates/admin-cli/src/host/reprovision/cmd.rscrates/admin-cli/src/inventory/cmds.rscrates/admin-cli/src/machine/hardware_info/cmd.rscrates/admin-cli/src/machine/metadata/cmd.rscrates/admin-cli/src/machine/nvlink_info/cmd.rscrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/managed_host/show/cmd.rscrates/admin-cli/src/rpc.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_managed_host.rscrates/api-core/src/tests/connected_device.rscrates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpu_agent_upgrade.rscrates/api-core/src/tests/dpu_nic_firmware.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/finder.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/ib_fabric_monitor.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/ib_machine.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/machine_health.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/machine_topology.rscrates/api-core/src/tests/machine_validation.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/nvl_instance.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/sku.rscrates/api-db/src/dpu_machine_update.rscrates/api-db/src/instance_address.rscrates/api-db/src/machine.rscrates/api-db/src/sku.rscrates/api-model/src/dpu_machine_update.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/machine/config.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/status.rscrates/api-web/src/dpu_versions.rscrates/api-web/src/health.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/network_status.rscrates/api-web/src/nvlink.rscrates/api-web/src/redfish_browser.rscrates/dpa-manager/src/card_handler/svpc.rscrates/health/src/api_client.rscrates/ib-fabric/src/lib.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/nvlink-manager/src/lib.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/rvs/src/client/mod.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/health_report.rscrates/site-explorer/tests/machine_creator.rscrates/site-explorer/tests/site_explorer.rscrates/ssh-console-mock-api-server/src/lib.rs
Matthias247
reviewed
Jun 25, 2026
Matthias247
left a comment
Matthias247
left a comment
Contributor
There was a problem hiding this comment.
Small comments on the proto. haven't yet looked into implementation changes
ianderson-nvidia
marked this pull request as draft
June 25, 2026 17:11
|
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. |
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
2 times, most recently
from
June 25, 2026 18:22
df2fc21 to
f363603
Compare
Contributor
|
@ianderson-nvidia any update here? I thought the last iterations were pretty good |
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 14, 2026 22:03
f363603 to
222fbec
Compare
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 14, 2026 22:29
222fbec to
b48546b
Compare
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
2 times, most recently
from
July 14, 2026 23:54
ba83faa to
eee09b3
Compare
Matthias247
reviewed
Jul 15, 2026
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 15, 2026 20:41
eee09b3 to
37bc474
Compare
ianderson-nvidia
marked this pull request as ready for review
July 15, 2026 22:54
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-15 22:56:31 UTC | Commit: 37bc474 |
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
3 times, most recently
from
July 16, 2026 17:38
af236e1 to
01d3373
Compare
Contributor
Author
Hey @Matthias247 whenever you get a second, i pushed some changes and made tests pass (finally). PTAL |
Contributor
Author
|
@coderabbitai PTAL |
Contributor
|
✅ Action performedFull review finished. |
Matthias247
approved these changes
Jul 16, 2026
Matthias247
left a comment
Matthias247
left a comment
Contributor
There was a problem hiding this comment.
some mini suggestions. But everything else in the RPC change looks great to me!
I didn't look at the implementation in depth - but I assume its just redirecting everything to use the new fields.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rpc/proto/forge.proto (1)
3778-3810: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBoth protobuf contracts document nonexistent replacement fields. The canonical names are
status.infinibandandstatus.hw_sku.
crates/rpc/proto/forge.proto#L3778-L3810: replacestatus.ib_statusandstatus.hw_sku_status.rest-api/proto/core/src/v1/nico_nico.proto#L3777-L3809: apply the identical correction to the REST mirror.As per path instructions, protobuf naming and generated-client impact must remain clear and stable.
🤖 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/rpc/proto/forge.proto` around lines 3778 - 3810, Correct the deprecated replacement references in the machine protobuf fields: in crates/rpc/proto/forge.proto lines 3778-3810, change status.ib_status to status.infiniband and status.hw_sku_status to status.hw_sku; apply the identical documentation correction in rest-api/proto/core/src/v1/nico_nico.proto lines 3777-3809. Preserve the protobuf field names and numbering so generated-client contracts remain stable.Source: Path instructions
♻️ Duplicate comments (1)
crates/rpc/src/model/machine/mod.rs (1)
342-342: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winClone
dpfbefore mirroring it into both contracts.Line 342 moves the generated
DpfMachineStateintoconfig_msg, then Line 455 reuses it for the deprecated alias.Proposed fix
- dpf, + dpf: dpf.clone(),#!/bin/bash set -euo pipefail rg -n -C3 \ 'DpfMachineState|type_attribute\(.*DpfMachineState|derive\(.*Copy' \ crates/rpcAlso applies to: 455-455
🤖 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/rpc/src/model/machine/mod.rs` at line 342, Clone dpf before assigning it to the first contract so the generated DpfMachineState remains available for the deprecated alias assignment later in the machine configuration flow. Update the dpf usage near both contract mappings, preserving the existing values while avoiding the move that prevents reuse.
🧹 Nitpick comments (6)
crates/api-core/src/tests/host_bmc_firmware_test.rs (2)
1967-2266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the RPC result variants into a table-driven test.
These cases invoke the same operation with success, failure, invalid state, stale ID, and oversized output inputs. Consolidate them using the repository’s scenario/
Outcomepattern.As per coding guidelines, “use a table whenever two or more tests invoke the same operation with different inputs.”
🤖 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/api-core/src/tests/host_bmc_firmware_test.rs` around lines 1967 - 2266, Consolidate the five report_scout_firmware_upgrade_status tests into one table-driven test using the repository’s scenario and Outcome pattern. Define scenarios for successful, failed, wrong-state, stale-task-ID, and oversized-output requests, then execute the shared setup/RPC flow and assert each scenario’s expected outcome, state changes, and truncation behavior. Remove the duplicated standalone test functions while preserving their existing coverage.Source: Coding guidelines
2391-2592: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table for the
WaitingForScoutUpgradetransition cases.Success, explicit failure, exit-code failure, timeout, and pre-deadline behavior are variants of the same state transition. A scenario table will keep setup and expectations aligned.
As per coding guidelines, “use a table whenever two or more tests invoke the same operation with different inputs.”
🤖 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/api-core/src/tests/host_bmc_firmware_test.rs` around lines 2391 - 2592, Consolidate the five `test_waiting_for_scout_upgrade_*` tests into a table-driven test covering success, explicit error failure, exit-code failure, timeout, and pre-deadline waiting. Define per-case inputs and expected state/reason assertions, then iterate through the cases while reusing the shared `put_in_waiting_for_scout_upgrade` and controller setup; preserve each scenario’s existing expectations, including `power_drains_needed`.Source: Coding guidelines
crates/api-model/src/machine/mod.rs (1)
1023-1031: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBorrow
HardwareInfoinstead of cloning the full inventory.
from_hardware_infoalready accepts a reference, so cloning the potentially large structure is unnecessary.Proposed fix
- self.status.hardware_info.clone().map(|info| { + self.status.hardware_info.as_ref().map(|info| { MachineCapabilitiesSet::from_hardware_info( - &info, + info,As per coding guidelines, avoid needless clones by borrowing where possible.
🤖 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/api-model/src/machine/mod.rs` around lines 1023 - 1031, Update to_capabilities to borrow hardware_info instead of cloning it: replace the Option::map flow with a borrowed access pattern that passes the existing HardwareInfo reference to MachineCapabilitiesSet::from_hardware_info, while preserving the None behavior and all other arguments.Source: Coding guidelines
crates/api-core/src/handlers/machine.rs (1)
348-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured fields for dynamic tracing values.
These changed logs interpolate IDs, addresses, serials, errors, and queries into message strings. Replace them with stable literals and semantic fields such as
machine_id = %machine.id,address = %addr, anderror = %e.As per coding guidelines: tracing messages must be stable human-readable literals, with dynamic values recorded as structured fields.
Also applies to: 378-380, 588-590, 593-596, 641-641, 653-656, 695-699, 821-824, 878-889
🤖 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/api-core/src/handlers/machine.rs` around lines 348 - 351, Update the tracing calls in the machine handler, including the admin_force_delete_machine log and the referenced locations, to use stable human-readable messages with dynamic values passed as structured fields. Replace interpolated query, ID, serial, address, and error values with semantic fields using appropriate display or debug formatting, while preserving the existing log levels and contextual information.Source: Coding guidelines
crates/admin-cli/src/rpc.rs (1)
1509-1509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the function-wide deprecation suppressions across the admin CLI.
These annotations can hide newly introduced deprecated API usage throughout large functions. Keep allowances around only the exact legacy fields/calls required for compatibility;
crates/admin-cli/src/rpc.rsalready uses this narrower pattern locally.
crates/admin-cli/src/rpc.rs#L1509-L1509: remove the function-wide allowance and retain only targeted local suppressions.crates/admin-cli/src/dpu/reprovision/cmd.rs#L27-L27: scope suppression to the legacy access intrigger_reprovisioning_set.crates/admin-cli/src/host/reprovision/cmd.rs#L51-L51: scope suppression to the legacy access inapply_health_report.crates/admin-cli/src/debug_bundle/cmds.rs#L583-L583: narrow suppression inget_bmc_ip_from_host_id.crates/admin-cli/src/debug_bundle/cmds.rs#L1702-L1702: narrow suppression inadd_machine_analysis_json.crates/admin-cli/src/debug_bundle/cmds.rs#L1806-L1806: narrow suppression inadd_metadata.crates/admin-cli/src/machine/metadata/cmd.rs#L120-L120: scope suppression to deprecated metadata reads.crates/admin-cli/src/machine/show/cmd.rs#L500-L500: scope suppression to deprecated display fields.crates/admin-cli/src/managed_host/show/cmd.rs#L500-L500: scope suppression to deprecated compatibility reads.As per coding guidelines, deprecation allowances are temporary backwards-compatibility measures, not a general pattern.
🤖 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/admin-cli/src/rpc.rs` at line 1509, Remove the function-wide deprecation allowance in crates/admin-cli/src/rpc.rs:1509 and retain only targeted local suppressions. Scope the compatibility suppression to the required legacy access in trigger_reprovisioning_set (crates/admin-cli/src/dpu/reprovision/cmd.rs:27), apply_health_report (crates/admin-cli/src/host/reprovision/cmd.rs:51), get_bmc_ip_from_host_id (crates/admin-cli/src/debug_bundle/cmds.rs:583), add_machine_analysis_json (crates/admin-cli/src/debug_bundle/cmds.rs:1702), add_metadata (crates/admin-cli/src/debug_bundle/cmds.rs:1806), deprecated metadata reads (crates/admin-cli/src/machine/metadata/cmd.rs:120), deprecated display fields (crates/admin-cli/src/machine/show/cmd.rs:500), and deprecated compatibility reads (crates/admin-cli/src/managed_host/show/cmd.rs:500), without leaving broad function-level suppressions.Source: Coding guidelines
crates/api-web/src/dpu_versions.rs (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the compatibility deprecation allowances.
Both modules suppress all deprecation warnings even though only legacy
rpc::forge::Machinefield reads require the exception.
crates/api-web/src/dpu_versions.rs#L17-L20: move#[allow(deprecated)]to the specific machine conversion code.crates/api-web/src/health.rs#L17-L20: apply the same narrow scoping and remove the allowance after the REST migration.🤖 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/api-web/src/dpu_versions.rs` around lines 17 - 20, The module-level deprecated allowances in the machine conversion code must be narrowed. In crates/api-web/src/dpu_versions.rs#L17-L20, move the allowance onto the specific conversion function or block that reads legacy rpc::forge::Machine fields; apply the same scoping change in crates/api-web/src/health.rs#L17-L20, preserving the allowance only around those reads and removing it after the REST migration.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/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 1185-1189: Update the error message constructed in the BMC IP
lookup within the machine handler so it starts with lowercase text and remains a
lowercase phrase without a trailing period; preserve the existing error type and
interpolation.
In `@crates/api-core/src/handlers/machine.rs`:
- Line 367: The log in the host-machine lookup branch incorrectly uses the
requested DPU’s ID. Update the tracing call near the host lookup to use the
found host machine’s ID, and emit it as a structured tracing field rather than
interpolating it into the message text; keep the surrounding force-delete
behavior unchanged.
In `@crates/api-core/src/tests/host_bmc_firmware_test.rs`:
- Around line 2200-2263: Strengthen
test_report_scout_firmware_upgrade_status_truncates_output to verify truncation
retains data rather than discarding fields: assert stdout, stderr, and error
each have the exact configured cap length of 1500 and contain the expected
prefix of large_output. Keep the existing setup and state verification
unchanged.
- Around line 2377-2386: Update the failure-reason strings emitted by the
relevant handler in handler.rs to use lowercase “firmware” and “scout” prefixes.
Then update the mirrored assertions in the firmware tests, including the reason
checks around the visible “Firmware version did not converge...” assertion, to
expect the lowercase wording while preserving the existing message content and
validation.
In `@crates/machine-controller/src/handler.rs`:
- Around line 745-752: Update all listed tracing calls in the relevant handler
flows to keep message arguments as stable human-readable literals and record
dynamic values through structured fields, using semantic names such as error,
msg, machine_id, host_id, or count. Replace interpolated values, positional
placeholders, and standalone dynamic messages in calls including the shown
managed-host error and nearby symbols, while preserving the existing log level
and information.
In `@crates/rpc/src/model/machine/mod.rs`:
- Around line 623-669: Clone the rpc::forge::StateSla value before assigning it
to rpc_machine.state_sla, then use the clone for status.lifecycle.sla in both
this DPU path and clone_based_rpc_machine_state. Ensure each assignment receives
an owned value without moving sla twice.
---
Outside diff comments:
In `@crates/rpc/proto/forge.proto`:
- Around line 3778-3810: Correct the deprecated replacement references in the
machine protobuf fields: in crates/rpc/proto/forge.proto lines 3778-3810, change
status.ib_status to status.infiniband and status.hw_sku_status to status.hw_sku;
apply the identical documentation correction in
rest-api/proto/core/src/v1/nico_nico.proto lines 3777-3809. Preserve the
protobuf field names and numbering so generated-client contracts remain stable.
---
Duplicate comments:
In `@crates/rpc/src/model/machine/mod.rs`:
- Line 342: Clone dpf before assigning it to the first contract so the generated
DpfMachineState remains available for the deprecated alias assignment later in
the machine configuration flow. Update the dpf usage near both contract
mappings, preserving the existing values while avoiding the move that prevents
reuse.
---
Nitpick comments:
In `@crates/admin-cli/src/rpc.rs`:
- Line 1509: Remove the function-wide deprecation allowance in
crates/admin-cli/src/rpc.rs:1509 and retain only targeted local suppressions.
Scope the compatibility suppression to the required legacy access in
trigger_reprovisioning_set (crates/admin-cli/src/dpu/reprovision/cmd.rs:27),
apply_health_report (crates/admin-cli/src/host/reprovision/cmd.rs:51),
get_bmc_ip_from_host_id (crates/admin-cli/src/debug_bundle/cmds.rs:583),
add_machine_analysis_json (crates/admin-cli/src/debug_bundle/cmds.rs:1702),
add_metadata (crates/admin-cli/src/debug_bundle/cmds.rs:1806), deprecated
metadata reads (crates/admin-cli/src/machine/metadata/cmd.rs:120), deprecated
display fields (crates/admin-cli/src/machine/show/cmd.rs:500), and deprecated
compatibility reads (crates/admin-cli/src/managed_host/show/cmd.rs:500), without
leaving broad function-level suppressions.
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 348-351: Update the tracing calls in the machine handler,
including the admin_force_delete_machine log and the referenced locations, to
use stable human-readable messages with dynamic values passed as structured
fields. Replace interpolated query, ID, serial, address, and error values with
semantic fields using appropriate display or debug formatting, while preserving
the existing log levels and contextual information.
In `@crates/api-core/src/tests/host_bmc_firmware_test.rs`:
- Around line 1967-2266: Consolidate the five
report_scout_firmware_upgrade_status tests into one table-driven test using the
repository’s scenario and Outcome pattern. Define scenarios for successful,
failed, wrong-state, stale-task-ID, and oversized-output requests, then execute
the shared setup/RPC flow and assert each scenario’s expected outcome, state
changes, and truncation behavior. Remove the duplicated standalone test
functions while preserving their existing coverage.
- Around line 2391-2592: Consolidate the five `test_waiting_for_scout_upgrade_*`
tests into a table-driven test covering success, explicit error failure,
exit-code failure, timeout, and pre-deadline waiting. Define per-case inputs and
expected state/reason assertions, then iterate through the cases while reusing
the shared `put_in_waiting_for_scout_upgrade` and controller setup; preserve
each scenario’s existing expectations, including `power_drains_needed`.
In `@crates/api-model/src/machine/mod.rs`:
- Around line 1023-1031: Update to_capabilities to borrow hardware_info instead
of cloning it: replace the Option::map flow with a borrowed access pattern that
passes the existing HardwareInfo reference to
MachineCapabilitiesSet::from_hardware_info, while preserving the None behavior
and all other arguments.
In `@crates/api-web/src/dpu_versions.rs`:
- Around line 17-20: The module-level deprecated allowances in the machine
conversion code must be narrowed. In crates/api-web/src/dpu_versions.rs#L17-L20,
move the allowance onto the specific conversion function or block that reads
legacy rpc::forge::Machine fields; apply the same scoping change in
crates/api-web/src/health.rs#L17-L20, preserving the allowance only around those
reads and removing it after the REST migration.
🪄 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: f6f53a8e-a6c8-4f81-bda2-c809ed3aa0ae
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (104)
crates/admin-cli/src/debug_bundle/cmds.rscrates/admin-cli/src/dpu/network/cmd.rscrates/admin-cli/src/dpu/reprovision/cmd.rscrates/admin-cli/src/dpu/status/cmd.rscrates/admin-cli/src/dpu/versions/cmd.rscrates/admin-cli/src/host/reprovision/cmd.rscrates/admin-cli/src/inventory/cmd.rscrates/admin-cli/src/machine/hardware_info/cmd.rscrates/admin-cli/src/machine/metadata/cmd.rscrates/admin-cli/src/machine/nvlink_info/cmd.rscrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/managed_host/show/cmd.rscrates/admin-cli/src/rpc.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_managed_host.rscrates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpu_agent_upgrade.rscrates/api-core/src/tests/dpu_nic_firmware.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/finder.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/ib_fabric_monitor.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/ib_machine.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/machine_health.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/machine_topology.rscrates/api-core/src/tests/machine_validation.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/nvl_instance.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/connected_device.rscrates/api-core/tests/integration/forge_agent_control.rscrates/api-db/src/dpu_machine_update.rscrates/api-db/src/instance_address.rscrates/api-db/src/machine.rscrates/api-db/src/sku.rscrates/api-model/src/dpu_machine_update.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/machine/config.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/status.rscrates/api-web/src/dpu_versions.rscrates/api-web/src/health.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/network_status.rscrates/api-web/src/nvlink.rscrates/api-web/src/redfish_browser.rscrates/dpa-manager/src/card_handler/svpc.rscrates/health/src/api_client.rscrates/ib-fabric/src/lib.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/nvlink-manager/src/lib.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/rvs/src/client/mod.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/health_report.rscrates/site-explorer/tests/integration/machine_creator.rscrates/site-explorer/tests/integration/site_explorer.rscrates/ssh-console-mock-api-server/src/lib.rsrest-api/proto/core/src/v1/nico_nico.proto
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
2 times, most recently
from
July 17, 2026 16:31
6a15c7c to
70c92a3
Compare
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 17, 2026 17:11
70c92a3 to
cf235fd
Compare
Contributor
Author
|
@Matthias247 would you mind re-approving? I addressed your concerns as well as coderabbits |
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 17, 2026 20:49
cf235fd to
4d786cd
Compare
…sages Add `MachineConfig` and `MachineStatus` to `forge.Machine` and the corresponding structs to `carbide-api-model`. Protobuf Changes: - Add `MachineConfig` for operator-set desired state (maintenance, instance-type assignment, firmware policy, DPF toggle). - Add `MachineStatus` for system-observed state. - Populate both in the `From<Machine>` RPC conversion while keeping all original flat fields as `[deprecated = true]` aliases so rest-api consumers are unaffected until a follow-up PR migrates them. See NVIDIA#2793 - Add `LifecycleStatus lifecycle` to `MachineStatus`, making machines consistent with Rack, Switch, VPC, and every other managed resource type that already carries a structured lifecycle sub-message. Ingestion-time fields (`rack_id`, `placement_in_rack`, `bmc_info`, `machine_type`, `inventory`) are set once from the expected-machine record or hardware topology and are not operator-mutable. They remain as non-deprecated `Machine` fields rather than being placed in `MachineConfig` or `MachineStatus`. api-model changes: - Extract `MachineConfig` (`config.rs`) and `MachineStatus` (`status.rs`) from the flat `Machine` struct; all internal call-sites updated to `machine.config.X` / `machine.status.X`. - `rack_id`, and `placement_in_rack` stay as direct fields on `Machine` — not in `MachineConfig` - `hw_sku` is moved to `MachineConfig.hw_sku`, because it is modified via API - Add `maintenance_reference` and `maintenance_start_time` to `MachineConfig`, populated at DB ingestion time from `health_reports.maintenance_override()`. This makes `machine.config` a complete operator-config snapshot; callers no longer need to re-derive maintenance state from `health_reports`. Other changes: * `ib_status` in `From<Machine>` now preserves `None` when `infiniband_status_observation` is absent, rather than always producing `Some(Default::default())`. * Pre-compute `rpc_state`, `rpc_state_version`, and `rpc_state_reason` in `From<Machine>` so they are shared between the deprecated fields and the new `status.lifecycle` without repeating the derivation. i Mirror each into the `status` sub-message so both paths stay in sync: `status.health` and `status.lifecycle.sla`alongside `health`/`state_sla` on the host path; `status.associated_host_machine_id` and `status.lifecycle.sla` alongside the flat fields on the DPU path. * Update rest-api/proto/core to reflect MachineConfig/MachineStatus additions: regenerate nico_nico.proto via core-proto-fetch and nico_nico.pb.go via buf generate so the REST core proto sync CI check passes. * Updating clone_based_rpc_machine_state (the reference impl) to set the same status sub-message fields that into_rpc_machine_state sets: - status.health and status.lifecycle.sla for the host path - status.associated_host_machine_id and status.lifecycle.sla for the DPU path
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
July 17, 2026 22:54
4d786cd to
6614eb7
Compare
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
4 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.
Related issues
#932
Type of Change
Testing