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.

Sign up for GitHub

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

Jump to bottom

libcontainer, cgroupv2: adapt to new API #2469

Merged
merged 2 commits into from
May 21, 2020
Merged

libcontainer, cgroupv2: adapt to new API #2469

merged 2 commits into from
May 21, 2020

Conversation

giuseppe
Copy link
Contributor

@giuseppe giuseppe commented Apr 6, 2020

adapt the handlers to use the new libcontainer API for cgroup v2.

Signed-off-by: Giuseppe Scrivano [email protected]

@k8s-ci-robot
Copy link
Collaborator

Hi @giuseppe. Thanks for your PR.

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

@giuseppe giuseppe force-pushed the update-libcontainer branch from 8f686ce to b0d55ce Compare April 6, 2020 15:23
@dashpole
Copy link
Collaborator

dashpole commented Apr 6, 2020

/ok-to-test

@giuseppe giuseppe mentioned this pull request Apr 7, 2020
Closed
@giuseppe giuseppe changed the title [WIP] libcontainer, cgroupv2: adapt to new API libcontainer, cgroupv2: adapt to new API Apr 7, 2020
@giuseppe
Copy link
Contributor Author

giuseppe commented Apr 7, 2020

I've not re-vendored github.com/opencontainers/runc as part of this PR, although the issue for cgroup v2 is fixed: opencontainers/runc#2297

@dashpole should I re-vendor runc here or do we wait for the next release?

@dims
Copy link
Collaborator

dims commented Apr 7, 2020

@giuseppe let's request runc folks to do a rc11 please! looks like a bunch of things have piled up already - opencontainers/runc@v1.0.0-rc10...HEAD and the last release was in Jan.

@giuseppe
Copy link
Contributor Author

giuseppe commented Apr 7, 2020

@AkihiroSuda could we have a new release for runc?

@AkihiroSuda
Copy link

cc @cyphar

@AkihiroSuda
Copy link

Maybe we should merge cgroup2 integration tests before rc11.

opencontainers/runc#2295

@h-vetinari h-vetinari mentioned this pull request Apr 9, 2020
Merged
@Nayana-ibm Nayana-ibm mentioned this pull request Apr 17, 2020
Closed
@dashpole dashpole closed this Apr 17, 2020
@dashpole dashpole reopened this Apr 17, 2020
@giuseppe giuseppe force-pushed the update-libcontainer branch 2 times, most recently from c32acc0 to 847ba06 Compare April 22, 2020 14:29
@giuseppe giuseppe mentioned this pull request Apr 25, 2020
Closed
dashpole
dashpole reviewed Apr 29, 2020
View reviewed changes
@giuseppe giuseppe force-pushed the update-libcontainer branch 2 times, most recently from 007a185 to eb9ce4d Compare May 6, 2020 07:49
@giuseppe
Copy link
Contributor Author

giuseppe commented May 6, 2020

@dims @dashpole it seems a runc release is going to take more time.

Would it be possible to re-vendor for now using the current master and then we can update the dependency again once the release is out?

A new cAdvisor+libcontainer is necessary for the Kubelet to work on cgroup v2

@dims
Copy link
Collaborator

dims commented May 6, 2020

@giuseppe looks like they are trying to fix something for us!! opencontainers/runc#2364 (comment) So no, i would not recommend updating now until they are ready.

@dims dims mentioned this pull request May 6, 2020
Closed
@giuseppe
Copy link
Contributor Author

giuseppe commented May 6, 2020

@giuseppe looks like they are trying to fix something for us!! opencontainers/runc#2364 (comment) So no, i would not recommend updating now until they are ready.

the devices cgroup is not used by the Kubelet, so the fix won't affect Kubernetes

@giuseppe giuseppe mentioned this pull request May 8, 2020
Closed
@giuseppe giuseppe force-pushed the update-libcontainer branch 2 times, most recently from 67d2c7a to c819ae5 Compare May 13, 2020 07:25
@giuseppe giuseppe force-pushed the update-libcontainer branch from c819ae5 to e763a54 Compare May 14, 2020 17:37
@mrunalp
Copy link
Collaborator

mrunalp commented May 19, 2020 •
edited
Loading

It is going to take some to establish a release cadence in runc that lets us iterate quick enough on cgroups v2. I propose that keep we updating runc and then pin to a release which gives us a way to keep making progress on cgroups v2.

@giuseppe
Copy link
Contributor Author

@dashpole @dims given the reasoning above and that we will probably need to iterate more times until we get cgroup v2 right, are you fine with the current PR so we can unblock the Kubelet?

@dims
Copy link
Collaborator

dims commented May 20, 2020

@giuseppe sounds reasonable. please update this PR to latest then?

giuseppe added 2 commits May 20, 2020 16:07
@giuseppe
libcontainer, cgroupv2: adapt to new API
6a95c22 
adapt the handlers to use the new libcontainer API for cgroup v2.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
libcontainer: use inactive_file on cgroup v2
ab2da09 
total_inactive_file is present only on cgroup v1

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the update-libcontainer branch from e763a54 to ab2da09 Compare May 20, 2020 14:07
@giuseppe
Copy link
Contributor Author

@giuseppe sounds reasonable. please update this PR to latest then?

yes sure. I've squashed the runc update commit into the libcontainer, cgroupv2: adapt to new API commit, otherwise the commit will fail to build during a git bisect.

dashpole
dashpole approved these changes May 21, 2020
View reviewed changes
Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm

@dashpole dashpole merged commit 9253ce0 into google:master May 21, 2020
@dims dims mentioned this pull request May 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@giuseppe @k8s-ci-robot @dashpole @dims @AkihiroSuda @mrunalp