feat(rest-api): expose dpfEnabled on expected-machine endpoints#2961
feat(rest-api): expose dpfEnabled on expected-machine endpoints#2961hwadekar-nv wants to merge 4 commits into
Conversation
WalkthroughThe change adds ChangesDpfEnabled field propagation for ExpectedMachine
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2961.docs.buildwithfern.com/infra-controller |
🔐 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-06-28 20:51:36 UTC | Commit: 5b93eb7 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/db/pkg/db/model/expectedmachine.go`:
- Line 244: `DpfEnabled` currently uses nil as both “no change” and “clear to
NULL,” which prevents the workflow from syncing inventory removals correctly.
Update the expected-machine update contract around `ExpectedMachine`,
`UpdateMultiple`, and the workflow reconciler so callers can explicitly
distinguish preserve-vs-clear semantics, using a presence flag or tri-state
field for `DpfEnabled`. Ensure the reconciler can mark `HasDpfEnabled` when
inventory should be reflected in the DB, and that the DAO applies NULL only when
that presence bit is set, keeping data-model compatibility for database changes.
🪄 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: cf7e790b-3b88-46e8-a908-7d2ab0991477
⛔ Files ignored due to path filters (31)
rest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_batch_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (9)
rest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/model/expectedmachine.gorest-api/api/pkg/api/model/expectedmachine_test.gorest-api/db/pkg/db/model/expectedmachine.gorest-api/db/pkg/db/model/expectedmachine_test.gorest-api/db/pkg/migrations/20260628120000_expected_machine_dpf_enabled.gorest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/expectedmachine/expectedmachine.go
| TrayIdx *int32 | ||
| HostID *int32 | ||
| Labels map[string]string | ||
| DpfEnabled *bool |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
DpfEnabled cannot be cleared back to NULL.
UpdateMultiple treats nil as “leave unchanged”, but the workflow reconciliation now compares cur.DpfEnabled against reported.DpfEnabled and forwards reported.DpfEnabled directly. When Site inventory stops sending is_dpf_enabled, the diff remains true forever while the DAO never clears dpf_enabled, so the DB value stays stale and this row will be “updated” on every sync. Please introduce an explicit presence bit / tri-state update contract so callers can distinguish “preserve existing” from “set column to NULL”. As per path instructions, review database changes for data-model compatibility.
Proposed direction
type ExpectedMachineUpdateInput struct {
ExpectedMachineID uuid.UUID
...
DpfEnabled *bool
+ HasDpfEnabled bool
}
- if input.DpfEnabled != nil {
+ if input.HasDpfEnabled {
em.DpfEnabled = input.DpfEnabled
columnsSet["dpf_enabled"] = true
}The workflow reconciler can then set HasDpfEnabled: true whenever DB state must match inventory, even when reported.DpfEnabled is nil.
Also applies to: 684-687
🤖 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 `@rest-api/db/pkg/db/model/expectedmachine.go` at line 244, `DpfEnabled`
currently uses nil as both “no change” and “clear to NULL,” which prevents the
workflow from syncing inventory removals correctly. Update the expected-machine
update contract around `ExpectedMachine`, `UpdateMultiple`, and the workflow
reconciler so callers can explicitly distinguish preserve-vs-clear semantics,
using a presence flag or tri-state field for `DpfEnabled`. Ensure the reconciler
can mark `HasDpfEnabled` when inventory should be reflected in the DB, and that
the DAO applies NULL only when that presence bit is set, keeping data-model
compatibility for database changes.
Source: Path instructions
There was a problem hiding this comment.
In proto, DpfEnabled is going to be deprecated, instead isDpfEnabled use, how for readable purpose we kept 'DpfEnabled' in model. However, HasDpfEnabled looks correct but we will stick with DpfEnabled, we can ingnore
There was a problem hiding this comment.
@hwadekar-nv Acknowledged — keeping DpfEnabled in the model for readability while the proto transitions toward isDpfEnabled makes sense.
My concern was specifically about the update semantics rather than the field name: today nil still means both “preserve existing value” and “clear DB column to NULL”, so inventory removals cannot be reflected in expected_machine.dpf_enabled. That said, if you’re choosing to defer that behavior change for this PR, I’m fine to leave it there and move on.
If you want, I can open a small follow-up issue for the tri-state/presence-bit cleanup separately.
✏️ Learnings added
Learnt from: hwadekar-nv
Repo: NVIDIA/infra-controller PR: 2961
File: rest-api/db/pkg/db/model/expectedmachine.go:235-235
Timestamp: 2026-06-28T22:19:15.745Z
Learning: In the NVIDIA infra-controller ExpectedMachine flow, the workflow proto is moving away from `DpfEnabled` toward `isDpfEnabled`, but the Go model layer keeps the field name `DpfEnabled` for readability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Allow operators to read and set per-host DPF eligibility via the REST API, persisting the flag in the cloud DB and forwarding it to Core through the existing expected-machine workflows and inventory sync. Signed-off-by: Hitesh Wadekar <hwadekar@nvidia.com>
hwadekar-nv
force-pushed
the
fix/expose-dpf-enabled
branch
from
6fd23b1 to
a6fb40c
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
Summary
dpfEnabledto the Expected Machine REST API (GET,POST,PATCH, and batch create/update) so operators can manage per-host DPF eligibility without admin-cli.expected_machinetable, forward it to Core viais_dpf_enabledon workflow proto, and reconcile it during site inventory sync.Test plan
go test ./db/pkg/db/model/... -run ExpectedMachinego test ./api/pkg/api/model/... -run ExpectedMachinego test ./api/pkg/api/handler/... -run ExpectedMachinego test ./workflow/pkg/activity/expectedmachine/.../expected-machinewith"dpfEnabled": falseand verify GET returnsfalsedpfEnabledpreserves existing value