feat(flow): rack-scale decommission workflow with proto mirror sync a… - #5063
feat(flow): rack-scale decommission workflow with proto mirror sync a…#5063kdhulipala-wq wants to merge 1 commit into
Conversation
…nd activity timeout bounds Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
🔐 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-17 19:46:38 UTC | Commit: 604c6e4 |
|
|
||
| func (x *DecommissionRackRequest) ProtoReflect() protoreflect.Message { | ||
| // TODO: replaced by buf generate — uses placeholder slot until then. | ||
| mi := &file_flow_proto_msgTypes[71] |
There was a problem hiding this comment.
This slot belongs to IngestRackRequest; the protobuf descriptor tables and raw descriptor were not updated for DecommissionRackRequest. As a result, ProtoReflect() reports v1.IngestRackRequest, and marshaling a populated DecommissionRackRequest triggers a SIGSEGV. Since gRPC must marshal this request before sending it, the new RPC is unusable despite compiling successfully.
Please regenerate the Flow protobuf mirror using make rest-api/flow-proto from the repository root instead of manually patching the generated files, and add a protobuf marshal round-trip test.
| bool override_readiness_check = 4; | ||
| } | ||
|
|
||
| message DecommissionRackRequest { |
There was a problem hiding this comment.
This file is generated too — make flow-proto-fetch copies flow/proto/v1/*.proto over it. The canonical source already declares this message at flow/proto/v1/flow.proto:723, where the field 3 comment reads // optional queue policy overrides rather than // optional: queuing behaviour on conflict, so this text would be overwritten on the next regen. Running make flow-proto from rest-api/ syncs the mirror and regenerates gen/v1/ in one step.
| codes.Unimplemented, | ||
| "decommission is not yet available: Core decommission RPCs are pending", | ||
| ) | ||
| return rs.decommissionRackImpl(ctx, req) |
There was a problem hiding this comment.
This ungates the RPC while the Core calls behind it are still stubs. grpcClient.DecommissionMachine, DecommissionSwitch, and DecommissionPowerShelf return not yet implemented unconditionally (flow/internal/nicoapi/grpc.go:784, 790, 796), and only testing.Testing() gets the mock — NewClient returns *grpcClient in production.
The three component managers call those methods directly from DecommissionControl, which is the main operation of every stage in the default rule (operationrules/resolver_defaults.go:1286, 1309, 1331). executeDecommissionControlAction replaces the step's activity options with a fire-once policy, so there's no retry to ride it out: a request now creates a task and fails in its first non-empty decommission stage, which is what the removed guard existed to prevent.
Was dropping the guard intended in this change? The description says the endpoint should be callable "once it is ungated", which reads like it was meant to stay.
| switch { | ||
| case state == "Decommissioned": | ||
| // Terminal success. | ||
| case state == "": |
There was a problem hiding this comment.
"" doesn't distinguish "Core removed the record" from "Core didn't report this component", so this reports success for a destructive operation that may not have happened.
FindMachineControllerStates drops a machine that isn't in the response (nicoapi/grpc.go:773-778), and the component managers backfill "" for every requested ID that came back absent (componentmanager/compute/nico/nico.go:670-678, same in nvswitch and powershelf). A component can be absent for reasons unrelated to decommissioning:
- An unknown or mistyped ID. Core's
find_machines_by_idsonly rejects empty and oversized ID lists; IDs it can't load are silently omitted (crates/api-core/src/handlers/machine.rs:118-139). - A machine whose snapshot loads but whose DPU sub-snapshot doesn't match, which Core drops via
.find(|dpu| dpu.id == *dpu_machine_id)?(crates/rpc/src/model/machine/mod.rs:648). Core logs that case, which suggests it's an anomaly rather than a terminal signal.
Could the reader layer carry found/not-found explicitly — (state string, found bool), or a distinct sentinel — so the workflow can tell the two apart? And is "record removed" actually Core's terminal signal here, or does Core expose a real Decommissioned state that could be waited on instead?
Related and pre-existing: allDecommissioned starts true, so an empty result.States also returns success.
| // errors that will cause the wait loop to abort rather than spin until the | ||
| // 4-hour deadline. A permanent error (e.g. Core unreachable) is caught within | ||
| // a few poll intervals instead of hours later. | ||
| const maxConsecutiveStatusFailures = 5 |
There was a problem hiding this comment.
Bounding the loop is the right fix — the previous continue would spin to the 4-hour deadline. The sizing is worth a second look though: with the configured PollInterval: 30s and Timeout: 4h (operationrules/resolver_defaults.go:1291-1293), plus the fire-once 30s status activity below, the budget works out to roughly 2.5 to 5 minutes. A Core outage or restart lasting that long during decommissioning would abort the rack workflow.
A time-based budget — consecutive failures spanning more than N minutes — would scale with the poll interval instead of being coupled to it. Either way, the question this raises: what's the recovery path for a rack that's half decommissioned when this returns an error? (Non-blocking, more like a design and recovery question than a defect)
3 participants
**This is a re-issue of an older pull request #5003 to restart the CI pipeline which was broken on the older version of main.
Proto mirror sync (rest-api/proto/flow/):
Add DecommissionRack RPC and DecommissionRackRequest message to the source proto (src/v1/flow.proto) so external consumers (REST API, site-workflow) can call the endpoint once it is ungated.
Manually patch the generated client and server stubs in gen/v1/ with a TODO to replace via buf generate from rest-api/proto/flow/.
Poll loop robustness (executeWaitDecommissionedAction):
Add a consecutive-failure budget (5 failures) so a permanent GetDecommissionStatus error aborts within a few poll intervals rather than spinning until the 4-hour deadline.
Treat a component absent from Core's response (state "") as already decommissioned: Core removes the resource record as the terminal step, so an absent ID is the expected success condition, not an error. Improve the error message for genuinely unexpected states.