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

Fix systemd cgroup driver's Apply #3782

Merged
merged 4 commits into from
Apr 3, 2023
Merged

Fix systemd cgroup driver's Apply #3782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6e8cb7
libct/cg/sd: refactor startUnit
kolyshkin Mar 23, 2023
c253342
libct/cg/sd: ignore UnitExists only for Apply(-1)
kolyshkin Mar 23, 2023
1d18743
libct/cg/sd: reset-failed and retry startUnit on UnitExists
kolyshkin Mar 23, 2023
82bc89c
runc run: refuse a non-empty cgroup
kolyshkin Mar 23, 2023
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
52 changes: 36 additions & 16 deletions libcontainer/cgroups/systemd/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,32 +124,52 @@ func isUnitExists(err error) bool {
return isDbusError(err, "org.freedesktop.systemd1.UnitExists")
}

func startUnit(cm *dbusConnManager, unitName string, properties []systemdDbus.Property) error {
func startUnit(cm *dbusConnManager, unitName string, properties []systemdDbus.Property, ignoreExist bool) error {
statusChan := make(chan string, 1)
retry := true

retry:
err := cm.retryOnDisconnect(func(c *systemdDbus.Conn) error {
_, err := c.StartTransientUnitContext(context.TODO(), unitName, "replace", properties, statusChan)
return err
})
if err == nil {
timeout := time.NewTimer(30 * time.Second)
defer timeout.Stop()

select {
case s := <-statusChan:
close(statusChan)
// Please refer to https://pkg.go.dev/github.com/coreos/go-systemd/v22/dbus#Conn.StartUnit
if s != "done" {
resetFailedUnit(cm, unitName)
return fmt.Errorf("error creating systemd unit `%s`: got `%s`", unitName, s)
}
case <-timeout.C:
if err != nil {
if !isUnitExists(err) {
return err
}
if ignoreExist {
// TODO: remove this hack.
// This is kubelet making sure a slice exists (see
// https://github.com/opencontainers/runc/pull/1124).
return nil
}
if retry {
// In case a unit with the same name exists, this may
// be a leftover failed unit. Reset it, so systemd can
// remove it, and retry once.
resetFailedUnit(cm, unitName)
return errors.New("Timeout waiting for systemd to create " + unitName)
retry = false
goto retry
}
} else if !isUnitExists(err) {
return err
}

timeout := time.NewTimer(30 * time.Second)
defer timeout.Stop()

select {
case s := <-statusChan:
close(statusChan)
// Please refer to https://pkg.go.dev/github.com/coreos/go-systemd/v22/dbus#Conn.StartUnit
if s != "done" {
resetFailedUnit(cm, unitName)
return fmt.Errorf("error creating systemd unit `%s`: got `%s`", unitName, s)
}
case <-timeout.C:
resetFailedUnit(cm, unitName)
return errors.New("Timeout waiting for systemd to create " + unitName)
}

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/systemd/v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (m *LegacyManager) Apply(pid int) error {

properties = append(properties, c.SystemdProps...)

if err := startUnit(m.dbus, unitName, properties); err != nil {
if err := startUnit(m.dbus, unitName, properties, pid == -1); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/systemd/v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func (m *UnifiedManager) Apply(pid int) error {

properties = append(properties, c.SystemdProps...)

if err := startUnit(m.dbus, unitName, properties); err != nil {
if err := startUnit(m.dbus, unitName, properties, pid == -1); err != nil {
return fmt.Errorf("unable to start unit %q (properties %+v): %w", unitName, properties, err)
}

Expand Down
4 changes: 1 addition & 3 deletions libcontainer/factory_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ func Create(root, id string, config *configs.Config) (*Container, error) {
return nil, fmt.Errorf("unable to get cgroup PIDs: %w", err)
}
if len(pids) != 0 {
// TODO: return an error.
logrus.Warnf("container's cgroup is not empty: %d process(es) found", len(pids))
logrus.Warn("DEPRECATED: running container in a non-empty cgroup won't be supported in runc 1.2; https://github.com/opencontainers/runc/issues/3132")
return nil, fmt.Errorf("container's cgroup is not empty: %d process(es) found", len(pids))
}
}

Expand Down
6 changes: 3 additions & 3 deletions tests/integration/cgroups.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ function setup() {
[ "$output" = "ok" ]
}

@test "runc run/create should warn about a non-empty cgroup" {
@test "runc run/create should error for a non-empty cgroup" {
Copy link
Member

@AkihiroSuda AkihiroSuda Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this a breaking change?
Probably fine for v1.2, but not sure backportable to v1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To begin with, it never worked anyway, because if the systemd unit exists, the error is ignored and the new container is never added to the unit (and to the cgroup). To repeat, it never worked anyway.

We can try making it work in 1.1 though. The fix would be very different from this one, something like "if startUnit returned UnitExists error, call setUnitProperties with properties of PIDs=[new pid]".

I am not sure that this will work (maybe, maybe not -- it's complicated. I am also unsure if we want to go that route at all -- I mean trying to fix something that never worked anyway.

In this version (1.2.x), I think this is the way it should be done.

In 1.1, we can discuss it later (for the 1.1.6 I guess).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reiterate -- let's concentrate on how can we fix it in main branch for now, and think about 1.1 backport later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about this a bit -- I think we can still allow shared cgroup when fs cgroup driver is used, keeping the warning, and error out in case of systemd cgroup driver. This is sort of a breaking change, but since

  1. the functionality never worked correctly (UnitExists error was ignored, and container was not placed into the proper systemd unit and/or cgroup), and
  2. will be deprecated in runc 1.2,
  3. implementing such a feature (adding a container to an existing systemd unit) is not very easy,

it makes little sense in trying to do that.

In particular, this test can be changed to look for a warning in case of an fs cgroup driver, and error in case of systemd cgroup driver.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kolyshkin That sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented as described above in #3806

[ $EUID -ne 0 ] && requires rootless_cgroup

set_cgroups_path
Expand All @@ -366,12 +366,12 @@ function setup() {

# Run a second container sharing the cgroup with the first one.
runc --debug run -d --console-socket "$CONSOLE_SOCKET" ct2
[ "$status" -eq 0 ]
[ "$status" -ne 0 ]
[[ "$output" == *"container's cgroup is not empty"* ]]

# Same but using runc create.
runc create --console-socket "$CONSOLE_SOCKET" ct3
[ "$status" -eq 0 ]
[ "$status" -ne 0 ]
[[ "$output" == *"container's cgroup is not empty"* ]]
}

Expand Down