Gfx 4379 - #14
Open
manojsaviz wants to merge 4 commits into
Open
Conversation
Collaborator
|
@manojsaviz please add curl based testing results here along with the commands for all the operations which are considered in related to your changes. For UT test cases consider this document for reference: |
Collaborator
|
Please consider this note when raising a PR and follow the structure: https://aviznetworks.atlassian.net/wiki/spaces/AD/pages/19496965/ONES-T+Developer+Handbook#Development-Workflow |
There was a problem hiding this comment.
Pull request overview
Adds support for externally managed GPU-port allocations via the /gpus API on UFM / NMX-C fabrics, introduces an inventory-sync action resource, and hardens server deallocation/reconciliation logic across UFM fabric topology variants.
Changes:
- Add new
fabricapi_tenant_gpusresource for GPU port assign/unassign (POST/fabrics/{fabric}/tenants/{tenant}/gpus). - Add new
fabricapi_inventory_syncaction resource (POST/fabrics/{fabric}/inventorySync). - Extend tenant and server resources/client logic to better reconcile allocations on fabrics where server attribution is missing/empty, and expose new read-only tenant allocation attributes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/tenant_servers_resource.go | Adds fallback server resolution and diagnostics to prevent “silent no-op” deallocations when allocation exists but server names can’t be determined. |
| internal/provider/tenant_resource.go | Adds computed UFM/NMX-C allocation attributes (including NMX-C partitions) and populates them on Create/Read. |
| internal/provider/tenant_gpus_resource.go | New resource implementing /gpus assign/unassign lifecycle with conflict checks, validation, and state handling. |
| internal/provider/provider.go | Registers the new tenant_gpus and inventory_sync resources with the provider. |
| internal/provider/inventory_sync_resource.go | New action-style resource to trigger an immediate UFM inventory sync. |
| internal/provider/client.go | Adds /gpus and /inventorySync client methods and extends tenant response models with UFM/NMX-C allocation fields plus improved server resolution logic. |
| examples/variables.tf | Adds example variables for configuring fabricapi_tenant_gpus. |
| examples/main.tf | Demonstrates usage of fabricapi_tenant_gpus in the primary example flow. |
| examples/decoupled/04-gpus/variables.tf | Adds a decoupled example module for GPU port assignment variables. |
| examples/decoupled/04-gpus/main.tf | Adds a decoupled example module demonstrating standalone fabricapi_tenant_gpus usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1420
to
+1431
| // hasUnreportedAllocation reports whether the tenant has any GPU allocation at all. It must check | ||
| // every counter, not just gpusAllocated: on NS+EW fabrics a /gpus (UFM) allocation shows only in | ||
| // ufmAllocatedPorts and leaves gpusAllocated at 0, so checking gpusAllocated alone would miss it | ||
| // and make tenant_gpus Delete/Update skip a release it actually needs (leaking the allocation). | ||
| // gpusAllocated covers the /tenants (NS) side; the ufm/nmxc counters cover the /gpus (EW) side. | ||
| func hasUnreportedAllocation(t *TenantResponse) bool { | ||
| if t == nil { | ||
| return false | ||
| } | ||
| return out | ||
| return t.GpusAllocated > 0 || | ||
| t.UFMAllocatedPorts > 0 || | ||
| t.NMXCGpusAllocated > 0 |
Comment on lines
+492
to
+494
| func stableTenantGpusID(fabricName, tenantName, operation string, serverNames []string) string { | ||
| return fmt.Sprintf("%s:%s:%s:%s", fabricName, tenantName, operation, strings.Join(serverNames, ",")) | ||
| } |
Comment on lines
+490
to
+494
| // Async+webhook: not blocking on a read, so these will be unknown until the next Read. | ||
| resp.Diagnostics.Append(setUFMNMXCFields(ctx, &data, &TenantResponse{})...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (5)
internal/provider/tenant_servers_resource.go:479
- Diagnostics from ElementsAs are being discarded. If decoding
state.Serversfails, Update will compute currentServers from an empty fallback and continue, which can lead to incorrect diffs and allocation operations.
var priorServers []string
_ = state.Servers.ElementsAs(ctx, &priorServers, false)
currentServers := normalizedServersFromTenant(tenantInfo, priorServers)
internal/provider/tenant_servers_resource.go:788
- Diagnostics from ElementsAs are being discarded. If decoding
data.Serversfails, Delete will treat priorServers as empty and may skip releasing a real allocation or produce misleading diagnostics.
// Fallback to what's already in state: allotedGpus is empty on EW-IBOnly fabrics.
var priorServers []string
_ = data.Servers.ElementsAs(ctx, &priorServers, false)
internal/provider/tenant_servers_resource.go:430
- Diagnostics from ElementsAs are being discarded. If state decoding fails here, Read will silently fall back to an empty priorServers slice, which can hide state/type issues and produce incorrect reconciliation behavior.
This issue also appears in the following locations of the same file:
- line 477
- line 785
var priorServers []string
_ = data.Servers.ElementsAs(ctx, &priorServers, false)
internal/provider/client.go:1229
- This comment says a nil shared “preserv[es] the old {"serverName":...} shape”, but the previous DELETE request shape was a plain string array. Consider clarifying that nil shared omits the shared field while still using the per-server object form.
// Both ADD and DELETE send per-server objects carrying the shared flag. DELETE must carry the
// same shared value used on ADD: for a shared=true server, shared:true tells FM to skip the
// E-W (UFM/NMX-C) whole-server dealloc it also skipped on ADD - otherwise FM attempts an E-W
// dealloc that was never done and returns GPU_NOT_ALLOCATED. (shared is *bool with omitempty,
// so a nil shared is simply omitted, preserving the old {"serverName":...} shape.)
examples/decoupled/04-gpus/variables.tf:18
- This description implies an empty list is sent through as gpuIds, but the example module actually converts an empty list to null (so gpu_ids is omitted). Rewording avoids confusion, especially since the provider treats an explicit empty gpu_ids list as invalid input.
description = "Optional 1-based GPU port indices on server_names (maps to API's gpuIds). Leave empty ([]) to act on the whole server (all GPUs on server_names); set specific indices to act at the per-GPU level."
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds external GPU-port allocation (
/gpus) support for UFM / NMX-C fabrics, an inventory-syncaction, and hardens the existing whole-server tenant flow to work correctly on both UFM
fabric topologies (NS+EW and EW-IBOnly).
Related Issue
Closes GFX-4379
Type of Change
How Has This Been Tested?
Full lifecycle — tenant create →
/tenantsserver allocate →/gpusassign →/gpusunassign →/tenantsdeallocate → tenant delete — driven through Terraform, on:NS+EW UFM (
Fabric):shared=trueandshared=falsepaths.EW-IBOnly UFM (
UFM):/gpuslifecycle, and/tenantswithshared=false.Unit tests added/updated
Integration tests added/updated
[✅]Manually tested (end-to-end against two live, deployed UFM fabrics)
Versioning Impact
resources + additive read-only attributes; no existing behavior changed)
Test Cases
https://docs.google.com/spreadsheets/d/1kWo_cmHH4XR6tv_fON2EGw7_Gc1VC_YYiaKXwNtLz3w/edit?usp=sharing
Test Result