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 cri-plugin to parse hugepages limit #1332

Open
wants to merge 2 commits into
base: master
from

Conversation

@bg-chun
Copy link

bg-chun commented Nov 4, 2019

The purpose of this PR

  • updating CRI vendor to refer the last CRI Update which includes hugepages update
  • updating cri-plugin(this repo) 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;
}
@k8s-ci-robot

This comment has been minimized.

Copy link
Collaborator

k8s-ci-robot commented Nov 4, 2019

Hi @bg-chun. Thanks for your PR.

I'm waiting for a containerd 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.

@@ -483,6 +484,14 @@ func WithResources(resources *runtime.LinuxContainerResources) oci.SpecOpts {
if limit != 0 {
s.Linux.Resources.Memory.Limit = &limit
}
if len(hugepages) != 0 {

This comment has been minimized.

Copy link
@jterry75

jterry75 Nov 4, 2019

Contributor

Why the extra check? range handles nil and len() == 0 and will simply not enumerate

This comment has been minimized.

Copy link
@Random-Liu

This comment has been minimized.

Copy link
@bg-chun
@mikebrow

This comment has been minimized.

Copy link
Member

mikebrow commented Nov 4, 2019

/ok-to-test

@mikebrow

This comment has been minimized.

Copy link
Member

mikebrow commented Nov 4, 2019

I1104 20:49:04.816] golangci-lint run
I1104 20:49:42.591] pkg/containerd/opts/spec_unix.go:462: File is not `gofmt`-ed with `-s` (gofmt)
I1104 20:49:42.592] 			p      = uint64(resources.GetCpuPeriod())
I1104 20:49:42.592] 			q      = resources.GetCpuQuota()
I1104 20:49:42.592] 			shares = uint64(resources.GetCpuShares())
I1104 20:49:42.594] 			limit  = resources.GetMemoryLimitInBytes()
W1104 20:50:08.701] make: *** [Makefile:58: lint] Error 1
@Random-Liu Random-Liu self-assigned this Nov 5, 2019
@Random-Liu

This comment has been minimized.

Copy link
Member

Random-Liu commented Nov 5, 2019

/ok-to-test

@Random-Liu

This comment has been minimized.

Copy link
Member

Random-Liu commented Nov 5, 2019

@bg-chun Can you link to the KEP in the first commit? So that we can better track features. :)

@Random-Liu Random-Liu added this to the v1.4 milestone Nov 5, 2019
Copy link
Member

Random-Liu left a comment

LGTM other than one nit.

Will get this in when you get the actual CRI vendor update.

@@ -483,6 +484,14 @@ func WithResources(resources *runtime.LinuxContainerResources) oci.SpecOpts {
if limit != 0 {
s.Linux.Resources.Memory.Limit = &limit
}
if len(hugepages) != 0 {

This comment has been minimized.

Copy link
@Random-Liu
@bg-chun

This comment has been minimized.

Copy link
Author

bg-chun commented Dec 3, 2019

I will update pr after CRI pr merged on kubernetes.

@bg-chun bg-chun force-pushed the bg-chun:update_cri_for_hugepages branch from cad837c to a1e50ac Dec 18, 2019
@bg-chun

This comment has been minimized.

Copy link
Author

bg-chun commented Dec 18, 2019

As temporary, I updated cri-api vendor to point master branch.
I think we will may have new alpha release tag for (1.18.0-alpha.1) soon.
When kubernetes sig-rel cut new tag I will update the vendor from master to new tag.
kubernetes/sig-release#927

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>
@bg-chun bg-chun force-pushed the bg-chun:update_cri_for_hugepages branch 3 times, most recently from 7935969 to d02097d Dec 19, 2019
@bg-chun

This comment has been minimized.

Copy link
Author

bg-chun commented Dec 20, 2019

/retest

Signed-off-by: Byonggon Chun <bg.chun@samsung.com>
@bg-chun bg-chun force-pushed the bg-chun:update_cri_for_hugepages branch from d02097d to ca43b12 Dec 20, 2019
Copy link
Member

mikebrow left a comment

LGTM

@bg-chun

This comment has been minimized.

Copy link
Author

bg-chun commented Dec 21, 2019

@bg-chun Can you link to the KEP in the first commit? So that we can better track features. :)

@Random-Liu
I updated first commit to mention KEP and CRI updates

Update cri-api vendor

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>
@bg-chun bg-chun changed the title [WIP] update cri-plugin to parse hugepages limit update cri-plugin to parse hugepages limit Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.