pod-overhead: add Overhead to PodSpec and RuntimeClass #76968

egernst · 2019-04-23T23:59:50Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Addition of overhead field to the PodSpec as part of the API changes described in the Pod Overhead KEP

Does this PR introduce a user-facing change?:

Yes

/sig node

Addition of Overhead field to the PodSpec and RuntimeClass types as part of the Pod Overhead KEP

tallclair

some wording tweaks

staging/src/k8s.io/api/core/v1/types.go

tallclair · 2019-04-24T00:25:25Z

/priority important-soon
/sig node
/milestone v1.15

egernst · 2019-04-24T00:51:35Z

Thanks for the quick feedback @tallclair - updated, PTAL!

fejta-bot · 2019-04-24T01:32:18Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

tallclair

If the PodOverhead feature is not enabled, the overhead field needs to be dropped. See https://github.com/kubernetes/kubernetes/blob/master/pkg/api/pod/util.go#L283

I also think it makes sense to add the RuntimeClass API in the same PR, since those fields are intrinsically linked.

pkg/apis/core/types.go

pkg/apis/core/validation/validation.go

yastij

@egernst - can you add the runtimeClass API on a separate commit ?

pkg/apis/core/validation/validation.go

tallclair · 2019-06-14T18:17:12Z

pkg/api/pod/util.go

+	if !utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) && !overheadInUse(oldPodSpec) {
+		// Set Overhead to nil only if the feature is disabled and it is not used
+		podSpec.Overhead = nil
+	}


We need to do the same for RuntimeClass. The framework isn't inplace for it though, so you'll need to call it from the storage strategy: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/node/runtimeclass/strategy.go#L58

(see the pod version for an example: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/pod/strategy.go#L72)

Makes sense, though i didn't see how that relates to storage, per se. Added a commit, PTAL, @tallclair

pkg/apis/core/types.go

pkg/features/kube_features.go

staging/src/k8s.io/api/core/v1/types.go

pkg/apis/node/types.go

k8s-ci-robot · 2019-06-17T21:01:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: egernst, thockin

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:

~~api/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

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

egernst · 2019-06-17T21:09:38Z

This needs a lgtm one more time, since the latest rebase. ping @thockin

egernst · 2019-06-18T04:24:04Z

Prior API review approval for reference: #76968 (review)

/cc @liggitt @tallclair @thockin @yastij

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

Update internal PodSpec to make use of Overhead field. Add validation and validation tests. Signed-off-by: Eric Ernst <eric.ernst@intel.com>

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

Co-authored-by: Tim Allclair <tallclair@google.com> Signed-off-by: Eric Ernst <eric.ernst@intel.com>

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

egernst · 2019-06-18T15:48:02Z

/test pull-kubernetes-e2e-gce-100-performance

egernst · 2019-06-18T16:35:20Z

/test pull-kubernetes-bazel-test

tallclair · 2019-06-18T21:30:44Z

/lgtm

thockin

API still LGTM

thockin · 2019-06-19T00:29:58Z

pkg/registry/node/runtimeclass/strategy.go

-	_ = obj.(*node.RuntimeClass)
+	rc := obj.(*node.RuntimeClass)
+
+	if !utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) && rc != nil {


Also need PrepareForUpdate() ?

thockin · 2019-06-19T00:32:13Z

pkg/apis/core/validation/validation.go

@@ -3095,6 +3101,10 @@ func ValidatePodSpec(spec *core.PodSpec, fldPath *field.Path) field.ErrorList {
 		allErrs = append(allErrs, ValidatePreemptionPolicy(spec.PreemptionPolicy, fldPath.Child("preemptionPolicy"))...)
 	}

+	if spec.Overhead != nil && utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) {


If you have the strategy blanking out the fields, you should not even need to check the gate here - it will be nil, right?

k8s-ci-robot requested review from david-mcmahon and derekwaynecarr April 24, 2019 00:00

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 24, 2019

tallclair reviewed Apr 24, 2019

View reviewed changes

k8s-ci-robot added this to the v1.15 milestone Apr 24, 2019

egernst changed the title ~~WIP: PodSpec overhead~~ pod-overhead: add overhead to the PodSpec Apr 24, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2019

egernst changed the title ~~pod-overhead: add overhead to the PodSpec~~ pod-overhead: add Overhead to the PodSpec Apr 24, 2019

egernst force-pushed the podspec-overhead branch from d2177ca to 0ae7470 Compare April 24, 2019 00:50

tallclair added the api-review Categorizes an issue or PR as actively needing an API review. label Apr 25, 2019

tallclair reviewed Apr 25, 2019

View reviewed changes

pkg/apis/core/types.go Show resolved Hide resolved

pkg/apis/core/types.go Outdated Show resolved Hide resolved

pkg/apis/core/validation/validation.go Show resolved Hide resolved

yastij reviewed Apr 25, 2019

View reviewed changes

pkg/apis/core/validation/validation.go Outdated Show resolved Hide resolved

egernst force-pushed the podspec-overhead branch from 0ae7470 to a115911 Compare April 26, 2019 20:25

egernst force-pushed the podspec-overhead branch from b0fe6bc to 63caae5 Compare June 5, 2019 15:47

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2019

egernst force-pushed the podspec-overhead branch from 63caae5 to 56bbb93 Compare June 10, 2019 15:29

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2019

tallclair reviewed Jun 14, 2019

View reviewed changes

egernst force-pushed the podspec-overhead branch from 56bbb93 to 013133d Compare June 17, 2019 20:59

k8s-ci-robot requested review from liggitt, tallclair, thockin and yastij June 18, 2019 04:24

egernst force-pushed the podspec-overhead branch from 013133d to 5e09568 Compare June 18, 2019 14:58

Eric Ernst and others added 8 commits June 18, 2019 08:05

pod-overhead: Introduce PodOverhead feature gate

fc312d4

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: add Overhead to PodSpec

5584774

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: add Overhead to PodSpec internal type

d0b0c0a

Update internal PodSpec to make use of Overhead field. Add validation and validation tests. Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: drop from PodSpec based on feature-gate

3371308

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: add Overhead to RuntimeClass

bcb046a

Co-authored-by: Tim Allclair <tallclair@google.com> Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: add Overhead to RuntimeClass internal type

2c52cc3

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod overhead: drop from RuntimeClass base on feature-gate

ec8ef0d

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

pod-overhead: autogenerated code updates

5e09568

Signed-off-by: Eric Ernst <eric.ernst@intel.com>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2019

k8s-ci-robot merged commit 155f4a6 into kubernetes:master Jun 18, 2019

thockin reviewed Jun 19, 2019

View reviewed changes

liggitt moved this from Assigned to API review completed, 1.16 in API Reviews Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pod-overhead: add Overhead to PodSpec and RuntimeClass #76968

pod-overhead: add Overhead to PodSpec and RuntimeClass #76968

egernst commented Apr 23, 2019 •

edited

tallclair left a comment

tallclair commented Apr 24, 2019

egernst commented Apr 24, 2019

fejta-bot commented Apr 24, 2019

tallclair left a comment

yastij left a comment

tallclair Jun 14, 2019

egernst Jun 17, 2019 •

edited

k8s-ci-robot commented Jun 17, 2019

egernst commented Jun 17, 2019

egernst commented Jun 18, 2019

egernst commented Jun 18, 2019

egernst commented Jun 18, 2019

tallclair commented Jun 18, 2019

thockin left a comment

thockin Jun 19, 2019

thockin Jun 19, 2019

pod-overhead: add Overhead to PodSpec and RuntimeClass #76968

pod-overhead: add Overhead to PodSpec and RuntimeClass #76968

Conversation

egernst commented Apr 23, 2019 • edited

tallclair left a comment

Choose a reason for hiding this comment

tallclair commented Apr 24, 2019

egernst commented Apr 24, 2019

fejta-bot commented Apr 24, 2019

tallclair left a comment

Choose a reason for hiding this comment

yastij left a comment

Choose a reason for hiding this comment

tallclair Jun 14, 2019

Choose a reason for hiding this comment

egernst Jun 17, 2019 • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 17, 2019

egernst commented Jun 17, 2019

egernst commented Jun 18, 2019

egernst commented Jun 18, 2019

egernst commented Jun 18, 2019

tallclair commented Jun 18, 2019

thockin left a comment

Choose a reason for hiding this comment

thockin Jun 19, 2019

Choose a reason for hiding this comment

thockin Jun 19, 2019

Choose a reason for hiding this comment

egernst commented Apr 23, 2019 •

edited

egernst Jun 17, 2019 •

edited