diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index b08c3e4a791..a6864f73f29 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -264,7 +264,7 @@ func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) { } } - dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec) + dropDisabledVolumeDevicesFields(podSpec, oldPodSpec) dropDisabledRunAsGroupField(podSpec, oldPodSpec) @@ -330,25 +330,16 @@ func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { } } -// dropDisabledVolumeDevicesAlphaFields removes disabled fields from []VolumeDevice. +// dropDisabledVolumeDevicesFields removes disabled fields from []VolumeDevice if it has not been already populated. // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a VolumeDevice -func dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec *api.PodSpec) { - if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) { +func dropDisabledVolumeDevicesFields(podSpec, oldPodSpec *api.PodSpec) { + if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeDevicesInUse(oldPodSpec) { for i := range podSpec.Containers { podSpec.Containers[i].VolumeDevices = nil } for i := range podSpec.InitContainers { podSpec.InitContainers[i].VolumeDevices = nil } - - if oldPodSpec != nil { - for i := range oldPodSpec.Containers { - oldPodSpec.Containers[i].VolumeDevices = nil - } - for i := range oldPodSpec.InitContainers { - oldPodSpec.InitContainers[i].VolumeDevices = nil - } - } } } @@ -436,3 +427,21 @@ func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool { } return false } + +// volumeDevicesInUse returns true if the pod spec is non-nil and has VolumeDevices set. +func volumeDevicesInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + for i := range podSpec.Containers { + if podSpec.Containers[i].VolumeDevices != nil { + return true + } + } + for i := range podSpec.InitContainers { + if podSpec.InitContainers[i].VolumeDevices != nil { + return true + } + } + return false +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 9105b2f0820..22e2e11ad2b 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -266,74 +266,146 @@ func TestPodConfigmaps(t *testing.T) { } func TestDropAlphaVolumeDevices(t *testing.T) { - testPod := api.Pod{ - Spec: api.PodSpec{ - RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{ - { - Name: "container1", - Image: "testimage", - VolumeDevices: []api.VolumeDevice{ - { - Name: "myvolume", - DevicePath: "/usr/test", + podWithVolumeDevices := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + Containers: []api.Container{ + { + Name: "container1", + Image: "testimage", + VolumeDevices: []api.VolumeDevice{ + { + Name: "myvolume", + DevicePath: "/usr/test", + }, + }, + }, + }, + InitContainers: []api.Container{ + { + Name: "container1", + Image: "testimage", + VolumeDevices: []api.VolumeDevice{ + { + Name: "myvolume", + DevicePath: "/usr/test", + }, + }, + }, + }, + Volumes: []api.Volume{ + { + Name: "myvolume", + VolumeSource: api.VolumeSource{ + HostPath: &api.HostPathVolumeSource{ + Path: "/dev/xvdc", + }, }, }, }, }, - InitContainers: []api.Container{ - { - Name: "container1", - Image: "testimage", - VolumeDevices: []api.VolumeDevice{ - { - Name: "myvolume", - DevicePath: "/usr/test", - }, - }, - }, - }, - Volumes: []api.Volume{ - { - Name: "myvolume", - VolumeSource: api.VolumeSource{ - HostPath: &api.HostPathVolumeSource{ - Path: "/dev/xvdc", + } + } + podWithoutVolumeDevices := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + Containers: []api.Container{ + { + Name: "container1", + Image: "testimage", + }, + }, + InitContainers: []api.Container{ + { + Name: "container1", + Image: "testimage", + }, + }, + Volumes: []api.Volume{ + { + Name: "myvolume", + VolumeSource: api.VolumeSource{ + HostPath: &api.HostPathVolumeSource{ + Path: "/dev/xvdc", + }, }, }, }, }, + } + } + + podInfo := []struct { + description string + hasVolumeDevices bool + pod func() *api.Pod + }{ + { + description: "has VolumeDevices", + hasVolumeDevices: true, + pod: podWithVolumeDevices, + }, + { + description: "does not have VolumeDevices", + hasVolumeDevices: false, + pod: podWithoutVolumeDevices, + }, + { + description: "is nil", + hasVolumeDevices: false, + pod: func() *api.Pod { return nil }, }, } - // Enable alpha feature BlockVolume - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)() + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasVolumeDevices, oldPod := oldPodInfo.hasVolumeDevices, oldPodInfo.pod() + newPodHasVolumeDevices, newPod := newPodInfo.hasVolumeDevices, newPodInfo.pod() + if newPod == nil { + continue + } - // now test dropping the fields - should not be dropped - DropDisabledFields(&testPod.Spec, nil) + t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, enabled)() - // check to make sure VolumeDevices is still present - // if featureset is set to true - if testPod.Spec.Containers[0].VolumeDevices == nil { - t.Error("VolumeDevices in Container should not have been dropped based on feature-gate") - } - if testPod.Spec.InitContainers[0].VolumeDevices == nil { - t.Error("VolumeDevices in InitContainers should not have been dropped based on feature-gate") - } + var oldPodSpec *api.PodSpec + if oldPod != nil { + oldPodSpec = &oldPod.Spec + } + DropDisabledFields(&newPod.Spec, oldPodSpec) - // Disable alpha feature BlockVolume - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)() + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { + t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod())) + } - // now test dropping the fields - DropDisabledFields(&testPod.Spec, nil) - - // check to make sure VolumeDevices is nil - // if featureset is set to false - if testPod.Spec.Containers[0].VolumeDevices != nil { - t.Error("DropDisabledFields for Containers failed") - } - if testPod.Spec.InitContainers[0].VolumeDevices != nil { - t.Error("DropDisabledFields for InitContainers failed") + switch { + case enabled || oldPodHasVolumeDevices: + // new pod should not be changed if the feature is enabled, or if the old pod had VolumeDevices + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + case newPodHasVolumeDevices: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have VolumeDevices + if !reflect.DeepEqual(newPod, podWithoutVolumeDevices()) { + t.Errorf("new pod had VolumeDevices: %v", diff.ObjectReflectDiff(newPod, podWithoutVolumeDevices())) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + } + }) + } + } } } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 57dea6207a5..bb8de13aa38 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2298,10 +2298,6 @@ func ValidateVolumeDevices(devices []core.VolumeDevice, volmounts map[string]str devicepath := sets.NewString() devicename := sets.NewString() - if devices != nil && !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("volumeDevices"), "Container volumeDevices is disabled by feature-gate")) - return allErrs - } if devices != nil { for i, dev := range devices { idxPath := fldPath.Index(i) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index ecbfcf77350..b25f955a2ea 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -4974,10 +4974,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) { return } - disabledAlphaVolDevice := []core.VolumeDevice{ - {Name: "abc", DevicePath: "/foo"}, - } - successCase := []core.VolumeDevice{ {Name: "abc", DevicePath: "/foo"}, {Name: "abc-123", DevicePath: "/usr/share/test"}, @@ -5005,8 +5001,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) { {Name: "abc-123", MountPath: "/this/path/exists"}, } - // enable BlockVolume - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)() // Success Cases: // Validate normal success cases - only PVC volumeSource if errs := ValidateVolumeDevices(successCase, GetVolumeMountMap(goodVolumeMounts), vols, field.NewPath("field")); len(errs) != 0 { @@ -5020,12 +5014,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) { t.Errorf("expected failure for %s", k) } } - - // disable BlockVolume - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)() - if errs := ValidateVolumeDevices(disabledAlphaVolDevice, GetVolumeMountMap(goodVolumeMounts), vols, field.NewPath("field")); len(errs) == 0 { - t.Errorf("expected failure: %v", errs) - } } func TestValidateProbe(t *testing.T) {