Skip to content

Conversation

@egernst
Copy link

@egernst egernst commented Feb 10, 2020

Pull Request for updating PodOverhead documentation for 1.18, beta.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 10, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Feb 10, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 9f6ed1c

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e6906a236db4c0008104ee5

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 10, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 10, 2020
@makoscafee
Copy link
Contributor

/milestone 1.18

@k8s-ci-robot k8s-ci-robot added this to the 1.18 milestone Feb 19, 2020
@irvifa
Copy link
Member

irvifa commented Mar 3, 2020

@egernst Hi 👋 please don't forget to add any technical reviewer for this docs if it's ready to be reviewed. Thanks!

@egernst
Copy link
Author

egernst commented Mar 3, 2020

@irvifa - can you help identify appropriate tech reviewers (are you one?)?

I have a local draft I'm cleaning up and will submit soon.

@irvifa
Copy link
Member

irvifa commented Mar 3, 2020

Hi @egernst I'm pinging Tim and Vineeth. Nope I'm not a tech reviewer for this, you also can probably assign one of your SIG member to review your docs as well..

Hi @sftim by the way do you aware on whom should I contact for the tech reviewer cc @VineethReddy02 as well..

@sftim
Copy link
Contributor

sftim commented Mar 4, 2020

SIG Node is the owning SIG here, so I'd ping @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Mar 4, 2020
@sftim
Copy link
Contributor

sftim commented Mar 4, 2020

@egernst you should add a /hold if the feature graduation is not confirmed, otherwise you don't need a hold
and then you should retitle to remove the WIP status (highlighting that this is ready for approval).

@irvifa
Copy link
Member

irvifa commented Mar 6, 2020

@egernst since we're close to the deadlines do you already confirm the graduation and whether this PR is ready to be reviewed? Thanks!

@egernst
Copy link
Author

egernst commented Mar 7, 2020

It will be graduating, but this PR isn't ready yet. Hoping to do finalize PR this weekend.

@VineethReddy02
Copy link
Contributor

👋 Please rebase this PR on dev-1.18 in order to avoid merge conflicts.

Feel free to /hold cancel after you've rebased.

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 7, 2020
@egernst egernst force-pushed the pod-overhead-docs branch 4 times, most recently from 7ab7b1d to 710355e Compare March 8, 2020 05:13
@egernst egernst changed the title WIP: pod-overhead: updates for beta pod-overhead: updates for beta Mar 8, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

A bit more feedback.

I don't really understand how the reader works out what do to when they see:

root@kind-worker2:~# cat /sys/fs/cgroup/memory/kubepods/podd7f4b509-cf94-4951-9417-d1087c92a5b2/memory.limit_in_bytes


Verify the pod level cgroup:
```bash
root@kind-worker2:~# cat /sys/fs/cgroup/memory/kubepods/podd7f4b509-cf94-4951-9417-d1087c92a5b2/memory.limit_in_bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't valid bash code.

You could revise this along the lines of:

Suggested change
root@kind-worker2:~# cat /sys/fs/cgroup/memory/kubepods/podd7f4b509-cf94-4951-9417-d1087c92a5b2/memory.limit_in_bytes
# Run this inside the node where the Pod is scheduled
#
# Also, change the name of the cgroup to match the cgroup that your container runtime allocated
cat /sys/fs/cgroup/memory/kubepods/podd7f4b509-cf94-4951-9417-d1087c92a5b2/memory.limit_in_bytes

something like that?

@egernst egernst force-pushed the pod-overhead-docs branch 2 times, most recently from e867203 to af088e0 Compare March 9, 2020 20:23
@egernst
Copy link
Author

egernst commented Mar 9, 2020

Thanks @sftim. I reworded the "on the node" portion - let me know if this makes more sense.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Nice, thank you @egernst
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2020
Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few minor suggestions.

After the RuntimeClass admission controller, you can check the updated PodSpec:

```bash
kubectl get pod test-pod -o jsonpath='{.spec.overhead}'
Copy link
Member

Choose a reason for hiding this comment

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

Is pod overhead included in the output of kubectl describe? If so, that might be a more user-friendly way of checking

Copy link
Author

Choose a reason for hiding this comment

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

It is not part of kubectl describe.

@egernst egernst force-pushed the pod-overhead-docs branch from af088e0 to b08c3ab Compare March 10, 2020 19:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2020
@egernst
Copy link
Author

egernst commented Mar 10, 2020

Great suggestions; thanks @tallclair

Updated ya'll - PTAL.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Looks good to me; one nit.

@egernst egernst force-pushed the pod-overhead-docs branch from b08c3ab to d357eca Compare March 11, 2020 15:40
@egernst egernst force-pushed the pod-overhead-docs branch from d357eca to 9f6ed1c Compare March 11, 2020 15:41
@egernst
Copy link
Author

egernst commented Mar 11, 2020

Updated @sftim PTAL

@VineethReddy02
Copy link
Contributor

Hi @egernst
We are very close to the docs final deadline i.e All 1.18 enhancements docs should be in ready to merge state by Monday 16th March. Please make sure to complete all the docs reviews before the deadline.

Thanks!

@egernst
Copy link
Author

egernst commented Mar 13, 2020

@tallclair @sftim -- thanks for the reviews and great feedback. Can you help take one more pass?

Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 13, 2020
@derekwaynecarr
Copy link
Member

/approve

@irvifa
Copy link
Member

irvifa commented Mar 14, 2020

/assign @sftim

@irvifa
Copy link
Member

irvifa commented Mar 14, 2020

/assign @VineethReddy02

@VineethReddy02
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit a534370 into kubernetes:dev-1.18 Mar 14, 2020
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants