fix: scope offloaded node status query to the page. Fixes #16611 - #16618
fix: scope offloaded node status query to the page. Fixes #16611#16618HsiuChuanHsu wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesKeyed offload status retrieval
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change narrows offloaded node-status lookups to the workflows on the requested page; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ListWorkflows
participant OffloadNodeStatusRepo
participant SQLDatabase
ListWorkflows->>ListWorkflows: Collect offloaded UID/version keys from current page
ListWorkflows->>OffloadNodeStatusRepo: List(namespace, keys)
OffloadNodeStatusRepo->>SQLDatabase: Query matching UID/version records
SQLDatabase-->>OffloadNodeStatusRepo: Matching node status records
OffloadNodeStatusRepo-->>ListWorkflows: Keyed node status map
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Joibel
left a comment
Joibel
left a comment
There was a problem hiding this comment.
The core change looks sound — I verified the empty-keys guard is genuinely load-bearing (an empty db.Or() is dropped by upper/db's template builder, so the query really would widen back to the whole namespace), the OR-of-ANDs parenthesisation renders correctly, and List has exactly one production caller. Inline comments below: one test assertion that can never fail, one latent scale edge, and a handful of smaller things.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
| _, err := server.ListWorkflows(ctx, &workflowpkg.WorkflowListRequest{Namespace: "argo"}) | ||
| require.NoError(t, err) | ||
|
|
||
| offloadNodeStatusRepo.AssertNotCalled(t, "List") |
There was a problem hiding this comment.
AssertNotCalled(t, "List") can never fail: testify matches the argument matchers you supply against each recorded call, and with none supplied a real two-argument List call never matches, so the assertion passes unconditionally. (Verified by mutation: removing the len(offloaded) > 0 guard and stubbing List still lets this test pass — today it only has teeth because an unstubbed call panics.)
Use offloadNodeStatusRepo.AssertNumberOfCalls(t, "List", 0) (counts by method name) or AssertNotCalled(t, "List", mock.Anything, mock.Anything).
There was a problem hiding this comment.
Switched to AssertNumberOfCalls(t, "List", 0).
Confirmed by mutation both ways: with the len(offloaded) > 0 guard removed and List stubbed, the new assertion fails and the old AssertNotCalled(t, "List") still passes.
| // MariaDB, Postgres and SQLite. | ||
| // | ||
| // Each pair costs two placeholders, so this tops out at roughly 32k pairs on MySQL. A page | ||
| // that large is not a real scenario; batch the keys here if that ever changes. |
There was a problem hiding this comment.
"A page that large is not a real scenario" isn't quite enforced anywhere: ListWorkflows treats options.Limit == 0 as unbounded, and argo list defaults --chunk-size to 0, so the default CLI listing puts every live workflow in the namespace into one "page". A namespace with >~32k offloaded live workflows would now hit the placeholder ceiling as a hard error on a request that previously worked (albeit slowly). Since the comment already names the mitigation, consider actually batching here — it's a small loop.
There was a problem hiding this comment.
Batched.
List now chunks at 1000 pairs via slices.Chunk and merges the per-batch results. You're right that nothing bounded the page: --chunk-size defaults to 0 and ListWorkflows treats Limit == 0 as unbounded.
The comment now says why the ceiling exists instead of asserting the case can't happen.
|
|
||
| offloadNodeStatusRepo.On("List", mock.Anything, mock.Anything).Return(map[sqldb.UUIDVersion]v1alpha1.Nodes{}, nil) | ||
|
|
||
| _, err := server.ListWorkflows(ctx, &workflowpkg.WorkflowListRequest{Namespace: "argo"}) |
There was a problem hiding this comment.
Neither of these tests asserts the fix's output: the response is discarded and the mock returns an empty map, so wfs[i].Status.Nodes = offloadedNodes[key] — the assignment this PR restructured into a second index-keyed loop — is never exercised. An implementation that assigned the wrong workflow's nodes (or none) would pass every test in the PR. Have the mock return real nodes for one key and assert the returned list carries them on the right workflow.
There was a problem hiding this comment.
Right, the response was discarded.
TestListWorkflows_PassesOnlyPageKeys now has the mock return real nodes for one key and asserts they land on that workflow and not on the other.
Verified by mutation: assigning under a wrong key fails it.
| // TestMySQLListOnlyReturnsRequestedKeys covers the behaviour the list path depends on: the | ||
| // query is scoped to the keys it is given, so a caller that needs one page of workflows does | ||
| // not pull every offloaded blob in the namespace. | ||
| func TestMySQLListOnlyReturnsRequestedKeys(t *testing.T) { |
There was a problem hiding this comment.
These fixtures can't distinguish OR-of-ANDs from a broken cross-product: each uid has exactly one stored version, so uid IN (...) AND version IN (...) would return the same rows and pass. Since Save deliberately leaves superseded rows behind, mismatched pairs genuinely exist in production. A fixture with uid-a at v1+v2 and uid-b at v1+v2, requesting (uid-a,v1) and (uid-b,v2), would pin the exact property uuidVersionIn exists to provide.
There was a problem hiding this comment.
Both uids now have the same two data payloads. Because the version number is based on the data, both uids now share the exact same version numbers.
Before I made this change, every version number was unique to its uid. Because of that, a bad database query (uid IN (...) AND version IN (...)) would accidentally return the correct rows. I checked this: my first test passed even though the query logic was wrong.
Now, by sharing the version numbers, the bad query returns four rows and correctly fails the test.
Because this updated test now catches the problem, we don't need TestMySQLListExcludesSupersededVersions anymore, so I have deleted it.
|
|
||
| // TestListWorkflows_PassesOnlyPageKeys asserts that the offload query is scoped to the | ||
| // workflows on this page, rather than to the whole namespace. | ||
| func TestListWorkflows_PassesOnlyPageKeys(t *testing.T) { |
There was a problem hiding this comment.
The name says PassesOnlyPageKeys but there's no pagination here — no ListOptions, so all three fixtures land on one page and the test proves "only offloaded workflows' keys are passed", not page-scoping. A request with a Limit that splits the offloaded workflows across two pages would test the thing the PR title claims.
There was a problem hiding this comment.
Added a Limit of 2 over three offloaded fixtures with distinct start times, so the third is on the next page and the test now proves page scoping.
| wdc.log.WithFields(logging.Fields{"namespace": namespace}).Debug(ctx, "Listing offloaded nodes") | ||
| // uuidVersionIn matches exactly the given (uid, version) pairs. Written as OR-of-ANDs rather | ||
| // than a row value `(uid, version) IN ((?,?),...)` so that it behaves the same on MySQL, | ||
| // MariaDB, Postgres and SQLite. |
There was a problem hiding this comment.
SQLite isn't a backend this code can run against — the persistence layer only wires up Postgres and MySQL/MariaDB (the SQLite in the tree is the separate in-memory live-workflow store, not upper/db). Suggest dropping "and SQLite". Relatedly, the comment claims cross-engine portability but only MySQL/MariaDB are tested; a Postgres testcontainer helper already exists in util/sqldb if you want parity, or narrow the comment to the engines covered.
There was a problem hiding this comment.
I have removed SQLite support here because dbTypeFromConfig only returns Postgres or MySQL, and updated the comments to exclusively mention MySQL, MariaDB, and Postgres.
Regarding Postgres parity: the persist/sqldb package currently cannot import setupPostgresContainer because it is located in a test file (util/sqldb/session_test.go).
To resolve this dependency issue, we would need to move it to a standard, non-test file, similar to how mysql_test_helper.go is set up.
I am happy to implement this change, but I'd prefer to handle this in a follow-up PR so we don't expand the scope of this one.
| if err != nil { | ||
| return nil, sutils.ToStatusError(err, codes.Internal) | ||
| } | ||
| // This page is already resolved, so we know exactly which offloaded rows we need. |
There was a problem hiding this comment.
Removing the unreachable else { logger.Warn(..., sqldb.OffloadNodeStatusDisabled) } branch is right (it's been dead inside the IsEnabled() guard since 2020), but it was the last reference to sqldb.OffloadNodeStatusDisabled — after this PR only the declaration remains, surviving lint because it's exported. Please delete the const with its last user.
There was a problem hiding this comment.
Deleted the const.
| // offloadTestServer builds the smallest server that can serve ListWorkflows, and hands back | ||
| // the offload mock so tests can assert on how it was called. It deliberately does not reuse | ||
| // getWorkflowServer, which is shared by many other tests and does not expose the mock. | ||
| func offloadTestServer(t *testing.T, wfs ...v1alpha1.Workflow) (Server, context.Context, *mocks.OffloadNodeStatusRepo) { |
There was a problem hiding this comment.
This is the third near-identical server harness in the package (after getWorkflowServer and getWorkflowServerWithArtifacts) — the reactor block is byte-identical and the 12-positional-arg NewServer call now needs updating in three places. The doc comment's justification ("does not expose the mock") argues for returning the mock from getWorkflowServer as a third value, not for a copy. I've asked for this kind of extraction before (#15936, #15237).
| ) | ||
|
|
||
| // offloadedWorkflow builds a workflow whose node status lives in the offload table. | ||
| func offloadedWorkflow(name, uid, version string) v1alpha1.Workflow { |
There was a problem hiding this comment.
offloadedWorkflow(name, uid, "") builds workflows that are explicitly not offloaded — the premise of TestListWorkflows_SkipsQueryWhenPageHasNoOffload hides in an empty-string argument to a helper named the opposite. A small inlineWorkflow(name, uid) wrapper would make those call sites read correctly.
There was a problem hiding this comment.
Added inlineWorkflow(name, uid) and used it at those call sites.
| offloadNodeStatusRepo := &mocks.OffloadNodeStatusRepo{} | ||
| offloadNodeStatusRepo.On("IsEnabled", mock.Anything).Return(true) | ||
| offloadNodeStatusRepo.On("List", mock.Anything).Return(map[sqldb.UUIDVersion]v1alpha1.Nodes{}, nil) | ||
| offloadNodeStatusRepo.On("List", mock.Anything, mock.Anything).Return(map[sqldb.UUIDVersion]v1alpha1.Nodes{}, nil) |
There was a problem hiding this comment.
This stub is dead — archivedWorkflowServer never calls List (it uses Get and Save), and there's no AssertExpectations to notice. Since the signature change forced you to touch the line anyway, deleting it would have been the better migration.
There was a problem hiding this comment.
Deleted the stub rather than migrating it.
Batch the offload query at 1000 (uid, version) pairs. ListWorkflows treats an unset limit as the whole namespace and `argo list` defaults --chunk-size to 0, so the key count is caller-controlled and needs a ceiling below the 65535 placeholder limit. Delete OffloadNodeStatusDisabled, whose last user went away with the unreachable else branch, and the dead List stub in the archive server test. Extract newTestServer from the three near-identical server harnesses in server/workflow, so the reactor block and the 12-argument NewServer call live in one place. Strengthen the tests: - AssertNumberOfCalls(t, "List", 0) instead of AssertNotCalled(t, "List"), which can never fail without argument matchers. - Assert that offloaded nodes land on the right workflow, not just that the right keys were requested. - Paginate TestListWorkflows_PassesOnlyPageKeys so it proves page scoping. - Give both uids in the MySQL fixture shared version values, so a cross-product implementation returns four rows and fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: HsiuChuanHsu <hchsu2106@gmail.com>
👋 PR readiness checkThanks for your contribution! A few automated checks need attention before a maintainer reviews — these are all things you can fix yourself: PR description / templateThe PR description does not appear to follow the template:
(A maintainer may waive this.) 🤖 Automated PR-readiness helper — it re-checks each time CI finishes. Unit/E2E test results are not covered here. Questions? See the contributing guide or ask a maintainer. |
|
Thanks for taking the time to review this. I've pushed a new commit addressing your comments. |
2 participants
Fixes #16611
Motivation
ListWorkflowsresolves a page of workflows and then callsOffloadNodeStatusRepo.List with only the namespace. As a result, the repository loads all offloaded node-status blobs in that namespace, even though the page usually needs only a small subset.The required rows can already be identified by the existing primary key (clustername, uid, version). This PR reuses that key and introduces no schema or index changes.
Modifications
OffloadNodeStatusRepo.Listnow accepts the required(uid, version)pairs.ListWorkflowscollects these keys from the resolved page and passes them to the repository.%%{init: {'theme':'base','themeVariables':{'actorBkg':'#ffffff','actorBorder':'#d1d5db','actorTextColor':'#374151','primaryTextColor':'#374151','lineColor':'#9ca3af','noteBkgColor':'#fffbeb','noteTextColor':'#4b5563','noteBorderColor':'#e5e7eb','messageTextColor':'#374151','sequenceNumberColor':'#ffffff'}}}%% sequenceDiagram participant CLI as argo CLI / UI participant SRV as argo-server ListWorkflows participant SRC as live lister + archive participant DB as argo_workflows table CLI->>SRV: argo list — one page of 10 SRV->>SRC: fetch this page (limit / offset applied) SRC-->>SRV: 10 items with UID + version — 2 are offloaded alt before — page keys are known, but not passed down rect rgb(255, 235, 238) SRV->>DB: ✗ SELECT nodes — namespace only, no uid, no LIMIT Note over DB: 20 offloaded rows here — all of them match DB-->>SRV: ✗ 20 blobs — 20.1 MB Note over SRV: 18 discarded, 2 used end else after — page keys go with the query rect rgb(232, 245, 233) SRV->>SRV: ✓ collect (uid, version) of the offloaded rows opt nothing on this page is offloaded Note over SRV: ✓ no query is issued at all end SRV->>DB: ✓ SELECT nodes — same query AND the 2 keys Note over DB: primary key is (clustername, uid, version) Note over DB: so each key is a point lookup, not a scan DB-->>SRV: ✓ exactly the 2 blobs this page needs end end SRV-->>CLI: one page of 10 workflows — identical responseVerification
Added tests covering:
ListWorkflowspasses only the required offloaded keys and skips the repository call when none are needed.(uid, version)pairs, including correct handling of multiple versions of the same workflow.Documentation
AI
Claude Code (Opus 5) assisted with the analysis, implementation and tests. All changes were reviewed by the me.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests