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

Update hugepages KEP for 1.18 rel #1540

Merged
merged 4 commits into from
Feb 7, 2020

Conversation

bg-chun
Copy link
Member

@bg-chun bg-chun commented Feb 5, 2020

Update hugepages KEP to meet release checklist.
In the checklist, there are :

  • Test plan is in place, giving consideration to SIG Architecture and SIG Testing input
  • "Implementation History" section is up-to-date for milestone

Full version of checklist can be found here:
https://github.com/kubernetes/enhancements/blob/master/keps/YYYYMMDD-kep-template.md#release-signoff-checklist

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Feb 5, 2020
@bg-chun
Copy link
Member Author

bg-chun commented Feb 5, 2020

/cc @derekwaynecarr @odinuge @bart0sh @kad
I opened PR to update hugepages KEP for 1.18 rel.
Can youguys take a look?

@derekwaynecarr
Copy link
Member

this all looks good to me and maps what was presented and discussed in sig-node over past couple months. appreciate the update.

/approve
/lgtm

/hold pending exception approval from sig-release.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2020
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 5, 2020
Byonggon Chun added 4 commits February 6, 2020 01:55
Update Implementation History for 1.18 rel to meet the release checklist.
The checklist requires that "Implementation History" section is up-to-date for milestone

Signed-off-by: Byonggon Chun <[email protected]>
@bg-chun bg-chun force-pushed the update_hugepages_kep branch from 86bb2f9 to 7333a5f Compare February 5, 2020 16:59
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 5, 2020
@bg-chun
Copy link
Member Author

bg-chun commented Feb 5, 2020

@derekwaynecarr
As sig-rel asked adding a release checklist, I added it.
Also, I added future plan of e2e tests.

Copy link
Contributor

@alejandrox1 alejandrox1 left a comment

Choose a reason for hiding this comment

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

Hi @bg-chun
thank you for the updates!


- A test plan will consist of the following tests
- Unit tests
- Each unit test for enhancement will be implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you already have unit tests in place or will these need to be built ?
If you do have unit tests in place can you give us an idea of what they test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have unit tests.
Here are major PRs for changes and all include unittests.
It's hard to explain how unittests works just right now.
(I also need a time to take a look, I reviewed unittest of the first PR alost months ago...)

  1. Implement support for setting hugepages limit on container cgroup sandbox. kubernetes#84154 //merged
  2. Add support for pre-allocated hugepages with 2+ sizes kubernetes#82820 //merged
  3. Implement support for multiple sizes huge pages kubernetes#84051 //derek will merge after got approval of this PR from sig-rel

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Container Isolation of hugepages
    A unit test validates size(in a string) whether it has been transrated to right format e.g., 1024(pod spec) ->1KB(CRI)
    Also, another unit test validates transformation results from v1 resource list to the list of CRI data structures.

  2. Multiple sizes of hugepage support
    A unit test checks node has the right amount of hugepage resources for each hugepage size as given.
    Other unit test requests multiple sizes of hugeapges for a container, then checks enabled number of hugepages sizes on a container.
    Also, another unit test validates whether requested medium HugePages-<hugepagesize> has the right format.

Comment on lines +565 to +566
- There is a test suit for huge pages in e2e test, it will be extended to validate enhancements.
- here: https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/hugepages_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

How will it be extended?
How much work do you think will have to be done to get those e2e test to validate the changes in this kep?
Are there any open PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

My team member opened PR to update e2etest only for container isolation of hugepages.
kubernetes/kubernetes#87118
(In the PR, we added a new test case on hugepages test suit.)

So, I think another PR should be opened for supporting multiple size of hugepages by these guys(@bart0sh @odinuge)

That(need for another PR) is the reason why I added e2etest ont the future plan:

### Version 1.19[TBD]

Extending of huge pages test suit of E2E tests and cri-tools for enhancements after GA.

Copy link
Member Author

@bg-chun bg-chun Feb 6, 2020

Choose a reason for hiding this comment

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

kubernetes/kubernetes#87118
In the e2e test, a new test case sets a hugepage limit before creating a container, and then when container created, it checks container has the limit as given.
(test case takes given hugepages limit numbers from CRI runtime logs by using regexp. It is the same mechanism as to how CPU Manager e2e test works.)

Comment on lines +568 to +569
- Test case will be added to cri-tools to be used in CRI runtime' test(CI).
- here: https://github.com/kubernetes-sigs/cri-tools
Copy link
Contributor

Choose a reason for hiding this comment

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

How much work will this be?
How much do you think needs to be built to test this kep?

Copy link
Member Author

Choose a reason for hiding this comment

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

This works will be done in the almost same manner of extending e2e-node testsuit of hugepages.
In the cri-tools, It already has testsuit for hugepages.
So, I will add additional test cases on the test suite for both of enhancements.

The mechanism will be:

  1. build container cgroup configuration with hugepages config.
    (Now hugepages config is set on only pod config with only one size of hugepages)
  2. send send container cgroup configuration to CRI runtimes
  3. get container creation result from CRI runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it can be done in 1.19

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for more detail: containerd project uses cri-tools in it's CI.

@jeremyrickard
Copy link
Contributor

Exception was granted!

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2020
@derekwaynecarr
Copy link
Member

/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 Feb 7, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bg-chun, derekwaynecarr

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 6427a0b into kubernetes:master Feb 7, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 7, 2020
RomanBednar pushed a commit to RomanBednar/enhancements that referenced this pull request Apr 5, 2024
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants