Merge pull request #3782 from kolyshkin/fix-sd-start

Fix systemd cgroup driver's Apply
opencontainers · Apr 3, 2023 · 9f24513 · 9f24513
2 parents 17922e3 + 82bc89c
commit 9f24513
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 24 deletions.
diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go
@@ -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
 }
 

diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
@@ -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
 	}
 

diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go
@@ -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)
 	}
 

diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go
@@ -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))
 		}
 	}
 

diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats
@@ -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" {
 	[ $EUID -ne 0 ] && requires rootless_cgroup
 
 	set_cgroups_path
@@ -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"* ]]
 }