refactor: Migrate Allocation API handlers to WithTx transaction helper#479
Open
chet wants to merge 1 commit intoNVIDIA:mainfrom
Open
refactor: Migrate Allocation API handlers to WithTx transaction helper#479chet wants to merge 1 commit intoNVIDIA:mainfrom
chet wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughReplaces manual SQL transaction management in Allocation create/update/delete with ChangesAllocation Handler Transaction Refactor
Instance Workflow Timeout Termination
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler
participant Database
participant Temporal
participant Site_Workflow
Client->>API_Handler: Create/Update/Delete/Reboot request
API_Handler->>Database: Start transaction (cdb.WithTx)
API_Handler->>Temporal: Start synchronous workflow call
alt Temporal times out or ctx deadline exceeded
Temporal-->>API_Handler: Timeout error
API_Handler->>API_Handler: set timeoutResp (terminate closure)
API_Handler->>Database: return from WithTx (transaction ends/rolled back)
API_Handler->>Site_Workflow: invoke timeoutResp -> terminate workflow
API_Handler-->>Client: HTTP 500 "workflow timed out"
else Temporal completes
Temporal-->>API_Handler: Workflow accepted / ID
API_Handler->>Database: commit transaction (within WithTx)
API_Handler-->>Client: Success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-05-04 17:24:35 UTC | Commit: 6e8f66e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/allocation.go`:
- Around line 1327-1398: The code can call ipam.DeleteChildIpamEntryFromCidr
with a nil parentIPBlock when ipbDAO.GetByID returned cdb.ErrDoesNotExist; add
an explicit guard after the parentIPBlock lookup (and before any IPAM cleanup)
to check parentIPBlock != nil (or return a clear API error) so
DeleteChildIpamEntryFromCidr is never invoked with a nil parent; locate the
logic around parentIPBlock, childIPBlock, ipbDAO.GetByID and the call to
ipam.DeleteChildIpamEntryFromCidr and either return cutil.NewAPIError(...) or
skip IPAM cleanup when parentIPBlock is nil.
- Around line 1015-1032: The rename check is raceable because aDAO.GetAll()
followed by Update can be interleaved; either wrap the uniqueness check and the
subsequent update in the same tenant/site lock (use a mutex keyed by
existingA.TenantID+existingA.SiteID so both the GetAll and the call to
aDAO.Update happen while holding that lock) or remove the pre-check and instead
detect a DB unique-constraint error returned by aDAO.Update and translate it to
a 409 via cutil.NewAPIError (include the conflicting allocation ID in
validation.Errors like the current code does). Ensure you update the code paths
that call aDAO.Update to map the DB unique-violation error to an HTTP 409 and
keep aDAO.GetAll only for non-racy read-only use.
- Around line 180-188: After acquiring the advisory lock in the cdb.WithTx block
(inside the function using cah.dbSession and tx.TryAcquireAdvisoryLock with
cdb.GetAdvisoryLockIDFromString), re-run the uniqueness/conflict check for an
Allocation with the same tenant, site and name inside this transaction (using
the same repository/query used earlier for the preflight check) and if a record
is found return a 409 Conflict (e.g. return
cutil.NewAPIError(http.StatusConflict, "Allocation with this name already
exists", nil)) instead of proceeding to create; this ensures concurrent requests
that passed the preflight are rejected correctly rather than creating duplicates
or surfacing a 500.
🪄 Autofix (Beta)
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: 3ef95bc4-90df-43a8-8000-c7cb2bf436cb
📒 Files selected for processing (1)
api/pkg/api/handler/allocation.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
api/pkg/api/handler/allocation.go (4)
206-209: 💤 Low valueDiscarded return values indicate potential dead code.
The call to
GetAllAllocationConstraintsForInstanceTypediscards both the constraints slice and the count. If this invocation serves a side-effect purpose (e.g., cache warming or implicit validation), a brief comment would clarify intent. Otherwise, consider removing this call to reduce unnecessary database round-trips.🤖 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 `@api/pkg/api/handler/allocation.go` around lines 206 - 209, The call to GetAllAllocationConstraintsForInstanceType currently discards the returned constraints and count (call located near the allocation handling using variables tx, cah.dbSession, ip, site, tenant), which looks like dead work; either use the returned values (constraints slice and count) where needed or remove the call entirely to avoid an unnecessary DB round-trip—if the call is intentional for side-effects (validation/cache warming), add a one-line comment above the GetAllAllocationConstraintsForInstanceType invocation explaining that intent and why the return values are ignored so future readers know it’s deliberate.
1253-1254: ⚡ Quick winUnused variables
imAcDelandimAcUpdconstitute dead code.Similar to the Create handler, these slices are populated but never consumed after the transaction. If post-commit IM interaction was intended, the implementation is incomplete. Otherwise, remove these declarations.
🧹 Proposed cleanup
- imAcDel := []cdbm.AllocationConstraint{} - imAcUpd := []cdbm.AllocationConstraint{} ipamStorage := ipam.NewIpamStorage(dah.dbSession.DB, tx.GetBunTx()) @@ ... inside the loop ... - if acCnt > 1 { - imAcUpd = append(imAcUpd, ac) - } else if acCnt == 1 { - imAcDel = append(imAcDel, ac) - }Also applies to: 1345-1349
🤖 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 `@api/pkg/api/handler/allocation.go` around lines 1253 - 1254, The local slices imAcDel and imAcUpd in the allocation handler are declared and populated but never used after the transaction (dead code); either remove their declarations and any code that appends to them, or implement the missing post-commit IM interaction that consumes them (mirror the pattern used in the Create handler: collect constraints during processing, then after tx commit invoke the IM/update function with imAcDel and imAcUpd). Update or remove all occurrences of imAcDel and imAcUpd so there are no unused variables left.
372-390: ⚡ Quick winUnused variables
imAcAddandimAcUpdconstitute dead code.These slices are populated within the closure but are never referenced after the transaction commits. If Instance Manager (IM) interaction was intended post-commit, the logic is missing. Otherwise, remove these allocations to improve clarity.
🧹 Proposed cleanup
- imAcAdd := []cdbm.AllocationConstraint{} - imAcUpd := []cdbm.AllocationConstraint{} for _, ac := range dbacs { retac, serr := acDAO.CreateFromParams(ctx, tx, a.ID, ac.ResourceType, ac.ResourceTypeID, ac.ConstraintType, ac.ConstraintValue, ac.DerivedResourceID, dbUser.ID) if serr != nil { logger.Error().Err(serr).Msg("error creating Allocation Constraint DB entry") return cutil.NewAPIError(http.StatusInternalServerError, "Failed to create Allocation Constraint entry for Allocation", nil) } dbacsRet = append(dbacsRet, *retac) - _, cnt, derr := common.GetAllAllocationConstraintsForInstanceType(ctx, tx, cah.dbSession, ip, site, tenant, &ac.ResourceTypeID) - if derr != nil { - logger.Error().Err(derr).Msg("error getting Allocation Constraints") - return cutil.NewAPIError(http.StatusInternalServerError, "Failed to create Allocation, db error", nil) - } - if cnt > 1 { - imAcUpd = append(imAcUpd, *retac) - } else if cnt == 1 { - imAcAdd = append(imAcAdd, *retac) - }🤖 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 `@api/pkg/api/handler/allocation.go` around lines 372 - 390, The slices imAcAdd and imAcUpd are allocated and appended to inside the dbacs loop but never used after commit (dead code); either remove their declarations and the append logic around imAcAdd/imAcUpd in the loop, or (if IM interaction was intended) move the IM-update logic to run after the transaction commits and replace the current appends with code that enqueues or executes the required IM calls using the populated slices; locate the loop that iterates over dbacs in allocation.go and update the code paths around imAcAdd, imAcUpd, and the cnt checks to implement one of these two fixes.
1107-1108: 💤 Low valueRedundant
acDAOdeclaration shadows earlier variable.
acDAOis already declared at line 1070 within the same closure scope. The second declaration with:=at line 1107 shadows it. While functionally harmless, using assignment (=) or reusing the existing variable would be cleaner.♻️ Suggested fix
- acDAO := cdbm.NewAllocationConstraintDAO(uah.dbSession) - retAcs, _, derr := acDAO.GetAll(ctx, tx, []uuid.UUID{a.ID}, nil, nil, nil, nil, nil, nil, nil, nil) + retAcs, _, derr := acDAO.GetAll(ctx, tx, []uuid.UUID{a.ID}, nil, nil, nil, nil, nil, nil, nil, nil)🤖 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 `@api/pkg/api/handler/allocation.go` around lines 1107 - 1108, The code re-declares acDAO with := causing a shadow of the earlier acDAO; change the second declaration to an assignment so the existing acDAO is reused (replace the := with =) where you call NewAllocationConstraintDAO(uah.dbSession) and then call acDAO.GetAll(ctx, tx, []uuid.UUID{a.ID}, ...), keeping retAcs and derr handling the same; ensure you reference the already-declared acDAO variable (used around NewAllocationConstraintDAO and GetAll) rather than creating a new scoped variable.
🤖 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 `@api/pkg/api/handler/allocation.go`:
- Around line 206-209: The call to GetAllAllocationConstraintsForInstanceType
currently discards the returned constraints and count (call located near the
allocation handling using variables tx, cah.dbSession, ip, site, tenant), which
looks like dead work; either use the returned values (constraints slice and
count) where needed or remove the call entirely to avoid an unnecessary DB
round-trip—if the call is intentional for side-effects (validation/cache
warming), add a one-line comment above the
GetAllAllocationConstraintsForInstanceType invocation explaining that intent and
why the return values are ignored so future readers know it’s deliberate.
- Around line 1253-1254: The local slices imAcDel and imAcUpd in the allocation
handler are declared and populated but never used after the transaction (dead
code); either remove their declarations and any code that appends to them, or
implement the missing post-commit IM interaction that consumes them (mirror the
pattern used in the Create handler: collect constraints during processing, then
after tx commit invoke the IM/update function with imAcDel and imAcUpd). Update
or remove all occurrences of imAcDel and imAcUpd so there are no unused
variables left.
- Around line 372-390: The slices imAcAdd and imAcUpd are allocated and appended
to inside the dbacs loop but never used after commit (dead code); either remove
their declarations and the append logic around imAcAdd/imAcUpd in the loop, or
(if IM interaction was intended) move the IM-update logic to run after the
transaction commits and replace the current appends with code that enqueues or
executes the required IM calls using the populated slices; locate the loop that
iterates over dbacs in allocation.go and update the code paths around imAcAdd,
imAcUpd, and the cnt checks to implement one of these two fixes.
- Around line 1107-1108: The code re-declares acDAO with := causing a shadow of
the earlier acDAO; change the second declaration to an assignment so the
existing acDAO is reused (replace the := with =) where you call
NewAllocationConstraintDAO(uah.dbSession) and then call acDAO.GetAll(ctx, tx,
[]uuid.UUID{a.ID}, ...), keeping retAcs and derr handling the same; ensure you
reference the already-declared acDAO variable (used around
NewAllocationConstraintDAO and GetAll) rather than creating a new scoped
variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8708791-d04c-4bca-b5f5-6191433c0952
📒 Files selected for processing (1)
api/pkg/api/handler/allocation.go
chet
changed the title
chet
changed the title
chet
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/pkg/api/handler/instance.go`:
- Around line 1597-1602: The timeout branch closes over the outer err which
later gets reassigned by cdb.WithTx, so capture the original workflow timeout
error into a new local variable before assigning timeoutResp; e.g., store the
current err into something like workflowTimeoutErr and have timeoutResp call
common.TerminateWorkflowOnTimeOut with workflowTimeoutErr (referencing
timeoutResp, err, cdb.WithTx, and common.TerminateWorkflowOnTimeOut to locate
the code).
🪄 Autofix (Beta)
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: 8624ae5a-d353-4ffb-b044-1f0f7c7d4166
📒 Files selected for processing (2)
api/pkg/api/handler/allocation.goapi/pkg/api/handler/instance.go
Applies `WithTx` (from NVIDIA#462) to the Create/Update/Delete `allocation` handlers. Used `WithTx` across the board (and not `WithTxResult`) since each handler needs multiple values out, which is the case in other handlers too, as well as including the `TerminateWorkflowOnTimeOut` helper @coderabbitai feedback addressed in advance (I just ran `coderabbit` CLI locally to save back/forth): - Log the correct error variable on the `GetAllAllocationConstraintsForInstanceType` failure path. - Defensive `nil` check on `ac.DerivedResourceID` before dereferencing it. Signed-off-by: Chet Nichols III <chetn@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
1 participant
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.
Applies
WithTx(from #462) to the Create/Update/Deleteallocationhandlers. UsedWithTxacross the board (and notWithTxResult) since each handler needs multiple values out, which is the case in other handlers too. We could probably change toWithTxResultby putting all values into its own struct, or making a newWithTxResults? That can happen later though. Might be a nice single PR to sweep through cases doing this to introduce multi-value behavior/support?Also includes using the
TerminateWorkflowOnTimeOuthelper.@coderabbitai feedback addressed in advance (I just ran
coderabbitCLI locally to save back/forth):GetAllAllocationConstraintsForInstanceTypefailure path.nilcheck onac.DerivedResourceIDbefore dereferencing it.Signed-off-by: Chet Nichols III chetn@nvidia.com
Description
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes