diff --git a/pkg/device/devices.go b/pkg/device/devices.go index 7fd0f89ed4..cea1061a24 100644 --- a/pkg/device/devices.go +++ b/pkg/device/devices.go @@ -291,12 +291,8 @@ func (d *DeviceUsage) DeepCopy() *DeviceUsage { } dup.MigUsage = d.MigUsage.DeepCopy() - if d.PodInfos != nil { - dup.PodInfos = make([]*PodInfo, len(d.PodInfos)) - for i, pi := range d.PodInfos { - dup.PodInfos[i] = pi.DeepCopy() - } - } + // Copy the slice while sharing its read-only PodInfo snapshots. + dup.PodInfos = slices.Clone(d.PodInfos) if d.CustomInfo != nil { dup.CustomInfo = make(map[string]any, len(d.CustomInfo)) diff --git a/pkg/device/devices_test.go b/pkg/device/devices_test.go index b8e26800d4..1107b8c8c9 100644 --- a/pkg/device/devices_test.go +++ b/pkg/device/devices_test.go @@ -1989,9 +1989,10 @@ func TestDeviceUsageDeepCopy(t *testing.T) { } if len(copy.PodInfos) > 0 { - originalNodeID := tt.original.PodInfos[0].NodeID - copy.PodInfos[0].NodeID = "mutated-node" - assert.Equal(t, tt.original.PodInfos[0].NodeID, originalNodeID) + assert.Assert(t, tt.original.PodInfos[0] == copy.PodInfos[0], "PodInfos entries should be shared with the copy") + originalEntry := tt.original.PodInfos[0] + copy.PodInfos[0] = &PodInfo{NodeID: "replacement-node"} + assert.Assert(t, tt.original.PodInfos[0] == originalEntry, "replacing a copied slice entry must not affect the original") } if copy.CustomInfo != nil { diff --git a/pkg/device/pod_test.go b/pkg/device/pod_test.go index 63824b6404..beb1c7198e 100644 --- a/pkg/device/pod_test.go +++ b/pkg/device/pod_test.go @@ -486,7 +486,7 @@ func TestUpdatePod(t *testing.T) { } } -func TestPodInfoDeepCopy(t *testing.T) { +func TestPodInfoSnapshot(t *testing.T) { tests := []struct { name string original *PodInfo @@ -523,7 +523,7 @@ func TestPodInfoDeepCopy(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - copy := tt.original.DeepCopy() + copy := tt.original.Snapshot() if tt.original == nil { if copy != nil { @@ -532,19 +532,17 @@ func TestPodInfoDeepCopy(t *testing.T) { return } - // 1. Copy must be deeply equal to original. assert.Equal(t, tt.original.NodeID, copy.NodeID) assert.Equal(t, tt.original.Devices, copy.Devices) if tt.original.Pod != nil { assert.Equal(t, tt.original.Name, copy.Name) } - // 2. Mutating the copy must not affect the original. - if copy.Pod != nil { - originalPodName := tt.original.Name - copy.Name = "mutated-pod" - assert.Equal(t, tt.original.Name, originalPodName) + if tt.original.Pod != nil { + assert.Same(t, tt.original.Pod, copy.Pod, "Pod pointer should be shared with the snapshot") } + + // Manager-owned fields remain independent of the snapshot. originalNodeID := tt.original.NodeID copy.NodeID = "mutated-node" assert.Equal(t, tt.original.NodeID, originalNodeID) @@ -647,7 +645,7 @@ func TestContainerDeviceDeepCopy(t *testing.T) { assert.False(t, exists, "original CustomInfo should not have key2") } -func TestListPodsInfoReturnsDeepCopy(t *testing.T) { +func TestListPodsInfoReturnsSnapshot(t *testing.T) { pm := NewPodManager() pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{UID: "uid-1", Name: "p", Namespace: "ns"}} pm.AddPod(pod, "node-1", PodDevices{"dev": {{{UUID: "GPU-0"}}}}) @@ -729,7 +727,7 @@ func TestGetScheduledPodsCopiesEntries(t *testing.T) { wg.Wait() } -// A caller must not be able to reach into the manager through what it hands back. +// Mutating snapshot accounting fields must not affect the manager. func TestGetScheduledPodsReturnsDetachedEntries(t *testing.T) { pm := NewPodManager() pod := &corev1.Pod{ diff --git a/pkg/device/pods.go b/pkg/device/pods.go index 1a953707d6..0dcbb1db9d 100644 --- a/pkg/device/pods.go +++ b/pkg/device/pods.go @@ -109,13 +109,14 @@ func (m *PodManager) UpdatePod(pod *corev1.Pod) { } } -// DeepCopy must include the new field. -func (p *PodInfo) DeepCopy() *PodInfo { +// Snapshot copies manager-owned state and shares the read-only Pod. +// Callers must not mutate the shared Pod. +func (p *PodInfo) Snapshot() *PodInfo { if p == nil { return nil } return &PodInfo{ - Pod: p.Pod.DeepCopy(), + Pod: p.Pod, NodeID: p.NodeID, Devices: p.Devices.DeepCopy(), InitContainerResourceReleased: p.InitContainerResourceReleased, @@ -140,8 +141,8 @@ func (m *PodManager) DelPod(pod *corev1.Pod) { } } -// GetPod returns a copy. AddPod and UpdatePod write to the stored PodInfo in -// place, so handing out the pointer would let the caller read it while the +// GetPod returns a snapshot. AddPod and UpdatePod write to the stored PodInfo +// in place, so handing out the pointer would let the caller read it while the // informer is rewriting it. func (m *PodManager) GetPod(pod *corev1.Pod) (*PodInfo, bool) { m.mutex.RLock() @@ -151,7 +152,7 @@ func (m *PodManager) GetPod(pod *corev1.Pod) (*PodInfo, bool) { if !ok { return nil, false } - return pi.DeepCopy(), true + return pi.Snapshot(), true } func (m *PodManager) TakeAndDeletePod(pod *corev1.Pod) (*PodInfo, bool) { @@ -224,7 +225,7 @@ func (m *PodManager) ListPodsInfo() []*PodInfo { pods := make([]*PodInfo, 0, len(m.pods)) for _, pod := range m.pods { - pods = append(pods, pod.DeepCopy()) + pods = append(pods, pod.Snapshot()) klog.V(5).InfoS("Pod info", "pod", klog.KRef(pod.Namespace, pod.Name), "nodeID", pod.NodeID, @@ -301,7 +302,7 @@ func (m *PodManager) GetScheduledPods() (map[k8stypes.UID]*PodInfo, error) { // over Devices after this returns, by which point the read lock is gone. podsCopy := make(map[k8stypes.UID]*PodInfo, podCount) for uid, pi := range m.pods { - podsCopy[uid] = pi.DeepCopy() + podsCopy[uid] = pi.Snapshot() } return podsCopy, nil } diff --git a/pkg/device/pods_bench_test.go b/pkg/device/pods_bench_test.go new file mode 100644 index 0000000000..a0287b32b9 --- /dev/null +++ b/pkg/device/pods_bench_test.go @@ -0,0 +1,103 @@ +/* +Copyright 2026 The HAMi Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package device + +import ( + "fmt" + "io" + "os" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" +) + +// newBenchmarkCachedPod includes nested fields to exercise Pod copying costs. +func newBenchmarkCachedPod(index int) *corev1.Pod { + envs := make([]corev1.EnvVar, 0, 20) + for i := range 20 { + envs = append(envs, corev1.EnvVar{Name: fmt.Sprintf("ENV_%d", i), Value: fmt.Sprintf("value-%d-%d", index, i)}) + } + mounts := make([]corev1.VolumeMount, 0, 6) + for i := range 6 { + mounts = append(mounts, corev1.VolumeMount{Name: fmt.Sprintf("vol-%d", i), MountPath: fmt.Sprintf("/mnt/%d", i)}) + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("cached-pod-%d", index), + Namespace: "default", + UID: k8stypes.UID(fmt.Sprintf("cached-uid-%d", index)), + Labels: map[string]string{"app": "train", "release": "v1", "team": "ml"}, + Annotations: map[string]string{ + "hami.io/vgpu-devices-allocated": "GPU-0,NVIDIA,1024,10:;", + }, + }, + Spec: corev1.PodSpec{ + NodeName: "node-0", + Containers: []corev1.Container{{Name: "main", Image: "example.com/train:v1", Env: envs, VolumeMounts: mounts}}, + Volumes: []corev1.Volume{{Name: "vol-0"}, {Name: "vol-1"}}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + ContainerStatuses: []corev1.ContainerStatus{{Name: "main", Image: "example.com/train:v1", Ready: true}}, + }, + } +} + +// newBenchmarkPodManager returns a manager holding podCount cached pods, each +// with a single-device allocation. +func newBenchmarkPodManager(podCount int) *PodManager { + manager := NewPodManager() + for i := range podCount { + manager.AddPod(newBenchmarkCachedPod(i), "node-0", PodDevices{ + "NVIDIA": PodSingleDevice{ + ContainerDevices{{UUID: "GPU-0", Type: "NVIDIA", Usedmem: 1024, Usedcores: 10}}, + }, + }) + } + return manager +} + +// BenchmarkListPodsInfo measures the cache snapshot taken once per Filter +// across different cached Pod counts. AddPod logs at Info level, so klog is +// silenced for the whole benchmark. SetOutput alone is not enough: klog +// defaults to logtostderr, which writes to stderr directly and never consults +// the configured output. +func BenchmarkListPodsInfo(b *testing.B) { + klog.LogToStderr(false) + klog.SetOutput(io.Discard) + b.Cleanup(func() { + klog.SetOutput(os.Stderr) + klog.LogToStderr(true) + }) + + for _, podCount := range []int{100, 1000, 5000} { + b.Run(fmt.Sprintf("pods=%d", podCount), func(b *testing.B) { + manager := newBenchmarkPodManager(podCount) + + b.ReportAllocs() + for b.Loop() { + if got := len(manager.ListPodsInfo()); got != podCount { + b.Fatalf("ListPodsInfo returned %d pods, want %d", got, podCount) + } + } + }) + } +} diff --git a/pkg/scheduler/score_bench_test.go b/pkg/scheduler/score_bench_test.go index 767b0c334e..ca15ae292c 100644 --- a/pkg/scheduler/score_bench_test.go +++ b/pkg/scheduler/score_bench_test.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" "github.com/Project-HAMi/HAMi/pkg/device" @@ -52,13 +53,18 @@ const ( ) // quietKlog silences klog for the duration of a benchmark. Fit logs once per -// container request at Info level, so writing those lines to stderr would -// otherwise dominate both the timing and the allocation counts. Uses the same -// SetOutput and restore pattern as routes/route_test.go. +// container request at Info level, so writing those lines out would otherwise +// dominate both the timing and the allocation counts. SetOutput alone is not +// enough: klog defaults to logtostderr, which writes to stderr directly and +// never consults the configured output. func quietKlog(b *testing.B) { b.Helper() + klog.LogToStderr(false) klog.SetOutput(io.Discard) - b.Cleanup(func() { klog.SetOutput(os.Stderr) }) + b.Cleanup(func() { + klog.SetOutput(os.Stderr) + klog.LogToStderr(true) + }) } // newBenchmarkNodes builds nodeCount nodes carrying gpusPerNode idle NVIDIA @@ -204,3 +210,85 @@ func BenchmarkScoreNode(b *testing.B) { }) } } + +// benchTenantPod includes nested fields to exercise Pod copying costs. +func benchTenantPod(index int) *corev1.Pod { + envs := make([]corev1.EnvVar, 0, 20) + for i := range 20 { + envs = append(envs, corev1.EnvVar{Name: fmt.Sprintf("ENV_%d", i), Value: fmt.Sprintf("value-%d-%d", index, i)}) + } + mounts := make([]corev1.VolumeMount, 0, 6) + for i := range 6 { + mounts = append(mounts, corev1.VolumeMount{Name: fmt.Sprintf("vol-%d", i), MountPath: fmt.Sprintf("/mnt/%d", i)}) + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("tenant-pod-%d", index), + Namespace: "default", + UID: k8stypes.UID(fmt.Sprintf("tenant-uid-%d", index)), + Labels: map[string]string{"app": "train", "release": "v1", "team": "ml"}, + Annotations: map[string]string{util.AssignedNodeAnnotations: "node-0"}, + }, + Spec: corev1.PodSpec{ + NodeName: "node-0", + Containers: []corev1.Container{{Name: "main", Image: "example.com/train:v1", Env: envs, VolumeMounts: mounts}}, + Volumes: []corev1.Volume{{Name: "vol-0"}, {Name: "vol-1"}}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + ContainerStatuses: []corev1.ContainerStatus{{Name: "main", Image: "example.com/train:v1", Ready: true}}, + }, + } +} + +// occupyBenchmarkNode marks every device on the node as hosting podsPerGPU +// tenants, the way getNodesUsage does when it replays cached allocations. +func occupyBenchmarkNode(node *NodeUsage, podsPerGPU int) { + index := 0 + for _, deviceList := range node.Devices.DeviceLists { + dev := deviceList.Device + for range podsPerGPU { + dev.PodInfos = append(dev.PodInfos, &device.PodInfo{Pod: benchTenantPod(index), NodeID: node.NodeInfo.ID}) + dev.Used++ + dev.Usedmem += benchMemreq + dev.Usedcores += benchCoresreq + index++ + } + } +} + +// BenchmarkScoreNodeOccupied measures one node whose devices already host +// other pods. Their PodInfos ride along in the NodeUsage copy scoreNode +// takes, so the spread across these cases is what resident pods cost per +// candidate node. +func BenchmarkScoreNodeOccupied(b *testing.B) { + quietKlog(b) + + scheduler := &Scheduler{} + weights := util.DefaultDeviceScoringWeights() + nodePolicy := util.NodeSchedulerPolicyBinpack.String() + pod := newBenchmarkPod(0) + requests := newBenchmarkRequests(0) + + for _, podsPerGPU := range []int{0, 2, 4} { + b.Run(fmt.Sprintf("podsPerGPU=%d", podsPerGPU), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + b.StopTimer() + nodes := *newBenchmarkNodes(1, 8) + node := nodes["node-0"] + occupyBenchmarkNode(node, podsPerGPU) + b.StartTimer() + + result := scheduler.scoreNode("node-0", node, requests, pod, nodePolicy, weights) + if result.err != nil { + b.Fatalf("scoreNode returned an error: %v", result.err) + } + if result.score == nil { + b.Fatalf("scoreNode did not fit the pod: %s", result.reason) + } + } + }) + } +}