feat(core): persist DPS resource groups and power profiles - #4734
Conversation
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
|
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. |
Summary by CodeRabbit
WalkthroughThe change adds optional ChangesPower metadata contracts and conversions
Persistence and integration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
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
| SET name=$1, version=$2, description=$3, network_security_group_id=$4, | ||
| labels=$5::json, | ||
| routing_profile_overrides=COALESCE($6::jsonb, routing_profile_overrides), | ||
| power_resource_group=COALESCE($7, power_resource_group), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'power_resource_group|VpcUpdateRequest|UpdateVpc|COALESCE' \
crates/rpc/proto/forge.proto \
crates/api-model/src \
crates/rpc/src \
crates/api-db/src \
crates/api-core/src \
crates/admin-cli/src \
crates/api-web/src \
crates/machine-a-tron/src || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- VPC model and RPC conversion ---'
sed -n '90,115p' crates/api-model/src/vpc/mod.rs
sed -n '270,320p' crates/rpc/src/model/vpc.rs
printf '%s\n' '--- VPC update handler and database binding ---'
sed -n '150,240p' crates/api-core/src/handlers/vpc.rs
sed -n '313,365p' crates/api-db/src/vpc.rs
printf '%s\n' '--- Proto contract ---'
sed -n '1838,1882p' crates/rpc/proto/forge.proto
printf '%s\n' '--- UpdateVpc construction and power-resource-group references ---'
rg -n -C 3 'UpdateVpc\s*\{|power_resource_group|VpcUpdateRequest' \
crates --glob '*.rs' --glob '*.yaml' --glob '*.yml' --glob '*.toml' --glob '*.md' \
| rg -v 'target|power_resource_group: None'
printf '%s\n' '--- Candidate tests ---'
rg -n -C 4 'update.*vpc|Vpc.*update|power_resource_group|resource group' \
crates/api-core crates/api-db crates/rpc --glob '*test*' --glob '*.rs' \
| head -n 300Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
node - <<'JS'
function coalesce(value, current) {
return value === null || value === undefined ? current : value;
}
const cases = [
['omitted', undefined, 'existing-group'],
['set', 'new-group', 'existing-group'],
['explicit empty string', '', 'existing-group'],
['explicit null', null, 'existing-group'],
];
for (const [name, value, current] of cases) {
console.log(`${name}: ${JSON.stringify(coalesce(value, current))}`);
}
JSRepository: NVIDIA/infra-controller
Length of output: 262
Document VPC update semantics.
power_resource_group omission preserves the stored value. A present value replaces it, but an empty string does not clear the column; it stores ''. Document this contract in the proto and update path, or add an explicit clear state with set, omit, and clear tests.
🤖 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/vpc.rs` at 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.
Source: Path instructions
|
|
||
| /// Power profile managed by the external power provisioning service. | ||
| pub power_profile: Option<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'\bpower_profile\b|update_instance_config' \
crates/api-model/src/instance/config.rs \
crates/rpc/src/model/instance/config.rs \
crates/api-core/src \
crates/api-db/src \
crates/api-web/src \
crates/admin-cli/src \
crates/machine-a-tron/srcRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== definitions and references =='
rg -n --glob '*.rs' \
'fn update_instance_config|update_instance_config\(|power_profile|InstanceConfigUpdateRequest|InstanceConfig\s*\{' \
crates/api-core/src crates/api-db/src crates/api-model/src crates/rpc/src \
| head -n 500
printf '%s\n' '== candidate files =='
git ls-files \
crates/api-core/src \
crates/api-db/src \
crates/api-model/src/instance \
crates/rpc/src/model/instance \
| rg 'instance_config|instance/config|instance.*(update|model)|config'Repository: NVIDIA/infra-controller
Length of output: 33144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== update handler =='
sed -n '1160,1345p' crates/api-core/src/handlers/instance.rs
printf '%s\n' '== database write path =='
sed -n '520,610p' crates/api-db/src/instance.rs
sed -n '790,850p' crates/api-db/src/instance.rs
sed -n '895,945p' crates/api-db/src/instance.rs
printf '%s\n' '== focused update test =='
sed -n '90,345p' crates/api-core/src/tests/instance_config_update.rs
printf '%s\n' '== model and RPC conversions =='
sed -n '35,165p' crates/rpc/src/model/instance/config.rsRepository: NVIDIA/infra-controller
Length of output: 29249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== protocol and documentation =='
rg -n -C 6 --glob '*.{proto,rs,md,yaml,yml,toml}' \
'power_profile|InstanceConfigUpdateRequest|complete replacement|full.*config|updateable configurations|external power' \
.
printf '%s\n' '== non-test update callers =='
rg -n -C 5 --glob '*.rs' \
'update_instance_config\(|InstanceConfigUpdateRequest\s*\{' \
crates \
| rg -v 'crates/api-core/src/tests|power_profile: None|power_profile: Some'
printf '%s\n' '== update handler continuation =='
sed -n '1340,1435p' crates/api-core/src/handlers/instance.rs
printf '%s\n' '== power profile references outside the initial files =='
rg -n -C 4 --glob '*.{rs,proto,md,yaml,yml,toml,json}' \
'\bpower_profile\b|power profile' \
. \
| head -n 500Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== files containing power_profile =='
rg -l --glob '*.{rs,proto,md,yaml,yml,toml}' '\bpower_profile\b' . | sort
printf '%s\n' '== protocol declarations =='
for f in $(rg -l --glob '*.proto' '\bpower_profile\b|InstanceConfigUpdateRequest'); do
printf '\n--- %s ---\n' "$f"
rg -n -C 8 '\bpower_profile\b|InstanceConfigUpdateRequest' "$f"
done
printf '%s\n' '== production Rust references excluding tests =='
rg -n -C 4 --glob '*.rs' '\bpower_profile\b' crates \
| rg -v '/tests/|power_profile: None|power_profile: Some' \
| head -n 300
printf '%s\n' '== read-only behavioral verifier =='
python3 - <<'PY'
from pathlib import Path
handler = Path("crates/api-core/src/handlers/instance.rs").read_text()
db = Path("crates/api-db/src/instance.rs").read_text()
test = Path("crates/api-core/src/tests/instance_config_update.rs").read_text()
assert "let mut config: InstanceConfig = match request.config" in handler
assert "db::instance::update_config" in handler
assert "power_profile=$15" in db
assert ".bind(config.power_profile)" in db
assert "updated_config_2.power_profile = None;" in test
assert "assert_config_equals(instance.config.as_ref().unwrap(), &updated_config_2);" in test
print("update_instance_config accepts a complete config")
print("update_config binds config.power_profile directly to power_profile")
print("the focused integration test explicitly clears power_profile with None")
print("no retention assertion is present in the focused test")
PYRepository: NVIDIA/infra-controller
Length of output: 1485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== protocol declaration =='
rg -n -C 12 'power_profile|InstanceConfigUpdateRequest' crates/rpc/proto/forge.proto
printf '%s\n' '== production-facing references =='
for f in crates/admin-cli/src/rpc.rs crates/machine-a-tron/src/api_client.rs crates/rvs/src/client/io.rs; do
printf '\n--- %s ---\n' "$f"
rg -n -C 8 'power_profile|update_instance_config|InstanceConfigUpdateRequest' "$f" || true
done
printf '%s\n' '== source-level behavioral verifier =='
python3 - <<'PY'
from pathlib import Path
handler = Path("crates/api-core/src/handlers/instance.rs").read_text()
db = Path("crates/api-db/src/instance.rs").read_text()
test = Path("crates/api-core/src/tests/instance_config_update.rs").read_text()
assert "let mut config: InstanceConfig = match request.config" in handler
assert "db::instance::update_config" in handler
assert "power_profile=$15" in db
assert ".bind(config.power_profile)" in db
assert "updated_config_2.power_profile = None;" in test
assert "assert_config_equals(instance.config.as_ref().unwrap(), &updated_config_2);" in test
print("update_instance_config accepts a complete config")
print("update_config binds config.power_profile directly to power_profile")
print("the focused integration test explicitly clears power_profile with None")
print("the focused test does not assert retention during an unrelated update")
PYRepository: NVIDIA/infra-controller
Length of output: 9660
Preserve power_profile during unrelated configuration updates.
InstanceConfigUpdateRequest replaces the complete configuration, and update_config writes config.power_profile directly. Clients that omit this newly added field therefore clear the externally managed profile. Treat omission as preservation with an explicit clear operation, or provide a compatibility path for older clients. Add a regression test for an unrelated update that retains a populated profile.
🤖 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/config.rs` around lines 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.
Source: Path instructions
| // Replaces the external power provisioning resource group when present. | ||
| optional string power_resource_group = 20; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check VPC update callers and tests for clear semantics.
rg -n -C 8 'power_resource_group|VpcUpdateRequest|update_vpc|COALESCE' \
crates/api-core crates/api-db crates/rpc crates/api-web crates/admin-cliRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact power_resource_group references (excluding migrations) ---'
rg -n -C 5 'power_resource_group' crates \
-g '!**/migrations/**' \
-g '!**/migrations.pre-squash/**' \
| head -n 400
printf '%s\n' '--- VPC database update functions and SQL ---'
rg -n -C 12 'UPDATE .*vpc|power_resource_group|update_vpc|VpcUpdateRequest' \
crates/api-db/src crates/api-core/src crates/api-web/src crates/admin-cli/src \
-g '*.rs' | head -n 500
printf '%s\n' '--- VPC-related tests ---'
rg -l 'power_resource_group|VpcUpdateRequest|UpdateVpc' \
crates -g '*test*.rs' -g '*.rs' \
| sort -uRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files and focused symbols ---'
git ls-files | rg '(^|/)(vpc|forge\.proto|.*vpc.*test.*|.*vpc.*)\.(rs|proto)$' | head -n 200
printf '%s\n' '--- all non-generated VPC field references ---'
rg -n 'power_resource_group' \
crates/api-db/src crates/api-core/src crates/api-web/src crates/admin-cli/src crates/rpc/proto/forge.proto \
-g '*.rs' -g '*.proto' \
--glob '!**/migrations/**' --glob '!**/migrations.pre-squash/**' \
| head -n 300Repository: NVIDIA/infra-controller
Length of output: 5188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RPC conversion ---'
sed -n '230,325p' crates/rpc/src/model/vpc.rs
printf '%s\n' '--- API update handler ---'
sed -n '150,245p' crates/api-core/src/handlers/vpc.rs
printf '%s\n' '--- VPC update tests ---'
sed -n '340,545p' crates/api-core/src/tests/vpc.rs
printf '%s\n' '--- VPC model update type ---'
sed -n '85,115p' crates/api-model/src/vpc/mod.rs
printf '%s\n' '--- database schema column ---'
rg -n -C 5 'power_resource_group' crates/api-db/migrations crates/api-db -g '*.sql' -g '*.rs' \
| head -n 120Repository: NVIDIA/infra-controller
Length of output: 20135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
rpc = Path("crates/rpc/src/model/vpc.rs").read_text()
db = Path("crates/api-db/src/vpc.rs").read_text()
proto = Path("crates/rpc/proto/forge.proto").read_text()
checks = {
"request_field_is_proto3_optional": "optional string power_resource_group = 20;" in proto,
"request_conversion_preserves_only_option": "power_resource_group: value.power_resource_group" in rpc,
"database_update_uses_coalesce": "power_resource_group=COALESCE($7, power_resource_group)" in db,
"database_binds_option": ".bind(&value.power_resource_group)" in db,
}
for name, passed in checks.items():
print(f"{name}: {'PASS' if passed else 'FAIL'}")
# Enumerate the only representable update values visible at the RPC/model boundary.
print("representations:")
for value in ("omitted", 'present("")', 'present("group")'):
sql_argument = {
"omitted": "NULL",
'present("")': "''",
'present("group")': "'group'",
}[value]
result = "existing value" if sql_argument == "NULL" else sql_argument
print(f" {value} -> SQL argument {sql_argument} -> COALESCE result {result}")
PYRepository: NVIDIA/infra-controller
Length of output: 518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
rpc = Path("crates/rpc/src/model/vpc.rs").read_text()
db = Path("crates/api-db/src/vpc.rs").read_text()
proto = Path("crates/rpc/proto/forge.proto").read_text()
checks = {
"request_field_is_proto3_optional": "optional string power_resource_group = 20;" in proto,
"request_conversion_preserves_only_option": "power_resource_group: value.power_resource_group" in rpc,
"database_update_uses_coalesce": "power_resource_group=COALESCE($7, power_resource_group)" in db,
"database_binds_option": ".bind(&value.power_resource_group)" in db,
}
for name, passed in checks.items():
print(f"{name}: {'PASS' if passed else 'FAIL'}")
print("representations:")
for value in ("omitted", 'present("")', 'present("group")'):
sql_argument = {
"omitted": "NULL",
'present("")': "''",
'present("group")': "'group'",
}[value]
result = "existing value" if sql_argument == "NULL" else sql_argument
print(f" {value} -> SQL argument {sql_argument} -> COALESCE result {result}")
PYRepository: NVIDIA/infra-controller
Length of output: 518
Document power_resource_group update semantics.
Omission preserves the current value, while an empty string stores '' and does not clear the column. Define an explicit clear representation with tests, or reject empty values and document that clearing is unsupported.
🤖 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 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.
Source: Path instructions
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