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

[1.0] cgroups: Set: fix freeze, avoid unnecessary freeze from systemd v1 #3093

Merged
merged 6 commits into from
Jul 16, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 15, 2021

NOTE this requires #3092 as the tests added also test the issue fixed by it. Draft until #3082 and #3092 are merged.

backport of #3082 to 1.0. Mostly clean cherry-pick, with a single trivial conflict in include ().

  1. Fix the inability to freeze the container using its Set() method (with r.Freezer set to Frozen). Add a test.
  2. Avoid unnecessary freeze/thaw from system v1 driver. Add a test.

Changelog entry

 - cgroup/systemd/v1:  avoid unnecessary freeze on Set. (#3093)

@kolyshkin kolyshkin changed the base branch from master to release-1.0 July 15, 2021 07:25
@kolyshkin kolyshkin mentioned this pull request Jul 15, 2021
@kolyshkin
Copy link
Contributor Author

This fails on v2 because it tests the issue which is fixed by #3092.

@kolyshkin kolyshkin marked this pull request as draft July 15, 2021 20:38
@cyphar
Copy link
Member

cyphar commented Jul 15, 2021

#3092 merged.

m.Freeze method changes m.cgroups.Resources.Freezer field, which should
not be done while we're temporarily freezing the cgroup in Set. If this
field is changed, and r == m.cgroups.Resources (as it often happens),
this results in inability to freeze the container using Set().

To fix, add and use a method which does not change r.Freezer field.

A test case for the bug will be added separately.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 67cfd3d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The t.Name() usage in libcontainer/integration prevented subtests
to be used, since in such case it returns a string containing "/",
and thus it can't be used to name a container.

Fix this by replacing slashes with underscores where appropriate.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit af1688a)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In addition to freezing and thawing a container via Pause/Resume,
there is a way to also do so via Set.

This way was broken though and is being fixed by a few preceding
commits. The test is added to make sure this is fixed and won't regress.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 5dc3260)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Introduce freezeBeforeSet, which contains the logic of figuring out
whether we need to freeze/thaw around setting systemd unit properties.

In particular, if SkipDevices is set, and the current unit properties
allow all devices, there is no need to freeze and thaw, as systemd
won't write any device rules in this case.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit f2db879,
 minor conflict in include() due to missing commit
 b60e2ed)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was initially added by commit 3e5c199 because Set (with
r.Freezer = Frozen) was not able to freeze a container.

Now (see a few previous commits) Set can do the freeze, so the explicit
Freeze is no longer needed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 52dd96d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true
does not result in spurious "permission denied" errors in a container
running under the pod. The test is somewhat similar in nature to the
@test "update devices [minimal transition rules]" in tests/integration,
but uses a pod.

This tests the validity of freezeBeforeSet in v1.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit a711026)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin force-pushed the 1.0-backport-3082 branch from 250355b to 4efb7a6 Compare July 15, 2021 22:27
@kolyshkin kolyshkin marked this pull request as ready for review July 15, 2021 22:28
@kolyshkin kolyshkin added this to the 1.0.1 milestone Jul 15, 2021
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

@cyphar cyphar merged commit 40dcf1f into opencontainers:release-1.0 Jul 16, 2021
breakings added a commit to breakings/packages that referenced this pull request Aug 8, 2021
This is the first stable release in the 1.0 branch, fixing a few medium
and high priority issues with runc 1.0.0, including a few that affect
Kubernetes' usage of libcontainer.

Bugfixes:

- Fixed occasional runc exec/run failure ("interrupted system call") on an
  Azure volume. ([#3074](opencontainers/runc#3074))
- Fixed "unable to find groups ... token too long" error with /etc/group
  containing lines longer than 64K characters. ([#3079](opencontainers/runc#3079))
- cgroup/systemd/v1: fix leaving cgroup frozen after Set if a parent cgroup is
  frozen. This is a regression in 1.0.0, not affecting runc itself but some
  of libcontainer users (e.g Kubernetes). ([#3085](opencontainers/runc#3085))
- cgroupv2: bpf: Ignore inaccessible existing programs in case of
  permission error when handling replacement of existing bpf cgroup
  programs. This fixes a regression in 1.0.0, where some SELinux
  policies would block runc from being able to run entirely. ([#3087](opencontainers/runc#3087))
- cgroup/systemd/v2: don't freeze cgroup on Set. ([#3092](opencontainers/runc#3092))
- cgroup/systemd/v1: avoid unnecessary freeze on Set. ([#3093](opencontainers/runc#3093))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants