Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from Jun 18, 2019

Conversation

@egernst
Copy link
Contributor

commented Apr 23, 2019

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
Copy link
Member

left a comment

some wording tweaks

Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
@tallclair

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

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

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

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

@egernst egernst force-pushed the egernst:podspec-overhead branch from d2177ca to 0ae7470 Apr 24, 2019

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 24, 2019

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
Copy link
Member

left a comment

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.

Show resolved Hide resolved 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
@yastij
Copy link
Member

left a comment

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

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

@egernst egernst force-pushed the egernst:podspec-overhead branch from 0ae7470 to a115911 Apr 26, 2019

@egernst egernst force-pushed the egernst:podspec-overhead branch from 63caae5 to 56bbb93 Jun 10, 2019

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/L labels Jun 10, 2019

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
}

This comment has been minimized.

Copy link
@tallclair

tallclair Jun 14, 2019

Member

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)

This comment has been minimized.

Copy link
@egernst

egernst Jun 17, 2019

Author Contributor

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

Show resolved Hide resolved pkg/apis/core/types.go Outdated
Show resolved Hide resolved pkg/features/kube_features.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/core/v1/types.go Outdated
Show resolved Hide resolved pkg/apis/node/types.go

@egernst egernst force-pushed the egernst:podspec-overhead branch from 56bbb93 to 013133d Jun 17, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

[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:

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

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

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

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

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

/cc @liggitt @tallclair @thockin @yastij

@egernst egernst force-pushed the egernst:podspec-overhead branch from 013133d to 5e09568 Jun 18, 2019

egernst and others added some commits Apr 23, 2019

pod-overhead: add Overhead to PodSpec internal type
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: add Overhead to PodSpec
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod-overhead: add Overhead to RuntimeClass internal type
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod-overhead: add Overhead to RuntimeClass
Co-authored-by: Tim Allclair <tallclair@google.com>
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod-overhead: Introduce PodOverhead feature gate
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod-overhead: drop from PodSpec based on feature-gate
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod-overhead: autogenerated code updates
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
pod overhead: drop from RuntimeClass base on feature-gate
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

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

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

/test pull-kubernetes-bazel-test

@tallclair

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 18, 2019

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

21 of 23 checks passed

pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation egernst authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@thockin
Copy link
Member

left a comment

API still LGTM

_ = obj.(*node.RuntimeClass)
rc := obj.(*node.RuntimeClass)

if !utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) && rc != nil {

This comment has been minimized.

Copy link
@thockin

thockin Jun 19, 2019

Member

Also need PrepareForUpdate() ?

@@ -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) {

This comment has been minimized.

Copy link
@thockin

thockin Jun 19, 2019

Member

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.