diff --git a/systemd/systemd_test.go b/systemd/systemd_test.go index 156e55e..5d34a86 100644 --- a/systemd/systemd_test.go +++ b/systemd/systemd_test.go @@ -2,7 +2,9 @@ package systemd import ( "os" + "os/exec" "reflect" + "strconv" "testing" systemdDbus "github.com/coreos/go-systemd/v22/dbus" @@ -157,6 +159,12 @@ func TestUnifiedResToSystemdProps(t *testing.T) { newProp("CPUWeight", uint64(1000)), }, }, + { + name: "memory.oom.group handled by Apply method", + res: map[string]string{ + "memory.oom.group": "1", + }, + }, } for _, tc := range testCases { @@ -272,3 +280,116 @@ func TestTasksMax(t *testing.T) { t.Fatalf("failed to set PidsLimit=nil: %v", err) } } + +func TestOOMPolicyApply(t *testing.T) { + if !IsRunningSystemd() { + t.Skip("Test requires systemd.") + } + if !cgroups.IsCgroup2UnifiedMode() { + t.Skip("cgroup v2 is required") + } + if os.Geteuid() != 0 { + t.Skip("Test requires root.") + } + + cm := newDbusConnManager(false) + if systemdVersion(cm) < oomPolicySupportedVersion { + t.Skipf("Test requires systemd >= %d (OOMPolicy on scopes)", oomPolicySupportedVersion) + } + + testCases := []struct { + name string + oomGroupValue string + expectedPolicy string + expectError bool + }{ + { + name: "memory.oom.group=0 sets OOMPolicy=continue", + oomGroupValue: "0", + expectedPolicy: "continue", + expectError: false, + }, + { + name: "memory.oom.group=1 sets OOMPolicy=kill", + oomGroupValue: "1", + expectedPolicy: "kill", + expectError: false, + }, + { + name: "invalid memory.oom.group value", + oomGroupValue: "invalid", + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + config := &cgroups.Cgroup{ + ScopePrefix: "test", + Name: "test-oom-policy-" + strconv.FormatInt(int64(os.Getpid()), 10), + Resources: &cgroups.Resources{ + Unified: map[string]string{ + "memory.oom.group": tc.oomGroupValue, + }, + }, + } + + manager, err := NewUnifiedManager(config, "") + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + defer func() { + _ = manager.Destroy() + }() + + // Scopes require a process inside. + cmd := exec.Command("bash", "-c", "sleep 1m") + if err := cmd.Start(); 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() + }() + + err = manager.Apply(cmd.Process.Pid) + if tc.expectError { + if err == nil { + t.Fatal("Expected error but got none") + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + unitName := getUnitName(config) + conn, err := systemdDbus.NewSystemdConnectionContext(t.Context()) + if err != nil { + t.Fatalf("Failed to connect to systemd: %v", err) + } + defer conn.Close() + + properties, err := conn.GetUnitTypePropertiesContext(t.Context(), unitName, "Scope") + if err != nil { + t.Fatalf("Failed to get unit type properties: %v", err) + } + + oomPolicyValue, exists := properties["OOMPolicy"] + if !exists { + t.Fatal("OOMPolicy property not found") + } + + oomPolicyStr, ok := oomPolicyValue.(string) + if !ok { + t.Fatalf("OOMPolicy value is not a string: %T", oomPolicyValue) + } + + if oomPolicyStr != tc.expectedPolicy { + t.Errorf("Expected OOMPolicy=%s, got %s", tc.expectedPolicy, oomPolicyStr) + } + }) + } +} diff --git a/systemd/v2.go b/systemd/v2.go index cb71e9e..bdfd446 100644 --- a/systemd/v2.go +++ b/systemd/v2.go @@ -20,7 +20,8 @@ import ( ) const ( - cpuIdleSupportedVersion = 252 + cpuIdleSupportedVersion = 252 + oomPolicySupportedVersion = 253 ) type UnifiedManager struct { @@ -183,13 +184,8 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props newProp("TasksMax", num)) case "memory.oom.group": - // Setting this to 1 is roughly equivalent to OOMPolicy=kill - // (as per systemd.service(5) and - // https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html), - // but it's not clear what to do if it is unset or set - // to 0 in runc update, as there are two other possible - // values for OOMPolicy (continue/stop). - fallthrough + // This was set before the unit started, so no need to + // warn about it here. default: // Ignore the unknown resource here -- will still be @@ -338,6 +334,29 @@ func (m *UnifiedManager) Apply(pid int) error { properties = append(properties, c.SystemdProps...) + // OOMPolicy must be set at unit creation time, systemd does not allow + // changing it on a running scope for some reason despite the kernel allowing so + // so we do this here in Apply() instead of Set() + if c.Resources != nil { + if v, ok := c.Resources.Unified["memory.oom.group"]; ok { + if systemdVersion(m.dbus) >= oomPolicySupportedVersion { + value, err := strconv.ParseUint(v, 10, 64) + if err != nil { + return fmt.Errorf("unified resource %q value conversion error: %w", "memory.oom.group", err) + } + + switch value { + case 0: + properties = append(properties, newProp("OOMPolicy", "continue")) + case 1: + properties = append(properties, newProp("OOMPolicy", "kill")) + default: + logrus.Debugf("don't know how to convert memory.oom.group=%d; skipping (will still be applied to cgroupfs)", value) + } + } + } + } + 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) }