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] Don't freeze cgroup on update for systemd cgroup v2 #3092

Merged
merged 2 commits into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions libcontainer/cgroups/systemd/v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,35 +418,10 @@ func (m *unifiedManager) Set(r *configs.Resources) error {
return err
}

// Figure out the current freezer state, so we can revert to it after we
// temporarily freeze the container.
targetFreezerState, err := m.GetFreezerState()
if err != nil {
return err
}
if targetFreezerState == configs.Undefined {
targetFreezerState = configs.Thawed
}

// We have to freeze the container while systemd sets the cgroup settings.
// The reason for this is that systemd's application of DeviceAllow rules
// is done disruptively, resulting in spurrious errors to common devices
// (unlike our fs driver, they will happily write deny-all rules to running
// containers). So we freeze the container to avoid them hitting the cgroup
// error. But if the freezer cgroup isn't supported, we just warn about it.
if err := m.Freeze(configs.Frozen); err != nil {
logrus.Infof("freeze container before SetUnitProperties failed: %v", err)
}

if err := setUnitProperties(m.dbus, getUnitName(m.cgroups), properties...); err != nil {
_ = m.Freeze(targetFreezerState)
return errors.Wrap(err, "error while setting unit properties")
}

// Reset freezer state before we apply the configuration, to avoid clashing
// with the freezer setting in the configuration.
_ = m.Freeze(targetFreezerState)

fsMgr, err := m.fsManager()
if err != nil {
return err
Expand Down
26 changes: 19 additions & 7 deletions tests/integration/update.bats
Original file line number Diff line number Diff line change
Expand Up @@ -586,13 +586,25 @@ EOF

@test "update devices [minimal transition rules]" {
[[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup
# This test currently only makes sense on cgroupv1.
requires cgroups_v1

# Run a basic shell script that tries to write to /dev/null. If "runc
# update" makes use of minimal transition rules, updates should not cause
# writes to fail at any point.
update_config '.process.args |= ["sh", "-c", "while true; do echo >/dev/null; done"]'
requires root

# Run a basic shell script that tries to read from /dev/kmsg, but
# due to lack of permissions, it prints the error message to /dev/null.
# If any data is read from /dev/kmsg, it will be printed to stdout, and the
# test will fail. In the same way, if access to /dev/null is denied, the
# error will be printed to stderr, and the test will also fail.
#
# "runc update" makes use of minimal transition rules, updates should not cause
# writes to fail at any point. For systemd cgroup driver on cgroup v1, the cgroup
# is frozen to ensure this.
update_config ' .linux.resources.devices = [{"allow": false, "access": "rwm"}, {"allow": false, "type": "c", "major": 1, "minor": 11, "access": "rwa"}]
| .linux.devices = [{"path": "/dev/kmsg", "type": "c", "major": 1, "minor": 11}]
| .process.capabilities.bounding += ["CAP_SYSLOG"]
| .process.capabilities.effective += ["CAP_SYSLOG"]
| .process.capabilities.inheritable += ["CAP_SYSLOG"]
| .process.capabilities.permitted += ["CAP_SYSLOG"]
| .process.args |= ["sh", "-c", "while true; do head -c 100 /dev/kmsg 2> /dev/null; done"]'

# Set up a temporary console socket and recvtty so we can get the stdio.
TMP_RECVTTY_DIR="$(mktemp -d "$BATS_RUN_TMPDIR/runc-tmp-recvtty.XXXXXX")"
Expand All @@ -611,7 +623,7 @@ EOF
# Trigger an update. This update doesn't actually change the device rules,
# but it will trigger the devices cgroup code to reapply the current rules.
# We trigger the update a few times to make sure we hit the race.
for _ in {1..12}; do
for _ in {1..30}; do
# TODO: Update "runc update" so we can change the device rules.
runc update --pids-limit 30 test_update
[ "$status" -eq 0 ]
Expand Down