systemd cgroup v1 freeze fixes

libcontainer/cgroups/fs/freezer.go

@@ -129,7 +129,25 @@ func (s *FreezerGroup) GetState(path string) (configs.FreezerState, error) {
 		case "THAWED":
 			return configs.Thawed, nil
 		case "FROZEN":
-			return configs.Frozen, nil
+			// Find out whether the cgroup is frozen directly,
+			// or indirectly via an ancestor.
+			self, err := cgroups.ReadFile(path, "freezer.self_freezing")
+			if err != nil {
+				// If the kernel is too old, then we just treat
+				// it as being frozen.
+				if errors.Is(err, os.ErrNotExist) || errors.Is(err, unix.ENODEV) {
+					err = nil
+				}
+				return configs.Frozen, err
+			}
+			switch self {
+			case "0\n":
+				return configs.Thawed, nil
+			case "1\n":
+				return configs.Frozen, nil
+			default:
+				return configs.Undefined, fmt.Errorf(`unknown "freezer.self_freezing" state: %q`, self)
+			}
 		case "FREEZING":
 			// Make sure we get a stable freezer state, so retry if the cgroup
 			// is still undergoing freezing. This should be a temporary delay.

libcontainer/cgroups/systemd/systemd_test.go

@@ -1,6 +1,7 @@
 package systemd
 
 import (
+	"bufio"
 	"bytes"
 	"os"
 	"os/exec"
@@ -193,3 +194,143 @@ func TestUnitExistsIgnored(t *testing.T) {
 		}
 	}
 }
+
+func TestFreezePodCgroup(t *testing.T) {
+	if !IsRunningSystemd() {
+		t.Skip("Test requires systemd.")
+	}
+	if os.Geteuid() != 0 {
+		t.Skip("Test requires root.")
+	}
+
+	podConfig := &configs.Cgroup{
+		Parent: "system.slice",
+		Name:   "system-runc_test_pod.slice",
+		Resources: &configs.Resources{
+			SkipDevices: true,
+			Freezer:     configs.Frozen,
+		},
+	}
+	// Create a "pod" cgroup (a systemd slice to hold containers),
+	// which is frozen initially.
+	pm := newManager(podConfig)
+	defer pm.Destroy() //nolint:errcheck
+	if err := pm.Apply(-1); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := pm.Freeze(configs.Frozen); err != nil {
+		t.Fatal(err)
+	}
+	if err := pm.Set(podConfig.Resources); err != nil {
+		t.Fatal(err)
+	}
+
+	// Check the pod is frozen.
+	pf, err := pm.GetFreezerState()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if pf != configs.Frozen {
+		t.Fatalf("expected pod to be frozen, got %v", pf)
+	}
+
+	// Create a "container" within the "pod" cgroup.
+	// This is not a real container, just a process in the cgroup.
+	containerConfig := &configs.Cgroup{
+		Parent:      "system-runc_test_pod.slice",
+		ScopePrefix: "test",
+		Name:        "inner-contianer",
+		Resources:   &configs.Resources{},
+	}
+
+	cmd := exec.Command("bash", "-c", "while read; do echo $REPLY; done")
+	cmd.Env = append(os.Environ(), "LANG=C")
+
+	// Setup stdin.
+	stdinR, stdinW, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cmd.Stdin = stdinR
+
+	// Setup stdout.
+	stdoutR, stdoutW, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cmd.Stdout = stdoutW
+	rdr := bufio.NewReader(stdoutR)
+
+	// Setup stderr.
+	var stderr bytes.Buffer
+	cmd.Stderr = &stderr
+
+	err = cmd.Start()
+	stdinR.Close()
+	stdoutW.Close()
+	defer func() {
+		_ = stdinW.Close()
+		_ = stdoutR.Close()
+	}()
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Make sure to not leave a zombie.
+	defer func() {
+		// These may fail, we don't care.
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+	}()
+
+	// Put the process into a cgroup.
+	cm := newManager(containerConfig)
+	defer cm.Destroy() //nolint:errcheck
+
+	if err := cm.Apply(cmd.Process.Pid); err != nil {
+		t.Fatal(err)
+	}
+	if err := cm.Set(containerConfig.Resources); err != nil {
+		t.Fatal(err)
+	}
+	// Check that we put the "container" into the "pod" cgroup.
+	if !strings.HasPrefix(cm.Path("freezer"), pm.Path("freezer")) {
+		t.Fatalf("expected container cgroup path %q to be under pod cgroup path %q",
+			cm.Path("freezer"), pm.Path("freezer"))
+	}
+	// Check the container is not reported as frozen despite the frozen parent.
+	cf, err := cm.GetFreezerState()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cf != configs.Thawed {
+		t.Fatalf("expected container to be thawed, got %v", cf)
+	}
+
+	// Unfreeze the pod.
+	if err := pm.Freeze(configs.Thawed); err != nil {
+		t.Fatal(err)
+	}
+
+	cf, err = cm.GetFreezerState()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cf != configs.Thawed {
+		t.Fatalf("expected container to be thawed, got %v", cf)
+	}
+
+	// Check the "container" works.
+	marker := "one two\n"
+	_, err = stdinW.WriteString(marker)
+	if err != nil {
+		t.Fatal(err)
+	}
+	reply, err := rdr.ReadString('\n')
+	if err != nil {
+		t.Fatalf("reading from container: %v", err)
+	}
+	if reply != marker {
+		t.Fatalf("expected %q, got %q", marker, reply)
+	}
+}

libcontainer/cgroups/systemd/v1.go

@@ -279,18 +279,23 @@ func getSubsystemPath(c *configs.Cgroup, subsystem string) (string, error) {
 }
 
 func (m *legacyManager) Freeze(state configs.FreezerState) error {
+	err := m.doFreeze(state)
+	if err == nil {
+		m.cgroups.Resources.Freezer = state
+	}
+	return err
+}
+
+// doFreeze is the same as Freeze but without
+// changing the m.cgroups.Resources.Frozen field.
+func (m *legacyManager) doFreeze(state configs.FreezerState) error {
 	path, ok := m.paths["freezer"]
 	if !ok {
 		return errSubsystemDoesNotExist
 	}
-	prevState := m.cgroups.Resources.Freezer
-	m.cgroups.Resources.Freezer = state
 	freezer := &fs.FreezerGroup{}
-	if err := freezer.Set(path, m.cgroups.Resources); err != nil {
-		m.cgroups.Resources.Freezer = prevState
-		return err
-	}
-	return nil
+	resources := &configs.Resources{Freezer: state}
+	return freezer.Set(path, resources)
 }
 
 func (m *legacyManager) GetPids() ([]int, error) {
@@ -340,14 +345,21 @@ func (m *legacyManager) 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()
+	// Figure out if cgroup needs to be frozen before applying systemd properties
+	// and thawed after, while trying to avoid unnecessary state changes.
+	freezerState, err := m.GetFreezerState()
 	if err != nil {
 		return err
 	}
-	if targetFreezerState == configs.Undefined {
-		targetFreezerState = configs.Thawed
+	needsFreeze := true
+	if freezerState == configs.Frozen {
+		// Already frozen, no need to do anything.
+		needsFreeze = false
+	}
+	needsThaw := needsFreeze
+	if r.Freezer == configs.Frozen {
+		// Will be frozen anyway, so no need to thaw.
+		needsThaw = false
 	}
 
 	// We have to freeze the container while systemd sets the cgroup settings.
@@ -356,18 +368,20 @@ func (m *legacyManager) Set(r *configs.Resources) error {
 	// (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 needsFreeze {
+		if err := m.doFreeze(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 err
+	setErr := setUnitProperties(m.dbus, getUnitName(m.cgroups), properties...)
+	if needsThaw {
+		if err := m.doFreeze(configs.Thawed); err != nil {
+			logrus.Infof("thaw container after SetUnitProperties failed: %v", err)
+		}
+	}
+	if setErr != nil {
+		return setErr
 	}
-
-	// Reset freezer state before we apply the configuration, to avoid clashing
-	// with the freezer setting in the configuration.
-	_ = m.Freeze(targetFreezerState)
 
 	for _, sys := range legacySubsystems {
 		// Get the subsystem path, but don't error out for not found cgroups.