Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

cpuset: fixes to address VM sizing and constraining based on cpuset.mems #2972

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

egernst
Copy link
Member

@egernst egernst commented Sep 18, 2020

  • CPUSet cgroup allows for pinning the memory associated with a cpuset to
    a given numa node. Similar to cpuset.cpus, we should take cpuset.mems
    into account for the sandbox-cgroup that Kata creates.
  • If quota is not being enforced on any containers but CPUsets are
    specified, take the number of CPU sets into account when sizing the
    virtual machine.

CPUSet cgroup allows for pinning the memory associated with a cpuset to
a given numa node. Similar to cpuset.cpus, we should take cpuset.mems
into account for the sandbox-cgroup that Kata creates.

Fixes: kata-containers#2970

Signed-off-by: Eric Ernst <[email protected]>
@egernst
Copy link
Member Author

egernst commented Sep 18, 2020

/cc @mcastelino @jcvenegas @lifupan

@egernst
Copy link
Member Author

egernst commented Sep 19, 2020

/test

@fidencio
Copy link
Member

@egernst, please take a look:

INFO: Running 'go test' as current user on package 'github.com/kata-containers/runtime/virtcontainers' with flags '-v -race -timeout 30s'
# github.com/kata-containers/runtime/virtcontainers [github.com/kata-containers/runtime/virtcontainers.test]
virtcontainers/sandbox_test.go:139:8: assignment mismatch: 1 variable but sandbox.calculateSandboxCPUs returns 2 values
FAIL	github.com/kata-containers/runtime/virtcontainers [build failed]
FAIL
make: *** [Makefile:680: go-test] Error 2
Build step 'Execute shell' marked build as failure

And a general comment, we're now mixing cpu and memory sets / getters. Wouldn't be better to have one function for each one of those, as we already do in the sandbox level?

@fidencio
Copy link
Member

And a general comment, we're now mixing cpu and memory sets / getters. Wouldn't be better to have one function for each one of those, as we already do in the sandbox level?

Hmm. No, thinking a little bit more about this, no. It's specfically related to cpuset. So, please, ignore my previous suggestion.

@egernst
Copy link
Member Author

egernst commented Sep 21, 2020

/test

@egernst
Copy link
Member Author

egernst commented Sep 21, 2020

/test

@egernst
Copy link
Member Author

egernst commented Sep 21, 2020

(sorry to keep thrashing the CI).

/test

@egernst
Copy link
Member Author

egernst commented Sep 21, 2020

/test

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #2972 into master will decrease coverage by 6.48%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##           master    #2972      +/-   ##
==========================================
- Coverage   51.65%   45.17%   -6.49%     
==========================================
  Files         118      118              
  Lines       17430    15565    -1865     
==========================================
- Hits         9003     7031    -1972     
- Misses       7342     7667     +325     
+ Partials     1085      867     -218     

If quota is not being enforced on any containers but CPUsets are
specified, take the number of CPU sets into account when sizing the
virtual machine.

Fixes: kata-containers#2971

Signed-off-by: Eric Ernst <[email protected]>
@egernst
Copy link
Member Author

egernst commented Sep 21, 2020

/test

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @egernst.

lgtm

@amshinde amshinde self-requested a review September 23, 2020 23:42
@egernst
Copy link
Member Author

egernst commented Sep 24, 2020

/test metrics-ubuntu

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @egernst

@devimc devimc merged commit 9934054 into kata-containers:master Sep 25, 2020
@jcvenegas jcvenegas mentioned this pull request Oct 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants