diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go index 21ff292b90..22a51331ff 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go @@ -250,7 +250,7 @@ func TestCDIAllocateResponse(t *testing.T) { } for i := range testCases { - tc := testCases[i] + tc := &testCases[i] t.Run(tc.description, func(t *testing.T) { deviceListStrategies, _ := v1.NewDeviceListStrategies(tc.deviceListStrategies) plugin := NvidiaDevicePlugin{ diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go b/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go index 35feccd562..51cec0bb92 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go @@ -41,6 +41,8 @@ import ( "k8s.io/klog/v2" spec "github.com/NVIDIA/k8s-device-plugin/api/config/v1" + "google.golang.org/protobuf/proto" + ) type deviceMapBuilder struct { @@ -340,10 +342,17 @@ func updateDeviceMapWithReplicas(replicatedResources *spec.ReplicatedResources, for _, id := range ids { for i := 0; i < r.Replicas; i++ { annotatedID := string(NewAnnotatedID(id, i)) - replicatedDevice := *(oDevices[r.Name][id]) + orig := oDevices[r.Name][id] + replicatedDevice := &Device{ + Paths: orig.Paths, + Index: orig.Index, + TotalMemory: orig.TotalMemory, + ComputeCapability: orig.ComputeCapability, + Replicas: r.Replicas, + } + proto.Merge(&replicatedDevice.Device, &orig.Device) replicatedDevice.ID = annotatedID - replicatedDevice.Replicas = r.Replicas - devices.insert(name, &replicatedDevice) + devices.insert(name, replicatedDevice) } } } diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go b/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go index 1b4b04e7eb..7395c28ca3 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go @@ -40,6 +40,7 @@ import ( spec "github.com/NVIDIA/k8s-device-plugin/api/config/v1" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" kubeletdevicepluginv1beta1 "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1" ) @@ -127,8 +128,22 @@ func TestDeviceMapInsert(t *testing.T) { } func TestUpdateDeviceMapWithReplicas(t *testing.T) { - device0 := Device{Device: kubeletdevicepluginv1beta1.Device{ID: "0"}, Index: "0"} - device1 := Device{Device: kubeletdevicepluginv1beta1.Device{ID: "1"}} + device0 := Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "0", Health: "Healthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 0}}}}, + Index: "0", + Paths: []string{"/dev/nvidia0"}, + TotalMemory: 1024, + ComputeCapability: "8.0", + Replicas: 1, + } + device1 := Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "1", Health: "Unhealthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 1}}}}, + Index: "1", + Paths: []string{"/dev/nvidia1"}, + TotalMemory: 2048, + ComputeCapability: "8.6", + Replicas: 1, + } device2 := Device{Device: kubeletdevicepluginv1beta1.Device{ID: "2"}} device3 := Device{Device: kubeletdevicepluginv1beta1.Device{ID: "3"}} @@ -179,10 +194,38 @@ func TestUpdateDeviceMapWithReplicas(t *testing.T) { }, expectedDeviceMap: DeviceMap{ "replicated-resource1": Devices{ - "0::0": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "0::0"}, Index: "0", Replicas: 2}, - "0::1": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "0::1"}, Index: "0", Replicas: 2}, - "1::0": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "1::0"}, Replicas: 2}, - "1::1": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "1::1"}, Replicas: 2}, + "0::0": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "0::0", Health: "Healthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 0}}}}, + Index: "0", + Paths: []string{"/dev/nvidia0"}, + TotalMemory: 1024, + ComputeCapability: "8.0", + Replicas: 2, + }, + "0::1": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "0::1", Health: "Healthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 0}}}}, + Index: "0", + Paths: []string{"/dev/nvidia0"}, + TotalMemory: 1024, + ComputeCapability: "8.0", + Replicas: 2, + }, + "1::0": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "1::0", Health: "Unhealthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 1}}}}, + Index: "1", + Paths: []string{"/dev/nvidia1"}, + TotalMemory: 2048, + ComputeCapability: "8.6", + Replicas: 2, + }, + "1::1": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "1::1", Health: "Unhealthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 1}}}}, + Index: "1", + Paths: []string{"/dev/nvidia1"}, + TotalMemory: 2048, + ComputeCapability: "8.6", + Replicas: 2, + }, }, "resource2": Devices{ "2::0": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "2::0"}, Replicas: 1}, @@ -220,8 +263,22 @@ func TestUpdateDeviceMapWithReplicas(t *testing.T) { }, expectedDeviceMap: DeviceMap{ "replicated-resource1": Devices{ - "0::0": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "0::0"}, Index: "0", Replicas: 2}, - "0::1": &Device{Device: kubeletdevicepluginv1beta1.Device{ID: "0::1"}, Index: "0", Replicas: 2}, + "0::0": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "0::0", Health: "Healthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 0}}}}, + Index: "0", + Paths: []string{"/dev/nvidia0"}, + TotalMemory: 1024, + ComputeCapability: "8.0", + Replicas: 2, + }, + "0::1": &Device{ + Device: kubeletdevicepluginv1beta1.Device{ID: "0::1", Health: "Healthy", Topology: &kubeletdevicepluginv1beta1.TopologyInfo{Nodes: []*kubeletdevicepluginv1beta1.NUMANode{{ID: 0}}}}, + Index: "0", + Paths: []string{"/dev/nvidia0"}, + TotalMemory: 1024, + ComputeCapability: "8.0", + Replicas: 2, + }, }, "resource1": Devices{ "1": &device1, @@ -233,7 +290,21 @@ func TestUpdateDeviceMapWithReplicas(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { devices, _ := updateDeviceMapWithReplicas(&tc.config.Config.Sharing.TimeSlicing, tc.devices) - require.EqualValues(t, tc.expectedDeviceMap, devices) + require.Equal(t, len(tc.expectedDeviceMap), len(devices)) + for resourceName, expectedDevices := range tc.expectedDeviceMap { + actualDevices := devices[resourceName] + require.Equal(t, len(expectedDevices), len(actualDevices)) + for id, expectedDevice := range expectedDevices { + actualDevice := actualDevices[id] + require.NotNil(t, actualDevice) + require.Equal(t, expectedDevice.Index, actualDevice.Index) + require.Equal(t, expectedDevice.Paths, actualDevice.Paths) + require.Equal(t, expectedDevice.TotalMemory, actualDevice.TotalMemory) + require.Equal(t, expectedDevice.ComputeCapability, actualDevice.ComputeCapability) + require.Equal(t, expectedDevice.Replicas, actualDevice.Replicas) + require.True(t, proto.Equal(&expectedDevice.Device, &actualDevice.Device), "proto fields should match") + } + } }) } } diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go b/pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go index 2c9c3b1447..aedd34e6f6 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go @@ -253,7 +253,7 @@ func (ds Devices) AlignedAllocationSupported() bool { } // AlignedAllocationSupported checks whether the device supports an aligned allocation -func (d Device) AlignedAllocationSupported() bool { +func (d *Device) AlignedAllocationSupported() bool { if d.IsMigDevice() { return false } @@ -268,12 +268,12 @@ func (d Device) AlignedAllocationSupported() bool { } // IsMigDevice returns checks whether d is a MIG device or not. -func (d Device) IsMigDevice() bool { +func (d *Device) IsMigDevice() bool { return strings.Contains(d.Index, ":") } // GetUUID returns the UUID for the device from the annotated ID. -func (d Device) GetUUID() string { +func (d *Device) GetUUID() string { return AnnotatedID(d.ID).GetID() } diff --git a/pkg/device/nvidia/calculate_score.go b/pkg/device/nvidia/calculate_score.go index 54a874e196..63d8b97309 100644 --- a/pkg/device/nvidia/calculate_score.go +++ b/pkg/device/nvidia/calculate_score.go @@ -22,6 +22,7 @@ import ( "github.com/NVIDIA/go-nvlib/pkg/nvlib/device" "github.com/NVIDIA/go-nvml/pkg/nvml" + "k8s.io/klog/v2" ) // Device represents a GPU device as reported by NVML, including all of its @@ -210,14 +211,20 @@ func calculateGPUPairScore(gpu0 *Device, gpu1 *Device) int { return 0 } - if len(gpu0.Links[gpu1.Index]) != len(gpu1.Links[gpu0.Index]) { - err := fmt.Errorf("internal error in bestEffort GPU allocator: all P2PLinks between 2 GPUs should be bidirectional") - panic(err) + links0 := gpu0.Links[gpu1.Index] + links1 := gpu1.Links[gpu0.Index] + + if len(links0) != len(links1) { + klog.Warningf("Asymmetric GPU topology detected: %s->%s=%d links, %s->%s=%d links. This may be caused by hardware issues, driver bugs, or inconsistent reporting. Evaluating common subset of links.", + gpu0.UUID, gpu1.UUID, len(links0), + gpu1.UUID, gpu0.UUID, len(links1)) } score := 0 + common := min(len(links0), len(links1)) - for _, link := range gpu0.Links[gpu1.Index] { + for i := 0; i < common; i++ { + link := links0[i] switch link.Type { case P2PLinkCrossCPU: score += 10 diff --git a/pkg/device/nvidia/calculate_score_test.go b/pkg/device/nvidia/calculate_score_test.go index 115b6f4d1e..d54ea8a825 100644 --- a/pkg/device/nvidia/calculate_score_test.go +++ b/pkg/device/nvidia/calculate_score_test.go @@ -108,6 +108,105 @@ func Test_calculateGPUScore(t *testing.T) { }, }, }, + { + name: "asymmetric nvlink test gracefully handles shorter slice", + args: []*Device{ + { + Index: 0, + nvlibDevice: nvlibDevice{ + UUID: "gpu0", + }, + Links: map[int][]P2PLink{ + 1: {{Type: SingleNVLINKLink}}, + }, + }, + { + Index: 1, + nvlibDevice: nvlibDevice{ + UUID: "gpu1", + }, + Links: map[int][]P2PLink{ + 0: {{Type: SingleNVLINKLink}, {Type: SingleNVLINKLink}}, // Extra links! + }, + }, + }, + want: ListDeviceScore{ + { + UUID: "gpu0", + Score: map[string]int{"gpu1": 100}, // Score for 1 SingleNVLINKLink + }, + { + UUID: "gpu1", + Score: map[string]int{"gpu0": 100}, // Evaluates minimum common subset (1 link) + }, + }, + }, + { + name: "one side zero links gracefully handles", + args: []*Device{ + { + Index: 0, + nvlibDevice: nvlibDevice{ + UUID: "gpu0", + }, + Links: map[int][]P2PLink{ + 1: {}, // Empty slice + }, + }, + { + Index: 1, + nvlibDevice: nvlibDevice{ + UUID: "gpu1", + }, + Links: map[int][]P2PLink{ + 0: {{Type: SingleNVLINKLink}}, + }, + }, + }, + want: ListDeviceScore{ + { + UUID: "gpu0", + Score: map[string]int{"gpu1": 0}, + }, + { + UUID: "gpu1", + Score: map[string]int{"gpu0": 0}, + }, + }, + }, + { + name: "nil slices gracefully handles missing index", + args: []*Device{ + { + Index: 0, + nvlibDevice: nvlibDevice{ + UUID: "gpu0", + }, + Links: map[int][]P2PLink{ + 1: nil, // Nil slice + }, + }, + { + Index: 1, + nvlibDevice: nvlibDevice{ + UUID: "gpu1", + }, + Links: map[int][]P2PLink{ + // gpu0 index 0 is missing entirely from the map + }, + }, + }, + want: ListDeviceScore{ + { + UUID: "gpu0", + Score: map[string]int{"gpu1": 0}, + }, + { + UUID: "gpu1", + Score: map[string]int{"gpu0": 0}, + }, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) {