chore(rest-api): Update machine gRPC call to use structured fields (config/status) - #4377
chore(rest-api): Update machine gRPC call to use structured fields (config/status)#4377nvlitagaki wants to merge 5 commits into
Conversation
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
nvlitagaki
added
the
rest-api
|
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. |
|
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:
Summary by CodeRabbit
WalkthroughMachine data handling is updated to read nested protobuf ChangesMachine status and config migration
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test f9b2681 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4377.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-07-30 16:00:08 UTC | Commit: f9b2681 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
rest-api/workflow/pkg/activity/machine/machine_test.go (1)
1102-1140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the expected capabilities set into a local.
The assertions are correct and index-aligned with the fixture, but the
machineInfo1.Machine.GetStatus().GetCapabilities()chain is repeated nine times inside the loop. A single local hoisted above the loop would read considerably better.♻️ Suggested refactor
expectedCaps := machineInfo1.Machine.GetStatus().GetCapabilities() // ...then use expectedCaps.Gpu[0], expectedCaps.Memory[1], expectedCaps.Infiniband[0], etc.🤖 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/workflow/pkg/activity/machine/machine_test.go` around lines 1102 - 1140, Extract machineInfo1.Machine.GetStatus().GetCapabilities() into a local expectedCaps variable before the capability iteration, then replace the repeated chain references inside the loop with expectedCaps while preserving all existing assertions and indexes.rest-api/workflow/pkg/activity/machine/machine.go (3)
1026-1027: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nil guards around a range loop.
Ranging over a nil slice is a no-op, and the generated getters are nil-receiver safe, so the entire double guard collapses to a single loop.
♻️ Suggested simplification
- if controllerMachineStatus.GetHealth() != nil && controllerMachineStatus.GetHealth().Alerts != nil { - for _, alert := range controllerMachineStatus.GetHealth().Alerts { + for _, alert := range controllerMachineStatus.GetHealth().GetAlerts() {Remember to unindent the loop body and drop the now-orphaned closing brace.
🤖 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/workflow/pkg/activity/machine/machine.go` around lines 1026 - 1027, In the alert-processing code using controllerMachineStatus.GetHealth().Alerts, remove the redundant GetHealth and Alerts nil guards and iterate directly over the alerts slice. Unindent the loop body and remove the orphaned closing brace, preserving the existing alert-processing behavior.
766-772: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the capabilities set into a local to reduce chain repetition.
The nil-safe chains are correct, but repeating
GetStatus().GetCapabilities()seven times is needlessly verbose — and inconsistent with the local-variable pattern introduced inUpdateMachinesInDB.♻️ Suggested refactor
- controllerCapsCpu := controllerMachine.GetStatus().GetCapabilities().GetCpu() - controllerCapsGpu := controllerMachine.GetStatus().GetCapabilities().GetGpu() - controllerCapsDpu := controllerMachine.GetStatus().GetCapabilities().GetDpu() - controllerCapsMemory := controllerMachine.GetStatus().GetCapabilities().GetMemory() - controllerCapsInfiniband := controllerMachine.GetStatus().GetCapabilities().GetInfiniband() - controllerCapsNetwork := controllerMachine.GetStatus().GetCapabilities().GetNetwork() - controllerCapsStorage := controllerMachine.GetStatus().GetCapabilities().GetStorage() + controllerCaps := controllerMachine.GetStatus().GetCapabilities() + + controllerCapsCpu := controllerCaps.GetCpu() + controllerCapsGpu := controllerCaps.GetGpu() + controllerCapsDpu := controllerCaps.GetDpu() + controllerCapsMemory := controllerCaps.GetMemory() + controllerCapsInfiniband := controllerCaps.GetInfiniband() + controllerCapsNetwork := controllerCaps.GetNetwork() + controllerCapsStorage := controllerCaps.GetStorage()🤖 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/workflow/pkg/activity/machine/machine.go` around lines 766 - 772, In the capability assignments near the machine update logic, store controllerMachine.GetStatus().GetCapabilities() in a local variable once, then use that local for the CPU, GPU, DPU, memory, InfiniBand, network, and storage lookups. Preserve the existing nil-safe behavior and follow the local-variable pattern used by UpdateMachinesInDB.
285-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
controllerMachineConfigaccess convention across both maintenance blocks. Both sites gate on the nil-safeGetMaintenanceStartTime()and then readMaintenanceReferenceas a raw field — one site adds a redundant!= nilguard, the other omits it. Neither can panic (a nil*MachineConfigreceiver returns nil from the getter), but the divergent style invites future misreading. Settle on one convention.
rest-api/workflow/pkg/activity/machine/machine.go#L285-L294: useGetMaintenanceReference()(or add the explicit nil guard) and hoist the duplicatedGetInterfaces()call into a local.rest-api/workflow/pkg/activity/machine/machine.go#L1052-L1057: drop the unreachable-falsecontrollerMachineConfig != nilcheck and match whichever accessor style is chosen above.🤖 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/workflow/pkg/activity/machine/machine.go` around lines 285 - 294, The maintenance handling in controllerMachineConfig uses inconsistent nil-check and accessor conventions. At rest-api/workflow/pkg/activity/machine/machine.go:285-294, use the nil-safe GetMaintenanceReference() accessor and hoist the duplicated controllerMachineStatus.GetInterfaces() call into a local; at rest-api/workflow/pkg/activity/machine/machine.go:1052-1057, remove the redundant controllerMachineConfig != nil check and apply the same accessor convention.rest-api/api/pkg/api/model/dpumachine_test.go (1)
32-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend assertions to cover
InterfacesandHealth.The fixture now populates
InterfacesandHealthunder the new nestedStatus, but no assertion verifiesdpuMachine.InterfacesordpuMachine.Healthwere actually mapped byFromProto. Since this is precisely the migrated path, add coverage to catch any regression in the new getter wiring.✅ Suggested additional assertions
assert.Equal(t, "test-sys-vendor", *dpuMachine.DMIData.SysVendor) + require.Len(t, dpuMachine.Interfaces, 1) + assert.Equal(t, "test-interface-id", dpuMachine.Interfaces[0].ID) + require.NotNil(t, dpuMachine.Health) + assert.Equal(t, "test-health-source", dpuMachine.Health.Source)Also applies to: 108-118
🤖 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/api/pkg/api/model/dpumachine_test.go` around lines 32 - 79, Extend the FromProto test assertions for the populated nested Status fixture to verify dpuMachine.Interfaces and dpuMachine.Health, including their representative IDs and health fields. Add these checks alongside the existing mapped-field assertions so regressions in the migrated getter wiring are detected.
🤖 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.
Nitpick comments:
In `@rest-api/api/pkg/api/model/dpumachine_test.go`:
- Around line 32-79: Extend the FromProto test assertions for the populated
nested Status fixture to verify dpuMachine.Interfaces and dpuMachine.Health,
including their representative IDs and health fields. Add these checks alongside
the existing mapped-field assertions so regressions in the migrated getter
wiring are detected.
In `@rest-api/workflow/pkg/activity/machine/machine_test.go`:
- Around line 1102-1140: Extract
machineInfo1.Machine.GetStatus().GetCapabilities() into a local expectedCaps
variable before the capability iteration, then replace the repeated chain
references inside the loop with expectedCaps while preserving all existing
assertions and indexes.
In `@rest-api/workflow/pkg/activity/machine/machine.go`:
- Around line 1026-1027: In the alert-processing code using
controllerMachineStatus.GetHealth().Alerts, remove the redundant GetHealth and
Alerts nil guards and iterate directly over the alerts slice. Unindent the loop
body and remove the orphaned closing brace, preserving the existing
alert-processing behavior.
- Around line 766-772: In the capability assignments near the machine update
logic, store controllerMachine.GetStatus().GetCapabilities() in a local variable
once, then use that local for the CPU, GPU, DPU, memory, InfiniBand, network,
and storage lookups. Preserve the existing nil-safe behavior and follow the
local-variable pattern used by UpdateMachinesInDB.
- Around line 285-294: The maintenance handling in controllerMachineConfig uses
inconsistent nil-check and accessor conventions. At
rest-api/workflow/pkg/activity/machine/machine.go:285-294, use the nil-safe
GetMaintenanceReference() accessor and hoist the duplicated
controllerMachineStatus.GetInterfaces() call into a local; at
rest-api/workflow/pkg/activity/machine/machine.go:1052-1057, remove the
redundant controllerMachineConfig != nil check and apply the same accessor
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 84b2a598-d224-4484-9ac9-31053b2b9c66
📒 Files selected for processing (11)
rest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/model/dpumachine.gorest-api/api/pkg/api/model/dpumachine_test.gorest-api/api/pkg/api/model/machine.gorest-api/api/pkg/api/model/machine_test.gorest-api/flow/internal/nicoapi/grpc.gorest-api/flow/internal/nicoapi/model.gorest-api/site-workflow/pkg/grpc/server/nico_test_server.gorest-api/workflow/pkg/activity/machine/machine.gorest-api/workflow/pkg/activity/machine/machine_test.go
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
|
/ok to test 5ea041d |
1 participant
Recently there were changes made in Core (#2847) that reworked the Machine gRPC proto to better conform to convention by sending config, status and metadata information as structs rather than a bunch of flat fields. This PR transitions the REST API to use these new structured fields and ignore the now deprecated flat fields.
Related issues
#2793
Type of Change
Breaking Changes
Testing
Additional Notes