Add support for hugepages in downward API #86102

derekwaynecarr · 2019-12-10T03:03:35Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add the ability to project hugepages-<pagesize> container resource requirements via downward API

Fixes #85148

Special notes for your reviewer:
Adds requests.hugepages-<pagesize> and limits.hugepages-<pagesize> to downward API consistent with cpu, memory, and ephemeral storage.

Does this PR introduce a user-facing change?:

Add support for hugepages to downward API

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

derekwaynecarr · 2019-12-10T03:04:11Z

/sig node

This works with local testing, but need to add more testing before merge.

fedebongio · 2019-12-10T21:18:18Z

/remove-sig api-machinery

liggitt · 2019-12-10T22:33:43Z

fyi - since this is loosening validation, we need to consider how to roll this out safely

derekwaynecarr · 2019-12-10T23:41:03Z

@liggitt agree - assume we could handle similar to ephemeral storage?

mattjmcnaughton

To save you clicking on the link, verify failure is just gofmt :)

Overall, this diff looks to me to be heading in a good direction. I defer to other's on the proper level of testing needed (and also how to address the issue w/ loosening validation).

mattjmcnaughton · 2019-12-11T01:05:35Z

pkg/api/v1/resource/helpers.go

@@ -193,7 +194,28 @@ func ExtractContainerResourceValue(fs *v1.ResourceFieldSelector, container *v1.C
 	case "requests.ephemeral-storage":
 		return convertResourceEphemeralStorageToString(container.Resources.Requests.StorageEphemeral(), divisor)
 	}
-
+	// handle extended standard resources with dynamic names


Slightly tangential question from me - it appears there are ~3 separate ExtractContainerResourceValue functions that we are updating here. Is there the opportunity to consolidate those into a single function which is reused? It feels less than optimal that we have to add the same 20 lines of code to three different functions.

Very possible I'm missing an important difference between them though :)

odinuge · 2020-02-20T10:12:18Z

/cc

derekwaynecarr · 2020-11-09T14:29:25Z

@liggitt ptal, let me know if this looks good to you.

liggitt

plumbing looks correct. primary comment is about computing the validation options consistently in one place

liggitt · 2020-11-05T22:38:39Z

pkg/apis/core/validation/validation.go

-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("resource"), fs.Resource, expressions.List()))
+		// check if the prefix is present
+		foundPrefix := false
+		for _, prefix := range prefixes.List() {


can prefixes be nil here, causing a npe? if not, suggest not passing a pointer

liggitt · 2020-11-09T15:27:37Z

pkg/registry/core/replicationcontroller/strategy.go

-	validationErrorList := validation.ValidateReplicationController(newRc)
-	updateErrorList := validation.ValidateReplicationControllerUpdate(newRc, oldRc)
+	opts := validation.PodValidationOptions{AllowDownwardAPIHugePages: false}
+	oldFailsDownwardAPIValidation := len(validation.ValidateReplicationController(oldRc, opts)) > 0


this is saying any validation failure from any field in the old object means we failed downwardAPI validation, which isn't accurate...

I expected a helper like this (in the same package as DropDisabledTemplateFields, used above) that could be called consistently everywhere:

podOpts := pod.GetValidationOptionsFromTemplate(newRc.Spec.Template, oldRc.Spec.Template) validationErrorList := validation.ValidateReplicationController(newRc, podOpts) updateErrorList := validation.ValidateReplicationControllerUpdate(newRc, oldRc, podOpts)

and used from rcStrategy#Validate like this:

podOpts := pod.GetValidationOptionsFromTemplate(newRc.Spec.Template, nil) return validation.ValidateReplicationController(controller, podOpts)

liggitt · 2020-11-09T15:30:38Z

pkg/registry/core/podtemplate/strategy.go

@@ -53,7 +56,13 @@ func (podTemplateStrategy) PrepareForCreate(ctx context.Context, obj runtime.Obj
 // Validate validates a new pod template.
 func (podTemplateStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	template := obj.(*api.PodTemplate)
-	return corevalidation.ValidatePodTemplate(template)
+	opts := validation.PodValidationOptions{


use unified helper to get opts, passing nil for old template (comment applies to all strategy computation of opts)

liggitt · 2020-11-09T15:30:59Z

pkg/registry/core/podtemplate/strategy.go

-	return corevalidation.ValidatePodTemplateUpdate(template, oldTemplate)
+
+	// Allow downward api usage of hugepages on pod update if feature is enabled or if the old pod already had used them.
+	opts := validation.PodValidationOptions{}


use unified helper to get opts (comment applies to all strategy computation of opts)

liggitt · 2020-11-09T15:31:31Z

pkg/registry/core/pod/strategy.go

@@ -111,9 +113,14 @@ func (podStrategy) AllowCreateOnUpdate() bool {
 func (podStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
 	oldFailsSingleHugepagesValidation := len(validation.ValidatePodSingleHugePageResources(old.(*api.Pod), field.NewPath("spec"))) > 0
 	opts := validation.PodValidationOptions{


use unified helper to get opts

liggitt · 2020-11-09T15:31:58Z

pkg/registry/core/pod/strategy.go

@@ -94,6 +94,8 @@ func (podStrategy) Validate(ctx context.Context, obj runtime.Object) field.Error
 	opts := validation.PodValidationOptions{


use unified helper to get opts, passing nil for old pod

liggitt · 2020-11-09T15:34:07Z

pkg/api/testing/backward_compatibility_test.go

@@ -159,7 +159,7 @@ func TestCompatibility_v1_PodSecurityContext(t *testing.T) {
 	}

 	validator := func(obj runtime.Object) field.ErrorList {
-		return validation.ValidatePodSpec(&(obj.(*api.Pod).Spec), &(obj.(*api.Pod).ObjectMeta), field.NewPath("spec"))
+		return validation.ValidatePodSpec(&(obj.(*api.Pod).Spec), &(obj.(*api.Pod).ObjectMeta), field.NewPath("spec"), validation.PodValidationOptions{})


this is very slightly concerning, because we're not necessarily using the same options we do when validating actual user objects... is there a way we can construct the options using the unified helper from here? (the package deps might not line up right)

liggitt · 2020-11-09T15:35:55Z

pkg/apis/core/validation/validation.go

+		localValidContainerResourceFieldPathPrefixes := sets.NewString()
+		if opts.AllowDownwardAPIHugePages {
+			localValidContainerResourceFieldPathPrefixes = localValidContainerResourceFieldPathPrefixes.Union(validContainerResourceFieldPathPrefixes)
+		}


nit, precompute the "with hugepages" and "without hugepages" sets as package vars and just select the appropriate one to use here... sets are expensive

liggitt · 2020-11-09T15:36:31Z

pkg/apis/core/validation/validation.go

+		localValidContainerResourceFieldPathPrefixes := sets.NewString()
+		if opts.AllowDownwardAPIHugePages {
+			localValidContainerResourceFieldPathPrefixes = localValidContainerResourceFieldPathPrefixes.Union(validContainerResourceFieldPathPrefixes)
+		}


same nit about selecting a precomputed set rather than constructing one

liggitt · 2020-11-09T15:37:37Z

pkg/apis/core/validation/validation.go

@@ -2337,6 +2358,11 @@ func validateContainerResourceDivisor(rName string, divisor resource.Quantity, f
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("divisor"), rName, "only divisor's values 1, 1k, 1M, 1G, 1T, 1P, 1E, 1Ki, 1Mi, 1Gi, 1Ti, 1Pi, 1Ei are supported with the local ephemeral storage resource"))
 		}
 	}
+	if strings.HasPrefix(rName, "requests.hugepages-") || strings.HasPrefix(rName, "limits.hugepages-") {


are these prefixes constants or can this prefix check be made a "is hugepages resource" method

derekwaynecarr · 2020-11-10T17:13:10Z

@liggitt ptal i think i got the structure you wanted now.

liggitt

yeah, that's exactly what I was looking for.

liggitt · 2020-11-10T17:20:08Z

pkg/api/pod/util.go

+	for _, container := range oldPodSpec.InitContainers {
+		opts.AllowDownwardAPIHugePages = opts.AllowDownwardAPIHugePages || usesHugePagesInProjectedEnv(container)
+	}
+	for _, container := range oldPodSpec.Containers {
+		opts.AllowDownwardAPIHugePages = opts.AllowDownwardAPIHugePages || usesHugePagesInProjectedEnv(container)
+	}


use the visitor so we check all containers:

if !opts.AllowDownwardAPIHugePages { VisitContainers(oldPodSpec, AllContainers, func(container *api.Container, containerType ContainerType) (shouldContinue bool) { opts.AllowDownwardAPIHugePages = opts.AllowDownwardAPIHugePages || usesHugePagesInProjectedEnv(container) return !opts.AllowDownwardAPIHugePages }) }

liggitt · 2020-11-10T17:22:44Z

pkg/registry/core/pod/strategy.go

@@ -182,7 +178,9 @@ type podEphemeralContainersStrategy struct {
 var EphemeralContainersStrategy = podEphemeralContainersStrategy{Strategy}

 func (podEphemeralContainersStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidatePodEphemeralContainersUpdate(obj.(*api.Pod), old.(*api.Pod))
+	// NOTE: ephemeral containers are not able to consume hugepages, so its safe to make this empty.
+	opts := validation.PodValidationOptions{}


this assumes these validation options will only be hugepages-related in the future... is there a downside to calling the opts helper here?

derekwaynecarr · 2020-11-10T21:47:06Z

@liggitt had not seen visitor func, thanks for pointer. got requested updates complete. thanks!

liggitt · 2020-11-10T22:43:50Z

change lgtm, CI is unhappy

liggitt · 2020-11-10T22:44:08Z

/approve

will tag once CI issues get figured out

k8s-ci-robot · 2020-11-10T22:44:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [derekwaynecarr,liggitt]
~~pkg/quota/v1/OWNERS~~ [derekwaynecarr,liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kubectl/OWNERS~~ [liggitt]
~~test/e2e/common/OWNERS~~ [derekwaynecarr,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

derekwaynecarr · 2020-11-10T23:03:02Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-gce-alpha-features

derekwaynecarr · 2020-11-11T04:46:12Z

/test pull-kubernetes-e2e-gce-alpha-features

liggitt · 2020-11-11T04:49:31Z

alpha test failures are due to #93873 (comment) and #92312 (comment)

/lgtm

k8s-ci-robot · 2020-11-11T05:36:12Z

@derekwaynecarr: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-alpha-features	`45bd6cb`	link	`/test pull-kubernetes-e2e-gce-alpha-features`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

derekwaynecarr requested a review from liggitt December 10, 2019 03:03

k8s-ci-robot requested review from deads2k and feiskyer December 10, 2019 03:05

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 10, 2019

mattjmcnaughton reviewed Dec 11, 2019

View reviewed changes

derekwaynecarr force-pushed the downward-api branch from 04b7424 to 1a3ee61 Compare December 11, 2019 02:29

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Dec 11, 2019

k8s-ci-robot requested a review from odinuge February 20, 2020 10:12

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 6, 2020

derekwaynecarr force-pushed the downward-api branch 4 times, most recently from 0575a4e to 724ed4a Compare November 6, 2020 20:07

liggitt reviewed Nov 9, 2020

View reviewed changes

derekwaynecarr force-pushed the downward-api branch from 724ed4a to 753bf0b Compare November 10, 2020 00:37

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Nov 10, 2020

derekwaynecarr force-pushed the downward-api branch 3 times, most recently from eef2361 to 2f1881e Compare November 10, 2020 17:12

liggitt reviewed Nov 10, 2020

View reviewed changes

Downward API hugepages

45bd6cb

derekwaynecarr force-pushed the downward-api branch from 2f1881e to 45bd6cb Compare November 10, 2020 21:09

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2020

k8s-ci-robot merged commit a27a357 into kubernetes:master Nov 11, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 11, 2020

github-actions bot mentioned this pull request Nov 18, 2020

Week Ending November 15, 2020 dev-obs/actus#273

Open

derekwaynecarr mentioned this pull request Feb 16, 2021

Add request value verification for hugepage #98515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for hugepages in downward API #86102

Add support for hugepages in downward API #86102

derekwaynecarr commented Dec 10, 2019 •

edited

Loading

derekwaynecarr commented Dec 10, 2019

fedebongio commented Dec 10, 2019

liggitt commented Dec 10, 2019

derekwaynecarr commented Dec 10, 2019

mattjmcnaughton left a comment

mattjmcnaughton Dec 11, 2019

odinuge commented Feb 20, 2020

derekwaynecarr commented Nov 9, 2020

liggitt left a comment

liggitt Nov 5, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

liggitt Nov 9, 2020

derekwaynecarr commented Nov 10, 2020

liggitt left a comment

liggitt Nov 10, 2020

liggitt Nov 10, 2020

derekwaynecarr commented Nov 10, 2020

liggitt commented Nov 10, 2020

liggitt commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

derekwaynecarr commented Nov 10, 2020

derekwaynecarr commented Nov 11, 2020

liggitt commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

		@@ -94,6 +94,8 @@ func (podStrategy) Validate(ctx context.Context, obj runtime.Object) field.Error
		opts := validation.PodValidationOptions{

Add support for hugepages in downward API #86102

Add support for hugepages in downward API #86102

Conversation

derekwaynecarr commented Dec 10, 2019 • edited Loading

derekwaynecarr commented Dec 10, 2019

fedebongio commented Dec 10, 2019

liggitt commented Dec 10, 2019

derekwaynecarr commented Dec 10, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odinuge commented Feb 20, 2020

derekwaynecarr commented Nov 9, 2020

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Nov 10, 2020

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Nov 10, 2020

liggitt commented Nov 10, 2020

liggitt commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

derekwaynecarr commented Nov 10, 2020

derekwaynecarr commented Nov 11, 2020

liggitt commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

derekwaynecarr commented Dec 10, 2019 •

edited

Loading