Skip to content

Commit

Permalink
Merge pull request #72371 from sbezverk/volume_sizelimit
Browse files Browse the repository at this point in the history 
EmptyVolume SizeLimit validation and tests
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Dec 27, 2018
2 parents ffc6751 + 4ad98db commit c20aca0
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 82 deletions.
1 change: 1 addition & 0 deletions pkg/api/pod/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
24 changes: 16 additions & 8 deletions pkg/api/pod/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}
111 changes: 111 additions & 0 deletions pkg/api/pod/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()))
}
}
})
}
}
}
}
23 changes: 3 additions & 20 deletions pkg/apis/core/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)))
Expand Down
55 changes: 1 addition & 54 deletions pkg/apis/core/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit c20aca0

Please sign in to comment.