feat: auto-generate TenantSite entries for privileged tenants and service accounts#3534
Open
RajMandaliya wants to merge 6 commits into
Open
feat: auto-generate TenantSite entries for privileged tenants and service accounts#3534RajMandaliya wants to merge 6 commits into
RajMandaliya wants to merge 6 commits into
Conversation
Keeping Pace with original repo
…vice accounts Historically, TenantSite records (which grant a tenant visibility and action rights on a site) were only created as a side effect of that tenant's first allocation on the site, and deleted when their last allocation there was removed. This no longer covers real access patterns: privileged tenants (TargetedInstanceCreation enabled) have long been able to create instances on sites without any compute allocation there, and zero-DPU instances mean they may not need a network allocation either. Service accounts have the same need. Adds EnsureTenantSitesForInfrastructureProvider, a shared helper that auto-creates TenantSite records for every site under a tenant's infrastructure provider. Wired into: - GetCurrentServiceAccountHandler (service accounts always get this) - GetCurrentTenantHandler (only when the tenant is privileged, i.e. TargetedInstanceCreation is enabled) Also updates the allocation-deletion cleanup path in allocation.go to skip removing a TenantSite record when the tenant is privileged, since these tenants shouldn't lose site access just because their last allocation there was deleted. Tested directly (creation + idempotency on a second call), plus full existing handler test suite passes (serially -- a couple of tests showed pre-existing flakiness under parallel execution against a shared test DB, unrelated to this change, and passed cleanly with -p 1 -parallel 1). Addresses NVIDIA#3370. Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe change adds idempotent TenantSite synchronization for infrastructure-provider sites, invokes it during service-account and privileged tenant initialization, and preserves TenantSite associations for privileged tenants when their last allocation is deleted. ChangesTenantSite lifecycle synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TenantHandler
participant EnsureTenantSitesForInfrastructureProvider
participant SiteDAO
participant TenantSiteDAO
TenantHandler->>EnsureTenantSitesForInfrastructureProvider: synchronize privileged tenant sites
EnsureTenantSitesForInfrastructureProvider->>SiteDAO: retrieve infrastructure-provider sites
EnsureTenantSitesForInfrastructureProvider->>TenantSiteDAO: check and create missing TenantSite records
TenantSiteDAO-->>TenantHandler: transaction completes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 4
🤖 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/api/pkg/api/handler/allocation.go`:
- Around line 1478-1484: Remove the redundant tenant lookup via tnDAO.GetByID in
the cleanup check and derive isPrivileged directly from the already-loaded
a.Tenant and its Config.TargetedInstanceCreation, preserving the existing
nil-safe behavior and error flow where applicable.
- Around line 1473-1509: Extend allocation_test.go with a case covering the
TargetedInstanceCreation privileged-tenant path in the allocation deletion flow.
Configure the tenant/service account as privileged, remove its last allocation,
and assert that its TenantSite association remains instead of being deleted,
while preserving existing regular-tenant coverage.
In `@rest-api/api/pkg/api/handler/tenantsite_helpers.go`:
- Around line 43-73: Optimize EnsureTenantSitesForInfrastructureProvider in
rest-api/api/pkg/api/handler/tenantsite_helpers.go:43-73 by fetching all
existing TenantSite rows for the tenant and supplied site IDs in one query, then
creating only missing entries instead of issuing per-site GetAll/Create calls.
In rest-api/api/pkg/api/handler/serviceaccount.go:180-188 and
rest-api/api/pkg/api/handler/tenant.go:209-230, retain these calls only if the
helper’s cheap no-op behavior makes them safe; otherwise gate them to avoid
repeated execution on every GetCurrent request.
- Around line 43-73: Batch the TenantSite lookup in the surrounding helper: call
tsDAO.GetAll once for tenant.ID without filtering by individual site, build a
set of existing SiteIDs from the returned rows, then iterate sites and invoke
tsDAO.Create only for missing IDs. Preserve the existing error context for
lookup and creation failures, and avoid per-site GetAll calls.
🪄 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: cee7183f-8743-44f5-8204-9e9c25b4aaa5
📒 Files selected for processing (5)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/tenant.gorest-api/api/pkg/api/handler/tenantsite_helpers.gorest-api/api/pkg/api/handler/tenantsite_helpers_test.go
- Remove the redundant Tenant lookup in the allocation-deletion cleanup check; a.Tenant is already loaded via includeRelations on the earlier GetByID for this allocation, so derive isPrivileged directly from it instead of an extra DB round-trip. - Batch EnsureTenantSitesForInfrastructureProvider's TenantSite lookup into a single query across all sites (previously issued one GetAll per site), then only Create the entries actually missing. This also makes the helper cheap enough to safely call on every GetCurrent request, as it was before. - Add TestAllocationHandler_Delete_PrivilegedTenantKeepsTenantSite, covering the actual gap CodeRabbit flagged: a privileged tenant's last allocation on a site is deleted, and its TenantSite association is verified to persist through the real delete handler, rather than only being covered indirectly via the unit-level helper test. Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #3370.
Problem
TenantSite records (which grant a tenant visibility and action rights on a site) were only ever created as a side effect of that tenant's first allocation on the site, and deleted when their last allocation there was removed.
This no longer matches real access patterns: privileged tenants (
TargetedInstanceCreationenabled) have long been able to create instances on sites without any compute allocation there, and zero-DPU instances mean they may not need a network allocation either. Service accounts have the same need but currently have no path to aTenantSiterecord at all unless they happen to also hold an allocation.Fix
EnsureTenantSitesForInfrastructureProvider, a shared helper that auto-createsTenantSiterecords for every site under a tenant's infrastructure provider (mirrors the existing get-or-create pattern already used inallocation.go).GetCurrentServiceAccountHandler(service accounts always get this) andGetCurrentTenantHandler(only when the tenant is privileged).TenantSiterecord for privileged tenants/service accounts, since per the issue, these tenants shouldn't lose site access just because their last allocation there was deleted.Testing
handlerpackage test suite passes. Note: two pre-existing tests showed flakiness under default parallel executionagainst the shared test DB (
relation "tenant" does not exist, duplicatepg_typekey) — confirmed unrelated to this change by running serially (-p 1 -parallel 1), where everything passes cleanly.