feat(core): persist DPS resource groups and power profiles - #4734
Conversation
|
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. |
|
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:
Summary by CodeRabbit
WalkthroughThe change adds optional ChangesPower provisioning metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC
participant Core
participant Database
Client->>RPC: Send power profile or resource-group update
RPC->>Core: Convert request to model update
Core->>Database: Persist set, clear, or preserved value
Database-->>Core: Return updated configuration
Core-->>RPC: Convert model to response
RPC-->>Client: Return power metadata
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/api-db/src/instance.rs (1)
569-588: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
power_profilewhen omittedAn absent RPC field becomes
Noneand is written directly topower_profile. An older client can therefore erase an existing profile during an unrelated update. Preserveinstance.config.power_profilewhen omitted, or add an explicit clear operation and a mixed-version regression test.🤖 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-db/src/instance.rs` around lines 569 - 588, Update the instance update flow around the power_profile binding to preserve the existing instance.config.power_profile when the RPC field is omitted instead of writing None. If clearing is required, distinguish an explicit clear from omission and add the corresponding mixed-version regression coverage; keep explicit profile updates unchanged.Source: Path instructions
🧹 Nitpick comments (1)
crates/api-model/src/instance/snapshot.rs (1)
303-303: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the persisted-row conversion path with
power_profile.The test at Lines 430-447 verifies the assignment at Line 187. The separate
InstanceSnapshot::try_fromassignment at Line 303 is not set or projected bytest_try_from_derives_os_from_instance_columns. A regression in the persisted-row path can therefore pass all tests.Add populated and unset
power_profilecases to that scenario projection.As per path instructions, optional-field round-trip tests should cover both unset and populated values.
Also applies to: 430-447
🤖 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/instance/snapshot.rs` at line 303, Extend test_try_from_derives_os_from_instance_columns to project both populated and unset power_profile values through InstanceSnapshot::try_from, covering the persisted-row conversion assignment at power_profile. Assert that populated data round-trips correctly and the unset case remains None.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/api-db/src/vpc.rs`:
- Line 338: Document the VPC update contract for power_resource_group in the
proto and update path: an omitted value preserves the stored value, while a
present value replaces it, including an empty string which must be stored as ''.
Update or add tests covering omit, set, and empty-string behavior without
introducing a clear state unless explicitly supported.
In `@crates/api-model/src/instance/config.rs`:
- Around line 69-71: Update InstanceConfigUpdateRequest and update_config so an
omitted power_profile preserves the existing config.power_profile, while an
explicit clear operation can still remove it; provide a compatibility path for
older clients as needed. Add a regression test covering an unrelated update with
a populated power profile and verify it remains unchanged.
In `@crates/rpc/proto/forge.proto`:
- Around line 1876-1877: Update the documentation and handling for
power_resource_group around the protobuf field power_resource_group to define
explicit update semantics: omission must preserve the existing value, while
empty strings must either represent a supported clear operation with tests or be
rejected and documented as unsupported for clearing. Ensure the chosen behavior
is enforced consistently by the update path.
---
Outside diff comments:
In `@crates/api-db/src/instance.rs`:
- Around line 569-588: Update the instance update flow around the power_profile
binding to preserve the existing instance.config.power_profile when the RPC
field is omitted instead of writing None. If clearing is required, distinguish
an explicit clear from omission and add the corresponding mixed-version
regression coverage; keep explicit profile updates unchanged.
---
Nitpick comments:
In `@crates/api-model/src/instance/snapshot.rs`:
- Line 303: Extend test_try_from_derives_os_from_instance_columns to project
both populated and unset power_profile values through
InstanceSnapshot::try_from, covering the persisted-row conversion assignment at
power_profile. Assert that populated data round-trips correctly and the unset
case remains None.
🪄 Autofix
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: 40080bc8-b700-4cc9-acda-697423810054
📒 Files selected for processing (37)
crates/admin-cli/src/rpc.rscrates/admin-cli/src/vpc/create/args.rscrates/admin-cli/src/vpc/show/cmd.rscrates/agent/src/tests/full.rscrates/api-core/src/cfg/file.rscrates/api-core/src/db_init.rscrates/api-core/src/tests/client_resolution.rscrates/api-core/src/tests/common/api_fixtures/instance.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_batch_allocate.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/instance_find.rscrates/api-core/src/tests/instance_ipxe_behaviors.rscrates/api-core/src/tests/instance_os.rscrates/api-core/src/tests/instance_type.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/machine_states.rscrates/api-core/src/tests/maintenance.rscrates/api-core/src/tests/network_security_group.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/vpc.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-core/tests/integration/compute_allocation.rscrates/api-db/migrations/20260806120000_power_provisioning_policy.sqlcrates/api-db/src/instance.rscrates/api-db/src/vpc.rscrates/api-model/src/instance/config.rscrates/api-model/src/instance/snapshot.rscrates/api-model/src/vpc/mod.rscrates/api-web/src/vpc.rscrates/machine-a-tron/src/api_client.rscrates/rpc/proto/forge.protocrates/rpc/src/model/instance/config.rscrates/rpc/src/model/instance/status.rscrates/rpc/src/model/vpc.rscrates/rvs/src/client/io.rs
🔐 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-08-10 16:55:22 UTC | Commit: 17716d3 |
There was a problem hiding this comment.
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)
1715-1716: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument creation semantics for the power-policy fields.
The update comments define omission and replacement behavior, but the create-side contract is not explicit for
VpcCreationRequestor the sharedInstanceConfigfield. State what omission produces during creation and whether an empty string is accepted, rejected, or stored as a literal value. Keep theVpcConfigresponse meaning consistent with that contract.As per path instructions, protobuf power-policy fields must document create omission behavior and preserve unset-versus-empty semantics across Rust and database layers.
Also applies to: 1820-1821, 3016-3020
🤖 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 1715 - 1716, Update the protobuf comments for the power-policy fields in VpcCreationRequest, InstanceConfig, and the corresponding VpcConfig response to explicitly document creation behavior when the field is omitted and whether an empty string is rejected, stored literally, or treated as unset. Preserve unset-versus-empty semantics through Rust and database layers, and ensure VpcConfig’s response meaning matches the documented create contract.Source: Path instructions
🧹 Nitpick comments (2)
crates/api-model/src/instance/snapshot.rs (1)
147-148: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the legacy snapshot deserialization path.
#[serde(default)]is the compatibility mechanism for snapshots that omitpower_profile, but the changed tests constructInstanceSnapshotPgJsondirectly. They do not verify that deserializing an older JSON payload producesNone. Add a serde-level regression test for an omitted field and a present-value round trip.Based on the provided change details, the compatibility behavior is at the serialization boundary and needs a serialization-level test.
🤖 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/instance/snapshot.rs` around lines 147 - 148, Add serde-level regression coverage for InstanceSnapshotPgJson: deserialize a legacy JSON payload omitting power_profile and assert it becomes None, then verify a payload with power_profile preserves its value through deserialization and serialization. Keep the test focused on the serde boundary rather than constructing the struct directly.crates/api-core/src/tests/vpc.rs (1)
536-554: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the RPC update path.
The new
Some("power-group")assertion callsdb::vpc::updatedirectly. The visibleVpcUpdateRequestcases setpower_resource_grouptoNone, so they do not verify protobuf presence, RPC conversion, handler forwarding, or response readback for a present value. Add one API-level update test withSome("power-group")and assert the returned VPC preserves it.As per path instructions, API changes require validation across request conversion, persistence, and response compatibility.
🤖 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/vpc.rs` around lines 536 - 554, Add an API-level VPC update test covering VpcUpdateRequest with power_resource_group set to Some("power-group"), exercising protobuf request conversion, handler forwarding, persistence, and response readback. Assert the returned VPC preserves the value, while keeping the existing direct db::vpc::update assertions unchanged.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.
Outside diff comments:
In `@crates/rpc/proto/forge.proto`:
- Around line 1715-1716: Update the protobuf comments for the power-policy
fields in VpcCreationRequest, InstanceConfig, and the corresponding VpcConfig
response to explicitly document creation behavior when the field is omitted and
whether an empty string is rejected, stored literally, or treated as unset.
Preserve unset-versus-empty semantics through Rust and database layers, and
ensure VpcConfig’s response meaning matches the documented create contract.
---
Nitpick comments:
In `@crates/api-core/src/tests/vpc.rs`:
- Around line 536-554: Add an API-level VPC update test covering
VpcUpdateRequest with power_resource_group set to Some("power-group"),
exercising protobuf request conversion, handler forwarding, persistence, and
response readback. Assert the returned VPC preserves the value, while keeping
the existing direct db::vpc::update assertions unchanged.
In `@crates/api-model/src/instance/snapshot.rs`:
- Around line 147-148: Add serde-level regression coverage for
InstanceSnapshotPgJson: deserialize a legacy JSON payload omitting power_profile
and assert it becomes None, then verify a payload with power_profile preserves
its value through deserialization and serialization. Keep the test focused on
the serde boundary rather than constructing the struct directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a5d42552-94d0-4ca7-b14a-51509c0251c6
📒 Files selected for processing (6)
crates/api-core/src/handlers/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc.rscrates/api-db/src/vpc.rscrates/api-model/src/instance/snapshot.rscrates/rpc/proto/forge.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-db/src/vpc.rs
- crates/api-core/src/tests/instance_config_update.rs
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 `@rest-api/proto/core/src/v1/nico_nico.proto`:
- Around line 1855-1859: Replace the optional power_resource_group field with a
oneof update operation that distinguishes setting a non-empty resource-group
identifier from explicitly clearing the association. Preserve omission as “leave
unchanged,” add a clear variant, and enforce rejection of empty values in the
set variant.
🪄 Autofix
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: 2b636904-6e5d-4c81-bcb8-be4792d0fae9
⛔ 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 (1)
rest-api/proto/core/src/v1/nico_nico.proto
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/rpc/proto/forge.proto`:
- Around line 1876-1883: The legacy REST JSON field name is lost because the set
arm is named set_power_resource_group; update the VpcUpdateRequest oneof in
rest-api/proto/core/src/v1/nico_nico.proto at lines 1855-1862 to preserve
power_resource_group or add explicit JSON compatibility handling, and test
legacy set, new set, and clear payloads. The corresponding forge.proto site at
lines 1876-1883 requires no direct change because Forge does not deserialize
this request with serde.
🪄 Autofix
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: a02c8e34-26d6-4d8b-94d5-ab22266f59c5
⛔ 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 (6)
crates/api-core/src/tests/vpc.rscrates/api-db/src/vpc.rscrates/api-model/src/vpc/mod.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-core/src/tests/vpc.rs
- crates/api-db/src/vpc.rs
9cd6a68 to
b55c5a5
Compare
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Looks good @pbreton, one comment.
f2e1fcf to
d865b91
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4734.docs.buildwithfern.com/infra-controller |
thossain-nv
removed
the
rest-api
d865b91 to
65fc1d2
Compare
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the implementation @pbreton, left some comments.
One other observation handlers/vpc::update only loads the VPC when network_security_group_id or routing_profile_overrides is set, so an update only setting power_resource_group against an unknown ID returns FindOneReturnedManyResultsError instead of NotFound. This issue already existed, but might be good to address.
| id: instance_id.to_string(), | ||
| })?; | ||
|
|
||
| // power_profile was added to the complete-config update request after the |
There was a problem hiding this comment.
VPC resolves this with PowerResourceGroupUpdate::{Set, Clear} at in rpc, the Instance patches Option<String> in the handler. Should they be done similarly?
There was a problem hiding this comment.
VPC uses a patch-specific request, so it can convert directly to a Set/Clear enum. Instance uses a complete InstanceConfig shared by creation and update. Omission can only be interpreted as “preserve” after loading the existing instance, so handler-level resolution remains appropriate.
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
60814fc to
3fc07e4
Compare
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
2 participants
Summary
Why
This provides the Core persistence and API foundation for #2092. REST exposure and Max-Q/DPS orchestration can build on these persisted associations in follow-up work.
Impact
The protobuf changes are additive, and the new database columns are nullable. Existing callers continue to omit both fields.
Testing
cargo check -p carbide-api-model -p carbide-api-db -p carbide-rpc -p carbide-api-core --no-default-featurescargo check -p nico-admin-cli -p carbide-api-web -p carbide-machine-a-tron -p carbide-rvscargo test -p carbide-rpc --features model power_profile_round_tripscargo test -p carbide-api-model instance::snapshot::tests::test_from_pg_json_and_os_uses_provided_osgit diff --checkRelated issues