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

cpuset: don't set cpuset.mems in the guest #2944

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

egernst
Copy link
Member

@egernst egernst commented Sep 10, 2020

Kata doesn't map any numa topologies in the guest. Let's make sure we
clear the Cpuset fields before passing container updates to the
guest. Without this, we could encounter a runtime failure:

process_linux.go:297: applying cgroup configuration for process caused
"failed to write 0,1 to cpuset.mems: writea 0,1 to cpuset.mems: write
0,1 to cpuset.mems: write /sys/fs/cgroup/cpuset .... /cpuset.mems:
numerical result out of range"": unknown

Note, in the future we may want to have a vCPU to guest CPU mapping and
still include the cpuset.Cpus. Until we have this support, clear this as
well.

Fixes: #2176
Depends-on: github.com/kata-containers/tests#2846

@egernst
Copy link
Member Author

egernst commented Sep 10, 2020

/test

@egernst
Copy link
Member Author

egernst commented Sep 10, 2020

/test

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #2944 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2944      +/-   ##
==========================================
+ Coverage   51.65%   51.67%   +0.02%     
==========================================
  Files         118      118              
  Lines       17430    17438       +8     
==========================================
+ Hits         9003     9011       +8     
  Misses       7342     7342              
  Partials     1085     1085              

@fidencio
Copy link
Member

@egernst, the errors seem related to your change, please, take a look at them.

Kata doesn't map any numa topologies in the guest. Let's make sure we
clear the Cpuset fields before passing container updates to the
guest. Without this, we could encounter a runtime failure:

```
process_linux.go:297: applying cgroup configuration for process caused
"failed to write 0,1 to cpuset.mems: writea 0,1 to cpuset.mems: write
0,1 to cpuset.mems: write /sys/fs/cgroup/cpuset .... /cpuset.mems:
numerical result out of range"": unknown
```

Note, in the future we may want to have a vCPU to guest CPU mapping and
still include the cpuset.Cpus. Until we have this support, clear this as
well.

Fixes: kata-containers#2176
Depends-on: github.com/kata-containers/tests#2846

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

egernst commented Sep 10, 2020

/test

@egernst
Copy link
Member Author

egernst commented Sep 10, 2020

@egernst, the errors seem related to your change, please, take a look at them.

Yeah, needed to update the tests. The existing cpuset tests assumed we were setting them in the guest. We shouldn't, imo - this is confusing, wrong, prone to not doing what you think it should, and doesn't add anything good at this point. :) Once we map vCPU - > guest CPU, we'll be able to apply a more appropriate CPUSet mapping from host to guest. Until then......

@egernst egernst requested review from devimc and fidencio and removed request for devimc September 10, 2020 23:38
@egernst
Copy link
Member Author

egernst commented Sep 10, 2020

flakey test from centos: /cc @chavafg @GabyCT

1..1
not ok 1 test netmon
# (in test file integration/netmon/netmon_test.bats, line 60)
#   `[ "$after_interfaces" -gt "$before_interfaces" ]' failed
# 
# cb3d5ab7a2d048dd66846b3ab119b1da0e0c2970abcbe5636967416090988d20
# ba034ab9d2b94c43a80356291de15b1fd3815494643d34fd706752a435c72c5e
# containerA
# containerA
# test

@egernst
Copy link
Member Author

egernst commented Sep 11, 2020

/test

@egernst
Copy link
Member Author

egernst commented Sep 11, 2020

It seems the metrics job has been failing for a while now. Footprint has really crept up, I think we should look at 'why'. The particular test has failed since we've updated virtiofsd?

| *F* | memory-footprint     | 95.0% | 106.1% | 105.0% | 10.0% | 106.1% | 106.1% | 0.0%  | 0.0% |   1 |

@egernst
Copy link
Member Author

egernst commented Sep 11, 2020

/test

@jodh-intel
Copy link
Contributor

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, but yeah, would be good to add a UT to this PR 😄

Also, presumably this needs porting to 2.0?

@jodh-intel jodh-intel added forward-port Change from an older branch / repository port-to-2.0 PRs that need to be ported to kata 2.0-dev branch labels Sep 11, 2020
Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

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

lgtm!

I'd like to have @devimc's input here before getting this one merged!

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.

@egernst thanks, could you update the limitations document?

@fidencio fidencio added the backport Change from a newer branch / repository label Sep 11, 2020
@fidencio
Copy link
Member

I've added the "backport" label as I'd really like to see this one as part of the stable-1.11 branch.

@fidencio fidencio added needs-forward-port Changes need to be applied to a newer branch / repository and removed backport Change from a newer branch / repository forward-port Change from an older branch / repository labels Sep 11, 2020
@jodh-intel jodh-intel added forward-port Change from an older branch / repository needs-backport Changes need to be applied to an older branch / repository labels Sep 11, 2020
@fidencio fidencio removed the forward-port Change from an older branch / repository label Sep 11, 2020
@egernst
Copy link
Member Author

egernst commented Sep 11, 2020

How should we handle metrics CI? Blocking until the test is fixed?

@fidencio
Copy link
Member

@jodh-intel, side note, I've changed your labels.
Seems that "forward-port" and "backport" labels are referring to the specific PR, whether this specific PR is a forward-port or a backport of a patch.

The labels we should be using, though, are the "needs-backport" and "needs-forward-port"

@jodh-intel
Copy link
Contributor

I've added the "backport" label as I'd really like to see this one as part of the stable-1.11 branch.

@fidencio - Agreed. However, the label to use for that is needs-backport: the backport label is for a PR that itself is a backport PR. The behaviour is summarised on kata-containers/kata-containers#634 (comment), but I'll be updating the docs hopefully today on kata-containers/community#172 and letting the ML know...

@jodh-intel
Copy link
Contributor

@fidencio - \o/ :smile:

@fidencio
Copy link
Member

/test

@jodh-intel
Copy link
Contributor

@egernst - I've respun the failing CI's. Are you planning on working on a 2.0 port of this?

@egernst egernst merged commit d520669 into kata-containers:master Oct 5, 2020
@fidencio fidencio mentioned this pull request Oct 19, 2020
@jodh-intel
Copy link
Contributor

forward port PR: kata-containers/kata-containers#933

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-backport Changes need to be applied to an older branch / repository needs-forward-port Changes need to be applied to a newer branch / repository port-to-2.0 PRs that need to be ported to kata 2.0-dev branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpuset.mems: numerical result out of range
4 participants