update createSandboxContainer to parse hugepages limits from CRI message #2940

bg-chun · 2019-11-04T05:59:22Z

The purpose of this PR

updating vendor to refer the last CRI Update which includes hugepages update
updating CRI-O to parse newly added hugepages fields from CRI message.

History

In Kubernetes, hugepages are requested as the container level resources, but limit is set on only pod level cgroup due to the lack of CRI support.
But now, CRI has been updated to have hugepages field so that kubernetes be able to set hugepages limit on container level cgroup.
The last step we have to do is updating container runtime which handles container level cgroup to parse new hugepage field from CRI message and set limit on container level cgroup.

The Detail of CRI Update

Now, LinuxContainerResources CRI message, which has linux resource fileds that equals to linux cgroup subsystems , has hugepage field(hugepage_limits), it is able to express list of hugepage configuration.
New hugepage fields are written based on existing hugepage fields of OCI runtime spec so that is is easy to transfrom hugepages fields and values from CRI message to OCI Runtime configuration.
Each hugepage configuration(HugepageLimit) expesss hugepage limit per page size.
The configuration [page_size: "2MB", limit: 209715200] means set 2GB limits for 2MB hugepage size on the container level cgroup sandbox.
More details are available on this slide and doc.

message LinuxContainerResources {
    ...
    string cpuset_mems = 7;
    // List of HugepageLimits to limit the HugeTLB usage of container per page size. Default: nil (not specified).
    repeated HugepageLimit hugepage_limits = 8;
}
// HugepageLimit corresponds to the file`hugetlb.<hugepagesize>.limit_in_byte` in container level cgroup.
// For example, `PageSize=1GB`, `Limit=1073741824` means setting `1073741824` bytes to hugetlb.1GB.limit_in_bytes.
message HugepageLimit {
    // The value of PageSize has the format <size><unit-prefix>B (2MB, 1GB),
    // and must match the <hugepagesize> of the corresponding control file found in `hugetlb.<hugepagesize>.limit_in_bytes`.
    // The values of <unit-prefix> are intended to be parsed using base 1024("1KB" = 1024, "1MB" = 1048576, etc).
    string page_size = 1;
    // limit in bytes of hugepagesize HugeTLB usage.
    uint64 limit = 2;
}

openshift-ci-robot · 2019-11-04T05:59:36Z

Hi @bg-chun. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2019-11-04T05:59:36Z

Hi @bg-chun. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2019-11-04T06:07:33Z

Codecov Report

Merging #2940 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
- Coverage   46.35%   46.33%   -0.02%     
==========================================
  Files          89       89              
  Lines        7346     7349       +3     
==========================================
  Hits         3405     3405              
- Misses       3639     3642       +3     
  Partials      302      302

saschagrunert · 2019-11-04T07:55:53Z

/ok-to-test

Thanks, @bg-chun 🙏

mrunalp · 2019-11-04T22:29:21Z

@bg-chun We need to update cri api in go.mod for pulling in the proto changes. Thanks!

bg-chun · 2019-12-03T23:08:12Z

I will update pr after CRI pr merged on kubernetes.

giuseppe · 2019-12-13T08:36:35Z

server/container_create_linux.go

@@ -491,6 +491,13 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID, contai
 			specgen.SetProcessOOMScoreAdj(int(resources.GetOomScoreAdj()))
 			specgen.SetLinuxResourcesCPUCpus(resources.GetCpusetCpus())
 			specgen.SetLinuxResourcesCPUMems(resources.GetCpusetMems())
+
+			hugepageLimits := resources.GetHugepageLimits()
+			if len(hugepageLimits) != 0 {


is the if len(hugepageLimits) != 0 check redundant?

Yes remove this the range command below will handle this situation.

bg-chun · 2019-12-21T02:03:05Z

The description of PR has been updated :)

update cri-api vendor to include hugepages changes KEP: kubernetes/enhancements#1199 CRI: kubernetes/kubernetes#83614 Signed-off-by: Byonggon Chun <bg.chun@samsung.com>

Signed-off-by: Byonggon Chun <bg.chun@samsung.com>

mrunalp · 2020-01-07T18:16:35Z

Looks fine to me. @saschagrunert you want to check the dependency update here?

bg-chun · 2020-01-07T21:29:47Z

@mrunalp
I think he checked it before.
https://kubernetes.slack.com/archives/CAZH62UR1/p1576735156005800
BTW, thank you for your effort.

saschagrunert

LGTM 👍

openshift-ci-robot · 2020-01-08T09:25:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [saschagrunert]

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

openshift-ci-robot · 2020-01-08T09:25:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [saschagrunert]

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

haircommander · 2020-01-08T10:30:19Z

/lgtm

bg-chun requested review from mrunalp and runcom as code owners November 4, 2019 05:59

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 4, 2019

openshift-ci-robot requested review from sameo, vrothberg, giuseppe and umohnani8 November 4, 2019 05:59

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 4, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 4, 2019

bg-chun mentioned this pull request Nov 6, 2019

Hugepages PRs Tracker kubernetes/kubernetes#84526

Closed

mrunalp mentioned this pull request Dec 3, 2019

update CRI(LinuxContainerResources ) to support hugepages kubernetes/kubernetes#83614

Merged

giuseppe reviewed Dec 13, 2019

View reviewed changes

bg-chun force-pushed the update_for_hugepages branch from 7e98586 to 0019bc0 Compare December 19, 2019 08:24

bg-chun changed the title ~~[WIP] update createSandboxContainer to parse hugepages limits from CRI message~~ update createSandboxContainer to parse hugepages limits from CRI message Dec 21, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2019

Byonggon Chun added 2 commits December 21, 2019 11:20

Update vendor to v1.18.0-alpha.1

7646a7f

update cri-api vendor to include hugepages changes KEP: kubernetes/enhancements#1199 CRI: kubernetes/kubernetes#83614 Signed-off-by: Byonggon Chun <bg.chun@samsung.com>

update createSandboxContainer to parse hugepages limit from CRI message

1c28b23

Signed-off-by: Byonggon Chun <bg.chun@samsung.com>

bg-chun force-pushed the update_for_hugepages branch from 0019bc0 to 1c28b23 Compare December 21, 2019 02:21

saschagrunert approved these changes Jan 8, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2020

openshift-ci-robot assigned haircommander Jan 8, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. labels Jan 8, 2020

openshift-merge-robot merged commit e2e1e54 into cri-o:master Jan 8, 2020

bg-chun mentioned this pull request Feb 5, 2020

Extending Hugepage Feature kubernetes/enhancements#1539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update createSandboxContainer to parse hugepages limits from CRI message #2940

update createSandboxContainer to parse hugepages limits from CRI message #2940

bg-chun commented Nov 4, 2019 •

edited

Loading

openshift-ci-robot commented Nov 4, 2019

openshift-ci-robot commented Nov 4, 2019

codecov bot commented Nov 4, 2019 •

edited

Loading

saschagrunert commented Nov 4, 2019

mrunalp commented Nov 4, 2019

bg-chun commented Dec 3, 2019

giuseppe Dec 13, 2019

rhatdan Dec 13, 2019

bg-chun Dec 21, 2019

bg-chun commented Dec 21, 2019

mrunalp commented Jan 7, 2020

bg-chun commented Jan 7, 2020

saschagrunert left a comment

openshift-ci-robot commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

haircommander commented Jan 8, 2020

update createSandboxContainer to parse hugepages limits from CRI message #2940

update createSandboxContainer to parse hugepages limits from CRI message #2940

Conversation

bg-chun commented Nov 4, 2019 • edited Loading

The purpose of this PR

History

The Detail of CRI Update

openshift-ci-robot commented Nov 4, 2019

openshift-ci-robot commented Nov 4, 2019

codecov bot commented Nov 4, 2019 • edited Loading

Codecov Report

saschagrunert commented Nov 4, 2019

mrunalp commented Nov 4, 2019

bg-chun commented Dec 3, 2019

giuseppe Dec 13, 2019

Choose a reason for hiding this comment

rhatdan Dec 13, 2019

Choose a reason for hiding this comment

bg-chun Dec 21, 2019

Choose a reason for hiding this comment

bg-chun commented Dec 21, 2019

mrunalp commented Jan 7, 2020

bg-chun commented Jan 7, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

haircommander commented Jan 8, 2020

bg-chun commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 4, 2019 •

edited

Loading