Skip to content

fix(flow): sync proto mirror and harden decommission poll loop - #4524

Open
kdhulipala-wq wants to merge 2 commits into
NVIDIA:mainfrom
kdhulipala-wq:kcd-decom-flow-fwup
Open

fix(flow): sync proto mirror and harden decommission poll loop#4524
kdhulipala-wq wants to merge 2 commits into
NVIDIA:mainfrom
kdhulipala-wq:kcd-decom-flow-fwup

Conversation

@kdhulipala-wq

Copy link
Copy Markdown
Contributor

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.

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.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b1d6c0d6-dbfc-4e6c-8ee2-a98a9f1cbc64

📥 Commits

Reviewing files that changed from the base of the PR and between 7e88e50 and 7d17e9f.

📒 Files selected for processing (2)
  • rest-api/flow/internal/service/server_impl.go
  • rest-api/flow/internal/task/executor/temporalworkflow/workflow/actions.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • rest-api/flow/internal/task/executor/temporalworkflow/workflow/actions.go

Summary by CodeRabbit

  • New Features

    • Added support for submitting rack decommissioning operations through the Flow API.
    • Rack decommission requests can include a description, queue options, and an optional operation-rule override.
  • Bug Fixes

    • Decommissioning now stops after five consecutive status-check failures.
    • Improved handling of missing, in-progress, successful, and unexpected decommissioning states.
    • Rack decommission requests now reject component targets.

Walkthrough

The Flow API now supports rack decommissioning requests and rejects component targets. Decommission waiting uses a 30-second fire-once activity policy, aborts after five consecutive retrieval failures, and treats missing component states as successful completion.

Changes

Rack decommissioning

Layer / File(s) Summary
Rack decommissioning API contract
rest-api/proto/flow/src/v1/flow.proto, rest-api/flow/internal/service/server_impl.go
Adds the Flow.DecommissionRack RPC and request fields. Rejects component-target requests with InvalidArgument.
Decommission status polling
rest-api/flow/internal/task/executor/temporalworkflow/workflow/actions.go
Uses a 30-second activity timeout with no retries. Aborts after five consecutive retrieval failures. Resets the counter after success. Treats missing states as successful, keeps Decommissioning/ in progress, and reports unexpected states as failures.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • NVIDIA/infra-controller#4679: Extends the managed-switch decommissioning Flow integration, including status normalization and the decommission RPC path.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the proto synchronization and decommission polling changes.
Description check ✅ Passed The description accurately explains the proto updates, polling safeguards, absent-state handling, and target validation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Aug 4, 2026 — with ChatGPT Codex Connector
@kdhulipala-wq
kdhulipala-wq marked this pull request as ready for review August 4, 2026 23:31
@kdhulipala-wq
kdhulipala-wq requested a review from a team as a code owner August 4, 2026 23:31
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-08-04 23:34:31 UTC | Commit: 7e88e50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/flow/internal/task/executor/temporalworkflow/workflow/actions.go`:
- Around line 650-666: Update executeWaitDecommissionedAction so its
GetDecommissionStatus activity invocation uses dedicated activity options with a
short StartToCloseTimeout and MaximumAttempts set to 1, instead of inheriting
the decommission step’s 3 attempts and 4-hour timeout. Keep the existing
consecutive-failure counter and polling behavior, allowing the loop—not Temporal
activity retries—to control retries.

In `@rest-api/proto/flow/src/v1/flow.proto`:
- Around line 717-722: Update the DecommissionRack RPC flow to validate that the
request target contains only racks before invoking TargetSpecFrom or
decommissionRackImpl. Reject component targets when the RPC is enabled, while
preserving existing handling for valid rack targets.
🪄 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: f17bdde4-aa00-47de-a285-7df48b5c7006

📥 Commits

Reviewing files that changed from the base of the PR and between 88b2b9c and 7e88e50.

⛔ Files ignored due to path filters (2)
  • rest-api/proto/flow/gen/v1/flow.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
  • rest-api/proto/flow/gen/v1/flow_grpc.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go, !rest-api/**/*_grpc.pb.go
📒 Files selected for processing (2)
  • rest-api/flow/internal/task/executor/temporalworkflow/workflow/actions.go
  • rest-api/proto/flow/src/v1/flow.proto

Comment thread rest-api/proto/flow/src/v1/flow.proto
@coderabbitai coderabbitai Bot mentioned this pull request Aug 6, 2026
10 tasks
Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants