fix: Remove deprecated instance/allocation relationships#371
Open
bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix: Remove deprecated instance/allocation relationships#371bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
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
Summary by CodeRabbit
WalkthroughRemoved instance-allocation linkage from the data model and tests; handlers now acquire a tenant+instance-type advisory quota lock and validate instance counts against aggregated allocation constraints at tenant/site/instance-type scope before allowing allocation/constraint mutations or instance creation/deletion. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bcavnvidia
force-pushed
the
inst-alloc
branch
3 times, most recently
from
1f48d51 to
8979291
Compare
![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: 6
🧹 Nitpick comments (8)
api/pkg/api/handler/operatingsystem_test.go (1)
1979-1980: Remove no-op UUID parses in test setup.Lines 1979-1980 parse values and discard results, which adds noise without meaningful assertion signal.
✂️ Proposed cleanup
- _ = uuid.MustParse(aIT.ID) - _ = uuid.MustParse(aIT.AllocationConstraints[0].ID)As per coding guidelines
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/operatingsystem_test.go` around lines 1979 - 1980, Remove the no-op uuid.MustParse calls that discard results in the test setup: delete the lines calling uuid.MustParse(aIT.ID) and uuid.MustParse(aIT.AllocationConstraints[0].ID) in operatingsystem_test.go; if the intent was to validate those IDs, replace them with explicit assertions (e.g., checking parsing errors or comparing to expected UUID values) rather than parsing and discarding, so references like aIT.ID and aIT.AllocationConstraints remain meaningful and test noise is eliminated.db/pkg/db/model/sshkeygroupinstanceassociation_test.go (1)
87-87: Make the intentional unused return explicit for readability.These calls are valid, but
_ =obscures intent. Prefer a direct call plus a short comment explaining why the constraint is still seeded in test setup.♻️ Suggested cleanup pattern
- _ = testBuildAllocationConstraint(t, dbSession, allocation, AllocationResourceTypeInstanceType, instanceType.ID, AllocationConstraintTypeReserved, 10, uuid.New()) + // Seed reserved-capacity preconditions used by instance-related validation in this test setup. + testBuildAllocationConstraint(t, dbSession, allocation, AllocationResourceTypeInstanceType, instanceType.ID, AllocationConstraintTypeReserved, 10, uuid.New())As per coding guidelines, "Document when you have intentionally omitted code that the reader might otherwise expect to be present."
Also applies to: 182-182, 285-285, 487-487, 615-615, 703-703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/sshkeygroupinstanceassociation_test.go` at line 87, The test uses `_ = testBuildAllocationConstraint(...)` which hides the intentional discard of the return; update calls to invoke testBuildAllocationConstraint(...) without assigning its return and add a short inline comment (e.g., "// intentionally ignoring return; seeding constraint for test setup") next to each call to make the intent explicit; apply this change for the occurrences of testBuildAllocationConstraint in sshkeygroupinstanceassociation_test.go (including the calls at the lines referenced and other similar calls) so readers know the result is deliberately unused.db/pkg/db/model/testing.go (1)
420-422: Remove unused placeholder parameters fromTestBuildInstancesignature.The function declares
_ *Allocationand_ *AllocationConstraintparameters that are never used in the implementation. The explicit underscore prefix and the comment "without any allocation linkage" confirm these were intentionally deprecated. Retaining them propagates stale API expectations and forces callers to construct unnecessary objects. The refactoring scope is minimal—only one callsite indb/pkg/migrations/migrations_test.go:591requires updating. Removing these parameters enforces cleaner intent at the call site and improves API expressiveness.Proposed signature cleanup
-func TestBuildInstance(t *testing.T, dbSession *db.Session, name string, _ *Allocation, _ *AllocationConstraint, tn *Tenant, ip *InfrastructureProvider, st *Site, it *InstanceType, vpc *Vpc, m *Machine, os *OperatingSystem) *Instance { +func TestBuildInstance(t *testing.T, dbSession *db.Session, name string, tn *Tenant, ip *InfrastructureProvider, st *Site, it *InstanceType, vpc *Vpc, m *Machine, os *OperatingSystem) *Instance {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/testing.go` around lines 420 - 422, Remove the two unused placeholder parameters "_ *Allocation" and "_ *AllocationConstraint" from the TestBuildInstance function signature and update its single test callsite accordingly; change the signature in TestBuildInstance(t *testing.T, dbSession *db.Session, name string, tn *Tenant, ip *InfrastructureProvider, st *Site, it *InstanceType, vpc *Vpc, m *Machine, os *OperatingSystem) *Instance and remove the corresponding arguments at the callsite(s) in the migrations tests so callers no longer construct or pass Allocation/AllocationConstraint values.api/pkg/api/handler/instance_test.go (2)
424-449: Document that this helper now intentionally omits allocation linkage.A short comment here would make the post-migration fixture contract explicit and prevent future tests from quietly reintroducing allocation-specific assumptions.
✏️ Suggested clarification
+// testInstanceBuildInstance persists an instance fixture without allocation linkage. +// Aggregate quota enforcement is derived from tenant/site/instance-type usage. func testInstanceBuildInstance(t *testing.T, dbSession *cdb.Session, name string, tn uuid.UUID, ip uuid.UUID, st uuid.UUID, ist *uuid.UUID, vpc uuid.UUID, mc *string, os *uuid.UUID, ipxeScript *string, status string) *cdbm.Instance {As per coding guidelines, "Document when you have intentionally omitted code that the reader might otherwise expect to be present."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instance_test.go` around lines 424 - 449, Add a brief comment above the test helper testInstanceBuildInstance explaining that it intentionally omits allocation linkage (i.e., it does not create or link allocation records/fields) so callers should not rely on allocation-specific state; state the expected post-migration fixture contract and when tests should create allocations explicitly, referencing the function name (testInstanceBuildInstance) so future readers do not reintroduce allocation assumptions.
1127-1130: Remove the stale commented-out helper call.This block still shows the pre-refactor helper shape and outdated argument form, so it is now more misleading than useful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instance_test.go` around lines 1127 - 1130, Remove the stale commented-out helper call block that references the old helper signature (the commented lines creating inst4 via testInstanceBuildInstance and the assert.NotNil call); locate the commented block containing "inst4 := testInstanceBuildInstance(...)" and simply delete those commented lines so the test file no longer contains the outdated pre-refactor example.api/pkg/api/handler/instancebatch.go (2)
900-955: Update the handler walkthrough for aggregate quota semantics.The execution-steps comment for this handler still talks about per-allocation-constraint capacity tracking and switching to the next constraint. After this change the handler enforces only aggregate tenant/site/instance-type quota, so the comment now points readers at logic that no longer exists. As per coding guidelines, "Document when you have intentionally omitted code that the reader might otherwise expect to be present".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 900 - 955, The existing handler walkthrough comment still describes per-allocation iterative capacity checks that were removed; update the top-of-handler comment near the Instance batch creation logic (the block around AcquireInstanceTypeQuotaLock, GetAllocationConstraintsForInstanceType and the insTotal/totalConstraintValue checks) to document the current aggregate tenant/site/instance-type quota semantics and explicitly note that per-allocation sequential allocation selection logic was intentionally omitted; mention the key symbols AcquireInstanceTypeQuotaLock, GetAllocationConstraintsForInstanceType and the insTotal + apiRequest.Count vs totalConstraintValue check so readers can find the enforced aggregate check and understand why there is no per-allocation loop.
936-940: Make the quota check explicitly count-only.This path only uses
insTotal, but it still runs while the quota lock is held. Please use the count-only pattern here so the critical section does not fetch a page of instances unnecessarily.♻️ Proposed change
_, insTotal, err := inDAO.GetAll(ctx, tx, cdbm.InstanceFilterInput{ TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs: []uuid.UUID{instancetype.ID}, -}, cdbp.PageInput{}, nil) +}, cdbp.PageInput{Limit: cdb.GetIntPtr(0)}, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 936 - 940, The current code calls inDAO.GetAll(...) while holding the quota lock but only uses insTotal; change it to a count-only call so the critical section doesn't fetch instance pages. Replace the GetAll invocation with the DAO's count-only API (e.g. call inDAO.GetAll or inDAO.GetCount with a cdbp.PageInput that has CountOnly: true, or use an explicit GetCount method if available) so only the total count (insTotal) is returned; keep the same filter (TenantIDs, SiteIDs, InstanceTypeIDs) and ensure the quota lock is still held only for the minimal duration.api/pkg/api/handler/allocation_test.go (1)
2393-2393: Keep these delete scenarios independent.
deleteInstanceIDsis applied after the handler call, so the new aggregate-capacity cases only reach their expected quota state because earlier subtests mutated the shared fixture first. Running one of the later success cases in isolation will exercise a different instance count and can flip the result. Please create per-case fixtures, or at minimum perform state setup before invoking the handler.Also applies to: 2634-2636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/allocation_test.go` at line 2393, The test shares mutable state via the slice deleteInstanceIDs causing earlier subtests to mutate the fixture and affect later quota assertions; fix by making each subtest create its own independent fixture (or reset state) and apply any deleteInstanceIDs mutations before invoking the handler rather than after; specifically, ensure per-case setup for the variable deleteInstanceIDs and perform the deletions (or fixture adjustments) prior to the handler call used in the allocation tests so each case exercises the expected aggregate-capacity independently (also apply the same fix to the other occurrences around the second group of cases referenced).
🤖 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 1314-1318: The deletion rejection message is misleading: when the
check if iCount > totalConstraintValue-deletedConstraintValue blocks the delete,
update the API error text passed to cutil.NewAPIErrorResponse (and the
logger.Warn() message if desired) to reflect the pool-scoped aggregate capacity
constraint (e.g., mention tenant/site/instance-type aggregate or “pool”
capacity) instead of saying instances exist “for this Allocation”; locate the
check using variables iCount, totalConstraintValue, deletedConstraintValue and
the calls logger.Warn() and cutil.NewAPIErrorResponse(c, http.StatusBadRequest,
...) and change the error string to communicate that existing instances in the
pool prevent deletion.
In `@api/pkg/api/handler/instance_test.go`:
- Around line 8302-8306: The two fixture assertions mistakenly check inst1
instead of the newly created fixtures, so failures creating instVpcNotReady and
instSiteNotReady are hidden; update the assertions after the calls to
testInstanceBuildInstance to assert.NotNil(t, instVpcNotReady) for the first
case and assert.NotNil(t, instSiteNotReady) for the second so each fixture
creation is validated (references: testInstanceBuildInstance, instVpcNotReady,
instSiteNotReady, inst1).
- Around line 1041-1042: The test creates a pre-existing instance inst6 using
testInstanceBuildInstance with tn5.ID, which places it under tenant 5 instead of
tenant 6 and thus fails to consume tenant-6 quota; update the call to
testInstanceBuildInstance to pass tn6.ID (i.e., replace the tn5.ID argument with
tn6.ID) so inst6 is owned by tenant 6 and the subsequent “multiple allocation
constraint” quota validation is exercised correctly.
In `@api/pkg/api/handler/instance.go`:
- Around line 1234-1238: The code only sets instanceCreateInput.InstanceTypeID
when apiRequest.InstanceTypeID is present, so instances created via a Machine
(where instanceTypeID was resolved earlier) do not persist the resolved instance
type and thus bypass quota/accounting; change the condition to set
instanceCreateInput.InstanceTypeID whenever the resolved instanceTypeID is
non-nil (not only when apiRequest.InstanceTypeID is provided) so that
instanceCreateInput.InstanceTypeID = instanceTypeID is assigned whenever
instanceTypeID was resolved, ensuring downstream quota/accounting keys see the
correct InstanceTypeID.
In `@api/pkg/api/handler/nvlinklogicalpartition_test.go`:
- Around line 858-861: The inst2 fixture passed to testInstanceBuildInstance is
using mc1.ID (a machine created for site1), making the site2 instance
inconsistent; update the test setup to create a dedicated machine for site2
(e.g., mc2) and pass that machine's ID into the second testInstanceBuildInstance
call (referencing inst2 and testInstanceBuildInstance) so the Instance ->
Machine relationship remains internally consistent for site2; apply the same
change for the other occurrence around lines ~1284-1287.
In `@db/pkg/db/model/interface_test.go`:
- Around line 551-552: The reserved allocation fixture is too small for the
number of instances created: update the call to testBuildAllocationConstraint
(the allocation reserved constraint for AllocationResourceTypeInstanceType /
AllocationConstraintTypeReserved) so the reserved quantity is increased from 10
to at least the number of instances created (e.g., 15) so aggregate quota
enforcement does not cause subsequent isd.Create inserts to fail; locate the
testInstanceBuildAllocation and testBuildAllocationConstraint usage and adjust
the numeric reserved value accordingly.
---
Nitpick comments:
In `@api/pkg/api/handler/allocation_test.go`:
- Line 2393: The test shares mutable state via the slice deleteInstanceIDs
causing earlier subtests to mutate the fixture and affect later quota
assertions; fix by making each subtest create its own independent fixture (or
reset state) and apply any deleteInstanceIDs mutations before invoking the
handler rather than after; specifically, ensure per-case setup for the variable
deleteInstanceIDs and perform the deletions (or fixture adjustments) prior to
the handler call used in the allocation tests so each case exercises the
expected aggregate-capacity independently (also apply the same fix to the other
occurrences around the second group of cases referenced).
In `@api/pkg/api/handler/instance_test.go`:
- Around line 424-449: Add a brief comment above the test helper
testInstanceBuildInstance explaining that it intentionally omits allocation
linkage (i.e., it does not create or link allocation records/fields) so callers
should not rely on allocation-specific state; state the expected post-migration
fixture contract and when tests should create allocations explicitly,
referencing the function name (testInstanceBuildInstance) so future readers do
not reintroduce allocation assumptions.
- Around line 1127-1130: Remove the stale commented-out helper call block that
references the old helper signature (the commented lines creating inst4 via
testInstanceBuildInstance and the assert.NotNil call); locate the commented
block containing "inst4 := testInstanceBuildInstance(...)" and simply delete
those commented lines so the test file no longer contains the outdated
pre-refactor example.
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 900-955: The existing handler walkthrough comment still describes
per-allocation iterative capacity checks that were removed; update the
top-of-handler comment near the Instance batch creation logic (the block around
AcquireInstanceTypeQuotaLock, GetAllocationConstraintsForInstanceType and the
insTotal/totalConstraintValue checks) to document the current aggregate
tenant/site/instance-type quota semantics and explicitly note that
per-allocation sequential allocation selection logic was intentionally omitted;
mention the key symbols AcquireInstanceTypeQuotaLock,
GetAllocationConstraintsForInstanceType and the insTotal + apiRequest.Count vs
totalConstraintValue check so readers can find the enforced aggregate check and
understand why there is no per-allocation loop.
- Around line 936-940: The current code calls inDAO.GetAll(...) while holding
the quota lock but only uses insTotal; change it to a count-only call so the
critical section doesn't fetch instance pages. Replace the GetAll invocation
with the DAO's count-only API (e.g. call inDAO.GetAll or inDAO.GetCount with a
cdbp.PageInput that has CountOnly: true, or use an explicit GetCount method if
available) so only the total count (insTotal) is returned; keep the same filter
(TenantIDs, SiteIDs, InstanceTypeIDs) and ensure the quota lock is still held
only for the minimal duration.
In `@api/pkg/api/handler/operatingsystem_test.go`:
- Around line 1979-1980: Remove the no-op uuid.MustParse calls that discard
results in the test setup: delete the lines calling uuid.MustParse(aIT.ID) and
uuid.MustParse(aIT.AllocationConstraints[0].ID) in operatingsystem_test.go; if
the intent was to validate those IDs, replace them with explicit assertions
(e.g., checking parsing errors or comparing to expected UUID values) rather than
parsing and discarding, so references like aIT.ID and aIT.AllocationConstraints
remain meaningful and test noise is eliminated.
In `@db/pkg/db/model/sshkeygroupinstanceassociation_test.go`:
- Line 87: The test uses `_ = testBuildAllocationConstraint(...)` which hides
the intentional discard of the return; update calls to invoke
testBuildAllocationConstraint(...) without assigning its return and add a short
inline comment (e.g., "// intentionally ignoring return; seeding constraint for
test setup") next to each call to make the intent explicit; apply this change
for the occurrences of testBuildAllocationConstraint in
sshkeygroupinstanceassociation_test.go (including the calls at the lines
referenced and other similar calls) so readers know the result is deliberately
unused.
In `@db/pkg/db/model/testing.go`:
- Around line 420-422: Remove the two unused placeholder parameters "_
*Allocation" and "_ *AllocationConstraint" from the TestBuildInstance function
signature and update its single test callsite accordingly; change the signature
in TestBuildInstance(t *testing.T, dbSession *db.Session, name string, tn
*Tenant, ip *InfrastructureProvider, st *Site, it *InstanceType, vpc *Vpc, m
*Machine, os *OperatingSystem) *Instance and remove the corresponding arguments
at the callsite(s) in the migrations tests so callers no longer construct or
pass Allocation/AllocationConstraint values.
🪄 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: Pro
Run ID: 8554b209-6df6-454a-b026-1aff505390ce
📒 Files selected for processing (38)
api/pkg/api/handler/allocation.goapi/pkg/api/handler/allocation_test.goapi/pkg/api/handler/allocationconstraint.goapi/pkg/api/handler/allocationconstraint_test.goapi/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/interface_test.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/networksecuritygroup_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/handler/nvlinklogicalpartition_test.goapi/pkg/api/handler/operatingsystem_test.goapi/pkg/api/handler/sshkeygroup_test.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/tenant_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/allocation.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.godb/pkg/db/model/dpuextensionservicedeployment_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/interface_test.godb/pkg/db/model/nvlinkinterface_test.godb/pkg/db/model/sshkeygroupinstanceassociation_test.godb/pkg/db/model/testing.godb/pkg/migrations/20240609234258_new_indecies.godb/pkg/migrations/20260409143000_remove_instance_allocation_relationship.godocs/index.htmlopenapi/spec.yaml
💤 Files with no reviewable changes (5)
- api/pkg/api/model/instance.go
- api/pkg/api/model/allocation.go
- api/pkg/api/model/instance_test.go
- db/pkg/migrations/20240609234258_new_indecies.go
- openapi/spec.yaml
bcavnvidia
commented
Apr 10, 2026
| // below the current instance count, then the delete must be blocked. | ||
| if iCount > totalConstraintValue-deletedConstraintValue { | ||
| logger.Warn().Msg("unable to delete Allocation, existing instances would exceed the remaining pool capacity") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, fmt.Sprintf("%v existing Instances in the tenant/site/instance type pool would exceed the remaining capacity after deleting this Allocation", iCount), nil) |
Contributor
Author
There was a problem hiding this comment.
Probably should be something like
"%d existing Instances would exceed the total allocation capacity after deleting this Allocation"
Contributor
There was a problem hiding this comment.
Better as:
fmt.Sprintf("Deleting this Allocation as specified would result in %d total Machines for Instance Type: %s allocated to Tenant, less than Tenant's active Instance count: %d for the Instance Type", totalConstraintValue-deletedConstraintValue, instanceType.Name, iCount)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
db/pkg/migrations/20260409143000_remove_instance_allocation_relationship.go (1)
28-65: Add a migration-specific upgrade test.
TestMigrationsonly proves the chain applies on a clean database. It does not verify the real rollout path for an existinginstancetable that still hasallocation_id,allocation_constraint_id, and the legacy FK/index state. A focused test indb/pkg/migrations/migrations_test.gowould cover the upgrade scenario this migration actually needs to handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/migrations/20260409143000_remove_instance_allocation_relationship.go` around lines 28 - 65, Add a targeted upgrade test in db/pkg/migrations/migrations_test.go that exercises the up migration registered via Migrations.MustRegister in 20260409143000_remove_instance_allocation_relationship.go: create a pre-upgrade schema and data that includes instance.allocation_id, instance.allocation_constraint_id, the legacy foreign keys (instance_allocation_id_fkey, instance_allocation_constraint_id_fkey) and the index instance_allocation_id_idx, then run the migration's up function (the first callback passed to MustRegister) against that DB and assert the post-up state: allocation_id and allocation_constraint_id columns removed, legacy FKs and index gone, and no unexpected errors; clean up transactionally and keep the test deterministic and isolated from TestMigrations (use a real DB fixture or an in-memory test DB as used elsewhere in migrations_test.go).api/pkg/api/handler/instancebatch.go (1)
936-940: Use a count-only page input for the quota check.This branch only consumes
insTotal, butcdbp.PageInput{}still asksGetAllto hydrate a page ofInstancerows while the quota lock is held. On large tenant pools that adds avoidable DB and allocation overhead. Please switch this call to the count-only pattern used elsewhere in the handlers, e.g.cdbp.PageInput{Limit: cdb.GetIntPtr(0)}(or a dedicated count helper if one exists).Based on learnings:
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 936 - 940, The call to inDAO.GetAll is only using the returned count (insTotal) but currently passes cdbp.PageInput{} which requests full row hydration while the quota lock is held; change the PageInput to a count-only request (e.g. use cdbp.PageInput{Limit: cdb.GetIntPtr(0)} or the project’s dedicated count helper) when calling inDAO.GetAll with the same InstanceFilterInput (TenantIDs: []uuid.UUID{tenant.ID}, InstanceTypeIDs: []uuid.UUID{instancetype.ID}, SiteIDs: siteIDs) so the DB performs a count-only query and avoids loading Instance rows under the lock.
🤖 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/instance_test.go`:
- Around line 8299-8303: The fixtures instVpcNotReady and instSiteNotReady are
created under tenant tn1 but the delete-readiness cases execute as tnOrg2/tnu2,
causing ownership/auth checks to block readiness branches; fix by creating the
fixtures under the same tenant/org used by the request or by running the failing
cases as the tenant that owns those fixtures: either call
testInstanceBuildInstance with tnOrg2/tnu2 (create a matching tenant if missing)
so instVpcNotReady and instSiteNotReady belong to the requesting org, or switch
the delete test cases to run as tnOrg1/tnu1 so they operate on tn1-owned
instances (update references to tn1, tnOrg2, tnu2, or tnOrg1/tnu1 and the
testInstanceBuildInstance calls accordingly).
In `@api/pkg/api/handler/instance.go`:
- Around line 943-947: The quota check is counting all instances because the
InstanceFilterInput passed to instanceDAO.GetAll (call that returns insTotal)
has no Statuses set; update the InstanceFilterInput in the calls around
instanceDAO.GetAll to include only active statuses (e.g., Ready, Provisioning,
Configuring, Updating) via the Statuses field so insTotal reflects only active
instances for quota enforcement; ensure the same change is applied to the
related calls in the same block (the other GetAll usages referenced) so quota
logic and the handler comment about "active instances" match the implementation.
In `@api/pkg/api/handler/nvlinklogicalpartition_test.go`:
- Around line 864-868: The test fixture inserts inst2 under the wrong tenant
(uses tn1.ID) while vpc2 and site2 belong to tn2, causing an invalid
cross-tenant setup; update the testInstanceBuildInstance call that creates inst2
in TestNVLinkLogicalPartitionHandler_GetAll to use tn2.ID (and ensure site2 is
built under tn2) so inst2, site2 and vpc2 share the same tenant; locate the
inst2 creation using the symbol testInstanceBuildInstance and adjust the tenant
argument to tn2.ID to keep the fixture consistent.
- Around line 809-813: The fixture creates mc2 on site2 but then attaches it to
ist1 (a site1-scoped instance type); create a site2-scoped instance type (e.g.,
ist2) before wiring mc2 to an instance type and use that when calling
testInstanceBuildMachineInstanceType so mcinst2 is built against site2; update
the two occurrences (around the mc2/mcinst2 block and the similar block at lines
1275-1279) to call the instance-type builder for site2 and pass that new
instance type instead of ist1.
In `@api/pkg/api/handler/util/common/common.go`:
- Around line 306-310: AcquireInstanceTypeQuotaLock must validate the
transaction precondition like other helpers (e.g.,
GetUnallocatedMachineForInstanceType): check if the tx parameter is nil at the
start of AcquireInstanceTypeQuotaLock and return ErrInvalidFunctionParams when
it is nil; otherwise proceed to build lockID and call tx.TryAcquireAdvisoryLock
as before.
In `@api/pkg/api/handler/util/common/testing.go`:
- Around line 510-512: The TestBuildInstance helper currently declares two
ignored UUID parameters ("_ uuid.UUID, _ uuid.UUID") in its signature; remove
these two parameters from the TestBuildInstance function signature so it matches
the shorter helper in db/pkg/db/model/testing.go, and update all call sites to
stop passing those obsolete allocation ID arguments; ensure the function body
and any usages reference the remaining parameters (e.g., tnID, ipID, stID, itID,
vpcID, mID, osID) and run tests to verify no remaining callers expect the
removed arguments.
---
Nitpick comments:
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 936-940: The call to inDAO.GetAll is only using the returned count
(insTotal) but currently passes cdbp.PageInput{} which requests full row
hydration while the quota lock is held; change the PageInput to a count-only
request (e.g. use cdbp.PageInput{Limit: cdb.GetIntPtr(0)} or the project’s
dedicated count helper) when calling inDAO.GetAll with the same
InstanceFilterInput (TenantIDs: []uuid.UUID{tenant.ID}, InstanceTypeIDs:
[]uuid.UUID{instancetype.ID}, SiteIDs: siteIDs) so the DB performs a count-only
query and avoids loading Instance rows under the lock.
In `@db/pkg/migrations/20260409143000_remove_instance_allocation_relationship.go`:
- Around line 28-65: Add a targeted upgrade test in
db/pkg/migrations/migrations_test.go that exercises the up migration registered
via Migrations.MustRegister in
20260409143000_remove_instance_allocation_relationship.go: create a pre-upgrade
schema and data that includes instance.allocation_id,
instance.allocation_constraint_id, the legacy foreign keys
(instance_allocation_id_fkey, instance_allocation_constraint_id_fkey) and the
index instance_allocation_id_idx, then run the migration's up function (the
first callback passed to MustRegister) against that DB and assert the post-up
state: allocation_id and allocation_constraint_id columns removed, legacy FKs
and index gone, and no unexpected errors; clean up transactionally and keep the
test deterministic and isolated from TestMigrations (use a real DB fixture or an
in-memory test DB as used elsewhere in migrations_test.go).
🪄 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: Pro
Run ID: 12eb77d3-c382-4461-992e-a1548cf9ed52
📒 Files selected for processing (39)
api/pkg/api/handler/allocation.goapi/pkg/api/handler/allocation_test.goapi/pkg/api/handler/allocationconstraint.goapi/pkg/api/handler/allocationconstraint_test.goapi/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/interface_test.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/networksecuritygroup_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/handler/nvlinklogicalpartition_test.goapi/pkg/api/handler/operatingsystem_test.goapi/pkg/api/handler/sshkeygroup_test.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/tenant_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/allocation.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.godb/pkg/db/model/dpuextensionservicedeployment_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/interface_test.godb/pkg/db/model/nvlinkinterface_test.godb/pkg/db/model/sshkeygroupinstanceassociation_test.godb/pkg/db/model/testing.godb/pkg/migrations/20240609234258_new_indecies.godb/pkg/migrations/20260409143000_remove_instance_allocation_relationship.godb/pkg/migrations/migrations_test.godocs/index.htmlopenapi/spec.yaml
💤 Files with no reviewable changes (5)
- api/pkg/api/model/instance_test.go
- api/pkg/api/model/allocation.go
- openapi/spec.yaml
- db/pkg/migrations/20240609234258_new_indecies.go
- api/pkg/api/model/instance.go
✅ Files skipped from review due to trivial changes (10)
- api/pkg/api/handler/infinibandinterface_test.go
- api/pkg/api/handler/subnet_test.go
- db/pkg/db/model/sshkeygroupinstanceassociation_test.go
- db/pkg/db/model/interface_test.go
- api/pkg/api/handler/allocationconstraint_test.go
- api/pkg/api/handler/machine_test.go
- api/pkg/api/handler/tenant_test.go
- api/pkg/api/handler/vpc_test.go
- db/pkg/db/model/infinibandinterface_test.go
- api/pkg/api/handler/allocationconstraint.go
🚧 Files skipped from review as they are similar to previous changes (11)
- api/pkg/api/handler/nvlinkinterface_test.go
- api/pkg/api/handler/operatingsystem_test.go
- db/pkg/db/model/dpuextensionservicedeployment_test.go
- api/pkg/api/handler/networksecuritygroup_test.go
- db/pkg/db/model/nvlinkinterface_test.go
- db/pkg/db/model/testing.go
- api/pkg/api/handler/interface_test.go
- api/pkg/api/handler/stats_test.go
- api/pkg/api/handler/allocation_test.go
- api/pkg/api/handler/instancetype_test.go
- db/pkg/db/model/instance_test.go
bcavnvidia
force-pushed
the
inst-alloc
branch
2 times, most recently
from
08752aa to
54bc434
Compare
🔐 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-04-10 18:08:15 UTC | Commit: 54bc434 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
api/pkg/api/handler/dpuextensionservice_test.go (1)
940-950: Drop the leftover allocation fixtures from these tests.
common.TestBuildInstanceno longer consumes allocation IDs, and these handler paths only need an instance plus deployment state. Keeping the allocation/allocation-constraint setup here adds dead fixture work and still suggests a deprecated dependency.Also applies to: 1325-1335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/dpuextensionservice_test.go` around lines 940 - 950, Remove the unused allocation fixtures from the test: delete calls to common.TestBuildAllocation and common.TestBuildAllocationConstraint (and any associated allocation variables like al1) in the setup for these handler tests since common.TestBuildInstance no longer consumes allocation IDs; retain only what's required (instance i1, machine m1, instance type it1 if used, VPC vpc1, OS os1, and deployment state st1) and remove any dead variables/assignments related to allocation setup (also apply the same removal to the other occurrence around the 1325-1335 range).api/pkg/api/handler/allocation.go (1)
1288-1308: Hoist the tenant/site allocation lookup out of the constraint loop.
GetAllocationIDsForTenantAtSite(...)is invariant for this delete request, so doing it once perInstanceTypeconstraint repeats the same DB query while the transaction is holding advisory locks. Computing it once before the loop will shorten the locked section and make this branch easier to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/allocation.go` around lines 1288 - 1308, The allocation ID lookup GetAllocationIDsForTenantAtSite(...) is invariant per delete request but is currently invoked inside the per-InstanceType constraint loop; move the call to GetAllocationIDsForTenantAtSite(...) so it runs once before iterating constraints and store its result in allocationIDs, then reuse allocationIDs when calling GetTotalAllocationConstraintValueForInstanceType(...) for each ac.ResourceTypeID and when computing deletedConstraintValue for []uuid.UUID{a.ID}; update error handling accordingly so the single pre-loop call returns the same API errors as before.api/pkg/api/handler/instancebatch.go (2)
936-940: UseInstanceDAO.GetCountfor the quota admission path.This branch only needs the cardinality.
GetAllstill fetches a page ofInstancerows before returninginsTotal, so every batch admission does unnecessary row materialization on the hot path.Suggested cleanup
- _, insTotal, err := inDAO.GetAll(ctx, tx, cdbm.InstanceFilterInput{ + insTotal, err := inDAO.GetCount(ctx, tx, cdbm.InstanceFilterInput{ TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs: []uuid.UUID{instancetype.ID}, - }, cdbp.PageInput{}, nil) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 936 - 940, The code currently calls inDAO.GetAll(...) to obtain insTotal which materializes rows; replace this with the lighter inDAO.GetCount(ctx, tx, cdbm.InstanceFilterInput{TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs: []uuid.UUID{instancetype.ID}}) and use the returned count for the quota admission path, remove the unused page/results variables and adjust error handling to check the single returned error from GetCount; update any references from insTotal to the new count variable and eliminate unnecessary allocations from the GetAll call.
900-905: Add regression coverage for the new aggregate quota admission model.This block now enforces quota at the tenant/site/instance-type pool level under a shared lock. That is the core behavior change in the PR, but there is no accompanying coverage here for multi-allocation summing, boundary rejection (
current + requested > max), or concurrent create vs. allocation-constraint mutation/delete.Also applies to: 946-956
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 900 - 905, Add regression tests that verify the new aggregate quota admission model enforced by AcquireInstanceTypeQuotaLock: create tests that (1) simulate multiple allocations in a single request and assert summing of requested units against the pool limit (ensure a request is rejected when current + requested > max), (2) create two concurrent create requests for the same tenant/site/instance-type and assert the lock prevents over-allocation (one should succeed, the other should be rejected), and (3) exercise concurrent create vs. quota-mutating operations (e.g., allocation-constraint update or delete) to ensure the advisory lock around AcquireInstanceTypeQuotaLock prevents races; locate test targets around the instance batch handler logic that calls AcquireInstanceTypeQuotaLock and assert the handler returns the expected API error response (the same shape produced by cutil.NewAPIErrorResponse) on boundary rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/pkg/db/model/instance_test.go`:
- Around line 548-551: The test case "GetById with supported relations" includes
NetworkSecurityGroupRelationName in paramRelations but does not assert the
relation was loaded; update the test to assert that got.NetworkSecurityGroup is
non-nil and equals the expected fixture (e.g., i1.NetworkSecurityGroup or the
test's expectedNetworkSecurityGroup variable) using the same assertion helpers
used elsewhere in instance_test.go so a failing preload will be caught.
In `@db/pkg/db/model/instance.go`:
- Around line 360-363: Update the DAO comments for GetByID and GetAll to reflect
current API surface: remove stale references to "Subnet" from GetByID's
includeRelation list and rewrite the GetAll comment to enumerate the actual
fields present on InstanceFilterInput (and note that allocation-filter was
removed); ensure comments mention any intentionally omitted relations/filters
(e.g., allocation) so readers know why they are absent and reference the
functions GetByID and GetAll and the InstanceFilterInput type so reviewers can
find and verify the updated docs.
---
Nitpick comments:
In `@api/pkg/api/handler/allocation.go`:
- Around line 1288-1308: The allocation ID lookup
GetAllocationIDsForTenantAtSite(...) is invariant per delete request but is
currently invoked inside the per-InstanceType constraint loop; move the call to
GetAllocationIDsForTenantAtSite(...) so it runs once before iterating
constraints and store its result in allocationIDs, then reuse allocationIDs when
calling GetTotalAllocationConstraintValueForInstanceType(...) for each
ac.ResourceTypeID and when computing deletedConstraintValue for
[]uuid.UUID{a.ID}; update error handling accordingly so the single pre-loop call
returns the same API errors as before.
In `@api/pkg/api/handler/dpuextensionservice_test.go`:
- Around line 940-950: Remove the unused allocation fixtures from the test:
delete calls to common.TestBuildAllocation and
common.TestBuildAllocationConstraint (and any associated allocation variables
like al1) in the setup for these handler tests since common.TestBuildInstance no
longer consumes allocation IDs; retain only what's required (instance i1,
machine m1, instance type it1 if used, VPC vpc1, OS os1, and deployment state
st1) and remove any dead variables/assignments related to allocation setup (also
apply the same removal to the other occurrence around the 1325-1335 range).
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 936-940: The code currently calls inDAO.GetAll(...) to obtain
insTotal which materializes rows; replace this with the lighter
inDAO.GetCount(ctx, tx, cdbm.InstanceFilterInput{TenantIDs:
[]uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs:
[]uuid.UUID{instancetype.ID}}) and use the returned count for the quota
admission path, remove the unused page/results variables and adjust error
handling to check the single returned error from GetCount; update any references
from insTotal to the new count variable and eliminate unnecessary allocations
from the GetAll call.
- Around line 900-905: Add regression tests that verify the new aggregate quota
admission model enforced by AcquireInstanceTypeQuotaLock: create tests that (1)
simulate multiple allocations in a single request and assert summing of
requested units against the pool limit (ensure a request is rejected when
current + requested > max), (2) create two concurrent create requests for the
same tenant/site/instance-type and assert the lock prevents over-allocation (one
should succeed, the other should be rejected), and (3) exercise concurrent
create vs. quota-mutating operations (e.g., allocation-constraint update or
delete) to ensure the advisory lock around AcquireInstanceTypeQuotaLock prevents
races; locate test targets around the instance batch handler logic that calls
AcquireInstanceTypeQuotaLock and assert the handler returns the expected API
error response (the same shape produced by cutil.NewAPIErrorResponse) on
boundary rejections.
🪄 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: Pro
Run ID: 4f8772e9-fa84-4314-abbf-46352549c745
📒 Files selected for processing (43)
api/pkg/api/handler/allocation.goapi/pkg/api/handler/allocation_test.goapi/pkg/api/handler/allocationconstraint.goapi/pkg/api/handler/allocationconstraint_test.goapi/pkg/api/handler/dpuextensionservice_test.goapi/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/interface_test.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machineinstancetype_test.goapi/pkg/api/handler/networksecuritygroup_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/handler/nvlinklogicalpartition_test.goapi/pkg/api/handler/operatingsystem_test.goapi/pkg/api/handler/sshkeygroup_test.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/tenant_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/handler/vpcprefix_test.goapi/pkg/api/model/allocation.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.godb/pkg/db/model/dpuextensionservicedeployment_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/interface_test.godb/pkg/db/model/nvlinkinterface_test.godb/pkg/db/model/sshkeygroupinstanceassociation_test.godb/pkg/db/model/testing.godb/pkg/migrations/20240609234258_new_indecies.godb/pkg/migrations/20250605002925_instance_allocation_nullable.godb/pkg/migrations/20260409143000_remove_instance_allocation_relationship.godb/pkg/migrations/migrations_test.godocs/index.htmlopenapi/spec.yaml
💤 Files with no reviewable changes (5)
- api/pkg/api/model/instance.go
- api/pkg/api/model/instance_test.go
- db/pkg/migrations/20240609234258_new_indecies.go
- api/pkg/api/model/allocation.go
- openapi/spec.yaml
✅ Files skipped from review due to trivial changes (6)
- db/pkg/db/model/infinibandinterface_test.go
- db/pkg/db/model/interface_test.go
- db/pkg/db/model/sshkeygroupinstanceassociation_test.go
- api/pkg/api/handler/machine_test.go
- api/pkg/api/handler/util/common/testing.go
- api/pkg/api/handler/allocation_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- api/pkg/api/handler/nvlinkinterface_test.go
- api/pkg/api/handler/operatingsystem_test.go
- api/pkg/api/handler/infinibandinterface_test.go
- api/pkg/api/handler/sshkeygroup_test.go
- api/pkg/api/handler/subnet_test.go
- db/pkg/migrations/migrations_test.go
- api/pkg/api/handler/tenant_test.go
- api/pkg/api/handler/util/common/common.go
- api/pkg/api/handler/networksecuritygroup_test.go
- api/pkg/api/handler/vpc_test.go
- api/pkg/api/handler/util/common/common_test.go
- db/pkg/db/model/testing.go
- db/pkg/migrations/20260409143000_remove_instance_allocation_relationship.go
- api/pkg/api/handler/stats_test.go
- api/pkg/api/handler/allocationconstraint.go
- api/pkg/api/handler/instance_test.go
- api/pkg/api/handler/instance.go
- api/pkg/api/handler/nvlinklogicalpartition_test.go
- api/pkg/api/handler/instancetype_test.go
bcavnvidia
commented
Apr 10, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
db/pkg/db/model/instance.go (1)
571-575:⚠️ Potential issue | 🟡 MinorRefresh this filter list to match
InstanceFilterInput.The comment still mentions
IDsNotIn,TenantOrgName,Labels, andHostnames, but those fields are not part of the current filter type anymore. That leaves stale guidance on a public DAO entry point.As per coding guidelines: "Document when you have intentionally omitted code that the reader might otherwise expect to be present".📝 Proposed comment fix
// GetAll returns all Instances filtered by the fields in InstanceFilterInput: -// InstanceIDs, Names, TenantIDs, InfrastructureProviderIDs, SiteIDs, InstanceTypeIDs, -// VpcIDs, MachineIDs, ControllerInstanceIDs, OperatingSystemIDs, IDsNotIn, SearchQuery, -// Statuses, TenantOrgName, Labels, NetworkSecurityGroupIDs, and Hostnames. +// InstanceIDs, Names, TenantIDs, InfrastructureProviderIDs, SiteIDs, InstanceTypeIDs, +// NetworkSecurityGroupIDs, VpcIDs, MachineIDs, ControllerInstanceIDs, +// OperatingSystemIDs, Statuses, and SearchQuery. // Allocation-based filters are intentionally omitted because direct instance-allocation linkage was removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/instance.go` around lines 571 - 575, The comment on GetAll (function GetAll) listing supported filters is stale: update the docblock to exactly match the current fields on InstanceFilterInput (remove IDsNotIn, TenantOrgName, Labels, Hostnames and any others no longer present) and ensure you call out any intentional omissions (e.g., allocation-based filters removed) per the coding guidelines; locate the comment above GetAll in instance.go and replace the filter list with the canonical fields from InstanceFilterInput and add a brief note that other expected fields were intentionally omitted.
🧹 Nitpick comments (4)
api/pkg/api/handler/instancebatch.go (1)
936-940: UseGetCounthere instead ofGetAll.This path only needs the total, but
GetAllstill builds and executes a paginated row query while the transaction and quota lock are held. Counting directly will shorten the locked section and avoid unnecessary row materialization.♻️ Proposed change
- _, insTotal, err := inDAO.GetAll(ctx, tx, cdbm.InstanceFilterInput{ - TenantIDs: []uuid.UUID{tenant.ID}, - SiteIDs: siteIDs, - InstanceTypeIDs: []uuid.UUID{instancetype.ID}, - }, cdbp.PageInput{}, nil) + insTotal, err := inDAO.GetCount(ctx, tx, cdbm.InstanceFilterInput{ + TenantIDs: []uuid.UUID{tenant.ID}, + SiteIDs: siteIDs, + InstanceTypeIDs: []uuid.UUID{instancetype.ID}, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 936 - 940, The code currently calls inDAO.GetAll(...) to obtain a total count while holding the transaction and quota lock, which materializes rows unnecessarily; replace that call with inDAO.GetCount(ctx, tx, cdbm.InstanceFilterInput{TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs: []uuid.UUID{instancetype.ID}}) and use its returned integer/count and error (instead of insTotal from GetAll), removing the extra cdbp.PageInput{} and trailing nil arguments so the code only runs a COUNT query and shortens the locked section.api/pkg/api/handler/allocation.go (1)
1288-1294: Hoist tenant/site allocation-ID lookup outside the loop.
GetAllocationIDsForTenantAtSiteis invariant for this request, but it is re-run per instance-type constraint. Precomputing once reduces avoidable DB calls.♻️ Proposed refactor
ipamStorage := ipam.NewIpamStorage(dah.dbSession.DB, tx.GetBunTx()) + allocationIDs, err := common.GetAllocationIDsForTenantAtSite(ctx, tx, dah.dbSession, a.InfrastructureProviderID, a.TenantID, a.SiteID) + if err != nil { + logger.Error().Err(err).Msg("error getting Allocation IDs for Tenant at Site") + return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Error getting Allocation IDs for Tenant at Site", nil) + } for _, ac := range acs { switch ac.ResourceType { case cdbm.AllocationResourceTypeInstanceType: @@ - allocationIDs, serr := common.GetAllocationIDsForTenantAtSite(ctx, tx, dah.dbSession, a.InfrastructureProviderID, a.TenantID, a.SiteID) - if serr != nil { - logger.Error().Err(serr).Msg("error getting Allocation IDs for Tenant at Site") - return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Error getting Allocation IDs for Tenant at Site", nil) - } - // Calculate the tenant/site aggregate capacity for the instance type. totalConstraintValue, serr := common.GetTotalAllocationConstraintValueForInstanceType( ctx, tx, dah.dbSession, allocationIDs, &ac.ResourceTypeID, cdb.GetStrPtr(cdbm.AllocationConstraintTypeReserved), )As per coding guidelines: "Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/allocation.go` around lines 1288 - 1294, GetAllocationIDsForTenantAtSite is invariant per request but is called inside the per-instance-type constraint loop; hoist the call so allocationIDs (and serr) are computed once before the loop that processes instance-type constraints, preserve the existing error handling (logger.Error/return NewAPIErrorResponse) if the call fails, and then reuse allocationIDs inside the loop instead of re-invoking common.GetAllocationIDsForTenantAtSite repeatedly.api/pkg/api/handler/instance.go (2)
953-965: Please add regression coverage for the new aggregate quota boundary.This block is now the admission-control check for instance creation, but the PR description indicates no test coverage. At minimum, add cases for split constraints across multiple allocations, exact-boundary rejection, and one-below-boundary acceptance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instance.go` around lines 953 - 965, Add regression tests that exercise the new aggregate quota boundary logic in instance creation by targeting the admission-control path that uses insTotal, alconstraints and totalConstraintValue in the instance handler; create tests that (1) set up multiple allocations whose ConstraintValue sum spans the boundary (split constraints across allocations), (2) attempt creation when insTotal equals the aggregated limit and assert the handler returns the Forbidden API error, and (3) attempt creation when insTotal is one less than the aggregated limit and assert success. Use the existing instance creation test harness or handler-level test (the instance handler/unit tests) to simulate allocations and active instances so the code path that calls cutil.NewAPIErrorResponse for quota rejection is executed.
943-947: Avoid fetching a full result set for a count-only check.This branch only uses
insTotal, butPageInput{}can still makeGetAllpull more instance rows than necessary. A count-specific DAO path would be ideal; otherwise, cap the page size to a single row here.♻️ Low-impact option
- _, insTotal, err := instanceDAO.GetAll(ctx, tx, cdbm.InstanceFilterInput{ + _, insTotal, err := instanceDAO.GetAll(ctx, tx, cdbm.InstanceFilterInput{ TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs, InstanceTypeIDs: []uuid.UUID{instanceType.ID}, - }, cdbp.PageInput{}, nil) + }, cdbp.PageInput{Limit: cdb.GetIntPtr(1)}, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instance.go` around lines 943 - 947, The current call to instanceDAO.GetAll(ctx, tx, ..., cdbp.PageInput{}, nil) only needs insTotal but may still fetch full rows; change this to a count-only call (e.g. use an existing instanceDAO.Count / GetCount method) to return just the total, or if no count method exists, cap the fetch by passing a PageInput with a page size/limit of 1 (populate the size/limit field on cdbp.PageInput) so GetAll only retrieves a single row while still returning insTotal; update the call site that references instanceDAO.GetAll and insTotal accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@db/pkg/db/model/instance.go`:
- Around line 571-575: The comment on GetAll (function GetAll) listing supported
filters is stale: update the docblock to exactly match the current fields on
InstanceFilterInput (remove IDsNotIn, TenantOrgName, Labels, Hostnames and any
others no longer present) and ensure you call out any intentional omissions
(e.g., allocation-based filters removed) per the coding guidelines; locate the
comment above GetAll in instance.go and replace the filter list with the
canonical fields from InstanceFilterInput and add a brief note that other
expected fields were intentionally omitted.
---
Nitpick comments:
In `@api/pkg/api/handler/allocation.go`:
- Around line 1288-1294: GetAllocationIDsForTenantAtSite is invariant per
request but is called inside the per-instance-type constraint loop; hoist the
call so allocationIDs (and serr) are computed once before the loop that
processes instance-type constraints, preserve the existing error handling
(logger.Error/return NewAPIErrorResponse) if the call fails, and then reuse
allocationIDs inside the loop instead of re-invoking
common.GetAllocationIDsForTenantAtSite repeatedly.
In `@api/pkg/api/handler/instance.go`:
- Around line 953-965: Add regression tests that exercise the new aggregate
quota boundary logic in instance creation by targeting the admission-control
path that uses insTotal, alconstraints and totalConstraintValue in the instance
handler; create tests that (1) set up multiple allocations whose ConstraintValue
sum spans the boundary (split constraints across allocations), (2) attempt
creation when insTotal equals the aggregated limit and assert the handler
returns the Forbidden API error, and (3) attempt creation when insTotal is one
less than the aggregated limit and assert success. Use the existing instance
creation test harness or handler-level test (the instance handler/unit tests) to
simulate allocations and active instances so the code path that calls
cutil.NewAPIErrorResponse for quota rejection is executed.
- Around line 943-947: The current call to instanceDAO.GetAll(ctx, tx, ...,
cdbp.PageInput{}, nil) only needs insTotal but may still fetch full rows; change
this to a count-only call (e.g. use an existing instanceDAO.Count / GetCount
method) to return just the total, or if no count method exists, cap the fetch by
passing a PageInput with a page size/limit of 1 (populate the size/limit field
on cdbp.PageInput) so GetAll only retrieves a single row while still returning
insTotal; update the call site that references instanceDAO.GetAll and insTotal
accordingly.
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 936-940: The code currently calls inDAO.GetAll(...) to obtain a
total count while holding the transaction and quota lock, which materializes
rows unnecessarily; replace that call with inDAO.GetCount(ctx, tx,
cdbm.InstanceFilterInput{TenantIDs: []uuid.UUID{tenant.ID}, SiteIDs: siteIDs,
InstanceTypeIDs: []uuid.UUID{instancetype.ID}}) and use its returned
integer/count and error (instead of insTotal from GetAll), removing the extra
cdbp.PageInput{} and trailing nil arguments so the code only runs a COUNT query
and shortens the locked section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b3c58de-56da-4d66-943a-01688c7f4609
📒 Files selected for processing (43)
api/pkg/api/handler/allocation.goapi/pkg/api/handler/allocation_test.goapi/pkg/api/handler/allocationconstraint.goapi/pkg/api/handler/allocationconstraint_test.goapi/pkg/api/handler/dpuextensionservice_test.goapi/pkg/api/handler/infinibandinterface_test.goapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/interface_test.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machineinstancetype_test.goapi/pkg/api/handler/networksecuritygroup_test.goapi/pkg/api/handler/nvlinkinterface_test.goapi/pkg/api/handler/nvlinklogicalpartition_test.goapi/pkg/api/handler/operatingsystem_test.goapi/pkg/api/handler/sshkeygroup_test.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/tenant_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/handler/vpcprefix_test.goapi/pkg/api/model/allocation.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.godb/pkg/db/model/dpuextensionservicedeployment_test.godb/pkg/db/model/infinibandinterface_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/interface_test.godb/pkg/db/model/nvlinkinterface_test.godb/pkg/db/model/sshkeygroupinstanceassociation_test.godb/pkg/db/model/testing.godb/pkg/migrations/20240609234258_new_indecies.godb/pkg/migrations/20250605002925_instance_allocation_nullable.godb/pkg/migrations/20260409143000_remove_instance_allocation_relationship.godb/pkg/migrations/migrations_test.godocs/index.htmlopenapi/spec.yaml
💤 Files with no reviewable changes (5)
- db/pkg/migrations/20240609234258_new_indecies.go
- api/pkg/api/model/instance.go
- api/pkg/api/model/instance_test.go
- api/pkg/api/model/allocation.go
- openapi/spec.yaml
✅ Files skipped from review due to trivial changes (7)
- api/pkg/api/handler/machineinstancetype_test.go
- db/pkg/db/model/sshkeygroupinstanceassociation_test.go
- api/pkg/api/handler/dpuextensionservice_test.go
- db/pkg/db/model/nvlinkinterface_test.go
- db/pkg/db/model/infinibandinterface_test.go
- api/pkg/api/handler/vpcprefix_test.go
- api/pkg/api/handler/subnet_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- api/pkg/api/handler/operatingsystem_test.go
- db/pkg/migrations/migrations_test.go
- db/pkg/migrations/20250605002925_instance_allocation_nullable.go
- api/pkg/api/handler/instancetype_test.go
- api/pkg/api/handler/util/common/common.go
- api/pkg/api/handler/machine_test.go
- api/pkg/api/handler/vpc_test.go
- api/pkg/api/handler/nvlinkinterface_test.go
- api/pkg/api/handler/networksecuritygroup_test.go
- api/pkg/api/handler/tenant_test.go
- api/pkg/api/handler/interface_test.go
- api/pkg/api/handler/allocationconstraint_test.go
- db/pkg/db/model/testing.go
- api/pkg/api/handler/nvlinklogicalpartition_test.go
- db/pkg/db/model/interface_test.go
- api/pkg/api/handler/stats_test.go
- api/pkg/api/handler/util/common/testing.go
- api/pkg/api/handler/allocationconstraint.go
- db/pkg/migrations/20260409143000_remove_instance_allocation_relationship.go
- api/pkg/api/handler/instance_test.go
- api/pkg/api/handler/allocation_test.go
- db/pkg/db/model/instance_test.go
thossain-nv
reviewed
Apr 17, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for this @bcavnvidia A few suggestions.
| @@ -0,0 +1,66 @@ | |||
| /* | |||
Contributor
There was a problem hiding this comment.
I'd defer the actual column removal in a subsequent PR. We can first remove the DB attributes and their usage everywhere. Then the migration can be run while previous version of API pods are still serving Instance creation.
| // below the current instance count, then the delete must be blocked. | ||
| if iCount > totalConstraintValue-deletedConstraintValue { | ||
| logger.Warn().Msg("unable to delete Allocation, existing instances would exceed the remaining pool capacity") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, fmt.Sprintf("%v existing Instances in the tenant/site/instance type pool would exceed the remaining capacity after deleting this Allocation", iCount), nil) |
Contributor
There was a problem hiding this comment.
Better as:
fmt.Sprintf("Deleting this Allocation as specified would result in %d total Machines for Instance Type: %s allocated to Tenant, less than Tenant's active Instance count: %d for the Instance Type", totalConstraintValue-deletedConstraintValue, instanceType.Name, iCount)| serr = tx.TryAcquireAdvisoryLock(ctx, cdb.GetAdvisoryLockIDFromString(dbit.ID.String()), nil) | ||
| // Acquire the shared quota lock for this tenant/site/instance-type pool. | ||
| // This lock is released when the transaction commits or rolls back. | ||
| serr = common.AcquireInstanceTypeQuotaLock(ctx, tx, a.TenantID, dbit.ID) |
Contributor
There was a problem hiding this comment.
The reason this was an Instance Type wide lock was to prevent simultaneously over allocating the same Instance Type to 2 or more Tenants. The Tenant/Instance Type ID lock circumvents that, no?
| // pool does not fall below what has already been allocated. | ||
| if iCount > sumConstraints { | ||
| logger.Warn().Msg("current number of Instances exceed Constraint value, failed to update Allocation Constraint") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Cannot update AllocationConstraint, more Instances exist than requested constraint for this Allocation", nil) |
Contributor
There was a problem hiding this comment.
Similar to informative message in a previous comment:
fmt.Sprintf("Updating this Allocation as specified would result in %d total Machines for Instance Type: %s allocated to Tenant, less than Tenant's active Instance count: %d for the Instance Type", sumConstraints, instanceType.Name, iCount)
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.
Description
This change removes the legacy per-instance allocation linkage from the REST/API and database layers and finishes the transition to enforcement based on aggregate allocation.
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes