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

fs2: fix cgroup.subtree_control EPERM on rootless + add CI #2340

Merged
merged 1 commit into from
Apr 28, 2020
Merged

fs2: fix cgroup.subtree_control EPERM on rootless + add CI #2340

merged 1 commit into from
Apr 28, 2020

Conversation

AkihiroSuda
Copy link
Member

Fix #2339 (regression in 813cb3e)

@AkihiroSuda AkihiroSuda mentioned this pull request Apr 21, 2020
Closed
kolyshkin
kolyshkin approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

kolyshkin commented Apr 21, 2020 •
edited
Loading

Can we add a test case for this (apparently we don't have one or it is disabled, since #2299 CI was green)?

@AkihiroSuda
Copy link
Member Author

I will open separate PR for CI

@AkihiroSuda AkihiroSuda changed the title fs2: ignore cgroup.subtree_control EPERM when rootless fs2: ignore cgroup.subtree_control EPERM when rootless + add CI Apr 21, 2020
@AkihiroSuda
Copy link
Member Author

added CI

@kolyshkin kolyshkin mentioned this pull request Apr 21, 2020
Closed
kolyshkin
kolyshkin reviewed Apr 21, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
kolyshkin
kolyshkin requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

left a few comments

@AkihiroSuda AkihiroSuda changed the title fs2: ignore cgroup.subtree_control EPERM when rootless + add CI fs2: fix cgroup.subtree_control EPERM on rootless + add CI Apr 21, 2020
@AkihiroSuda AkihiroSuda force-pushed the fix-2339 branch 2 times, most recently from 26d44d9 to 473a18d Compare April 21, 2020 22:27
@AkihiroSuda AkihiroSuda mentioned this pull request Apr 21, 2020
Merged
kolyshkin
kolyshkin reviewed Apr 21, 2020
View reviewed changes
Vagrantfile Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the fix-2339 branch 4 times, most recently from 5e157cd to ce64430 Compare April 21, 2020 23:59
@AkihiroSuda AkihiroSuda force-pushed the fix-2339 branch 4 times, most recently from 9867192 to 753857c Compare April 22, 2020 00:58
kolyshkin
kolyshkin reviewed Apr 22, 2020
View reviewed changes
tests/rootless.sh Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member Author

Weird failure, fails only with idmap mode.

[03] run rootless tests ... (idmap)
path: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
1..78
ok 1 runc update --kernel-memory{,-tcp} (initialized) # skip test requires rootless_cgroup
ok 2 runc update --kernel-memory (uninitialized) # skip test requires rootless_cgroup
ok 3 runc create (no limits + no cgrouppath + no permission) succeeds
not ok 4 runc create (rootless + no limits + cgrouppath + no permission) fails with permission error
# (in test file tests/integration/cgroups.bats, line 86)
#   `[ "$status" -eq 1 ]' failed
...

@AkihiroSuda AkihiroSuda force-pushed the fix-2339 branch 4 times, most recently from cbf350e to 603bd8c Compare April 24, 2020 07:36
@AkihiroSuda
Copy link
Member Author

CI green. Ready for review.

@kolyshkin
Copy link
Contributor

Needs a rebase

@AkihiroSuda
Copy link
Member Author

rebased

@AkihiroSuda
Copy link
Member Author

@opencontainers/runc-maintainers PTAL?

I'd like to get almost all cgroup v2 issues to be resolved before Docker 20.0X freeze (May 1) and Podman 2.0

kolyshkin
kolyshkin reviewed Apr 27, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
kolyshkin
kolyshkin requested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Do we need to test rootless+systemd? Otherwise LGTM

@AkihiroSuda
Copy link
Member Author

@kolyshkin
No, the systemd PR is not merged yet: #2281

Let's merge this PR now so that we can rebase #2281 on top of this and add test for that

kolyshkin
kolyshkin approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

kolyshkin commented Apr 28, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Contributor

@mrunalp PTAL

@mrunalp
Copy link
Contributor

mrunalp commented Apr 28, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit a1f007e into opencontainers:master Apr 28, 2020
@cyphar cyphar mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

Assignees
No one assigned
Labels
area/cgroupv2 area/ci area/rootless kind/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cgroup2: \"open /sys/fs/cgroup/cgroup.subtree_control: permission denied\"": unknown.
3 participants
@AkihiroSuda @kolyshkin @mrunalp