diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 7e85158d27..752d0ce1c0 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -120,7 +120,7 @@ func (dev *Devices) MutateAdmission(ctr *corev1.Container, p *corev1.Pod) (bool, count, ok := ctr.Resources.Limits[corev1.ResourceName(dev.config.ResourceName)] if !ok { if dev.config.OverwriteEnv { - ctr.Env = append(ctr.Env, corev1.EnvVar{ + device.AppendEnvIfAbsent(ctr, corev1.EnvVar{ Name: "ASCEND_VISIBLE_DEVICES", Value: "", }) diff --git a/pkg/device/ascend/device_test.go b/pkg/device/ascend/device_test.go index d06abd7ef1..0525ea57f0 100644 --- a/pkg/device/ascend/device_test.go +++ b/pkg/device/ascend/device_test.go @@ -707,6 +707,28 @@ func Test_MutateAdmission(t *testing.T) { } } +func Test_MutateAdmission_IsIdempotent(t *testing.T) { + dev := Devices{config: VNPUConfig{ + ResourceName: "huawei.com/Ascend910A", + OverwriteEnv: true, + }} + ctr := &corev1.Container{ + Env: []corev1.EnvVar{{Name: "ASCEND_VISIBLE_DEVICES", Value: "0"}}, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{}, + }, + } + pod := &corev1.Pod{} + + _, err := dev.MutateAdmission(ctr, pod) + assert.NilError(t, err) + afterFirstMutation := ctr.DeepCopy() + + _, err = dev.MutateAdmission(ctr, pod) + assert.NilError(t, err) + assert.DeepEqual(t, ctr, afterFirstMutation) +} + func Test_MutateAdmission_NilRequests(t *testing.T) { // Regression test: a pod that declares only limits (no requests block) // must not panic when MutateAdmission writes the trimmed memory request. diff --git a/pkg/device/devices.go b/pkg/device/devices.go index ab58bd173c..ef50d1fb9d 100644 --- a/pkg/device/devices.go +++ b/pkg/device/devices.go @@ -47,6 +47,18 @@ type Devices interface { Fit(devices []*DeviceUsage, request ContainerDeviceRequest, pod *corev1.Pod, nodeInfo *NodeInfo, allocated *PodDevices) (bool, map[string]ContainerDevices, string) } +// AppendEnvIfAbsent appends env only when the same literal name and value are +// not already present. Entries with the same name but a different value are +// retained to preserve the admission handler's existing overwrite behavior. +func AppendEnvIfAbsent(ctr *corev1.Container, env corev1.EnvVar) { + for _, existing := range ctr.Env { + if existing.Name == env.Name && existing.Value == env.Value && existing.ValueFrom == nil && env.ValueFrom == nil { + return + } + } + ctr.Env = append(ctr.Env, env) +} + type MigPlacement struct { Start uint32 `json:"start"` Size uint32 `json:"size"` diff --git a/pkg/device/devices_test.go b/pkg/device/devices_test.go index 269b241480..29b897a6da 100644 --- a/pkg/device/devices_test.go +++ b/pkg/device/devices_test.go @@ -38,6 +38,55 @@ func init() { inRequestDevices["NVIDIA"] = "hami.io/vgpu-devices-to-allocate" } +func TestAppendEnvIfAbsent(t *testing.T) { + wanted := corev1.EnvVar{Name: "NVIDIA_VISIBLE_DEVICES", Value: "none"} + fromFieldRef := corev1.EnvVar{ + Name: "NVIDIA_VISIBLE_DEVICES", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, + }, + } + + tests := []struct { + name string + env []corev1.EnvVar + want []corev1.EnvVar + }{ + { + name: "appends a missing variable", + want: []corev1.EnvVar{wanted}, + }, + { + name: "does not append an identical literal variable", + env: []corev1.EnvVar{wanted}, + want: []corev1.EnvVar{wanted}, + }, + { + name: "keeps the existing overwrite behavior for a different value", + env: []corev1.EnvVar{ + {Name: "NVIDIA_VISIBLE_DEVICES", Value: "all"}, + }, + want: []corev1.EnvVar{ + {Name: "NVIDIA_VISIBLE_DEVICES", Value: "all"}, + wanted, + }, + }, + { + name: "appends after a value from reference", + env: []corev1.EnvVar{fromFieldRef}, + want: []corev1.EnvVar{fromFieldRef, wanted}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctr := &corev1.Container{Env: tt.env} + AppendEnvIfAbsent(ctr, wanted) + assert.DeepEqual(t, ctr.Env, tt.want) + }) + } +} + func TestEmptyContainerDevicesCoding(t *testing.T) { cd1 := ContainerDevices{} s := EncodeContainerDevices(cd1) diff --git a/pkg/device/nvidia/device.go b/pkg/device/nvidia/device.go index 10b35dcdfd..b04103def4 100644 --- a/pkg/device/nvidia/device.go +++ b/pkg/device/nvidia/device.go @@ -340,7 +340,7 @@ func (dev *NvidiaGPUDevices) MutateAdmission(ctr *corev1.Container, p *corev1.Po } priority, ok := ctr.Resources.Limits[corev1.ResourceName(dev.config.ResourcePriority)] if ok { - ctr.Env = append(ctr.Env, corev1.EnvVar{ + device.AppendEnvIfAbsent(ctr, corev1.EnvVar{ Name: util.TaskPriority, Value: fmt.Sprint(priority.Value()), }) @@ -348,7 +348,7 @@ func (dev *NvidiaGPUDevices) MutateAdmission(ctr *corev1.Container, p *corev1.Po if dev.config.GPUCorePolicy != "" && dev.config.GPUCorePolicy != DefaultCorePolicy { - ctr.Env = append(ctr.Env, corev1.EnvVar{ + device.AppendEnvIfAbsent(ctr, corev1.EnvVar{ Name: util.CoreLimitSwitch, Value: string(dev.config.GPUCorePolicy), }) @@ -367,7 +367,7 @@ func (dev *NvidiaGPUDevices) MutateAdmission(ctr *corev1.Container, p *corev1.Po } if !hasResource && dev.config.OverwriteEnv { - ctr.Env = append(ctr.Env, corev1.EnvVar{ + device.AppendEnvIfAbsent(ctr, corev1.EnvVar{ Name: "NVIDIA_VISIBLE_DEVICES", Value: "none", }) diff --git a/pkg/device/nvidia/device_test.go b/pkg/device/nvidia/device_test.go index 09b9ff8600..b87680208a 100644 --- a/pkg/device/nvidia/device_test.go +++ b/pkg/device/nvidia/device_test.go @@ -2650,6 +2650,60 @@ func TestMutateAdmission_OverwriteEnv(t *testing.T) { assert.Assert(t, found, "expected NVIDIA_VISIBLE_DEVICES=none env") } +func TestMutateAdmissionIsIdempotent(t *testing.T) { + tests := []struct { + name string + dev *NvidiaGPUDevices + ctr *corev1.Container + }{ + { + name: "priority and core policy", + dev: &NvidiaGPUDevices{config: NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceCoreName: "nvidia.com/gpucores", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + ResourcePriority: "nvidia.com/priority", + GPUCorePolicy: ForceCorePolicy, + }}, + ctr: &corev1.Container{ + Env: []corev1.EnvVar{{Name: "EXISTING", Value: "value"}}, + Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{ + "nvidia.com/gpu": resource.MustParse("1"), + "nvidia.com/priority": resource.MustParse("5"), + }}, + }, + }, + { + name: "overwrite visible devices preserves conflicting user value", + dev: &NvidiaGPUDevices{config: NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceCoreName: "nvidia.com/gpucores", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + OverwriteEnv: true, + }}, + ctr: &corev1.Container{ + Env: []corev1.EnvVar{{Name: "NVIDIA_VISIBLE_DEVICES", Value: "all"}}, + Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{}}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + pod := &corev1.Pod{} + _, err := test.dev.MutateAdmission(test.ctr, pod) + assert.NilError(t, err) + afterFirstMutation := test.ctr.DeepCopy() + + _, err = test.dev.MutateAdmission(test.ctr, pod) + assert.NilError(t, err) + assert.DeepEqual(t, test.ctr, afterFirstMutation) + }) + } +} + func TestDefaultExclusiveCoreIfNeeded_NilContainer(t *testing.T) { dev := &NvidiaGPUDevices{config: NvidiaConfig{ResourceCountName: "nvidia.com/gpu", ResourceCoreName: "nvidia.com/gpucores"}} assert.Equal(t, dev.defaultExclusiveCoreIfNeeded(nil), false)