From 0eb65bd7da297b8f039245fc93dcc56b1e6b2e67 Mon Sep 17 00:00:00 2001 From: Ed Bartosh Date: Thu, 17 Oct 2019 15:25:06 +0300 Subject: [PATCH 1/3] Implement support for multiple sizes huge pages This implementation allows Pod to request multiple hugepage resources of different size and mount hugepage volumes using storage medium HugePage-, e.g. spec: containers: resources: requests: hugepages-2Mi: 2Mi hugepages-1Gi: 2Gi volumeMounts: - mountPath: /hugepages-2Mi name: hugepage-2mi - mountPath: /hugepages-1Gi name: hugepage-1gi ... volumes: - name: hugepage-2mi emptyDir: medium: HugePages-2Mi - name: hugepage-1gi emptyDir: medium: HugePages-1Gi NOTE: This is an alpha feature. Feature gate HugePageStorageMediumSize must be enabled for it to work. --- pkg/apis/core/types.go | 7 +-- pkg/apis/core/v1/helper/helpers.go | 21 +++++++++ pkg/apis/core/validation/validation.go | 50 ++++++++++++++------ pkg/features/kube_features.go | 9 ++++ pkg/kubelet/config/BUILD | 2 + pkg/kubelet/config/common.go | 13 +++++- pkg/kubelet/config/common_test.go | 2 +- pkg/kubelet/config/file_linux_test.go | 4 +- pkg/kubelet/config/http_test.go | 2 +- pkg/registry/core/pod/strategy.go | 15 ++++-- pkg/volume/emptydir/BUILD | 6 +++ pkg/volume/emptydir/empty_dir.go | 61 ++++++++++++++++--------- staging/src/k8s.io/api/core/v1/types.go | 7 +-- 13 files changed, 148 insertions(+), 51 deletions(-) diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index c5ada193eff..af4d3dba507 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -583,9 +583,10 @@ type StorageMedium string // These are the valid value for StorageMedium const ( - StorageMediumDefault StorageMedium = "" // use whatever the default is for the node - StorageMediumMemory StorageMedium = "Memory" // use memory (tmpfs) - StorageMediumHugePages StorageMedium = "HugePages" // use hugepages + StorageMediumDefault StorageMedium = "" // use whatever the default is for the node + StorageMediumMemory StorageMedium = "Memory" // use memory (tmpfs) + StorageMediumHugePages StorageMedium = "HugePages" // use hugepages + StorageMediumHugePagesPrefix StorageMedium = "HugePages-" // prefix for full medium notation HugePages- ) // Protocol defines network protocols supported for things like container ports. diff --git a/pkg/apis/core/v1/helper/helpers.go b/pkg/apis/core/v1/helper/helpers.go index a930520f1b1..ac3018c9992 100644 --- a/pkg/apis/core/v1/helper/helpers.go +++ b/pkg/apis/core/v1/helper/helpers.go @@ -103,6 +103,27 @@ func HugePageUnitSizeFromByteSize(size int64) (string, error) { return fmt.Sprintf("%d%s", size, hugePageSizeUnitList[idx]), nil } +// IsHugePageMedium returns true if the volume medium is in 'HugePages[-size]' format +func IsHugePageMedium(medium v1.StorageMedium) bool { + if medium == v1.StorageMediumHugePages { + return true + } + return strings.HasPrefix(string(medium), string(v1.StorageMediumHugePagesPrefix)) +} + +// HugePageSizeFromMedium returns the page size for the specified huge page medium. +// If the specified input is not a valid huge page medium an error is returned. +func HugePageSizeFromMedium(medium v1.StorageMedium) (resource.Quantity, error) { + if !IsHugePageMedium(medium) { + return resource.Quantity{}, fmt.Errorf("medium: %s is not a hugepage medium", medium) + } + if medium == v1.StorageMediumHugePages { + return resource.Quantity{}, fmt.Errorf("medium: %s doesn't have size information", medium) + } + pageSize := strings.TrimPrefix(string(medium), string(v1.StorageMediumHugePagesPrefix)) + return resource.ParseQuantity(pageSize) +} + // IsOvercommitAllowed returns true if the resource is in the default // namespace and is not hugepages. func IsOvercommitAllowed(name v1.ResourceName) bool { diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index cec7ebf1d32..6c13172208c 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3083,8 +3083,33 @@ func validateContainersOnlyForPod(containers []core.Container, fldPath *field.Pa return allErrs } +// PodValidationOptions contains the different settings for pod validation +type PodValidationOptions struct { + // Allow pod spec to have more than one huge page resource (with different sizes) + AllowMultipleHugePageResources bool +} + +// ValidatePodSingleHugePageResources checks if there are multiple huge +// pages resources in the pod object. +func ValidatePodSingleHugePageResources(pod *core.Pod, specPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + hugePageResources := sets.NewString() + for i := range pod.Spec.Containers { + resourceSet := toContainerResourcesSet(&pod.Spec.Containers[i]) + for resourceStr := range resourceSet { + if v1helper.IsHugePageResourceName(v1.ResourceName(resourceStr)) { + hugePageResources.Insert(resourceStr) + } + } + } + if len(hugePageResources) > 1 { + allErrs = append(allErrs, field.Invalid(specPath, hugePageResources.List(), "must use a single hugepage size in a pod spec")) + } + return allErrs +} + // ValidatePod tests if required fields in the pod are set. -func ValidatePod(pod *core.Pod) field.ErrorList { +func ValidatePod(pod *core.Pod, opts PodValidationOptions) field.ErrorList { fldPath := field.NewPath("metadata") allErrs := ValidateObjectMeta(&pod.ObjectMeta, true, ValidatePodName, fldPath) allErrs = append(allErrs, ValidatePodSpecificAnnotations(pod.ObjectMeta.Annotations, &pod.Spec, fldPath.Child("annotations"))...) @@ -3111,17 +3136,8 @@ func ValidatePod(pod *core.Pod) field.ErrorList { allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.Containers, specPath.Child("containers"))...) allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.InitContainers, specPath.Child("initContainers"))...) - hugePageResources := sets.NewString() - for i := range pod.Spec.Containers { - resourceSet := toContainerResourcesSet(&pod.Spec.Containers[i]) - for resourceStr := range resourceSet { - if v1helper.IsHugePageResourceName(v1.ResourceName(resourceStr)) { - hugePageResources.Insert(resourceStr) - } - } - } - if len(hugePageResources) > 1 { - allErrs = append(allErrs, field.Invalid(specPath, hugePageResources, "must use a single hugepage size in a pod spec")) + if !opts.AllowMultipleHugePageResources { + allErrs = append(allErrs, ValidatePodSingleHugePageResources(pod, specPath)...) } podIPsField := field.NewPath("status", "podIPs") @@ -3679,8 +3695,8 @@ func ValidateContainerUpdates(newContainers, oldContainers []core.Container, fld } // ValidatePodCreate validates a pod in the context of its initial create -func ValidatePodCreate(pod *core.Pod) field.ErrorList { - allErrs := ValidatePod(pod) +func ValidatePodCreate(pod *core.Pod, opts PodValidationOptions) field.ErrorList { + allErrs := ValidatePod(pod, opts) fldPath := field.NewPath("spec") // EphemeralContainers can only be set on update using the ephemeralcontainers subresource @@ -3693,12 +3709,16 @@ func ValidatePodCreate(pod *core.Pod) field.ErrorList { // ValidatePodUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields // that cannot be changed. -func ValidatePodUpdate(newPod, oldPod *core.Pod) field.ErrorList { +func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) field.ErrorList { fldPath := field.NewPath("metadata") allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath) allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"))...) specPath := field.NewPath("spec") + if !opts.AllowMultipleHugePageResources { + allErrs = append(allErrs, ValidatePodSingleHugePageResources(newPod, specPath)...) + } + // validate updateable fields: // 1. spec.containers[*].image // 2. spec.initContainers[*].image diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 5413aaf5176..7319820dda9 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -539,6 +539,14 @@ const ( // // Enables a feature to make secrets and configmaps data immutable. ImmutableEphemeralVolumes featuregate.Feature = "ImmutableEphemeralVolumes" + + // owner: @bart0sh + // alpha: v1.18 + // + // Enables usage of HugePages- in a volume medium, + // e.g. emptyDir: + // medium: HugePages-1Gi + HugePageStorageMediumSize featuregate.Feature = "HugePageStorageMediumSize" ) func init() { @@ -624,6 +632,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS PodDisruptionBudget: {Default: true, PreRelease: featuregate.Beta}, ServiceTopology: {Default: false, PreRelease: featuregate.Alpha}, ImmutableEphemeralVolumes: {Default: false, PreRelease: featuregate.Alpha}, + HugePageStorageMediumSize: {Default: false, PreRelease: featuregate.Alpha}, // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/pkg/kubelet/config/BUILD b/pkg/kubelet/config/BUILD index 25595bd3e9b..b34fb86f199 100644 --- a/pkg/kubelet/config/BUILD +++ b/pkg/kubelet/config/BUILD @@ -24,6 +24,7 @@ go_library( "//pkg/apis/core/install:go_default_library", "//pkg/apis/core/v1:go_default_library", "//pkg/apis/core/validation:go_default_library", + "//pkg/features:go_default_library", "//pkg/kubelet/checkpoint:go_default_library", "//pkg/kubelet/checkpointmanager:go_default_library", "//pkg/kubelet/container:go_default_library", @@ -40,6 +41,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/yaml:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/kubernetes:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", "//staging/src/k8s.io/client-go/tools/record:go_default_library", diff --git a/pkg/kubelet/config/common.go b/pkg/kubelet/config/common.go index cd557c28147..6ddf219b4f1 100644 --- a/pkg/kubelet/config/common.go +++ b/pkg/kubelet/config/common.go @@ -27,8 +27,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilyaml "k8s.io/apimachinery/pkg/util/yaml" + utilfeature "k8s.io/apiserver/pkg/util/feature" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" + "k8s.io/kubernetes/pkg/features" // TODO: remove this import if // api.Registry.GroupOrDie(v1.GroupName).GroupVersion.String() is changed @@ -133,7 +135,10 @@ func tryDecodeSinglePod(data []byte, defaultFn defaultFunc) (parsed bool, pod *v if err = defaultFn(newPod); err != nil { return true, pod, err } - if errs := validation.ValidatePod(newPod); len(errs) > 0 { + opts := validation.PodValidationOptions{ + AllowMultipleHugePageResources: utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), + } + if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 { return true, pod, fmt.Errorf("invalid pod: %v", errs) } v1Pod := &v1.Pod{} @@ -157,13 +162,17 @@ func tryDecodePodList(data []byte, defaultFn defaultFunc) (parsed bool, pods v1. return false, pods, err } + opts := validation.PodValidationOptions{ + AllowMultipleHugePageResources: utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), + } + // Apply default values and validate pods. for i := range newPods.Items { newPod := &newPods.Items[i] if err = defaultFn(newPod); err != nil { return true, pods, err } - if errs := validation.ValidatePod(newPod); len(errs) > 0 { + if errs := validation.ValidatePod(newPod, opts); len(errs) > 0 { err = fmt.Errorf("invalid pod: %v", errs) return true, pods, err } diff --git a/pkg/kubelet/config/common_test.go b/pkg/kubelet/config/common_test.go index c220a14e504..b29b83d080f 100644 --- a/pkg/kubelet/config/common_test.go +++ b/pkg/kubelet/config/common_test.go @@ -251,7 +251,7 @@ func TestStaticPodNameGenerate(t *testing.T) { if c.overwrite != "" { pod.Name = c.overwrite } - errs := validation.ValidatePod(pod) + errs := validation.ValidatePod(pod, validation.PodValidationOptions{}) if c.shouldErr { specNameErrored := false for _, err := range errs { diff --git a/pkg/kubelet/config/file_linux_test.go b/pkg/kubelet/config/file_linux_test.go index ee9f1517b75..4c7a7ba8f34 100644 --- a/pkg/kubelet/config/file_linux_test.go +++ b/pkg/kubelet/config/file_linux_test.go @@ -90,7 +90,7 @@ func TestReadPodsFromFileExistAlready(t *testing.T) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod); len(errs) > 0 { + if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs) } } @@ -369,7 +369,7 @@ func expectUpdate(t *testing.T, ch chan interface{}, testCase *testCase) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod); len(errs) > 0 { + if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) > 0 { t.Fatalf("%s: Invalid pod %#v, %#v", testCase.desc, internalPod, errs) } } diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index 398dbc8e679..2778bfc535b 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -319,7 +319,7 @@ func TestExtractPodsFromHTTP(t *testing.T) { if err := k8s_api_v1.Convert_v1_Pod_To_core_Pod(pod, internalPod, nil); err != nil { t.Fatalf("%s: Cannot convert pod %#v, %#v", testCase.desc, pod, err) } - if errs := validation.ValidatePod(internalPod); len(errs) != 0 { + if errs := validation.ValidatePod(internalPod, validation.PodValidationOptions{}); len(errs) != 0 { t.Errorf("%s: Expected no validation errors on %#v, Got %v", testCase.desc, pod, errs.ToAggregate()) } } diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 917769e400c..2f9e8235685 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -88,7 +88,11 @@ func (podStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object // Validate validates a new pod. func (podStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { pod := obj.(*api.Pod) - allErrs := validation.ValidatePodCreate(pod) + opts := validation.PodValidationOptions{ + // Allow multiple huge pages on pod create if feature is enabled + AllowMultipleHugePageResources: utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), + } + allErrs := validation.ValidatePodCreate(pod, opts) allErrs = append(allErrs, validation.ValidateConditionalPod(pod, nil, field.NewPath(""))...) return allErrs } @@ -104,8 +108,13 @@ func (podStrategy) AllowCreateOnUpdate() bool { // ValidateUpdate is the default update validation for an end user. func (podStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - errorList := validation.ValidatePod(obj.(*api.Pod)) - errorList = append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod))...) + oldFailsSingleHugepagesValidation := len(validation.ValidatePodSingleHugePageResources(old.(*api.Pod), field.NewPath("spec"))) > 0 + opts := validation.PodValidationOptions{ + // Allow multiple huge pages on pod create if feature is enabled or if the old pod already has multiple hugepages specified + AllowMultipleHugePageResources: oldFailsSingleHugepagesValidation || utilfeature.DefaultFeatureGate.Enabled(features.HugePageStorageMediumSize), + } + errorList := validation.ValidatePod(obj.(*api.Pod), opts) + errorList = append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod), opts)...) errorList = append(errorList, validation.ValidateConditionalPod(obj.(*api.Pod), old.(*api.Pod), field.NewPath(""))...) return errorList } diff --git a/pkg/volume/emptydir/BUILD b/pkg/volume/emptydir/BUILD index c4a58fda540..b220a50107e 100644 --- a/pkg/volume/emptydir/BUILD +++ b/pkg/volume/emptydir/BUILD @@ -44,6 +44,7 @@ go_test( embed = [":go_default_library"], deps = select({ "@io_bazel_rules_go//go/platform:android": [ + "//pkg/features:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/testing:go_default_library", "//pkg/volume/util:go_default_library", @@ -51,10 +52,13 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/util/testing:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", "//vendor/k8s.io/utils/mount:go_default_library", ], "@io_bazel_rules_go//go/platform:linux": [ + "//pkg/features:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/testing:go_default_library", "//pkg/volume/util:go_default_library", @@ -62,7 +66,9 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/util/testing:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", "//vendor/k8s.io/utils/mount:go_default_library", ], "//conditions:default": [], diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index a890a34802c..1102262f766 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -216,12 +216,12 @@ func (ed *emptyDir) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { } } - switch ed.medium { - case v1.StorageMediumDefault: + switch { + case ed.medium == v1.StorageMediumDefault: err = ed.setupDir(dir) - case v1.StorageMediumMemory: + case ed.medium == v1.StorageMediumMemory: err = ed.setupTmpfs(dir) - case v1.StorageMediumHugePages: + case v1helper.IsHugePageMedium(ed.medium): err = ed.setupHugepages(dir) default: err = fmt.Errorf("unknown storage medium %q", ed.medium) @@ -290,11 +290,11 @@ func (ed *emptyDir) setupHugepages(dir string) error { } // If the directory is a mountpoint with medium hugepages, there is no // work to do since we are already in the desired state. - if isMnt && medium == v1.StorageMediumHugePages { + if isMnt && v1helper.IsHugePageMedium(medium) { return nil } - pageSizeMountOption, err := getPageSizeMountOptionFromPod(ed.pod) + pageSizeMountOption, err := getPageSizeMountOption(ed.medium, ed.pod) if err != nil { return err } @@ -303,33 +303,52 @@ func (ed *emptyDir) setupHugepages(dir string) error { return ed.mounter.Mount("nodev", dir, "hugetlbfs", []string{pageSizeMountOption}) } -// getPageSizeMountOptionFromPod retrieves pageSize mount option from Pod's resources -// and validates pageSize options in all containers of given Pod. -func getPageSizeMountOptionFromPod(pod *v1.Pod) (string, error) { +// getPageSizeMountOption retrieves pageSize mount option from Pod's resources +// and medium and validates pageSize options in all containers of given Pod. +func getPageSizeMountOption(medium v1.StorageMedium, pod *v1.Pod) (string, error) { pageSizeFound := false pageSize := resource.Quantity{} - // In some rare cases init containers can also consume Huge pages. - containers := append(pod.Spec.Containers, pod.Spec.InitContainers...) - for _, container := range containers { + + var mediumPageSize resource.Quantity + if medium != v1.StorageMediumHugePages { + // medium is: Hugepages- + var err error + mediumPageSize, err = v1helper.HugePageSizeFromMedium(medium) + if err != nil { + return "", err + } + } + + // In some rare cases init containers can also consume Huge pages + for _, container := range append(pod.Spec.Containers, pod.Spec.InitContainers...) { // We can take request because limit and requests must match. for requestName := range container.Resources.Requests { - if v1helper.IsHugePageResourceName(requestName) { - currentPageSize, err := v1helper.HugePageSizeFromResourceName(requestName) - if err != nil { - return "", err - } - // PageSize for all volumes in a POD are equal, except for the first one discovered. + if !v1helper.IsHugePageResourceName(requestName) { + continue + } + currentPageSize, err := v1helper.HugePageSizeFromResourceName(requestName) + if err != nil { + return "", err + } + if medium == v1.StorageMediumHugePages { // medium is: Hugepages, size is not specified + // PageSize for all volumes in a POD must be equal if medium is "Hugepages" if pageSizeFound && pageSize.Cmp(currentPageSize) != 0 { - return "", fmt.Errorf("multiple pageSizes for huge pages in a single PodSpec") + return "", fmt.Errorf("medium: %s can't be used if container requests multiple huge page sizes", medium) } - pageSize = currentPageSize + pageSizeFound = true + pageSize = currentPageSize + } else { // medium is: Hugepages- + if currentPageSize.Cmp(mediumPageSize) == 0 { + pageSizeFound = true + pageSize = currentPageSize + } } } } if !pageSizeFound { - return "", fmt.Errorf("hugePages storage requested, but there is no resource request for huge pages") + return "", fmt.Errorf("medium %s: hugePages storage requested, but there is no resource request for huge pages", medium) } return fmt.Sprintf("%s=%s", hugePagesPageSizeMountOption, pageSize.String()), nil diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index fb350175dc8..ecc177e7189 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -887,9 +887,10 @@ type FlockerVolumeSource struct { type StorageMedium string const ( - StorageMediumDefault StorageMedium = "" // use whatever the default is for the node, assume anything we don't explicitly handle is this - StorageMediumMemory StorageMedium = "Memory" // use memory (e.g. tmpfs on linux) - StorageMediumHugePages StorageMedium = "HugePages" // use hugepages + StorageMediumDefault StorageMedium = "" // use whatever the default is for the node, assume anything we don't explicitly handle is this + StorageMediumMemory StorageMedium = "Memory" // use memory (e.g. tmpfs on linux) + StorageMediumHugePages StorageMedium = "HugePages" // use hugepages + StorageMediumHugePagesPrefix StorageMedium = "HugePages-" // prefix for full medium notation HugePages- ) // Protocol defines network protocols supported for things like container ports. From 882d6e93afd4eb5782993d599f6beb1d25e7971c Mon Sep 17 00:00:00 2001 From: Ed Bartosh Date: Thu, 17 Oct 2019 16:38:37 +0300 Subject: [PATCH 2/3] Implement tests for multiple sizes huge pages Co-Authored-By: Odin Ugedal --- pkg/apis/core/v1/helper/helpers_test.go | 67 +++++ pkg/apis/core/validation/validation_test.go | 284 ++++++++++++-------- pkg/volume/emptydir/empty_dir_test.go | 188 +++++++++++-- 3 files changed, 409 insertions(+), 130 deletions(-) diff --git a/pkg/apis/core/v1/helper/helpers_test.go b/pkg/apis/core/v1/helper/helpers_test.go index 27d5b157903..c418e82b4fb 100644 --- a/pkg/apis/core/v1/helper/helpers_test.go +++ b/pkg/apis/core/v1/helper/helpers_test.go @@ -113,6 +113,73 @@ func TestHugePageSizeFromResourceName(t *testing.T) { } } +func TestHugePageSizeFromMedium(t *testing.T) { + testCases := []struct { + description string + medium v1.StorageMedium + expectVal resource.Quantity + expectErr bool + }{ + { + description: "Invalid hugepages medium", + medium: "Memory", + expectVal: resource.Quantity{}, + expectErr: true, + }, + { + description: "Invalid hugepages medium", + medium: "Memory", + expectVal: resource.Quantity{}, + expectErr: true, + }, + { + description: "Invalid: HugePages without size", + medium: "HugePages", + expectVal: resource.Quantity{}, + expectErr: true, + }, + { + description: "Invalid: HugePages without size", + medium: "HugePages", + expectVal: resource.Quantity{}, + expectErr: true, + }, + { + description: "Valid: HugePages-1Gi", + medium: "HugePages-1Gi", + expectVal: resource.MustParse("1Gi"), + expectErr: false, + }, + { + description: "Valid: HugePages-2Mi", + medium: "HugePages-2Mi", + expectVal: resource.MustParse("2Mi"), + expectErr: false, + }, + { + description: "Valid: HugePages-64Ki", + medium: "HugePages-64Ki", + expectVal: resource.MustParse("64Ki"), + expectErr: false, + }, + } + for i, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + v, err := HugePageSizeFromMedium(tc.medium) + if err == nil && tc.expectErr { + t.Errorf("[%v]expected error but got none.", i) + } + if err != nil && !tc.expectErr { + t.Errorf("[%v]did not expect error but got: %v", i, err) + } + if v != tc.expectVal { + t.Errorf("Got %v but expected %v", v, tc.expectVal) + } + }) + } +} + func TestIsOvercommitAllowed(t *testing.T) { testCases := []struct { resourceName v1.ResourceName diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 1e44306a4d9..bfb1cc2026d 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -24,7 +24,7 @@ import ( "strings" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -3908,114 +3908,186 @@ func TestValidateVolumes(t *testing.T) { } func TestHugePagesIsolation(t *testing.T) { - successCases := []core.Pod{ - { // Basic fields. - ObjectMeta: metav1.ObjectMeta{Name: "123", Namespace: "ns"}, - Spec: core.PodSpec{ - Containers: []core.Container{ - { - Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", - Resources: core.ResourceRequirements{ - Requests: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), - }, - Limits: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + testCases := map[string]struct { + pod *core.Pod + enableHugePageStorageMediumSize bool + expectError bool + }{ + "Valid: request hugepages-2Mi": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "123", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + }, }, }, }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, }, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, }, }, + "Valid: HugePageStorageMediumSize enabled: request more than one hugepages size": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "hugepages-shared", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + }, + enableHugePageStorageMediumSize: true, + expectError: false, + }, + "Invalid: HugePageStorageMediumSize disabled: request hugepages-1Gi, limit hugepages-2Mi and hugepages-1Gi": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "hugepages-multiple", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + }, + expectError: true, + }, + "Invalid: not requesting cpu and memory": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "hugepages-requireCpuOrMemory", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + }, + }, + }, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + }, + expectError: true, + }, + "Invalid: request 1Gi hugepages-2Mi but limit 2Gi": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "hugepages-shared", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + }, + expectError: true, + }, + "Invalid: HugePageStorageMediumSize disabled: request more than one hugepages size": { + pod: &core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "hugepages-shared", Namespace: "ns"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("1Gi"), + }, + Limits: core.ResourceList{ + core.ResourceName(core.ResourceCPU): resource.MustParse("10"), + core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), + core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), + core.ResourceName("hugepages-1Gi"): resource.MustParse("1Gi"), + }, + }, + }, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + }, + expectError: true, + }, } - failureCases := []core.Pod{ - { // Basic fields. - ObjectMeta: metav1.ObjectMeta{Name: "hugepages-requireCpuOrMemory", Namespace: "ns"}, - Spec: core.PodSpec{ - Containers: []core.Container{ - { - Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", - Resources: core.ResourceRequirements{ - Requests: core.ResourceList{ - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), - }, - Limits: core.ResourceList{ - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), - }, - }, - }, - }, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - }, - }, - { // Basic fields. - ObjectMeta: metav1.ObjectMeta{Name: "hugepages-shared", Namespace: "ns"}, - Spec: core.PodSpec{ - Containers: []core.Container{ - { - Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", - Resources: core.ResourceRequirements{ - Requests: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), - }, - Limits: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-2Mi"): resource.MustParse("2Gi"), - }, - }, - }, - }, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - }, - }, - { // Basic fields. - ObjectMeta: metav1.ObjectMeta{Name: "hugepages-multiple", Namespace: "ns"}, - Spec: core.PodSpec{ - Containers: []core.Container{ - { - Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", - Resources: core.ResourceRequirements{ - Requests: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), - }, - Limits: core.ResourceList{ - core.ResourceName(core.ResourceCPU): resource.MustParse("10"), - core.ResourceName(core.ResourceMemory): resource.MustParse("10G"), - core.ResourceName("hugepages-2Mi"): resource.MustParse("1Gi"), - core.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), - }, - }, - }, - }, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - }, - }, - } - for i := range successCases { - pod := &successCases[i] - if errs := ValidatePod(pod); len(errs) != 0 { - t.Errorf("Unexpected error for case[%d], err: %v", i, errs) - } - } - for i := range failureCases { - pod := &failureCases[i] - if errs := ValidatePod(pod); len(errs) == 0 { - t.Errorf("Expected error for case[%d], pod: %v", i, pod.Name) - } + for tcName, tc := range testCases { + t.Run(tcName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HugePageStorageMediumSize, tc.enableHugePageStorageMediumSize)() + errs := ValidatePod(tc.pod, PodValidationOptions{tc.enableHugePageStorageMediumSize}) + if tc.expectError && len(errs) == 0 { + t.Errorf("Unexpected success") + } + if !tc.expectError && len(errs) != 0 { + t.Errorf("Unexpected error(s): %v", errs) + } + }) } } @@ -7375,7 +7447,7 @@ func TestValidatePod(t *testing.T) { }, } for _, pod := range successCases { - if errs := ValidatePod(&pod); len(errs) != 0 { + if errs := ValidatePod(&pod, PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } } @@ -8225,7 +8297,7 @@ func TestValidatePod(t *testing.T) { }, } for k, v := range errorCases { - if errs := ValidatePod(&v.spec); len(errs) == 0 { + if errs := ValidatePod(&v.spec, PodValidationOptions{}); len(errs) == 0 { t.Errorf("expected failure for %q", k) } else if v.expectedError == "" { t.Errorf("missing expectedError for %q, got %q", k, errs.ToAggregate().Error()) @@ -8965,7 +9037,7 @@ func TestValidatePodUpdate(t *testing.T) { for _, test := range tests { test.new.ObjectMeta.ResourceVersion = "1" test.old.ObjectMeta.ResourceVersion = "1" - errs := ValidatePodUpdate(&test.new, &test.old) + errs := ValidatePodUpdate(&test.new, &test.old, PodValidationOptions{}) if test.err == "" { if len(errs) != 0 { t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.new, test.old) @@ -14876,7 +14948,7 @@ func TestPodIPsValidation(t *testing.T) { } for _, testCase := range testCases { - errs := ValidatePod(&testCase.pod) + errs := ValidatePod(&testCase.pod, PodValidationOptions{}) if len(errs) == 0 && testCase.expectError { t.Errorf("expected failure for %s, but there were none", testCase.pod.Name) return diff --git a/pkg/volume/emptydir/empty_dir_test.go b/pkg/volume/emptydir/empty_dir_test.go index 892ec1ec342..12e5820f2f0 100644 --- a/pkg/volume/emptydir/empty_dir_test.go +++ b/pkg/volume/emptydir/empty_dir_test.go @@ -23,16 +23,18 @@ import ( "path/filepath" "testing" - "k8s.io/utils/mount" - - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + utilfeature "k8s.io/apiserver/pkg/util/feature" utiltesting "k8s.io/client-go/util/testing" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume" volumetest "k8s.io/kubernetes/pkg/volume/testing" "k8s.io/kubernetes/pkg/volume/util" + "k8s.io/utils/mount" ) // Construct an instance of a plugin, by name. @@ -83,12 +85,33 @@ func TestPluginEmptyRootContext(t *testing.T) { } func TestPluginHugetlbfs(t *testing.T) { - doTestPlugin(t, pluginTestConfig{ - medium: v1.StorageMediumHugePages, - expectedSetupMounts: 1, - expectedTeardownMounts: 0, - shouldBeMountedBeforeTeardown: true, - }) + testCases := map[string]struct { + medium v1.StorageMedium + enableHugePageStorageMediumSize bool + }{ + "HugePageStorageMediumSize enabled: medium without size": { + medium: "HugePages", + enableHugePageStorageMediumSize: true, + }, + "HugePageStorageMediumSize disabled: medium without size": { + medium: "HugePages", + }, + "HugePageStorageMediumSize enabled: medium with size": { + medium: "HugePages-2Mi", + enableHugePageStorageMediumSize: true, + }, + } + for tcName, tc := range testCases { + t.Run(tcName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HugePageStorageMediumSize, tc.enableHugePageStorageMediumSize)() + doTestPlugin(t, pluginTestConfig{ + medium: tc.medium, + expectedSetupMounts: 1, + expectedTeardownMounts: 0, + shouldBeMountedBeforeTeardown: true, + }) + }) + } } type pluginTestConfig struct { @@ -307,11 +330,13 @@ func TestMetrics(t *testing.T) { func TestGetHugePagesMountOptions(t *testing.T) { testCases := map[string]struct { - pod *v1.Pod - shouldFail bool - expectedResult string + pod *v1.Pod + medium v1.StorageMedium + shouldFail bool + expectedResult string + enableHugePageStorageMediumSize bool }{ - "testWithProperValues": { + "ProperValues": { pod: &v1.Pod{ Spec: v1.PodSpec{ Containers: []v1.Container{ @@ -325,10 +350,11 @@ func TestGetHugePagesMountOptions(t *testing.T) { }, }, }, + medium: v1.StorageMediumHugePages, shouldFail: false, expectedResult: "pagesize=2Mi", }, - "testWithProperValuesAndDifferentPageSize": { + "ProperValuesAndDifferentPageSize": { pod: &v1.Pod{ Spec: v1.PodSpec{ Containers: []v1.Container{ @@ -349,6 +375,7 @@ func TestGetHugePagesMountOptions(t *testing.T) { }, }, }, + medium: v1.StorageMediumHugePages, shouldFail: false, expectedResult: "pagesize=1Gi", }, @@ -373,6 +400,7 @@ func TestGetHugePagesMountOptions(t *testing.T) { }, }, }, + medium: v1.StorageMediumHugePages, shouldFail: false, expectedResult: "pagesize=1Gi", }, @@ -397,6 +425,7 @@ func TestGetHugePagesMountOptions(t *testing.T) { }, }, }, + medium: v1.StorageMediumHugePages, shouldFail: true, expectedResult: "", }, @@ -421,24 +450,135 @@ func TestGetHugePagesMountOptions(t *testing.T) { }, }, }, - shouldFail: true, - expectedResult: "", + medium: v1.StorageMediumHugePages, + shouldFail: true, + expectedResult: "", + enableHugePageStorageMediumSize: true, }, "PodWithNoHugePagesRequest": { pod: &v1.Pod{}, + medium: v1.StorageMediumHugePages, + shouldFail: true, + expectedResult: "", + }, + "ProperValuesMultipleSizes": { + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + medium: v1.StorageMediumHugePagesPrefix + "1Gi", + shouldFail: false, + expectedResult: "pagesize=1Gi", + enableHugePageStorageMediumSize: true, + }, + "InitContainerAndContainerHasProperValuesMultipleSizes": { + pod: &v1.Pod{ + Spec: v1.PodSpec{ + InitContainers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-1Gi"): resource.MustParse("4Gi"), + v1.ResourceName("hugepages-2Mi"): resource.MustParse("50Mi"), + }, + }, + }, + }, + }, + }, + medium: v1.StorageMediumHugePagesPrefix + "2Mi", + shouldFail: false, + expectedResult: "pagesize=2Mi", + enableHugePageStorageMediumSize: true, + }, + "MediumWithoutSizeMultipleSizes": { + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + medium: v1.StorageMediumHugePagesPrefix, + shouldFail: true, + expectedResult: "", + }, + "IncorrectMediumFormatMultipleSizes": { + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + medium: "foo", + shouldFail: true, + expectedResult: "", + }, + "MediumSizeDoesntMatchResourcesMultipleSizes": { + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + medium: v1.StorageMediumHugePagesPrefix + "1Mi", shouldFail: true, expectedResult: "", }, } for testCaseName, testCase := range testCases { - value, err := getPageSizeMountOptionFromPod(testCase.pod) - if testCase.shouldFail && err == nil { - t.Errorf("Expected an error in %v", testCaseName) - } else if !testCase.shouldFail && err != nil { - t.Errorf("Unexpected error in %v, got %v", testCaseName, err) - } else if testCase.expectedResult != value { - t.Errorf("Unexpected mountOptions for Pod. Expected %v, got %v", testCase.expectedResult, value) - } + t.Run(testCaseName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HugePageStorageMediumSize, testCase.enableHugePageStorageMediumSize)() + value, err := getPageSizeMountOption(testCase.medium, testCase.pod) + if testCase.shouldFail && err == nil { + t.Errorf("%s: Unexpected success", testCaseName) + } else if !testCase.shouldFail && err != nil { + t.Errorf("%s: Unexpected error: %v", testCaseName, err) + } else if testCase.expectedResult != value { + t.Errorf("%s: Unexpected mountOptions for Pod. Expected %v, got %v", testCaseName, testCase.expectedResult, value) + } + }) } } From 03ecc20b19972ecb4825ecf4ce6868bc2a45f3c0 Mon Sep 17 00:00:00 2001 From: Ed Bartosh Date: Mon, 17 Feb 2020 17:21:50 +0200 Subject: [PATCH 3/3] empty_dir: Check if hugetlbfs volume is mounted with a correct pagesize Extended GetMountMedium function to check if hugetlbfs volume is mounted with the page size equal to the medium size. Page size is obtained from the 'pagesize' mount option of the mounted hugetlbfs volume. --- pkg/volume/emptydir/empty_dir.go | 31 +- pkg/volume/emptydir/empty_dir_linux.go | 77 ++++- pkg/volume/emptydir/empty_dir_test.go | 334 ++++++++++++++++++- pkg/volume/emptydir/empty_dir_unsupported.go | 5 +- 4 files changed, 423 insertions(+), 24 deletions(-) diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index 1102262f766..2ab16a93636 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -25,7 +25,7 @@ import ( "k8s.io/utils/mount" utilstrings "k8s.io/utils/strings" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -160,7 +160,7 @@ type mountDetector interface { // returns (v1.StorageMediumMemory, false, nil), the caller knows that the path is // on a memory FS (tmpfs on Linux) but is not the root mountpoint of // that tmpfs. - GetMountMedium(path string) (v1.StorageMedium, bool, error) + GetMountMedium(path string, requestedMedium v1.StorageMedium) (v1.StorageMedium, bool, *resource.Quantity, error) } // EmptyDir volumes are temporary directories exposed to the pod. @@ -261,7 +261,7 @@ func (ed *emptyDir) setupTmpfs(dir string) error { return err } // Make SetUp idempotent. - medium, isMnt, err := ed.mountDetector.GetMountMedium(dir) + medium, isMnt, _, err := ed.mountDetector.GetMountMedium(dir, ed.medium) if err != nil { return err } @@ -284,13 +284,30 @@ func (ed *emptyDir) setupHugepages(dir string) error { return err } // Make SetUp idempotent. - medium, isMnt, err := ed.mountDetector.GetMountMedium(dir) + medium, isMnt, mountPageSize, err := ed.mountDetector.GetMountMedium(dir, ed.medium) + klog.V(3).Infof("pod %v: setupHugepages: medium: %s, isMnt: %v, dir: %s, err: %v", ed.pod.UID, medium, isMnt, dir, err) if err != nil { return err } - // If the directory is a mountpoint with medium hugepages, there is no - // work to do since we are already in the desired state. + // If the directory is a mountpoint with medium hugepages of the same page size, + // there is no work to do since we are already in the desired state. if isMnt && v1helper.IsHugePageMedium(medium) { + // Medium is: Hugepages + if ed.medium == v1.StorageMediumHugePages { + return nil + } + if mountPageSize == nil { + return fmt.Errorf("pod %v: mounted dir %s pagesize is not determined", ed.pod.UID, dir) + } + // Medium is: Hugepages- + // Mounted page size and medium size must be equal + mediumSize, err := v1helper.HugePageSizeFromMedium(ed.medium) + if err != nil { + return err + } + if mountPageSize == nil || mediumSize.Cmp(*mountPageSize) != 0 { + return fmt.Errorf("pod %v: mounted dir %s pagesize '%s' and requested medium size '%s' differ", ed.pod.UID, dir, mountPageSize.String(), mediumSize.String()) + } return nil } @@ -412,7 +429,7 @@ func (ed *emptyDir) TearDownAt(dir string) error { } // Figure out the medium. - medium, isMnt, err := ed.mountDetector.GetMountMedium(dir) + medium, isMnt, _, err := ed.mountDetector.GetMountMedium(dir, ed.medium) if err != nil { return err } diff --git a/pkg/volume/emptydir/empty_dir_linux.go b/pkg/volume/emptydir/empty_dir_linux.go index 93b48e27dc6..63a25dc4ed0 100644 --- a/pkg/volume/emptydir/empty_dir_linux.go +++ b/pkg/volume/emptydir/empty_dir_linux.go @@ -20,12 +20,14 @@ package emptydir import ( "fmt" + "strings" "golang.org/x/sys/unix" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/klog" "k8s.io/utils/mount" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" ) // Defined by Linux - the type number for tmpfs mounts. @@ -39,22 +41,71 @@ type realMountDetector struct { mounter mount.Interface } -func (m *realMountDetector) GetMountMedium(path string) (v1.StorageMedium, bool, error) { +// getPageSize obtains page size from the 'pagesize' mount option of the +// mounted volume +func getPageSize(path string, mounter mount.Interface) (*resource.Quantity, error) { + // Get mount point data for the path + mountPoints, err := mounter.List() + if err != nil { + return nil, fmt.Errorf("error listing mount points: %v", err) + } + // Find mount point for the path + mountPoint, err := func(mps []mount.MountPoint, mpPath string) (*mount.MountPoint, error) { + for _, mp := range mps { + if mp.Path == mpPath { + return &mp, nil + } + } + return nil, fmt.Errorf("mount point for %s not found", mpPath) + }(mountPoints, path) + if err != nil { + return nil, err + } + // Get page size from the 'pagesize' option value + for _, opt := range mountPoint.Opts { + opt = strings.TrimSpace(opt) + prefix := "pagesize=" + if strings.HasPrefix(opt, prefix) { + // NOTE: Adding suffix 'i' as result should be comparable with a medium size. + // pagesize mount option is specified without a suffix, + // e.g. pagesize=2M or pagesize=1024M for x86 CPUs + pageSize, err := resource.ParseQuantity(strings.TrimPrefix(opt, prefix) + "i") + if err != nil { + return nil, fmt.Errorf("error getting page size from '%s' mount option: %v", opt, err) + } + return &pageSize, nil + } + } + return nil, fmt.Errorf("no pagesize option specified for %s mount", mountPoint.Path) +} + +func (m *realMountDetector) GetMountMedium(path string, requestedMedium v1.StorageMedium) (v1.StorageMedium, bool, *resource.Quantity, error) { klog.V(5).Infof("Determining mount medium of %v", path) notMnt, err := m.mounter.IsLikelyNotMountPoint(path) if err != nil { - return v1.StorageMediumDefault, false, fmt.Errorf("IsLikelyNotMountPoint(%q): %v", path, err) - } - buf := unix.Statfs_t{} - if err := unix.Statfs(path, &buf); err != nil { - return v1.StorageMediumDefault, false, fmt.Errorf("statfs(%q): %v", path, err) + return v1.StorageMediumDefault, false, nil, fmt.Errorf("IsLikelyNotMountPoint(%q): %v", path, err) } - klog.V(5).Infof("Statfs_t of %v: %+v", path, buf) - if buf.Type == linuxTmpfsMagic { - return v1.StorageMediumMemory, !notMnt, nil - } else if int64(buf.Type) == linuxHugetlbfsMagic { - return v1.StorageMediumHugePages, !notMnt, nil + buf := unix.Statfs_t{} + if err := unix.Statfs(path, &buf); err != nil { + return v1.StorageMediumDefault, false, nil, fmt.Errorf("statfs(%q): %v", path, err) } - return v1.StorageMediumDefault, !notMnt, nil + + klog.V(3).Infof("Statfs_t of %v: %+v", path, buf) + + if buf.Type == linuxTmpfsMagic { + return v1.StorageMediumMemory, !notMnt, nil, nil + } else if int64(buf.Type) == linuxHugetlbfsMagic { + // Skip page size detection if requested medium doesn't have size specified + if requestedMedium == v1.StorageMediumHugePages { + return v1.StorageMediumHugePages, !notMnt, nil, nil + } + // Get page size for the volume mount + pageSize, err := getPageSize(path, m.mounter) + if err != nil { + return v1.StorageMediumHugePages, !notMnt, nil, err + } + return v1.StorageMediumHugePages, !notMnt, pageSize, nil + } + return v1.StorageMediumDefault, !notMnt, nil, nil } diff --git a/pkg/volume/emptydir/empty_dir_test.go b/pkg/volume/emptydir/empty_dir_test.go index 12e5820f2f0..bd813926c2c 100644 --- a/pkg/volume/emptydir/empty_dir_test.go +++ b/pkg/volume/emptydir/empty_dir_test.go @@ -19,6 +19,8 @@ limitations under the License. package emptydir import ( + "fmt" + "io/ioutil" "os" "path/filepath" "testing" @@ -73,8 +75,8 @@ type fakeMountDetector struct { isMount bool } -func (fake *fakeMountDetector) GetMountMedium(path string) (v1.StorageMedium, bool, error) { - return fake.medium, fake.isMount, nil +func (fake *fakeMountDetector) GetMountMedium(path string, requestedMedium v1.StorageMedium) (v1.StorageMedium, bool, *resource.Quantity, error) { + return fake.medium, fake.isMount, nil, nil } func TestPluginEmptyRootContext(t *testing.T) { @@ -582,3 +584,331 @@ func TestGetHugePagesMountOptions(t *testing.T) { }) } } + +type testMountDetector struct { + pageSize *resource.Quantity + isMnt bool + err error +} + +func (md *testMountDetector) GetMountMedium(path string, requestedMedium v1.StorageMedium) (v1.StorageMedium, bool, *resource.Quantity, error) { + return v1.StorageMediumHugePages, md.isMnt, md.pageSize, md.err +} + +func TestSetupHugepages(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "TestSetupHugepages") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + + pageSize2Mi := resource.MustParse("2Mi") + + testCases := map[string]struct { + path string + ed *emptyDir + shouldFail bool + }{ + "Valid: mount expected": { + path: tmpdir, + ed: &emptyDir{ + medium: v1.StorageMediumHugePages, + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: &mount.FakeMounter{}, + mountDetector: &testMountDetector{ + pageSize: &resource.Quantity{}, + isMnt: false, + err: nil, + }, + }, + shouldFail: false, + }, + "Valid: already mounted with correct pagesize": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages-2Mi", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: mount.NewFakeMounter([]mount.MountPoint{{Path: tmpdir, Opts: []string{"rw", "pagesize=2M", "realtime"}}}), + mountDetector: &testMountDetector{ + pageSize: &pageSize2Mi, + isMnt: true, + err: nil, + }, + }, + shouldFail: false, + }, + "Valid: already mounted": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: mount.NewFakeMounter([]mount.MountPoint{{Path: tmpdir, Opts: []string{"rw", "pagesize=2M", "realtime"}}}), + mountDetector: &testMountDetector{ + pageSize: nil, + isMnt: true, + err: nil, + }, + }, + shouldFail: false, + }, + "Invalid: mounter is nil": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages-2Mi", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: nil, + }, + shouldFail: true, + }, + "Invalid: GetMountMedium error": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages-2Mi", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: mount.NewFakeMounter([]mount.MountPoint{{Path: tmpdir, Opts: []string{"rw", "pagesize=2M", "realtime"}}}), + mountDetector: &testMountDetector{ + pageSize: &pageSize2Mi, + isMnt: true, + err: fmt.Errorf("GetMountMedium error"), + }, + }, + shouldFail: true, + }, + "Invalid: medium and page size differ": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages-1Gi", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-1Gi"): resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + mounter: mount.NewFakeMounter([]mount.MountPoint{{Path: tmpdir, Opts: []string{"rw", "pagesize=2M", "realtime"}}}), + mountDetector: &testMountDetector{ + pageSize: &pageSize2Mi, + isMnt: true, + err: nil, + }, + }, + shouldFail: true, + }, + "Invalid medium": { + path: tmpdir, + ed: &emptyDir{ + medium: "HugePages-NN", + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: &mount.FakeMounter{}, + mountDetector: &testMountDetector{ + pageSize: &resource.Quantity{}, + isMnt: false, + err: nil, + }, + }, + shouldFail: true, + }, + "Invalid: setupDir fails": { + path: "", + ed: &emptyDir{ + medium: v1.StorageMediumHugePages, + pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName("hugepages-2Mi"): resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + }, + mounter: &mount.FakeMounter{}, + }, + shouldFail: true, + }, + } + + for testCaseName, testCase := range testCases { + t.Run(testCaseName, func(t *testing.T) { + err := testCase.ed.setupHugepages(testCase.path) + if testCase.shouldFail && err == nil { + t.Errorf("%s: Unexpected success", testCaseName) + } else if !testCase.shouldFail && err != nil { + t.Errorf("%s: Unexpected error: %v", testCaseName, err) + } + }) + } +} + +func TestGetPageSize(t *testing.T) { + mounter := &mount.FakeMounter{ + MountPoints: []mount.MountPoint{ + { + Device: "/dev/sda2", + Type: "ext4", + Path: "/", + Opts: []string{"rw", "relatime", "errors=remount-ro"}, + }, + { + Device: "/dev/hugepages", + Type: "hugetlbfs", + Path: "/mnt/hugepages-2Mi", + Opts: []string{"rw", "relatime", "pagesize=2M"}, + }, + { + Device: "sysfs", + Type: "sysfs", + Path: "/sys", + Opts: []string{"rw", "nosuid", "nodev", "noexec", "relatime"}, + }, + { + Device: "/dev/hugepages", + Type: "hugetlbfs", + Path: "/mnt/hugepages-1Gi", + Opts: []string{"rw", "relatime", "pagesize=1024M"}, + }, + { + Device: "/dev/hugepages", + Type: "hugetlbfs", + Path: "/mnt/noopt", + Opts: []string{"rw", "relatime"}, + }, + { + Device: "/dev/hugepages", + Type: "hugetlbfs", + Path: "/mnt/badopt", + Opts: []string{"rw", "relatime", "pagesize=NN"}, + }, + }, + } + + testCases := map[string]struct { + path string + mounter mount.Interface + expectedResult resource.Quantity + shouldFail bool + }{ + "Valid: existing 2Mi mount": { + path: "/mnt/hugepages-2Mi", + mounter: mounter, + shouldFail: false, + expectedResult: resource.MustParse("2Mi"), + }, + "Valid: existing 1Gi mount": { + path: "/mnt/hugepages-1Gi", + mounter: mounter, + shouldFail: false, + expectedResult: resource.MustParse("1Gi"), + }, + "Invalid: mount point doesn't exist": { + path: "/mnt/nomp", + mounter: mounter, + shouldFail: true, + }, + "Invalid: no pagesize option": { + path: "/mnt/noopt", + mounter: mounter, + shouldFail: true, + }, + "Invalid: incorrect pagesize option": { + path: "/mnt/badopt", + mounter: mounter, + shouldFail: true, + }, + } + + for testCaseName, testCase := range testCases { + t.Run(testCaseName, func(t *testing.T) { + pageSize, err := getPageSize(testCase.path, testCase.mounter) + if testCase.shouldFail && err == nil { + t.Errorf("%s: Unexpected success", testCaseName) + } else if !testCase.shouldFail && err != nil { + t.Errorf("%s: Unexpected error: %v", testCaseName, err) + } + if err == nil && pageSize.Cmp(testCase.expectedResult) != 0 { + t.Errorf("%s: Unexpected result: %s, expected: %s", testCaseName, pageSize.String(), testCase.expectedResult.String()) + } + }) + } +} diff --git a/pkg/volume/emptydir/empty_dir_unsupported.go b/pkg/volume/emptydir/empty_dir_unsupported.go index b048f76081a..45dc0defa47 100644 --- a/pkg/volume/emptydir/empty_dir_unsupported.go +++ b/pkg/volume/emptydir/empty_dir_unsupported.go @@ -19,6 +19,7 @@ limitations under the License. package emptydir import ( + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/mount" v1 "k8s.io/api/core/v1" @@ -29,6 +30,6 @@ type realMountDetector struct { mounter mount.Interface } -func (m *realMountDetector) GetMountMedium(path string) (v1.StorageMedium, bool, error) { - return v1.StorageMediumDefault, false, nil +func (m *realMountDetector) GetMountMedium(path string, requestedMedium v1.StorageMedium) (v1.StorageMedium, bool, *resource.Quantity, error) { + return v1.StorageMediumDefault, false, nil, nil }