diff --git a/pkg/api/pod/BUILD b/pkg/api/pod/BUILD index 00c07c3b549..4bdc4c39d56 100644 --- a/pkg/api/pod/BUILD +++ b/pkg/api/pod/BUILD @@ -38,6 +38,7 @@ go_test( deps = [ "//pkg/apis/core:go_default_library", "//pkg/features:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 04148571c12..b08c3e4a791 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -242,19 +242,12 @@ func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) { podSpec.PriorityClassName = "" } - if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { + if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) && !emptyDirSizeLimitInUse(oldPodSpec) { for i := range podSpec.Volumes { if podSpec.Volumes[i].EmptyDir != nil { podSpec.Volumes[i].EmptyDir.SizeLimit = nil } } - if oldPodSpec != nil { - for i := range oldPodSpec.Volumes { - if oldPodSpec.Volumes[i].EmptyDir != nil { - oldPodSpec.Volumes[i].EmptyDir.SizeLimit = nil - } - } - } } if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) && !subpathInUse(oldPodSpec) { @@ -428,3 +421,18 @@ func podPriorityInUse(podSpec *api.PodSpec) bool { } return false } + +// emptyDirSizeLimitInUse returns true if any pod's EptyDir volumes use SizeLimit. +func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + for i := range podSpec.Volumes { + if podSpec.Volumes[i].EmptyDir != nil { + if podSpec.Volumes[i].EmptyDir.SizeLimit != nil { + return true + } + } + } + return false +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index a09513f96e8..9105b2f0820 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -22,6 +22,7 @@ import ( "strings" "testing" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" @@ -728,3 +729,113 @@ func TestDropPodPriority(t *testing.T) { } } } + +func TestDropEmptyDirSizeLimit(t *testing.T) { + sizeLimit := resource.MustParse("1Gi") + podWithEmptyDirSizeLimit := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + Volumes: []api.Volume{ + { + Name: "a", + VolumeSource: api.VolumeSource{ + EmptyDir: &api.EmptyDirVolumeSource{ + Medium: "memory", + SizeLimit: &sizeLimit, + }, + }, + }, + }, + }, + } + } + podWithoutEmptyDirSizeLimit := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + Volumes: []api.Volume{ + { + Name: "a", + VolumeSource: api.VolumeSource{ + EmptyDir: &api.EmptyDirVolumeSource{ + Medium: "memory", + }, + }, + }, + }, + }, + } + } + + podInfo := []struct { + description string + hasEmptyDirSizeLimit bool + pod func() *api.Pod + }{ + { + description: "has EmptyDir Size Limit", + hasEmptyDirSizeLimit: true, + pod: podWithEmptyDirSizeLimit, + }, + { + description: "does not have EmptyDir Size Limit", + hasEmptyDirSizeLimit: false, + pod: podWithoutEmptyDirSizeLimit, + }, + { + description: "is nil", + hasEmptyDirSizeLimit: false, + pod: func() *api.Pod { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasEmptyDirSizeLimit, oldPod := oldPodInfo.hasEmptyDirSizeLimit, oldPodInfo.pod() + newPodHasEmptyDirSizeLimit, newPod := newPodInfo.hasEmptyDirSizeLimit, newPodInfo.pod() + if newPod == nil { + continue + } + + 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.LocalStorageCapacityIsolation, enabled)() + + var oldPodSpec *api.PodSpec + if oldPod != nil { + oldPodSpec = &oldPod.Spec + } + DropDisabledFields(&newPod.Spec, oldPodSpec) + + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { + t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod())) + } + + switch { + case enabled || oldPodHasEmptyDirSizeLimit: + // new pod should not be changed if the feature is enabled, or if the old pod had EmptyDir SizeLimit + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod())) + } + case newPodHasEmptyDirSizeLimit: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have EmptyDir SizeLimit + if !reflect.DeepEqual(newPod, podWithoutEmptyDirSizeLimit()) { + t.Errorf("new pod had EmptyDir SizeLimit: %v", diff.ObjectReflectDiff(newPod, podWithoutEmptyDirSizeLimit())) + } + 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 43dc6562790..57dea6207a5 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -412,14 +412,8 @@ func validateVolumeSource(source *core.VolumeSource, fldPath *field.Path, volNam allErrs := field.ErrorList{} if source.EmptyDir != nil { numVolumes++ - if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { - if source.EmptyDir.SizeLimit != nil && source.EmptyDir.SizeLimit.Cmp(resource.Quantity{}) != 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "SizeLimit field disabled by feature-gate for EmptyDir volumes")) - } - } else { - if source.EmptyDir.SizeLimit != nil && source.EmptyDir.SizeLimit.Cmp(resource.Quantity{}) < 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "SizeLimit field must be a valid resource quantity")) - } + if source.EmptyDir.SizeLimit != nil && source.EmptyDir.SizeLimit.Cmp(resource.Quantity{}) < 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "SizeLimit field must be a valid resource quantity")) } if !utilfeature.DefaultFeatureGate.Enabled(features.HugePages) && source.EmptyDir.Medium == core.StorageMediumHugePages { allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("medium"), "HugePages medium is disabled by feature-gate for EmptyDir volumes")) @@ -2119,8 +2113,6 @@ func validateContainerResourceFieldSelector(fs *core.ResourceFieldSelector, expr allErrs = append(allErrs, field.Required(fldPath.Child("resource"), "")) } else if !expressions.Has(fs.Resource) { allErrs = append(allErrs, field.NotSupported(fldPath.Child("resource"), fs.Resource, expressions.List())) - } else if fsResourceIsEphemeralStorage(fs.Resource) && !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { - allErrs = append(allErrs, field.Forbidden(fldPath, "Containers' ephemeral storage requests/limits disabled by feature-gate for Downward API")) } allErrs = append(allErrs, validateContainerResourceDivisor(fs.Resource, fs.Divisor, fldPath)...) return allErrs @@ -4451,9 +4443,7 @@ func isLocalStorageResource(name string) bool { // Refer to docs/design/resources.md for more details. func ValidateResourceQuotaResourceName(value string, fldPath *field.Path) field.ErrorList { allErrs := validateResourceName(value, fldPath) - if isLocalStorageResource(value) && !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { - return append(allErrs, field.Forbidden(fldPath, "ResourceEphemeralStorage field disabled by feature-gate for ResourceQuota")) - } + if len(strings.Split(value, "/")) == 1 { if !helper.IsStandardQuotaResourceName(value) { return append(allErrs, field.Invalid(fldPath, value, isInvalidQuotaResource)) @@ -4484,10 +4474,6 @@ func validateLimitRangeTypeName(value string, fldPath *field.Path) field.ErrorLi // Validate limit range resource name // limit types (other than Pod/Container) could contain storage not just cpu or memory func validateLimitRangeResourceName(limitType core.LimitType, value string, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - if value == string(core.ResourceEphemeralStorage) && !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { - return append(allErrs, field.Forbidden(fldPath, "ResourceEphemeralStorage field disabled by feature-gate for Resource LimitRange")) - } switch limitType { case core.LimitTypePod, core.LimitTypeContainer: return validateContainerResourceName(value, fldPath) @@ -4802,9 +4788,6 @@ func ValidateResourceRequirements(requirements *core.ResourceRequirements, fldPa // Validate resource quantity. allErrs = append(allErrs, ValidateResourceQuantityValue(string(resourceName), quantity, fldPath)...) - if resourceName == core.ResourceEphemeralStorage && !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { - allErrs = append(allErrs, field.Forbidden(limPath, "ResourceEphemeralStorage field disabled by feature-gate for ResourceRequirements")) - } if helper.IsHugePageResourceName(resourceName) { if !utilfeature.DefaultFeatureGate.Enabled(features.HugePages) { allErrs = append(allErrs, field.Forbidden(limPath, fmt.Sprintf("%s field disabled by feature-gate for ResourceRequirements", resourceName))) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index f707aad0f4f..ecbfcf77350 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -3991,20 +3991,12 @@ func TestAlphaLocalStorageCapacityIsolation(t *testing.T) { testCases := []core.VolumeSource{ {EmptyDir: &core.EmptyDirVolumeSource{SizeLimit: resource.NewQuantity(int64(5), resource.BinarySI)}}, } - // Enable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, true)() + for _, tc := range testCases { if errs := validateVolumeSource(&tc, field.NewPath("spec"), "tmpvol"); len(errs) != 0 { t.Errorf("expected success: %v", errs) } } - // Disable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, false)() - for _, tc := range testCases { - if errs := validateVolumeSource(&tc, field.NewPath("spec"), "tmpvol"); len(errs) == 0 { - t.Errorf("expected failure: %v", errs) - } - } containerLimitCase := core.ResourceRequirements{ Limits: core.ResourceList{ @@ -4013,17 +4005,9 @@ func TestAlphaLocalStorageCapacityIsolation(t *testing.T) { resource.BinarySI), }, } - // Enable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, true)() if errs := ValidateResourceRequirements(&containerLimitCase, field.NewPath("resources")); len(errs) != 0 { t.Errorf("expected success: %v", errs) } - // Disable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, false)() - if errs := ValidateResourceRequirements(&containerLimitCase, field.NewPath("resources")); len(errs) == 0 { - t.Errorf("expected failure: %v", errs) - } - } func TestValidateResourceQuotaWithAlphaLocalStorageCapacityIsolation(t *testing.T) { @@ -4054,24 +4038,9 @@ func TestValidateResourceQuotaWithAlphaLocalStorageCapacityIsolation(t *testing. Spec: spec, } - // Enable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, true)() if errs := ValidateResourceQuota(resourceQuota); len(errs) != 0 { t.Errorf("expected success: %v", errs) } - - // Disable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, false)() - errs := ValidateResourceQuota(resourceQuota) - if len(errs) == 0 { - t.Errorf("expected failure for %s", resourceQuota.Name) - } - expectedErrMes := "ResourceEphemeralStorage field disabled by feature-gate for ResourceQuota" - for i := range errs { - if !strings.Contains(errs[i].Detail, expectedErrMes) { - t.Errorf("[%s]: expected error detail either empty or %s, got %s", resourceQuota.Name, expectedErrMes, errs[i].Detail) - } - } } func TestValidatePorts(t *testing.T) { @@ -4194,21 +4163,11 @@ func TestLocalStorageEnvWithFeatureGate(t *testing.T) { }, }, } - // Enable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, true)() for _, testCase := range testCases { if errs := validateEnvVarValueFrom(testCase, field.NewPath("field")); len(errs) != 0 { t.Errorf("expected success, got: %v", errs) } } - - // Disable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, false)() - for _, testCase := range testCases { - if errs := validateEnvVarValueFrom(testCase, field.NewPath("field")); len(errs) == 0 { - t.Errorf("expected failure for %v", testCase.Name) - } - } } func TestValidateEnv(t *testing.T) { @@ -11056,24 +11015,12 @@ func TestValidateLimitRangeForLocalStorage(t *testing.T) { }, } - // Enable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, true)() for _, testCase := range testCases { limitRange := &core.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: testCase.name, Namespace: "foo"}, Spec: testCase.spec} if errs := ValidateLimitRange(limitRange); len(errs) != 0 { t.Errorf("Case %v, unexpected error: %v", testCase.name, errs) } } - - // Disable feature LocalStorageCapacityIsolation - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LocalStorageCapacityIsolation, false)() - for _, testCase := range testCases { - limitRange := &core.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: testCase.name, Namespace: "foo"}, Spec: testCase.spec} - if errs := ValidateLimitRange(limitRange); len(errs) == 0 { - t.Errorf("Case %v, expected feature gate unable error but actually no error", testCase.name) - } - } - } func TestValidateLimitRange(t *testing.T) {