diff --git a/src/compute-plane-services/nvca/pkg/types/resource_types.go b/src/compute-plane-services/nvca/pkg/types/resource_types.go index 7a9b65162..af8571e19 100644 --- a/src/compute-plane-services/nvca/pkg/types/resource_types.go +++ b/src/compute-plane-services/nvca/pkg/types/resource_types.go @@ -401,6 +401,33 @@ func roundUpCPUToInteger(cpu resource.Quantity) uint64 { return uint64(cpu.Value()) } +// instanceTypePrecedes reports whether a must be considered before b when building +// a registration, ordering by descending capacity: GPU count first, then the +// non-GPU resources, with FullName as a final tie-break. +// +// This must be a strict ordering that totally orders any two instance types. Ties +// are resolved by whichever entry is considered first, and that entry supplies the +// resource profile published for a name it collides on. The input arrives in node +// listing order, which is not stable, so a comparator that leaves equal-capacity +// entries unordered would let the published profile change from one reconcile to +// the next. +func instanceTypePrecedes(a, b InstanceType) bool { + if a.GPUCount != b.GPUCount { + return a.GPUCount > b.GPUCount + } + for _, c := range []int{ + a.CPU.Cmp(b.CPU), + a.SystemMemory.Cmp(b.SystemMemory), + a.Storage.Cmp(b.Storage), + a.GPUMemoryPerGPU.Cmp(b.GPUMemoryPerGPU), + } { + if c != 0 { + return c > 0 + } + } + return a.FullName < b.FullName +} + func (g BackendGPU) toDynamicRegistration(allowMultiNodeWorkloads bool, infraOverhead corev1.ResourceList) (rg RegistrationGPU) { rg.Name = string(g.Name) // use the Capacity AS IS from BackendGPU for @@ -421,8 +448,15 @@ func (g BackendGPU) toDynamicRegistration(allowMultiNodeWorkloads bool, infraOve // instance type. This doesn't fix the cross-cluster instance type compatibility issue // (what if "GPU.NCP.A100-6_1x" looks different between two target-able clusters), but does fix the problem // on individual clusters with intentionally different machine sizes for the same GPU. + // + // The same applies to nodes carrying different products that normalize to one GPUName + // (ex. A100 SXM4 and A100 PCIe): they collapse to a single registered instance type, + // derived from the largest node because of the ordering below. Until instance names can + // distinguish them, the smaller node is not separately addressable. That is preferable to + // registering both under one name, which makes a single-instance request create one + // instance per duplicate. sort.Slice(g.InstanceTypes, func(i, j int) bool { - return g.InstanceTypes[i].GPUCount >= g.InstanceTypes[j].GPUCount + return instanceTypePrecedes(g.InstanceTypes[i], g.InstanceTypes[j]) }) instSetByIDStr := map[string]RegistrationInstanceType{} @@ -434,9 +468,18 @@ func (g BackendGPU) toDynamicRegistration(allowMultiNodeWorkloads bool, infraOve // Calculate the per-GPU memory count for instance multiples. for i := uint64(1); i < it.GPUCount; i *= 2 { - // Use the full GPU name to dedup entries, since some features - // are captured in the full name only. - instIDStr := fmt.Sprintf("%s-%dx", it.FullName, i) + // Dedup on the published instance type name, which is the only handle + // downstream services have on an instance type. Two registered entries + // sharing a name are indistinguishable to them: a request naming it + // resolves to every match and is dispatched once per match, so a request + // for one instance creates one instance per duplicate. + // + // FullName is not a safe key here because several distinct products + // normalize to the same GPUName, and therefore to the same instance name: + // board SKUs of one model (NVIDIA-A100-SXM4-80GB vs NVIDIA-A100-80GB-PCIe), + // capacity variants (NVIDIA-A100-SXM4-40GB vs -80GB), and time-sliced + // nodes (the "-SHARED" suffix is dropped by ParseGPUName). + instIDStr := it.Name.WithMultiplier(i) if _, ok := instSetByIDStr[instIDStr]; ok { continue @@ -451,7 +494,7 @@ func (g BackendGPU) toDynamicRegistration(allowMultiNodeWorkloads bool, infraOve rg.InstanceTypes = append(rg.InstanceTypes, instType) } - lastInstIDStr := fmt.Sprintf("%s-%dx", it.FullName, it.GPUCount) + lastInstIDStr := it.Name.WithMultiplier(it.GPUCount) if _, ok := instSetByIDStr[lastInstIDStr]; ok { continue } diff --git a/src/compute-plane-services/nvca/pkg/types/resource_types_test.go b/src/compute-plane-services/nvca/pkg/types/resource_types_test.go index 187f54045..b330a6f4d 100644 --- a/src/compute-plane-services/nvca/pkg/types/resource_types_test.go +++ b/src/compute-plane-services/nvca/pkg/types/resource_types_test.go @@ -21,6 +21,8 @@ import ( "context" "encoding/json" "fmt" + "maps" + "slices" "strings" "sync/atomic" "testing" @@ -400,22 +402,9 @@ func TestToRegistration(t *testing.T) { Storage: "85Gi", NodeType: RegistrationInstanceTypeNodeTypeSingle, }, - { - Name: "ON-PREM.GPU.A100_1x", - Value: "ON-PREM.GPU.A100", - Description: "Desc", - Default: false, - CPUCores: 2, - CPU: "2", - SystemMemory: "2Gi", - GPUCount: 1, - GPUMemory: "80Gi", - CPUArch: "amd64", - OS: "Linux", - DriverVersion: "525.2.02", - Storage: "512Gi", - NodeType: RegistrationInstanceTypeNodeTypeSingle, - }, + // The 80GB node also yields an "ON-PREM.GPU.A100_1x", since ParseGPUName + // normalizes both capacities to "A100". Only one entry may be registered + // under a given name, so it is dropped in favor of the larger node's. { Name: "ON-PREM.GPU.A100_2x", Value: "ON-PREM.GPU.A100", @@ -593,6 +582,111 @@ func TestToRegistration(t *testing.T) { assert.Equal(t, expRegistrationGPUs, inBackendGPUs.ToRegistration(true, corev1.ResourceList{})) } +// Two board SKUs of one GPU model normalize to the same GPUName, and so to the same +// instance type name. Registering both makes a single-instance request resolve to two +// destinations upstream and create two instances, so only one may be published. +func TestToRegistrationDedupsInstanceNamesAcrossBoardSKUs(t *testing.T) { + newIT := func(fullName string, gpuCount uint64, cpu, mem string) InstanceType { + return InstanceType{ + Name: "NCP.GPU.A100", + FullName: fullName, + Description: fullName, + CPU: resource.MustParse(cpu), + SystemMemory: resource.MustParse(mem), + GPUCount: gpuCount, + GPUMemoryPerGPU: resource.MustParse("81920Mi"), + CPUArch: "amd64", + OS: "linux", + DriverVersion: "570.211.01", + Storage: resource.MustParse("512Gi"), + } + } + + // An 8-GPU DGX (SXM4) and a 2-GPU VMware host (PCIe), both reporting A100. + in := BackendGPUs{ + { + Name: "A100", + InstanceTypes: []InstanceType{ + newIT("NVIDIA-A100-SXM4-80GB", 8, "256", "2015Gi"), + newIT("NVIDIA-A100-80GB-PCIe", 2, "48", "62Gi"), + }, + }, + } + + got := in.ToRegistration(false, corev1.ResourceList{}) + require.Len(t, got, 1) + + seen := map[string]int{} + for _, it := range got[0].InstanceTypes { + seen[it.Name]++ + } + for name, count := range seen { + assert.Equalf(t, 1, count, "instance type %q registered %d times", name, count) + } + + // The PCIe node contributes no new names: 1x/2x collide with the DGX subdivisions. + assert.Equal(t, + []string{"NCP.GPU.A100_1x", "NCP.GPU.A100_2x", "NCP.GPU.A100_4x", "NCP.GPU.A100_8x"}, + slices.Sorted(maps.Keys(seen)), + ) + + // The surviving entries are the DGX-derived ones, since the larger node sorts first. + for _, it := range got[0].InstanceTypes { + assert.Equal(t, "NVIDIA-A100-SXM4-80GB", it.Description) + } +} + +// Nodes arrive in listing order, which is not stable. When two SKUs of equal GPU count +// collide on a published name, the winner supplies that name's resource profile, so the +// choice must not depend on the order they happen to be listed in. +func TestToRegistrationCollisionWinnerIsIndependentOfInputOrder(t *testing.T) { + newIT := func(fullName, cpu, mem, storage string) InstanceType { + return InstanceType{ + Name: "NCP.GPU.A100", + FullName: fullName, + Description: fullName, + CPU: resource.MustParse(cpu), + SystemMemory: resource.MustParse(mem), + GPUCount: 2, + GPUMemoryPerGPU: resource.MustParse("81920Mi"), + CPUArch: "amd64", + OS: "linux", + DriverVersion: "570.211.01", + Storage: resource.MustParse(storage), + } + } + + // Same GPU count, different machines. Only the resource profile distinguishes them. + sxm := newIT("NVIDIA-A100-SXM4-80GB", "64", "503Gi", "424Gi") + pcie := newIT("NVIDIA-A100-80GB-PCIe", "48", "62Gi", "973Gi") + + profileOf := func(its []InstanceType) []RegistrationInstanceType { + got := BackendGPUs{{Name: "A100", InstanceTypes: its}}.ToRegistration(false, corev1.ResourceList{}) + require.Len(t, got, 1) + + // Both SKUs subdivide to the same two names, so the collision keeps one entry + // each. Asserted here so the comparisons below cannot hold vacuously on an + // empty registration. + names := make([]string, 0, len(got[0].InstanceTypes)) + for _, it := range got[0].InstanceTypes { + names = append(names, it.Name) + } + require.Equal(t, []string{"NCP.GPU.A100_1x", "NCP.GPU.A100_2x"}, names) + + return got[0].InstanceTypes + } + + forward := profileOf([]InstanceType{sxm, pcie}) + reversed := profileOf([]InstanceType{pcie, sxm}) + + assert.Equal(t, forward, reversed, "published registration must not depend on node listing order") + + // The larger machine wins the collision, consistent with the descending ordering. + for _, it := range forward { + assert.Equal(t, "NVIDIA-A100-SXM4-80GB", it.Description) + } +} + func Test_calcFractionCPU(t *testing.T) { type spec struct { q resource.Quantity