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

PodOverhead to GA #3146

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Conversation

SergeyKanzhelev
Copy link
Member

PodOverhead is in beta for a long time. Feedback collected: https://docs.google.com/document/d/1uZNJvR3IPDL--z6H5prgDuOAi22lhUYRXejnbaZHmSk/edit# The feature meets graduation requirement

KEP: #688

/sig node
/assign @egernst
/cc @tallclair
/cc @ehashman

@ehashman for PRR review. This is mostly back-filling. So should be straightforward. Lmk if I need to find somebody else

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jan 15, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Jan 15, 2022
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed first pass for PRR.

keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/kep.yaml Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member Author

Completed first pass for PRR.

Thank you for review! I addressed all the feedback.

@sftim
Copy link
Contributor

sftim commented Jan 31, 2022

Before this graduates to GA, I want to check something. I don't think the docs have covered how to set a default pod overhead.

For example, if you have only a single container runtime and you want to account for the overhead of that runtime, can you do that? Ideally, you then also have a mechanism that lets you add a new RuntimeClass and leave everything where you don't specify a RuntimeClass still using the default runtime and its default resource overheads.

If we don't have that, it's fine. The Pod overhead feature can still graduate. However, I strongly encourage documenting this situation clearly so that if it can't be done, readers can confidently tell that it's not possible.

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
for PRR

Holding off on LGTM due to issue with metadata in kep.yaml

keps/sig-node/688-pod-overhead/kep.yaml Outdated Show resolved Hide resolved
keps/sig-node/688-pod-overhead/README.md Outdated Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member Author

Before this graduates to GA, I want to check something. I don't think the docs have covered how to set a default pod overhead.

Impossible today. Part of the feedback from @tallclair as well:

“Vision for PodOverhead was always that it would be turned on by default eventually. The way I pictured doing so was by configuring a default RuntimeClass that would be auto-applied to pods that don't specify one. Alternatively we could just set a default PodOverhead instead. I don't think that needs to be a blocker for GA, but perhaps something to think about.”

Created: kubernetes/website#31580 to track it

@SergeyKanzhelev
Copy link
Member Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 31, 2022
@dchen1107
Copy link
Member

/approve from SIG Node

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, ehashman, SergeyKanzhelev

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

@k8s-ci-robot k8s-ci-robot merged commit bcda68c into kubernetes:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Development

Successfully merging this pull request may close these issues.

6 participants