Repository navigation
feat(rod): build user allocations from per-author storage rows - #1596
Closed
janzak-dev wants to merge 2 commits into
Closed
janzak-dev wants to merge 2 commits into
janzak-dev wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the rod_data_provider service to dynamically map and group author resource allocations instead of using static mock user allocations. It introduces new data structures in the storage package and implements logic to resolve generic accelerators to specific queue flavors. The review feedback points out a potential nil pointer dereference panic in buildUniqueQueueAcceleratorMap when retrieving a cluster queue from the cache, suggesting an additional nil check.
This was referenced Nov 6, 2026
janzak-dev
commented
Nov 6, 2026
Collaborator
Author
There was a problem hiding this comment.
Thanks for the review. I checked each suggestion against the code in this stack; replies with the reasoning are inline. No code changes were needed in this round.
Add UserResourceAllocation (user, resource type, running and pending units) and GetRunningAndPendingJobsResponse field 4 (user_allocations), plus the matching UI response type. The field stays empty until the backend aggregates allocations from active work units. Bug: 582253355 TAG=agy CONV=4ed50232-a6f2-4c76-aa6f-ef1c3833a550
Add storage.AuthorResourceAllocation (author, resource type, running or pending state, units) and WorkUnitStore.AggregateActiveResourcesByAuthor. GetRunningAndPendingJobs calls it after fetching the jobs page and maps the rows to user_allocations: group per user and resource type, attribute a generic nvidia.com/gpu request to the queue's GPU model when the queue has exactly one GPU flavor, and sort by total units. The Spanner and Postgres implementations return no rows until the aggregation over active work units is implemented, so user_allocations stays empty. Bug: 582253355 TAG=agy CONV=4ed50232-a6f2-4c76-aa6f-ef1c3833a550
janzak-dev
force-pushed
the
feat/rod-user-alloc-backend-mapping
branch
from
November 6, 2026 13:10
3898e44 to
98ee93c
Compare
janzak-dev
force-pushed
the
feat/rod-user-alloc-api-scaffold
branch
from
November 6, 2026 13:10
1d1d649 to
1c8a5aa
Compare
janzak-dev
force-pushed
the
feat/rod-user-alloc-api-scaffold
branch
from
November 6, 2026 13:55
1c8a5aa to
a458068
Compare
Collaborator
Author
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
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.
Note
Stacked PR (part of #1515): per-user "dominant resource" allocation on
/resources. Each layer has a production caller and tests (no dead code); unfinished backend pieces return empty data, never placeholder values.Endpoint scaffold
user_allocations+ UI response type; the field stays emptyBackend: RPC presents correct information
2. #1596 Storage contract + per-author mapping (backends return no rows yet, so the field stays empty)
3. #1597 Spanner + Postgres aggregation (field is populated)
4. #1598 Jobs page and aggregation from one read-only snapshot
Frontend: UI presents correct data
5. #1599 Per-user allocation bar on
/resourcesSummary
Adds the storage contract
WorkUnitStore.AggregateActiveResourcesByAuthor, returning per-author rows (storage.AuthorResourceAllocation: author, resource type, running or pending state, units).GetRunningAndPendingJobsnow buildsuser_allocationsfrom those rows. The Spanner and Postgres implementations return no rows until the next PR implements the aggregation, so the field stays empty.Motivation & Context
The first backend layer: it lands the storage contract and the real mapping to the API, so the next layer only has to implement the queries.
Key Changes
internal/storage/storage.go:ActiveResourceState(RUNNING/PENDING),AuthorResourceAllocation, andWorkUnitStore.AggregateActiveResourcesByAuthor.MockStorageis updated.AggregateActiveResourcesByAuthor: return an empty slice (documented as not yet implemented).cmd/rod_data_provider/user_allocations.go: groups rows per user and resource type, sums running and pending, and sorts by total units with deterministic tie-breakers. When the queue has exactly one GPU flavor, a genericnvidia.com/gpurequest is attributed to that flavor's model (via the existingresolveResourceType).jobs.go:fetchJobsAndAllocationsfetches the jobs page, then the per-author rows, and maps them. An aggregation error fails the RPC withcodes.Internal.WorkUnitFetcheris updated.Assumptions
nvidia.com/gpu, so a specific-model filter can miss it, but the parent filter always includes it.Verification & Testing
bazel test //cmd/rod_data_provider/... //internal/storage/...: all pass. That includes mapping, ordering and tie-breaker tests, single-flavor vs multi-flavor GPU attribution, handler tests (allocations mapped from storage rows, aggregation error →Internal), and empty-result tests for both backends../.githooks/pre-commit: passes.Related Issues / Bugs
Part of #1515
TAG=agy
CONV=4ed50232-a6f2-4c76-aa6f-ef1c3833a550