feat(rest-api): Recovery of missing VPCs from Site inventory - #4460
feat(rest-api): Recovery of missing VPCs from Site inventory#4460hwadekar-nv wants to merge 3 commits into
Conversation
hwadekar-nv
added
the
rest-api
|
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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Summary by CodeRabbit
WalkthroughChangesVPC inventory recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SiteInventory
participant ManageVpc
participant VpcSQLDAO
participant Database
SiteInventory->>ManageVpc: Submit VPC inventory
ManageVpc->>VpcSQLDAO: Load active and deleted VPCs
ManageVpc->>Database: Validate ownership, allocations, and resources
ManageVpc->>VpcSQLDAO: Restore newer tombstone or create VPC
VpcSQLDAO->>Database: Persist VPC and Ready status detail
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
rest-api/workflow/pkg/activity/vpc/vpc_test.go (2)
443-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the unmatched inventory entries do not trigger auto-creation.
The entries at lines 443-449 carry random controller UUIDs, so they match no existing record and now enter the recovery path. Auto-creation is suppressed only because this fixture never calls
testVPCSiteBuildAllocation, so recovery skips with "tenant does not have an active Allocation for the Site".The expectations therefore depend on the absence of an Allocation, which is invisible at the assertion site. If a future change adds an Allocation to this fixture, two VPCs are auto-created and several expectations break for reasons that are hard to trace.
Add a comment stating the dependency, or assert the resulting VPC count explicitly so the intent is enforced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/workflow/pkg/activity/vpc/vpc_test.go` around lines 443 - 449, Clarify the fixture’s dependency on having no active Allocation for the Site around the unmatched inventory entries using a concise comment, or explicitly assert the resulting VPC count to enforce that auto-creation remains suppressed. Anchor the change near the entries using random controller UUIDs and preserve the existing expectations.
860-892: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the table to cover the resource-ownership skip branches.
The table covers three skip reasons. The security-relevant branches remain untested:
- "referenced Network Security Group belongs to a different Tenant or Site"
- "referenced NVLink Logical Partition belongs to a different Tenant or Site"
- "referenced NVLink Logical Partition is not Ready"
- "an active VPC with the same name already exists for the Tenant and Site"
- "tenant organization resolves to multiple REST Tenants"
These branches prevent inventory from attaching a resource owned by another tenant, which is the strongest justification for the validation code. The table structure already in place makes each addition a small fixture plus one row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/workflow/pkg/activity/vpc/vpc_test.go` around lines 860 - 892, Extend the existing table-driven test around the current skip-reason cases to add fixtures and rows for all five untested branches: cross-tenant/site Network Security Group ownership, cross-tenant/site NVLink Logical Partition ownership, a non-Ready NVLink Logical Partition, an existing active same-name VPC, and an organization resolving to multiple REST Tenants. Reuse the test’s established setup and assert each branch’s exact skip reason, preserving the current table structure.rest-api/workflow/pkg/activity/vpc/vpc.go (2)
79-86: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the soft-deleted rows loaded on this inventory path.
This query now loads every VPC row for the Site, including soft-deleted rows, and it runs on every inventory page. Soft-deleted rows accumulate indefinitely unless a purge job removes them. Over time the tombstone set can exceed the active set, and the activity pays that cost repeatedly for a paginated inventory.
Two options reduce the exposure:
- Restrict the deleted rows to a recovery window, for example rows deleted within a recent retention period, because older tombstones can never satisfy the "inventory is newer than the delete marker" check anyway.
- Build the indexes once for the whole paginated inventory run instead of once per page, if the activity boundary allows it.
Neither is required for correctness. Both protect the hot path as tombstones grow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 79 - 86, Bound the soft-deleted VPC rows loaded by the GetAll call in the inventory path to a recent recovery/retention window, using the existing VpcFilterInput and established retention configuration or helper. Preserve active-row loading and the current pagination behavior; do not alter correctness for recently deleted records.
386-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDecompose
recoverVpcFromInventoryinto named validation and write steps.This function spans roughly 340 lines and mixes six distinct responsibilities: proto validation, tenant resolution, allocation checks, dependency validation, name-conflict detection, and the locked write transaction. Every branch also repeats the verbose
util.InventoryRecoveryResult[*cdbm.Vpc]{Action: ..., Reason: ...}literal, which obscures the control flow.Consider introducing a small
vpcRecoverystruct that carriessite,inventory,controllerVpc,existingVpc, and the resolvedtenant, then splitting the body into receiver methods such asvalidateProto,resolveTenant,validateDependencies,checkNameConflict, andcommit. A localskip(reason string)helper would also remove the repeated generic literal.One detail deserves attention during that refactor. Lines 407-413 infer an invalid NVLink partition UUID by observing that the proto value is non-empty while
FromProtoproduced nil. That couples this function to the internal error handling ofFromProto. Parsing the UUID directly here would express the intent without the indirection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 386 - 425, Refactor recoverVpcFromInventory into named validation, tenant-resolution, dependency, conflict-check, and commit steps, using a vpcRecovery receiver that carries the shared inputs and resolved tenant. Add a local skip(reason string) helper to replace repeated InventoryRecoveryResult literals while preserving outcomes. In validateProto, parse controllerVpc.GetDefaultNvlinkLogicalPartitionId().GetValue() directly and skip when a non-empty value is not a valid UUID, instead of inferring invalidity from reportedVpc.NVLinkLogicalPartitionID after FromProto.Source: Path instructions
rest-api/db/pkg/db/model/vpc.go (2)
320-331: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument that
VpcRestoreInputuses reset semantics, not PATCH semantics.Every optional field in
VpcRestoreInputis a pointer, which matches the shape ofVpcUpdateInput. The semantics differ.VpcUpdateInputtreats a nil pointer as "leave the column unchanged".Restorewrites every column in its fixed column set, so a nil pointer here clears the column. Callers can easily assume the wrong contract.State the reset contract in the doc comment so callers pass complete values.
📝 Proposed documentation change
// VpcRestoreInput contains the Site-owned fields refreshed while restoring a // soft-deleted VPC that is still reported by Site inventory. +// +// Unlike VpcUpdateInput, every field is written unconditionally: a nil pointer +// clears the corresponding column rather than preserving its previous value. +// Callers must supply the complete desired post-restore state. type VpcRestoreInput struct {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/vpc.go` around lines 320 - 331, Update the doc comment for VpcRestoreInput to explicitly state that restore uses reset semantics: every field in its fixed column set is written, and nil optional pointers clear the corresponding column rather than leaving it unchanged. Clarify that callers must provide complete values, and distinguish this contract from VpcUpdateInput’s PATCH semantics.
247-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicate
uint32-to-intpointer helpers.cutil.Uint32PtrToIntPtrandutil.GetUint32PtrToIntPtrhave identical behavior, and workflow code already imports the common utility package. The documented VNI range is safe forint, including on 32-bit targets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/vpc.go` around lines 247 - 257, The VNI conversion in the VPC model currently uses the duplicate cutil helper; replace cutil.Uint32PtrToIntPtr calls in the VPC mapping logic with the shared util.GetUint32PtrToIntPtr helper, update imports as needed, and remove the redundant helper if it is only used here.
🤖 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 `@rest-api/db/pkg/db/model/vpc_test.go`:
- Around line 1366-1448: The TestVpcSQLDAO_Restore test should be reorganized
into named table-driven t.Run subtests, following the pattern used by
TestVpcSQLDAO_DeleteByID. Keep the existing successful restore scenario, add a
case using an unknown VpcID that asserts db.ErrDoesNotExist, and add a case
passing nil optional fields that verifies Restore clears the corresponding
persisted columns. Use testify assertions and keep all cases organized under the
single top-level test.
In `@rest-api/workflow/pkg/activity/vpc/vpc_test.go`:
- Around line 704-843: Refactor
TestManageVpc_UpdateVpcsInDB_AutoCreatesAndRestores into named t.Run subtests
for creation, idempotent replay, newer-snapshot restoration, and older-snapshot
rejection, preserving the required phase order and shared setup. In the
restoration subtest, additionally assert ControllerVpcID, Vni, ActiveVni, and
RoutingProfile retain the expected values after restoration, using testify
assertions and the existing DAO results.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go`:
- Around line 641-663: Update rest-api/workflow/pkg/activity/vpc/vpc.go lines
641-663 to resolve RoutingProfile from reportedVpc first and fall back to
deletedVpc.RoutingProfile when nil, then pass that resolved value to
VpcSQLDAO.Restore via VpcRestoreInput. In rest-api/db/pkg/db/model/vpc.go lines
320-331, document on VpcRestoreInput that all fields are written
unconditionally, nil clears the corresponding column, and callers must provide
the complete post-restore state.
- Around line 512-536: Update the NVLink Logical Partition validation around
nvLinkLogicalPartitionID to distinguish restoration from new inventory creation:
always retain the existing existence and Site/Tenant ownership checks, but
enforce NVLinkLogicalPartitionStatusReady only when the reference originates
from inventory. Allow restoration of an unchanged soft-deleted VPC with a
non-Ready partition, while preserving the current skip behavior for new
inventory references.
- Around line 626-639: Update the recovery logic around timestamp and
deletedVpc.Deleted so missing or invalid inventory timestamps return a reason
identifying the payload as malformed, while a valid timestamp not newer than the
soft-delete marker returns a distinct stale-inventory reason. Keep both branches
returning InventoryRecoverySkipped and preserve the existing valid-newer flow.
- Around line 572-577: Update the transaction callback in UpdateVpcsInDB around
TryAcquireAdvisoryLock to detect cdb.ErrXactAdvisoryLockFailed and return
InventoryRecoverySkipped instead of an error. Continue wrapping and propagating
all other lock-acquisition errors as activity failures.
- Around line 542-561: Add authoritative active-VPC name uniqueness by creating
a partial unique index on site_id, tenant_id, and name for non-deleted/active
rows, then update the VPC creation flow around vpcDAO.GetAll to detect that
constraint violation and return InventoryRecoverySkipped with the existing
conflict reason. Ensure concurrent inserts cannot bypass the guard; do not rely
solely on the pre-transaction query or controller VPC ID advisory lock.
- Around line 467-480: Update the allocation filter used by GetCount in the VPC
recovery flow to set Statuses to the active allocation states that authorize
recovery, while retaining the tenant and site filters. Add regression coverage
confirming allocations in Error and Deleting states are excluded from recovery.
---
Nitpick comments:
In `@rest-api/db/pkg/db/model/vpc.go`:
- Around line 320-331: Update the doc comment for VpcRestoreInput to explicitly
state that restore uses reset semantics: every field in its fixed column set is
written, and nil optional pointers clear the corresponding column rather than
leaving it unchanged. Clarify that callers must provide complete values, and
distinguish this contract from VpcUpdateInput’s PATCH semantics.
- Around line 247-257: The VNI conversion in the VPC model currently uses the
duplicate cutil helper; replace cutil.Uint32PtrToIntPtr calls in the VPC mapping
logic with the shared util.GetUint32PtrToIntPtr helper, update imports as
needed, and remove the redundant helper if it is only used here.
In `@rest-api/workflow/pkg/activity/vpc/vpc_test.go`:
- Around line 443-449: Clarify the fixture’s dependency on having no active
Allocation for the Site around the unmatched inventory entries using a concise
comment, or explicitly assert the resulting VPC count to enforce that
auto-creation remains suppressed. Anchor the change near the entries using
random controller UUIDs and preserve the existing expectations.
- Around line 860-892: Extend the existing table-driven test around the current
skip-reason cases to add fixtures and rows for all five untested branches:
cross-tenant/site Network Security Group ownership, cross-tenant/site NVLink
Logical Partition ownership, a non-Ready NVLink Logical Partition, an existing
active same-name VPC, and an organization resolving to multiple REST Tenants.
Reuse the test’s established setup and assert each branch’s exact skip reason,
preserving the current table structure.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go`:
- Around line 79-86: Bound the soft-deleted VPC rows loaded by the GetAll call
in the inventory path to a recent recovery/retention window, using the existing
VpcFilterInput and established retention configuration or helper. Preserve
active-row loading and the current pagination behavior; do not alter correctness
for recently deleted records.
- Around line 386-425: Refactor recoverVpcFromInventory into named validation,
tenant-resolution, dependency, conflict-check, and commit steps, using a
vpcRecovery receiver that carries the shared inputs and resolved tenant. Add a
local skip(reason string) helper to replace repeated InventoryRecoveryResult
literals while preserving outcomes. In validateProto, parse
controllerVpc.GetDefaultNvlinkLogicalPartitionId().GetValue() directly and skip
when a non-empty value is not a valid UUID, instead of inferring invalidity from
reportedVpc.NVLinkLogicalPartitionID after FromProto.
🪄 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: f1ea5f79-1387-4c03-8773-9e454ed24c85
📒 Files selected for processing (5)
rest-api/db/pkg/db/model/vpc.gorest-api/db/pkg/db/model/vpc_test.gorest-api/workflow/pkg/activity/vpc/vpc.gorest-api/workflow/pkg/activity/vpc/vpc_test.gorest-api/workflow/pkg/util/common.go
| func TestVpcSQLDAO_Restore(t *testing.T) { | ||
| dbSession := testInitDB(t) | ||
| defer dbSession.Close() | ||
| testVpcSetupSchema(t, dbSession) | ||
|
|
||
| ipu := testBuildUser(t, dbSession, nil, testGenerateStarfleetID(), cutil.GetPtr("johnd@test.com"), cutil.GetPtr("John"), cutil.GetPtr("Doe")) | ||
| ip := testBuildInfrastructureProvider(t, dbSession, nil, "test-ip", "Test Provider", ipu.ID) | ||
| tnu := testBuildUser(t, dbSession, nil, testGenerateStarfleetID(), cutil.GetPtr("jdoe@test.com"), cutil.GetPtr("John"), cutil.GetPtr("Doe")) | ||
| tn := testBuildTenant(t, dbSession, nil, "test-tenant", "test-tenant-org", tnu.ID) | ||
| st := testBuildSite(t, dbSession, nil, ip.ID, "test-site", "Test Site", ip.Org, ipu.ID) | ||
| oldControllerID := uuid.New() | ||
| vpc := testBuildVpc( | ||
| t, | ||
| dbSession, | ||
| nil, | ||
| "test-vpc", | ||
| nil, | ||
| tn.Org, | ||
| ip.ID, | ||
| tn.ID, | ||
| st.ID, | ||
| nil, | ||
| cutil.GetPtr(VpcEthernetVirtualizer), | ||
| &oldControllerID, | ||
| nil, | ||
| cutil.GetPtr(VpcStatusError), | ||
| tnu.ID, | ||
| nil, | ||
| ) | ||
|
|
||
| vpcDAO := NewVpcDAO(dbSession) | ||
| require.NoError(t, vpcDAO.DeleteByID(context.Background(), nil, vpc.ID)) | ||
|
|
||
| activeVpcs, activeCount, err := vpcDAO.GetAll( | ||
| context.Background(), | ||
| nil, | ||
| VpcFilterInput{VpcIDs: []uuid.UUID{vpc.ID}}, | ||
| paginator.PageInput{}, | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
| assert.Empty(t, activeVpcs) | ||
| assert.Zero(t, activeCount) | ||
|
|
||
| deletedVpcs, deletedCount, err := vpcDAO.GetAll( | ||
| context.Background(), | ||
| nil, | ||
| VpcFilterInput{VpcIDs: []uuid.UUID{vpc.ID}, IncludeDeleted: true}, | ||
| paginator.PageInput{}, | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Len(t, deletedVpcs, 1) | ||
| assert.Equal(t, 1, deletedCount) | ||
| require.NotNil(t, deletedVpcs[0].Deleted) | ||
|
|
||
| newControllerID := uuid.New() | ||
| networkVirtualizationType := VpcFNN | ||
| routingProfile := "INTERNAL" | ||
| restored, err := vpcDAO.Restore(context.Background(), nil, VpcRestoreInput{ | ||
| VpcID: vpc.ID, | ||
| ControllerVpcID: &newControllerID, | ||
| NetworkVirtualizationType: &networkVirtualizationType, | ||
| RoutingProfile: &routingProfile, | ||
| ActiveVni: cutil.GetPtr(202), | ||
| Vni: cutil.GetPtr(101), | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, restored) | ||
| assert.Nil(t, restored.Deleted) | ||
| assert.False(t, restored.IsMissingOnSite) | ||
| assert.Equal(t, VpcStatusReady, restored.Status) | ||
| require.NotNil(t, restored.ControllerVpcID) | ||
| assert.Equal(t, newControllerID, *restored.ControllerVpcID) | ||
| require.NotNil(t, restored.NetworkVirtualizationType) | ||
| assert.Equal(t, VpcFNN, *restored.NetworkVirtualizationType) | ||
| require.NotNil(t, restored.RoutingProfile) | ||
| assert.Equal(t, "INTERNAL", *restored.RoutingProfile) | ||
| require.NotNil(t, restored.ActiveVni) | ||
| assert.Equal(t, 202, *restored.ActiveVni) | ||
| require.NotNil(t, restored.Vni) | ||
| assert.Equal(t, 101, *restored.Vni) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restructure TestVpcSQLDAO_Restore as table-driven subtests and cover the remaining branches.
Two points apply here.
First, the coding guidelines require named table-driven t.Run subtests organized around the method under test. This test runs a single linear scenario with no subtests. The sibling TestVpcSQLDAO_DeleteByID already follows the table pattern.
Second, two important branches of Restore are untested:
Restorereturnsdb.ErrDoesNotExistwhen no row matches. Add a case with an unknownVpcID.Restorewrites every column in its fixed set, so nil input fields clear existing values. Add a case that passes aVpcRestoreInputwith nil optional fields and asserts the columns become nil. That behavior is the main hazard of the new contract and deserves a regression test.
As per coding guidelines: "Use testify assertions and organize tests around the production function or method under test, with one top-level Test... function and named table-driven t.Run subtests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/db/pkg/db/model/vpc_test.go` around lines 1366 - 1448, The
TestVpcSQLDAO_Restore test should be reorganized into named table-driven t.Run
subtests, following the pattern used by TestVpcSQLDAO_DeleteByID. Keep the
existing successful restore scenario, add a case using an unknown VpcID that
asserts db.ErrDoesNotExist, and add a case passing nil optional fields that
verifies Restore clears the corresponding persisted columns. Use testify
assertions and keep all cases organized under the single top-level test.
Source: Coding guidelines
| func TestManageVpc_UpdateVpcsInDB_AutoCreatesAndRestores(t *testing.T) { | ||
| ctx := context.Background() | ||
| dbSession := testVPCInitDB(t) | ||
| defer dbSession.Close() | ||
| testVPCSetupSchema(t, dbSession) | ||
|
|
||
| providerOrg := "test-provider-org" | ||
| providerUser := testVPCBuildUser(t, dbSession, uuid.NewString(), providerOrg, []string{"FORGE_PROVIDER_ADMIN"}) | ||
| provider := testVPCSiteBuildInfrastructureProvider(t, dbSession, "test-provider", providerOrg, providerUser) | ||
| tenantOrg := "test-tenant-org" | ||
| tenantUser := testVPCBuildUser(t, dbSession, uuid.NewString(), tenantOrg, []string{"FORGE_TENANT_ADMIN"}) | ||
| tenant := testVPCBuildTenant(t, dbSession, "test-tenant", tenantOrg, tenantUser) | ||
| site := testVPCBuildSite(t, dbSession, provider, "test-site", providerUser) | ||
| testVPCSiteBuildAllocation(t, dbSession, site, tenant, "test-allocation", tenantUser) | ||
|
|
||
| siteClientPool := testTemporalSiteClientPool(t) | ||
| siteClientPool.IDClientMap[site.ID.String()] = &tmocks.Client{} | ||
| manager := ManageVpc{ | ||
| dbSession: dbSession, | ||
| siteClientPool: siteClientPool, | ||
| } | ||
|
|
||
| controllerVpcID := uuid.New() | ||
| networkVirtualizationType := cwssaws.VpcVirtualizationType_FNN | ||
| requestedVni := uint32(101) | ||
| activeVni := uint32(202) | ||
| routingProfile := "INTERNAL" | ||
| controllerVpc := &cwssaws.Vpc{ | ||
| Id: &cwssaws.VpcId{Value: controllerVpcID.String()}, | ||
| TenantOrganizationId: tenantOrg, | ||
| NetworkVirtualizationType: &networkVirtualizationType, | ||
| RoutingProfileType: &routingProfile, | ||
| Vni: &requestedVni, | ||
| Status: &cwssaws.VpcStatus{Vni: &activeVni}, | ||
| Metadata: &cwssaws.Metadata{ | ||
| Name: "site-created-vpc", | ||
| Description: "created directly on Site", | ||
| Labels: []*cwssaws.Label{ | ||
| {Key: "origin", Value: cutil.GetPtr("site")}, | ||
| }, | ||
| }, | ||
| } | ||
| inventory := &cwssaws.VPCInventory{ | ||
| Vpcs: []*cwssaws.Vpc{controllerVpc}, | ||
| Timestamp: timestamppb.Now(), | ||
| } | ||
|
|
||
| _, err := manager.UpdateVpcsInDB(ctx, site.ID, inventory) | ||
| require.NoError(t, err) | ||
|
|
||
| vpcDAO := cdbm.NewVpcDAO(dbSession) | ||
| createdVpc, err := vpcDAO.GetByID(ctx, nil, controllerVpcID, nil) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, controllerVpcID, createdVpc.ID) | ||
| require.NotNil(t, createdVpc.ControllerVpcID) | ||
| assert.Equal(t, controllerVpcID, *createdVpc.ControllerVpcID) | ||
| assert.Equal(t, tenant.ID, createdVpc.TenantID) | ||
| assert.Equal(t, site.ID, createdVpc.SiteID) | ||
| assert.Equal(t, site.InfrastructureProviderID, createdVpc.InfrastructureProviderID) | ||
| assert.Equal(t, site.ID, createdVpc.CreatedBy) | ||
| assert.Equal(t, cdbm.VpcStatusReady, createdVpc.Status) | ||
| assert.Equal(t, "site-created-vpc", createdVpc.Name) | ||
| assert.Equal(t, cdbm.Labels{"origin": "site"}, createdVpc.Labels) | ||
| require.NotNil(t, createdVpc.NetworkVirtualizationType) | ||
| assert.Equal(t, cdbm.VpcFNN, *createdVpc.NetworkVirtualizationType) | ||
| require.NotNil(t, createdVpc.RoutingProfile) | ||
| assert.Equal(t, "INTERNAL", *createdVpc.RoutingProfile) | ||
| require.NotNil(t, createdVpc.Vni) | ||
| assert.Equal(t, 101, *createdVpc.Vni) | ||
| require.NotNil(t, createdVpc.ActiveVni) | ||
| assert.Equal(t, 202, *createdVpc.ActiveVni) | ||
|
|
||
| statusDetailDAO := cdbm.NewStatusDetailDAO(dbSession) | ||
| statusDetails, _, err := statusDetailDAO.GetAllByEntityID(ctx, nil, createdVpc.ID.String(), nil, nil, nil) | ||
| require.NoError(t, err) | ||
| require.Len(t, statusDetails, 1) | ||
| require.NotNil(t, statusDetails[0].Message) | ||
| assert.Equal(t, "VPC auto-created from Site inventory", *statusDetails[0].Message) | ||
|
|
||
| // Replaying the inventory is idempotent. | ||
| _, err = manager.UpdateVpcsInDB(ctx, site.ID, inventory) | ||
| require.NoError(t, err) | ||
| vpcs, count, err := vpcDAO.GetAll( | ||
| ctx, | ||
| nil, | ||
| cdbm.VpcFilterInput{VpcIDs: []uuid.UUID{controllerVpcID}}, | ||
| cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Len(t, vpcs, 1) | ||
| assert.Equal(t, 1, count) | ||
|
|
||
| // A later inventory snapshot restores the row if Site still reports it. | ||
| require.NoError(t, vpcDAO.DeleteByID(ctx, nil, controllerVpcID)) | ||
| deletedVpcs, _, err := vpcDAO.GetAll( | ||
| ctx, | ||
| nil, | ||
| cdbm.VpcFilterInput{VpcIDs: []uuid.UUID{controllerVpcID}, IncludeDeleted: true}, | ||
| cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Len(t, deletedVpcs, 1) | ||
| require.NotNil(t, deletedVpcs[0].Deleted) | ||
| inventory.Timestamp = timestamppb.New(deletedVpcs[0].Deleted.Add(time.Second)) | ||
|
|
||
| _, err = manager.UpdateVpcsInDB(ctx, site.ID, inventory) | ||
| require.NoError(t, err) | ||
| restoredVpc, err := vpcDAO.GetByID(ctx, nil, controllerVpcID, nil) | ||
| require.NoError(t, err) | ||
| assert.Nil(t, restoredVpc.Deleted) | ||
| assert.False(t, restoredVpc.IsMissingOnSite) | ||
| assert.Equal(t, cdbm.VpcStatusReady, restoredVpc.Status) | ||
|
|
||
| statusDetails, _, err = statusDetailDAO.GetAllByEntityID(ctx, nil, restoredVpc.ID.String(), nil, nil, nil) | ||
| require.NoError(t, err) | ||
| require.Len(t, statusDetails, 2) | ||
| require.NotNil(t, statusDetails[0].Message) | ||
| assert.Equal(t, "VPC restored from Site inventory", *statusDetails[0].Message) | ||
|
|
||
| // An older inventory snapshot must not resurrect a newer tombstone. | ||
| require.NoError(t, vpcDAO.DeleteByID(ctx, nil, controllerVpcID)) | ||
| deletedVpcs, _, err = vpcDAO.GetAll( | ||
| ctx, | ||
| nil, | ||
| cdbm.VpcFilterInput{VpcIDs: []uuid.UUID{controllerVpcID}, IncludeDeleted: true}, | ||
| cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Len(t, deletedVpcs, 1) | ||
| require.NotNil(t, deletedVpcs[0].Deleted) | ||
| inventory.Timestamp = timestamppb.New(deletedVpcs[0].Deleted.Add(-time.Second)) | ||
|
|
||
| _, err = manager.UpdateVpcsInDB(ctx, site.ID, inventory) | ||
| require.NoError(t, err) | ||
| _, err = vpcDAO.GetByID(ctx, nil, controllerVpcID, nil) | ||
| assert.ErrorIs(t, err, cdb.ErrDoesNotExist) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Split this test into named subtests and assert the restored field values.
Two points apply here.
First, the coding guidelines require named table-driven t.Run subtests. This test runs four order-dependent phases in one linear body: create, idempotent replay, restore from a newer snapshot, and refusal of an older snapshot. Each phase mutates inventory.Timestamp for the next one. A failure in an early phase cascades with no phase label, which makes diagnosis slow. Wrap each phase in a named t.Run so the failing phase is identified.
Second, the restore phase asserts only Deleted, IsMissingOnSite, and Status. It does not assert that ControllerVpcID, Vni, ActiveVni, and RoutingProfile hold the expected values after restoration. That gap matters: VpcSQLDAO.Restore writes every column in a fixed set, so a nil input silently clears a column. Assertions on those four fields would catch such regressions, including the RoutingProfile clearing behavior flagged separately at rest-api/workflow/pkg/activity/vpc/vpc.go line 658.
As per coding guidelines: "Use testify assertions and organize tests around the production function or method under test, with one top-level Test... function and named table-driven t.Run subtests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc_test.go` around lines 704 - 843,
Refactor TestManageVpc_UpdateVpcsInDB_AutoCreatesAndRestores into named t.Run
subtests for creation, idempotent replay, newer-snapshot restoration, and
older-snapshot rejection, preserving the required phase order and shared setup.
In the restoration subtest, additionally assert ControllerVpcID, Vni, ActiveVni,
and RoutingProfile retain the expected values after restoration, using testify
assertions and the existing DAO results.
Source: Coding guidelines
| allocationDAO := cdbm.NewAllocationDAO(mv.dbSession) | ||
| var allocationFilter cdbm.AllocationFilterInput | ||
| allocationFilter.TenantIDs = []uuid.UUID{tenant.ID} | ||
| allocationFilter.SiteIDs = []uuid.UUID{site.ID} | ||
| allocationCount, err := allocationDAO.GetCount(ctx, nil, allocationFilter) | ||
| if err != nil { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{}, fmt.Errorf("count VPC tenant allocations: %w", err) | ||
| } | ||
| if allocationCount == 0 { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "tenant does not have an active Allocation for the Site", | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect AllocationFilterInput fields, GetCount, and Allocation status constants.
ast-grep run --pattern 'AllocationFilterInput struct { $$$ }' --lang go .
ast-grep run --pattern 'func ($_ $_) GetCount(ctx context.Context, tx *db.Tx, filter AllocationFilterInput) (int, error) { $$$ }' --lang go .
rg -nP --type=go '\bAllocationStatus[A-Za-z]*\s*=' -C1Repository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -e
rg -n --type=go 'type AllocationFilterInput|AllocationFilterInput|func .*GetCount|GetCount\(' .Repository: NVIDIA/infra-controller
Length of output: 25139
🏁 Script executed:
#!/bin/bash
set -e
sed -n '130,330p' rest-api/db/pkg/db/model/allocation.go
sed -n '570,620p' rest-api/db/pkg/db/model/allocation.go
rg -n --type=go 'const \(|AllocationStatusPending|AllocationStatusDeleting|AllocationStatus[A-Za-z]+' rest-api/db/pkg/db/model rest-api/workflow/pkg/activity/vpc/vpc.go | head -160Repository: NVIDIA/infra-controller
Length of output: 16028
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,115p' rest-api/db/pkg/db/model/allocation.go
sed -n '265,385p' rest-api/db/pkg/db/model/allocation.go
sed -n '580,615p' rest-api/db/pkg/db/model/allocation_test.goRepository: NVIDIA/infra-controller
Length of output: 11665
🏁 Script executed:
#!/bin/bash
set -e
rg -n --type=go 'AllocationStatus( pending|registered|error|deleting)|AllocationStatusPending|AllocationStatusRegistered|AllocationStatusError|AllocationStatusDeleting|active Allocation|active allocation' rest-api/workflow rest-api/api rest-api/db/pkg/db/model | head -240
sed -n '400,510p' rest-api/workflow/pkg/activity/vpc/vpc.go
rg -n --type=go 'does not have an active Allocation|InventoryRecoverySkipped|tenant.*allocation' rest-api/workflowRepository: NVIDIA/infra-controller
Length of output: 14867
🏁 Script executed:
#!/bin/bash
set -e
sed -n '280,370p' rest-api/api/pkg/api/handler/allocation.go
sed -n '760,830p' rest-api/api/pkg/api/handler/allocation_test.go
sed -n '1070,1225p' rest-api/db/pkg/db/model/allocation_test.go
rg -n --type=go 'Statuses:.*AllocationStatus|Statuses: \[\]string|Status:.*AllocationStatusDeleting|AllocationStatusDeleting' rest-api/api rest-api/workflow | head -160Repository: NVIDIA/infra-controller
Length of output: 16062
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
model = Path("rest-api/db/pkg/db/model/allocation.go").read_text()
activity = Path("rest-api/workflow/pkg/activity/vpc/vpc.go").read_text()
required_fields = ["TenantIDs", "SiteIDs", "Statuses"]
status_constants = {
"AllocationStatusPending": '"Pending"',
"AllocationStatusRegistered": '"Registered"',
"AllocationStatusError": '"Error"',
"AllocationStatusDeleting": '"Deleting"',
}
print("AllocationFilterInput supports status filtering:", all(f in model for f in required_fields))
for name, value in status_constants.items():
print(f"{name} defined:", f"{name} = {value}" in model)
start = model.index("func (asd AllocationSQLDAO) setQueryWithFilter")
end = model.index("\n}\n\n// GetAll returns all Allocations", start)
filter_impl = model[start:end]
print("setQueryWithFilter applies Statuses:", 'filter.Statuses' in filter_impl)
print("VPC activity sets Statuses:", "allocationFilter.Statuses" in activity)
print("VPC activity filters tenant and site:",
"allocationFilter.TenantIDs" in activity and "allocationFilter.SiteIDs" in activity)
PYRepository: NVIDIA/infra-controller
Length of output: 488
Filter VPC recovery by active allocation status.
GetCount applies only tenant and site filters, so it can count non-soft-deleted allocations in Error or Deleting states. Set allocationFilter.Statuses to the statuses that authorize recovery, and add regression tests for excluded states.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 467 - 480, Update the
allocation filter used by GetCount in the VPC recovery flow to set Statuses to
the active allocation states that authorize recovery, while retaining the tenant
and site filters. Add regression coverage confirming allocations in Error and
Deleting states are excluded from recovery.
| if nvLinkLogicalPartitionID != nil { | ||
| nvLinkLogicalPartitionDAO := cdbm.NewNVLinkLogicalPartitionDAO(mv.dbSession) | ||
| nvLinkLogicalPartition, nvLinkErr := nvLinkLogicalPartitionDAO.GetByID(ctx, nil, *nvLinkLogicalPartitionID, nil) | ||
| if errors.Is(nvLinkErr, cdb.ErrDoesNotExist) { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "referenced NVLink Logical Partition does not exist in REST", | ||
| }, nil | ||
| } | ||
| if nvLinkErr != nil { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{}, fmt.Errorf("get inventory VPC NVLink Logical Partition: %w", nvLinkErr) | ||
| } | ||
| if nvLinkLogicalPartition.SiteID != site.ID || nvLinkLogicalPartition.TenantID != tenant.ID { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "referenced NVLink Logical Partition belongs to a different Tenant or Site", | ||
| }, nil | ||
| } | ||
| if nvLinkLogicalPartition.Status != cdbm.NVLinkLogicalPartitionStatusReady { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "referenced NVLink Logical Partition is not Ready", | ||
| }, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The NVLink Ready check can block restoration of an otherwise valid VPC.
On the restore path, nvLinkLogicalPartitionID is taken from the existing record and is never written back, because VpcRestoreInput carries no NVLink field. The validation therefore has no effect on the restore write, yet it can still cause a skip.
Consider the case where a VPC is soft-deleted and its referenced NVLink Logical Partition is temporarily not Ready. The Site still reports the VPC, so restoration is correct, but line 530 skips it and the record stays invisible in REST. The strict Ready requirement is appropriate for creating a new record. It is too strict for restoring an unchanged reference.
Apply the existence and ownership checks in both paths, and apply the Ready check only when the partition reference comes from inventory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 512 - 536, Update the
NVLink Logical Partition validation around nvLinkLogicalPartitionID to
distinguish restoration from new inventory creation: always retain the existing
existence and Site/Tenant ownership checks, but enforce
NVLinkLogicalPartitionStatusReady only when the reference originates from
inventory. Allow restoration of an unchanged soft-deleted VPC with a non-Ready
partition, while preserving the current skip behavior for new inventory
references.
| var nameFilter cdbm.VpcFilterInput | ||
| nameFilter.Name = &name | ||
| nameFilter.InfrastructureProviderID = &site.InfrastructureProviderID | ||
| nameFilter.TenantIDs = []uuid.UUID{tenant.ID} | ||
| nameFilter.SiteIDs = []uuid.UUID{site.ID} | ||
| var page cdbp.PageInput | ||
| page.Limit = cwutil.GetPtr(cdbp.TotalLimit) | ||
| vpcDAO := cdbm.NewVpcDAO(mv.dbSession) | ||
| vpcsWithName, _, err := vpcDAO.GetAll(ctx, nil, nameFilter, page, nil) | ||
| if err != nil { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{}, fmt.Errorf("check inventory VPC name conflict: %w", err) | ||
| } | ||
| for i := range vpcsWithName { | ||
| if existingVpc == nil || vpcsWithName[i].ID != existingVpc.ID { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "an active VPC with the same name already exists for the Tenant and Site", | ||
| }, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for a unique constraint or unique index on the vpc table covering name.
rg -nP --type=go 'unique' -C2 -g '**/model/vpc.go'
fd -e go -e sql . --full-path -p 'migration' --exec rg -nil 'vpc' {} \; | head -50
rg -nPi 'create\s+unique\s+index|unique\s*\(' -g '*.sql' -C2Repository: NVIDIA/infra-controller
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- VPC-related files ---'
git ls-files | rg -i '(^|/)(vpc|.*migration.*|.*schema.*).*\\.(go|sql|yaml|yml)$' | head -200
printf '%s\n' '--- VPC model and schema references ---'
rg -n -i 'type Vpc|CREATE TABLE.*vpc|ALTER TABLE.*vpc|unique.*(vpc|name)|vpc.*unique|site_id.*tenant_id.*name|tenant_id.*site_id.*name' --glob '*.go' --glob '*.sql' --glob '*.yaml' --glob '*.yml' .
printf '%s\n' '--- Activity structure and relevant call sites ---'
sed -n '500,720p' rest-api/workflow/pkg/activity/vpc/vpc.go
rg -n 'NewVpcDAO|GetAll\\(|Advisory|advisory|Begin|Transaction|Create\\(' rest-api/workflow/pkg/activity/vpc rest-api -g '*.go' | head -250Repository: NVIDIA/infra-controller
Length of output: 41892
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- REST API database migrations ---'
git ls-files rest-api/db/pkg/migrations | sed -n '1,240p'
printf '%s\n' '--- VPC model definition and DAO methods ---'
sed -n '110,430p' rest-api/db/pkg/db/model/vpc.go
printf '%s\n' '--- VPC name constraints and indexes ---'
rg -n -i 'vpc.*name|name.*vpc|unique.*name|name.*unique|CREATE UNIQUE|ADD CONSTRAINT.*UNIQUE|unique:' \
rest-api/db rest-api/api rest-api/workflow -g '*.go' -g '*.sql' | head -300
printf '%s\n' '--- Existing VPC create/update uniqueness flow ---'
sed -n '130,205p' rest-api/api/pkg/api/handler/vpc.go
sed -n '575,625p' rest-api/api/pkg/api/handler/vpc.goRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Initial VPC table definition ---'
sed -n '1,180p' rest-api/db/pkg/migrations/20220526234258_initial.go
printf '%s\n' '--- VPC DAO query and insert transaction handling ---'
rg -n 'func \\(vsd VpcSQLDAO\\) (GetAll|Create)|db.GetIDB|NewInsert|WhereAllWithDeleted' rest-api/db/pkg/db/model/vpc.go
sed -n '430,620p' rest-api/db/pkg/db/model/vpc.go
sed -n '620,820p' rest-api/db/pkg/db/model/vpc.go
printf '%s\n' '--- API VPC uniqueness checks and create path ---'
sed -n '145,190p' rest-api/api/pkg/api/handler/vpc.go
sed -n '190,280p' rest-api/api/pkg/api/handler/vpc.go
sed -n '590,620p' rest-api/api/pkg/api/handler/vpc.go
printf '%s\n' '--- Transaction and advisory-lock semantics ---'
rg -n 'func WithTxResult|func \\(.*TryAcquireAdvisoryLock|TryAcquireAdvisoryLock|WithTxResult' rest-api/db rest-api -g '*.go' | head -100Repository: NVIDIA/infra-controller
Length of output: 27710
Add an authoritative guard for VPC name uniqueness.
No unique constraint or index protects active VPC names. The check runs before the transaction, and the advisory lock uses the controller VPC ID. Concurrent creates can therefore insert duplicate names. Moving the query into the transaction alone does not lock the absent row. Add a partial unique index on (site_id, tenant_id, name) for active rows, and map its violation to InventoryRecoverySkipped. Alternatively, acquire a shared advisory lock keyed by the name scope before checking and inserting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 542 - 561, Add
authoritative active-VPC name uniqueness by creating a partial unique index on
site_id, tenant_id, and name for non-deleted/active rows, then update the VPC
creation flow around vpcDAO.GetAll to detect that constraint violation and
return InventoryRecoverySkipped with the existing conflict reason. Ensure
concurrent inserts cannot bypass the guard; do not rely solely on the
pre-transaction query or controller VPC ID advisory lock.
| return cdb.WithTxResult(ctx, mv.dbSession, func(tx *cdb.Tx) (util.InventoryRecoveryResult[*cdbm.Vpc], error) { | ||
| lockID := cdb.GetAdvisoryLockIDFromString(fmt.Sprintf("vpc-inventory-%s-%s", site.ID, controllerVpcID)) | ||
| lockErr := tx.TryAcquireAdvisoryLock(ctx, lockID, nil) | ||
| if lockErr != nil { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{}, fmt.Errorf("acquire VPC inventory lock: %w", lockErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the advisory lock helper signature, return contract, and existing call sites.
ast-grep run --pattern 'func ($_ $_) TryAcquireAdvisoryLock($$$) $$$ { $$$ }' --lang go .
ast-grep run --pattern 'func GetAdvisoryLockIDFromString($$$) $_ { $$$ }' --lang go .
rg -nP --type=go '\bTryAcquireAdvisoryLock\s*\(' -C6Repository: NVIDIA/infra-controller
Length of output: 2094
🏁 Script executed:
#!/bin/bash
# Inspect lock acquisition semantics, retry behavior, and activity error handling.
printf '%s\n' '--- tx lock implementation ---'
sed -n '130,245p' rest-api/db/pkg/db/tx.go
printf '%s\n' '--- activity call site ---'
sed -n '130,180p' rest-api/workflow/pkg/activity/vpc/vpc.go
printf '%s\n' '--- lock-related tests and configuration ---'
rg -n --type=go 'AcquireAdvisoryLock|TryAcquireAdvisoryLock|LockRetryOptions|defaultRetries|UpdateVpcsInDB' rest-api -C4Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Inspect transaction result handling and the inventory recovery result used for skips.
printf '%s\n' '--- transaction wrappers ---'
rg -n 'func WithTxResult|type InventoryRecoveryResult|InventoryRecoverySkipped|InventoryRecovery' rest-api/db rest-api/workflow -g '*.go' -C6
printf '%s\n' '--- VPC recovery result paths ---'
sed -n '540,650p' rest-api/workflow/pkg/activity/vpc/vpc.go
printf '%s\n' '--- sentinel definition ---'
rg -n 'ErrXactAdvisoryLockFailed' rest-api -g '*.go' -C3Repository: NVIDIA/infra-controller
Length of output: 35361
Handle cdb.ErrXactAdvisoryLockFailed as a skipped recovery.
TryAcquireAdvisoryLock returns this sentinel after its non-blocking attempts are exhausted. Propagating it causes UpdateVpcsInDB to stop processing the page. Return InventoryRecoverySkipped for this sentinel and preserve genuine database errors as activity failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 572 - 577, Update the
transaction callback in UpdateVpcsInDB around TryAcquireAdvisoryLock to detect
cdb.ErrXactAdvisoryLockFailed and return InventoryRecoverySkipped instead of an
error. Continue wrapping and propagating all other lock-acquisition errors as
activity failures.
| timestamp := inventory.GetTimestamp() | ||
| if timestamp == nil { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "inventory is not newer than the VPC soft-delete marker", | ||
| }, nil | ||
| } | ||
| timestampErr := timestamp.CheckValid() | ||
| if timestampErr != nil || !timestamp.AsTime().After(*deletedVpc.Deleted) { | ||
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | ||
| Action: util.InventoryRecoverySkipped, | ||
| Reason: "inventory is not newer than the VPC soft-delete marker", | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use distinct skip reasons for a missing timestamp and a stale timestamp.
Both branches report "inventory is not newer than the VPC soft-delete marker". The conditions differ. A nil or invalid timestamp means the inventory payload is malformed. A timestamp that is not after Deleted means the snapshot is stale. The Reason field is the only diagnostic surfaced to the operator at line 163, so conflating the two hides a Site Agent defect behind an expected condition.
🔍 Proposed change to separate the reasons
timestamp := inventory.GetTimestamp()
if timestamp == nil {
return util.InventoryRecoveryResult[*cdbm.Vpc]{
Action: util.InventoryRecoverySkipped,
- Reason: "inventory is not newer than the VPC soft-delete marker",
+ Reason: "inventory does not report a timestamp",
}, nil
}
- timestampErr := timestamp.CheckValid()
- if timestampErr != nil || !timestamp.AsTime().After(*deletedVpc.Deleted) {
+ if timestampErr := timestamp.CheckValid(); timestampErr != nil {
+ return util.InventoryRecoveryResult[*cdbm.Vpc]{
+ Action: util.InventoryRecoverySkipped,
+ Reason: "inventory timestamp is not valid",
+ }, nil
+ }
+ if !timestamp.AsTime().After(*deletedVpc.Deleted) {
return util.InventoryRecoveryResult[*cdbm.Vpc]{
Action: util.InventoryRecoverySkipped,
Reason: "inventory is not newer than the VPC soft-delete marker",
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timestamp := inventory.GetTimestamp() | |
| if timestamp == nil { | |
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | |
| Action: util.InventoryRecoverySkipped, | |
| Reason: "inventory is not newer than the VPC soft-delete marker", | |
| }, nil | |
| } | |
| timestampErr := timestamp.CheckValid() | |
| if timestampErr != nil || !timestamp.AsTime().After(*deletedVpc.Deleted) { | |
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | |
| Action: util.InventoryRecoverySkipped, | |
| Reason: "inventory is not newer than the VPC soft-delete marker", | |
| }, nil | |
| } | |
| timestamp := inventory.GetTimestamp() | |
| if timestamp == nil { | |
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | |
| Action: util.InventoryRecoverySkipped, | |
| Reason: "inventory does not report a timestamp", | |
| }, nil | |
| } | |
| if timestampErr := timestamp.CheckValid(); timestampErr != nil { | |
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | |
| Action: util.InventoryRecoverySkipped, | |
| Reason: "inventory timestamp is not valid", | |
| }, nil | |
| } | |
| if !timestamp.AsTime().After(*deletedVpc.Deleted) { | |
| return util.InventoryRecoveryResult[*cdbm.Vpc]{ | |
| Action: util.InventoryRecoverySkipped, | |
| Reason: "inventory is not newer than the VPC soft-delete marker", | |
| }, 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 `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 626 - 639, Update the
recovery logic around timestamp and deletedVpc.Deleted so missing or invalid
inventory timestamps return a reason identifying the payload as malformed, while
a valid timestamp not newer than the soft-delete marker returns a distinct
stale-inventory reason. Keep both branches returning InventoryRecoverySkipped
and preserve the existing valid-newer flow.
| networkVirtualizationType := reportedVpc.NetworkVirtualizationType | ||
| if networkVirtualizationType == nil { | ||
| networkVirtualizationType = deletedVpc.NetworkVirtualizationType | ||
| } | ||
| activeVni := reportedVpc.ActiveVni | ||
| if activeVni == nil { | ||
| activeVni = deletedVpc.ActiveVni | ||
| } | ||
| requestedVni := reportedVpc.Vni | ||
| if requestedVni == nil { | ||
| requestedVni = deletedVpc.Vni | ||
| } | ||
|
|
||
| restoredVpc, restoreErr := vpcDAO.Restore(ctx, tx, cdbm.VpcRestoreInput{ | ||
| VpcID: deletedVpc.ID, | ||
| ControllerVpcID: &controllerVpcID, | ||
| NetworkVirtualizationType: networkVirtualizationType, | ||
| RoutingProfile: reportedVpc.RoutingProfile, | ||
| ActiveVni: activeVni, | ||
| NetworkSecurityGroupID: networkSecurityGroupID, | ||
| NetworkSecurityGroupPropagationDetails: propagationDetails, | ||
| Vni: requestedVni, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Restore uses reset semantics on a PATCH-shaped input, and one field already loses data. VpcSQLDAO.Restore writes every column in a fixed column set, so a nil pointer in VpcRestoreInput clears the column instead of preserving it. The struct is shaped exactly like VpcUpdateInput, where nil means "leave unchanged". The caller compensates for three fields and misses RoutingProfile, which produces silent data loss on restoration.
rest-api/workflow/pkg/activity/vpc/vpc.go#L641-L663: add aRoutingProfilefallback todeletedVpc.RoutingProfilealongside the existing fallbacks forNetworkVirtualizationType,ActiveVni, andVni, then pass the resolved value toRestore.rest-api/db/pkg/db/model/vpc.go#L320-L331: document onVpcRestoreInputthat every field is written unconditionally and that a nil pointer clears the column, so callers must supply the complete post-restore state.
📍 Affects 2 files
rest-api/workflow/pkg/activity/vpc/vpc.go#L641-L663(this comment)rest-api/db/pkg/db/model/vpc.go#L320-L331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/workflow/pkg/activity/vpc/vpc.go` around lines 641 - 663, Update
rest-api/workflow/pkg/activity/vpc/vpc.go lines 641-663 to resolve
RoutingProfile from reportedVpc first and fall back to deletedVpc.RoutingProfile
when nil, then pass that resolved value to VpcSQLDAO.Restore via
VpcRestoreInput. In rest-api/db/pkg/db/model/vpc.go lines 320-331, document on
VpcRestoreInput that all fields are written unconditionally, nil clears the
corresponding column, and callers must provide the complete post-restore state.
🔐 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-07-31 21:00:31 UTC | Commit: 900dba9 |
Resolve VPC protobuf migration conflicts while preserving inventory recovery. Signed-off-by: Hitesh Wadekar <hwadekar@nvidia.com>
Preserve restored state and skip unsafe recovery attempts while expanding regression coverage. Signed-off-by: Hitesh Wadekar <hwadekar@nvidia.com>
1 participant
Description
Site inventory previously skipped VPCs (which are existed in Site) that had no active REST database record, leaving Site and REST state inconsistent. This PR reconciles those VPCs using their controller ID.
Soft-deleted VPCs are restored when reported by a newer inventory snapshot, while true orphans are auto-created after validating tenant ownership, Site allocation, NSG, NVLink partition, and name conflicts. Recovery writes are transactional and retry-safe.
Related issues
This PR part of the issue (#3436)
Type of Change
Testing
Tests executed:
make test-workflow
make test-db
go vet ./workflow/pkg/util ./workflow/pkg/activity/vpc
Additional Notes