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

Huge pages KEP: support multiple sizes huge pages on a container level #1271

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Oct 2, 2019

This change adds support of multiple sizes huge pages on a
container level to support the following use cases:

  • VMs running on a Kubernetes infrastructure (QEMU, libvirt, etc)
  • Applications using more than one huge page size

@k8s-ci-robot
Copy link
Contributor

Welcome @bart0sh!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added 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 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. labels Oct 2, 2019
@derekwaynecarr
Copy link
Member

/assign

Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

Just a question on the validation

keps/sig-node/20190129-hugepages.md Show resolved Hide resolved
keps/sig-node/20190129-hugepages.md Show resolved Hide resolved
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Thanks for opening! Other than some small comments, this looks good to me! :)

PR to add node level support is here kubernetes/kubernetes#82820, and I have already been looking at implementing this, so i'll be happy to help with the implementation.

keps/sig-node/20190129-hugepages.md Show resolved Hide resolved
keps/sig-node/20190129-hugepages.md Show resolved Hide resolved
@odinuge
Copy link
Member

odinuge commented Oct 3, 2019

I think we should rewrite (or remove) this chapter:

A system may support multiple huge page sizes. It is assumed that most nodes

I think there is some kind of ambiguity in It is assumed that most nodes will be configured to primarily use the default huge page size as returned via grep Hugepagesize /proc/meminfo. regarding support for multiple page sizes at node level, and that is needed in order to supporting on pod level.

@bart0sh bart0sh force-pushed the PR0002-container-hugepages-multiple-sizes branch from 404fb3d to 27c7e5e Compare October 3, 2019 08:35
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 3, 2019

@derekwaynecarr @odinuge Thank you for the review!

I've updated the PR according to your suggestions.

@bart0sh bart0sh force-pushed the PR0002-container-hugepages-multiple-sizes branch 2 times, most recently from 782854f to 679c33e Compare October 3, 2019 08:57
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

With the latest changes, this looks good to me.

It is really nice with all the clear examples. I cannot see any explicit info (or example) about the fact that mounting hugetlbfs is optional. A pod can request 1GiB of 2MiB pages, without using a volume mount to mount a hugetlbfs. May be nice to have it explicitly stated.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019
@bg-chun
Copy link
Member

bg-chun commented Oct 8, 2019

Updates looks good.

I have just one thing to suggest here.
How about putting a yaml example for the case that Pod consumes without emptyDir backing.
It would look like below.

apiVersion: v1
kind: Pod
metadata:
  name: example
spec:
  containers:
...
    resources:
      requests:
        hugepages-1Gi: 1Gi
      limits:
        hugepages-1Gi: 1Gi

As i know(and KEP says) there are three ways to consume hugepages.
Those are :

  1. mmap() a file backed by hugetlbfs / sample
  2. mmap with MAP_ANONYMOUS / sample
  3. shmget() with SHM_HUGETLB / sample

@bart0sh bart0sh force-pushed the PR0002-container-hugepages-multiple-sizes branch from 679c33e to 8d1a71d Compare October 8, 2019 14:44
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2019
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 8, 2019

@odinuge @bg-chun Thank you for the review and suggestions!

I've added the following text to explicitly mention that emptyDir(hugetlbfs mounts) are optional:

Also, it is important to note that emptyDir usage is not required if pod
consumes huge pages via shmat/shmget system calls or mmap with MAP_HUGETLB.
This is an example of the pod that consumes 1Gi huge pages of size 2Mi and
2Gi huge pages of size 1Gi without using emptyDir:

apiVersion: v1
kind: Pod
metadata:
  name: example
spec:
  containers:
...
    resources:
      requests:
        hugepages-2Mi: 1Gi
        hugepages-1Gi: 2Gi
      limits:
        hugepages-2Mi: 1Gi
        hugepages-1Gi: 2Gi

Does this look more clear now?

@bart0sh bart0sh force-pushed the PR0002-container-hugepages-multiple-sizes branch from 8d1a71d to e92eafc Compare October 8, 2019 14:48
This change adds support of multiple sizes huge pages on a
container level to support the following use cases:

 - VMs running on a Kubernetes infrastructure (QEMU, libvirt, etc)
 - Applications using more than one huge page size
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

Thank you for the updates and clarifying the validation error scenarios.

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, derekwaynecarr, odinuge

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 Oct 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit d41f349 into kubernetes:master Oct 8, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 8, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants