feat(rest-api): dispatch every Flow endpoint through the gRPC proxy - #4706
Draft
kunzhao-nv wants to merge 7 commits into
Draft
feat(rest-api): dispatch every Flow endpoint through the gRPC proxy#4706kunzhao-nv wants to merge 7 commits into
kunzhao-nv wants to merge 7 commits into
Conversation
The eight Run endpoints now call ExecuteFlowGRPC instead of a bespoke workflow per method. Derived workflow IDs keep their parameter rules but move under a flow-grpc- namespace, so a proxy request cannot attach to a bespoke execution of the same derived name; those workflows stay registered on the site agent until a later release retires them. The shared proxy allows the activity a single attempt, so these calls no longer get the second attempt the bespoke Run workflows permitted. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Contributor
|
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
WalkthroughTaskRun handlers now proxy Flow gRPC calls through ChangesTaskRun Flow proxy migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 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-07 23:00:26 UTC | Commit: d329e5b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rest-api/api/pkg/api/handler/taskrun_test.go (2)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every deterministic workflow-ID input.
The tests do not verify all inputs that prevent incompatible requests from coalescing. A regression in
includeStatshandling, query hashing, transport namespacing, or conflict policy can pass these tests.
rest-api/api/pkg/api/handler/taskrun_test.go#L316-L322: add a successfulincludeStats=truecase and assert its distinctflow-grpc-task-run-get-...-trueID.rest-api/api/pkg/api/handler/taskrun_test.go#L404-L409: captureStartWorkflowOptionsand assert the namespaced, query-derived list ID andUSE_EXISTINGpolicy.rest-api/api/pkg/api/handler/taskrun_test.go#L520-L525: captureStartWorkflowOptionsand assert the namespaced, run-and-query-derived target-list ID andUSE_EXISTINGpolicy.🤖 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/handler/taskrun_test.go` around lines 316 - 322, Expand the task-run workflow-ID tests to cover every deterministic input: in rest-api/api/pkg/api/handler/taskrun_test.go:316-322 add a successful includeStats=true case asserting its distinct flow-grpc-task-run-get-...-true ID; at :404-409 capture StartWorkflowOptions and assert the namespaced, query-derived list ID with USE_EXISTING; at :520-525 capture StartWorkflowOptions and assert the namespaced, run-and-query-derived target-list ID with USE_EXISTING.
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the proxied Flow method in the mock.
testRunProxyDispatchmatches thegrpcproxy.Requestargument withmock.Anything, so tests can pass when a handler supplies the wrongFullMethod. Pass the expected method to the helper and matchgrpcproxy.Request.FullMethodagainst the endpoint'sflowv1.Flow_*_FullMethodName.🤖 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/handler/taskrun_test.go` around lines 70 - 76, Update testRunProxyDispatch to accept the expected Flow method and replace the grpcproxy.Request mock.Anything matcher with an assertion that Request.FullMethod equals the corresponding flowv1.Flow_*_FullMethodName for each endpoint. Update all helper call sites to pass the appropriate expected method while preserving the existing workflow-start capture and error behavior.
🤖 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/handler/taskrun_test.go`:
- Around line 316-322: Expand the task-run workflow-ID tests to cover every
deterministic input: in rest-api/api/pkg/api/handler/taskrun_test.go:316-322 add
a successful includeStats=true case asserting its distinct
flow-grpc-task-run-get-...-true ID; at :404-409 capture StartWorkflowOptions and
assert the namespaced, query-derived list ID with USE_EXISTING; at :520-525
capture StartWorkflowOptions and assert the namespaced, run-and-query-derived
target-list ID with USE_EXISTING.
- Around line 70-76: Update testRunProxyDispatch to accept the expected Flow
method and replace the grpcproxy.Request mock.Anything matcher with an assertion
that Request.FullMethod equals the corresponding flowv1.Flow_*_FullMethodName
for each endpoint. Update all helper call sites to pass the appropriate expected
method while preserving the existing workflow-start capture and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73f1f28a-a167-45de-9df8-14a8b3abbde4
📒 Files selected for processing (3)
rest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
The proxy's workflow timeout expires inside the caller's context budget, so a timeout reaching the handler comes from an execution that already closed itself. Terminating it failed and reported a data desync that had not happened; the handler now returns the proxy's 504 unchanged. Tests pin the Flow method each endpoint proxies, which the workflow ID alone could not show, plus the derived IDs and conflict policies for the list and target-list reads. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
kunzhao-nv
marked this pull request as draft
August 7, 2026 23:01
|
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. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/api/pkg/api/handler/taskrun_test.go (1)
67-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout response regression test.
The changed proxy contract returns
504for a proxy timeout and does not callTerminateWorkflow. The shown failure cases only inject a generic scheduling error and expect500.Add a case that injects the proxy timeout condition, asserts
504, and asserts thatTerminateWorkflowis not called. This protects the changed timeout behavior.As per coding guidelines, follow the shared Engineering Guidelines for “verification expectations.”
🤖 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/handler/taskrun_test.go` around lines 67 - 82, Add a regression case to the existing proxy failure tests that injects the proxy timeout condition, expects HTTP 504, and verifies TerminateWorkflow is not called. Reuse the existing test helper and mock setup around testRunProxyDispatch, preserving the current generic scheduling-error case and its 500 expectation.Source: Coding guidelines
🤖 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/handler/taskrun_test.go`:
- Around line 67-82: Add a regression case to the existing proxy failure tests
that injects the proxy timeout condition, expects HTTP 504, and verifies
TerminateWorkflow is not called. Reuse the existing test helper and mock setup
around testRunProxyDispatch, preserving the current generic scheduling-error
case and its 500 expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44f5ec16-f6dd-4c6c-8a82-1461868c4d2b
📒 Files selected for processing (3)
rest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- rest-api/skills/rest-flow-grpc-proxy/SKILL.md
Extends the migration from the eight Run endpoints to all 26 Flow-backed workflow types, reached from 29 call sites across the rack, tray, task, task-rule, and run handlers plus the shared power, bring-up, and firmware helpers. Each site keeps its own workflow ID derivation and conflict policy, namespaced under flow-grpc- so a proxy request cannot attach to a bespoke execution of the same derived name. The tray get and list sites set only a reuse policy and leave the conflict policy unspecified, so identical concurrent reads still do not coalesce there; that asymmetry is carried over rather than normalized under cover of a transport change. Promotes the workflow-ID and dispatch helpers into common so all five handler files share them, and drops the initializer clauses the Run migration introduced. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
kunzhao-nv
changed the title
A DeadlineExceeded in the workflow result was read as proof the caller had stopped waiting, but it can also come from inside a live execution. Only wfCtx carries that evidence, and a Temporal timeout is the stronger signal when both hold, so it is classified first. Neither case terminates the execution: the activity does not heartbeat, so cancellation cannot reach an in-flight Flow RPC, and freeing a deterministic ID would let a retry start a duplicate mutation instead of attaching through USE_EXISTING. A caller that goes away during the start now reports 504 rather than 500. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
The power, bring-up and firmware helpers pick the Flow method themselves and are the only route to it, so the handler tests matching any method left the choice unverified. Assert the method, workflow ID, conflict policy and decoded request per power state, including the Forced flag that is all that separates force-off from off on the wire. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Routing these endpoints through the generic proxy changed their timeout response from 500 to 504, which the spec did not describe. The shared GatewayTimeoutError says what a client should do about it, since a timed-out mutation may still be running at the site and a retry attaches to it. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Covers 24c060d. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every REST endpoint backed by on-site Flow had its own Temporal workflow and activity pair: 26 workflow types whose bodies were all the same single-activity pass-through, each needing its own registration on the site agent. This switches all of them to the generic Flow gRPC proxy added in #4560, so a new Flow endpoint no longer needs a workflow, an activity, and an agent release to reach the site.
Description
ExecuteFlowGRPC: the run, rack, tray, task, and task-rule handlers, plus the sharedExecutePowerControlWorkflow,ExecuteBringUpRackWorkflow, andExecuteFirmwareUpdateWorkflowhelpers, which together cover the power, bring-up, and firmware endpoints. Nothing underapi/still submits a bespoke Flow workflow type.flow-grpc-. The parameter-derivation rules are unchanged, but the resulting string has to differ: a deterministic ID plusUSE_EXISTINGattaches to whichever execution already holds that name, and the bespoke workflows are still registered during the rollout, so a collision would hand the proxy a payload it cannot decode.common.FlowWorkflowIDandcommon.ProxyFlowGRPCso all five handler files share the namespacing and dispatch instead of repeating them.resolveTrayIDsBySlotis not a handler and returns a plainerror, so it callsExecuteFlowGRPCdirectly.Related issues
Closes #4271. Builds on #4560.
Type of Change
Breaking Changes
Testing
Additional Notes