refactor(machine): separate Machine fields into config/status#2847
Draft
ianderson-nvidia wants to merge 1 commit into
Draft
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
bbb991e to
56875b9
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
Action performedFull review triggered. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 splits machine state into nested ChangesMachine Config/Status Migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
| optional uint32 port = 3; | ||
| } | ||
|
|
||
| message MachineConfig { |
Contributor
There was a problem hiding this comment.
I'm honestly not 100% sure yet what we would put into Config for machines, we never really standardized on it.
I think some ideas would be:
- Whatever can be modified via mutating API calls (and thereby increments the version number stored in the object).
- Whatever can be set via ExpectedMachine
- Both of the latter.
E.g. instance_type_id falls into 1) at the moment, while rack_id falls into 2 (and is actually immutable after the host is ingested). On some other resources we don't have rack_id and the placement stuff as part of config, but just directly part of the resource.
Maybe it should just be 1) so that we could eventually offer UpdateXYZConfig methods that accept the whole struct?
cc @poroh @yoks @bcavnvidia @chet @wminckler @kensimon for more opinions.
Contributor
Author
There was a problem hiding this comment.
Yes. I also had a difficult time figuring out "what goes where," specifically with MachineConfig.
I think you're right about #1. It seems to be the logical first step, but I'd be interested in hearing others' opinions as well.
Contributor
Author
There was a problem hiding this comment.
I pushed a new commit that takes a more conservative approach (in addition to adding LifecyleStatus)
I moved rack_id, hw_sku, and placement_in_rack, bmc_info, machine_type, and inventory back to Machine.
Added LifecycleStatus to MachineStatus
Contributor
There was a problem hiding this comment.
thanks! I'd be fine referencing that 1) is the way to go in the styleguide (which already talks about RPC style and config/status patterns).
Maybe for ingestion time things we could add a new field like static_config or ingestion_config?
Let's see what others think.
Contributor
Author
There was a problem hiding this comment.
thanks! I'd be fine referencing that 1) is the way to go in the styleguide (which already talks about RPC style and config/status patterns).
Maybe for ingestion time things we could add a new field like
static_configoringestion_config? Let's see what others think.
Agreed.
For the sake of discussion -
bmc_info could change after ingestion. It contains version and firmware_version, which would change after an upgrade. We could move those values (see below).
I can see ip existing on BmcEndpoint or BmcStatus. I like it on BmcEndpoint because BmcEndpoint contains everything you'd need to connect to the BMC, while BmcStatus contains observed values.
message BmcEndpoint {
optional string ip = 1;
optional string mac = 2;
optional unit32 port = 3;
optional common.MachineInterfaceId machine_interface_id = 4;
}
message BmcStatus {
optional string version = 2;
optional string firmware_version = 3;
}
message Machine {
...
BmcEndpoint bmc = 52;
}
message MachineStatus {
...
optional BmcStatus bmc_status = 25;
}
I think inventory could move to MachineStatus because it contains observed data, such as software component names and versions.
I am less opinionated on rack_id, hw_sku, placement_in_rack, and machine_type.
Contributor
There was a problem hiding this comment.
I think hw_sku can be modified via APIs? Something like AssignSku? Or is it different? If yes, then it would fall into 1.
Contributor
There was a problem hiding this comment.
I think it would make sense to split the pure IP behavior of the BMC (endpoint) from the observed properties (e.g. version). It's just a bit more effort than purely moving things around - but if you want do it I'm +1.
That said I think the BmcEndpoint should still be on the same field as the interfaces, since they are really of the same nature (the BMC interface is just a special one not used for sending other data). => Both on message Machine or both on MachineStatus
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
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
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
| optional string instance_type_id = 4; | ||
|
|
||
| // DPF configuration for this machine. | ||
| optional DpfMachineState dpf = 5; |
Contributor
There was a problem hiding this comment.
can it be reconfigured at runtime or is it once during ingestion?
@abvarshney-nv
But I guess I'Mm fine here in both cases.
| optional uint32 port = 3; | ||
| } | ||
|
|
||
| message MachineConfig { |
Contributor
There was a problem hiding this comment.
thanks! I'd be fine referencing that 1) is the way to go in the styleguide (which already talks about RPC style and config/status patterns).
Maybe for ingestion time things we could add a new field like static_config or ingestion_config?
Let's see what others think.
|
|
||
| message MachineStatus { | ||
| // Deprecated Machine.interfaces is superseded by this field. | ||
| repeated MachineInterface interfaces = 1; |
Contributor
There was a problem hiding this comment.
this one I'm a little bit less sure about at the moment. It's very close to bmc_info (both contain interfaces), so they might need to go onto the same level.
Maybe both are ok on the status level since they are dynamically discovered (not configured). But I'm open for both options.
|
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
from
af8b56f to
df2fc21
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`, `hw_sku`, `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`, `hw_sku`, and `placement_in_rack` stay as direct fields on `Machine` — not in `MachineConfig` Other changes: * 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. 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.
ianderson-nvidia
force-pushed
the
refactor/machine-config-status
branch
from
df2fc21 to
f363603
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
3 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
Breaking Changes
Testing
Additional Notes