Repository: opencontainers/runc
Pull request #2812 (2021-02-19 02:56:44)

Better handle hitting the memory limit
This improves runc error reporting in cases where memory limit is set too low.

NOTE that while this is clearly an enhancement I want this in rc94 because
 - it does not change runc behavior other than error text;
 - it helps a lot with some issues reported by customers.

## 1. Improve EBUSY handling on setting cgroup memory limit ##

 On cgroup v1 + fs driver, setting memory.limit_in_bytes results in EBUSY. This is reported as

> ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "1000": write /sys/fs/cgroup/memory/user.slice/user-1000.slice/session-1011.scope/xe3/memory.limit_in_bytes: device or resource busy

To decipher the error a user needs to know that EBUSY is returned by the kernel when the limit being set is too low. In addition, it would be handy to know what the current usage is.

Handle EBUSY and report this:

> ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: unable to set memory limit to 1000 (current usage: 1204224, peak usage: 2936832)

Related to https://github.com/opencontainers/runc/issues/2736

## 2. Check for OOM kill on container start ##

When a container fails to start due to memory limit set too low, and container init being OOM-killed,
error messages returned by runc are semi-random and rather cryptic. Here are a few examples
(truncated to remove the common prefix for clarity):

 - `process_linux.go:348: copying bootstrap data to pipe caused: write init-p: broken pipe` (cgroup v1 + systemd driver)
 - `process_linux.go:352: getting the final child's pid from pipe caused: EOF` (cgroup v1 + systemd driver)
 - `process_linux.go:495: container init caused: read init-p: connection reset by peer` (cgroup v2)
 - `process_linux.go:484: writing syncT 'resume' caused: write init-p: broken pipe` (cgroup v2)

On container start error path, add a check if OOM kill has happened, and report it instead of the original (cryptic) error:

> ERRO[0000] container_linux.go:367: starting container process caused: container init was OOM-killed (memory limit too low?)

(or, if `--debug` is set, also provide the original error):

> ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:343: container init was OOM-killed (memory limit too low?) caused: process_linux.go:520: container init caused: process_linux.go:509: writing syncT 'resume' caused: write init-p: broken pipe 

## 3. Check for OOM kill on container exec ##

Same as above, with some nuances:

1. The container is already running and OOM kill counter might not be
       zero.  This is why we have to read the counter before exec and after
       it failed.
    
2. An unrelated OOM kill event might occur in parallel with our exec
       (and I see no way to find out which process was killed, except to
       parse kernel logs which seems excessive and not very reliable).
       This is why we report _possible_ OOM kill.

The error message changed from 

> ERRO[0000] exec failed: container_linux.go:367: starting container process caused: read init-p: connection reset by peer

to

> ERRO[0000] exec failed: container_linux.go:367: starting container process caused: process_linux.go:105: possibly OOM-killed caused: read init-p: connection reset by peer 

## 4. Some minor improvements and refactoring along the way. ##

Please see individual commits for details.

## TODO ##
 - look into why runc init memory current and max differs that much
 - ~~look into why systemd v1 driver does not fail to set the limits like fs driver does~~ see #2813 / #2814.

kolyshkin: Before:

> ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "1000": write /sys/fs/cgroup/memory/user.slice/user-1000.slice/session-1011.scope/xe3/memory.limit_in_bytes: device or resource busy

After:
> ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: unable to set memory limit to 1000 (current usage: 1204224, peak usage: 2936832)

This at least gives some info about what's wrong as well as a hint about the lowest possible limit.

This also sheds some light about what is going on in #2736 -- apparently runc init has some peak memory use upon the start, this is why retrying on EBUSY helps @wzshiming.

Yet I am not convinced this should be handled by a retry.
kolyshkin: Failure on centos 7, seems like a random one (some rare race in old systemd?):

```
not ok 68 ps -e -x
(in test file tests/integration/ps.bats, line 53)
  `[ "$status" -eq 0 ]' failed
 runc spec (status=0):
 runc run -d --console-socket /tmp/console.sock test_busybox (status=1):
 time="2021-02-19T11:44:35Z" level=error msg="container_linux.go:367: starting container process
    caused: process_linux.go:502: container init
    caused: process_linux.go:465: setting cgroup config for procHooks process
    caused: Unit runc-test_busybox.scope is not loaded."
 ```
kolyshkin: @wzshiming can you test this PR on your setup to reproduce https://github.com/opencontainers/runc/issues/2736 and copy-paste the runc errors here? It will help to understand what is going on in your case.
kolyshkin: > why systemd v1 driver does not fail to set the limits like fs driver does;
￼￼
This happens because we do
 - cgroup Manager.Apply()
 - start init
 - cgroup Manager.Set()

Now, the difference between fs and systemd managers are:
 - fs manager Apply() merely creates cgroups (it and does not set any limits)
 - systemd manager Apply() creates the systemd with all properties already set, i.e. it sets limits (those that systemd understands)

Because of this difference, when fs driver is used, we can actually start a container init (which gets OOM-killed a moment later). When systemd driver is used, we can not start a container as the limits are too tight.

I think we should somehow unify the behavior. The two options are
1. Call Set() before container is started.
2. Make systemd's Apply() create a unit with no limits, only adding those later during Set().

@opencontainers/runc-maintainers WDYT?
kolyshkin: No longer a wip. `runc init` memory usage analysis were not done, but it won't change anything in this code.
kolyshkin: Moved to draft to add OOM handling in runc exec. Added, tested, ready for the prime time!

Patch:
diff --git a/libcontainer/cgroups/cgroups.go b/libcontainer/cgroups/cgroups.go
index a16a68e9..c418fbae 100644
--- a/libcontainer/cgroups/cgroups.go
+++ b/libcontainer/cgroups/cgroups.go
@@ -48,4 +48,7 @@ type Manager interface {
 
 	// Whether the cgroup path exists or not
 	Exists() bool
+
+	// OOMKillCount reports OOM kill count for the cgroup.
+	OOMKillCount() (uint64, error)
 }
diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go
index 1d5f455d..194f3a54 100644
--- a/libcontainer/cgroups/fs/cpu.go
+++ b/libcontainer/cgroups/fs/cpu.go
@@ -97,7 +97,7 @@ func (s *CpuGroup) GetStats(path string, stats *cgroups.Stats) error {
 
 	sc := bufio.NewScanner(f)
 	for sc.Scan() {
-		t, v, err := fscommon.GetCgroupParamKeyValue(sc.Text())
+		t, v, err := fscommon.ParseKeyValue(sc.Text())
 		if err != nil {
 			return err
 		}
diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go
index a42ce453..bede91e2 100644
--- a/libcontainer/cgroups/fs/fs.go
+++ b/libcontainer/cgroups/fs/fs.go
@@ -9,6 +9,7 @@ import (
 	"sync"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
+	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
 	"github.com/pkg/errors"
@@ -421,3 +422,11 @@ func (m *manager) GetFreezerState() (configs.FreezerState, error) {
 func (m *manager) Exists() bool {
 	return cgroups.PathExists(m.Path("devices"))
 }
+
+func OOMKillCount(path string) (uint64, error) {
+	return fscommon.GetValueByKey(path, "memory.oom_control", "oom_kill")
+}
+
+func (m *manager) OOMKillCount() (uint64, error) {
+	return OOMKillCount(m.Path("memory"))
+}
diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go
index 61be6d7e..5d52d325 100644
--- a/libcontainer/cgroups/fs/memory.go
+++ b/libcontainer/cgroups/fs/memory.go
@@ -14,11 +14,15 @@ import (
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/opencontainers/runc/libcontainer/configs"
+	"github.com/pkg/errors"
+	"golang.org/x/sys/unix"
 )
 
 const (
 	cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes"
 	cgroupMemoryLimit     = "memory.limit_in_bytes"
+	cgroupMemoryUsage     = "memory.usage_in_bytes"
+	cgroupMemoryMaxUsage  = "memory.max_usage_in_bytes"
 )
 
 type MemoryGroup struct {
@@ -57,20 +61,52 @@ func (s *MemoryGroup) Apply(path string, d *cgroupData) (err error) {
 	return join(path, d.pid)
 }
 
-func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
+func setMemory(path string, val int64) error {
+	if val == 0 {
+		return nil
+	}
+
+	err := fscommon.WriteFile(path, cgroupMemoryLimit, strconv.FormatInt(val, 10))
+	if !errors.Is(err, unix.EBUSY) {
+		return err
+	}
+
+	// EBUSY means the kernel can't set new limit as it's too low
+	// (lower than the current usage). Return more specific error.
+	usage, err := fscommon.GetCgroupParamUint(path, cgroupMemoryUsage)
+	if err != nil {
+		return err
+	}
+	max, err := fscommon.GetCgroupParamUint(path, cgroupMemoryMaxUsage)
+	if err != nil {
+		return err
+	}
+
+	return errors.Errorf("unable to set memory limit to %d (current usage: %d, peak usage: %d)", val, usage, max)
+}
+
+func setSwap(path string, val int64) error {
+	if val == 0 {
+		return nil
+	}
+
+	return fscommon.WriteFile(path, cgroupMemorySwapLimit, strconv.FormatInt(val, 10))
+}
+
+func setMemoryAndSwap(path string, r *configs.Resources) error {
 	// If the memory update is set to -1 and the swap is not explicitly
 	// set, we should also set swap to -1, it means unlimited memory.
-	if cgroup.Resources.Memory == -1 && cgroup.Resources.MemorySwap == 0 {
+	if r.Memory == -1 && r.MemorySwap == 0 {
 		// Only set swap if it's enabled in kernel
 		if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) {
-			cgroup.Resources.MemorySwap = -1
+			r.MemorySwap = -1
 		}
 	}
 
 	// When memory and swap memory are both set, we need to handle the cases
 	// for updating container.
-	if cgroup.Resources.Memory != 0 && cgroup.Resources.MemorySwap != 0 {
-		memoryUsage, err := getMemoryData(path, "")
+	if r.Memory != 0 && r.MemorySwap != 0 {
+		curLimit, err := fscommon.GetCgroupParamUint(path, cgroupMemoryLimit)
 		if err != nil {
 			return err
 		}
@@ -78,39 +114,29 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
 		// When update memory limit, we should adapt the write sequence
 		// for memory and swap memory, so it won't fail because the new
 		// value and the old value don't fit kernel's validation.
-		if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) {
-			if err := fscommon.WriteFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
-				return err
-			}
-			if err := fscommon.WriteFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
-				return err
-			}
-		} else {
-			if err := fscommon.WriteFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
-				return err
-			}
-			if err := fscommon.WriteFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
-				return err
-			}
-		}
-	} else {
-		if cgroup.Resources.Memory != 0 {
-			if err := fscommon.WriteFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
+		if r.MemorySwap == -1 || curLimit < uint64(r.MemorySwap) {
+			if err := setSwap(path, r.MemorySwap); err != nil {
 				return err
 			}
-		}
-		if cgroup.Resources.MemorySwap != 0 {
-			if err := fscommon.WriteFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
+			if err := setMemory(path, r.Memory); err != nil {
 				return err
 			}
+			return nil
 		}
 	}
 
+	if err := setMemory(path, r.Memory); err != nil {
+		return err
+	}
+	if err := setSwap(path, r.MemorySwap); err != nil {
+		return err
+	}
+
 	return nil
 }
 
 func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
-	if err := setMemoryAndSwap(path, cgroup); err != nil {
+	if err := setMemoryAndSwap(path, cgroup.Resources); err != nil {
 		return err
 	}
 
@@ -162,7 +188,7 @@ func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error {
 
 	sc := bufio.NewScanner(statsFile)
 	for sc.Scan() {
-		t, v, err := fscommon.GetCgroupParamKeyValue(sc.Text())
+		t, v, err := fscommon.ParseKeyValue(sc.Text())
 		if err != nil {
 			return fmt.Errorf("failed to parse memory.stat (%q) - %v", sc.Text(), err)
 		}
diff --git a/libcontainer/cgroups/fs2/cpu.go b/libcontainer/cgroups/fs2/cpu.go
index 0dffe664..b7331338 100644
--- a/libcontainer/cgroups/fs2/cpu.go
+++ b/libcontainer/cgroups/fs2/cpu.go
@@ -57,7 +57,7 @@ func statCpu(dirPath string, stats *cgroups.Stats) error {
 
 	sc := bufio.NewScanner(f)
 	for sc.Scan() {
-		t, v, err := fscommon.GetCgroupParamKeyValue(sc.Text())
+		t, v, err := fscommon.ParseKeyValue(sc.Text())
 		if err != nil {
 			return err
 		}
diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go
index 3f0b9e0d..733f9dcf 100644
--- a/libcontainer/cgroups/fs2/fs2.go
+++ b/libcontainer/cgroups/fs2/fs2.go
@@ -257,3 +257,11 @@ func (m *manager) GetFreezerState() (configs.FreezerState, error) {
 func (m *manager) Exists() bool {
 	return cgroups.PathExists(m.dirPath)
 }
+
+func OOMKillCount(path string) (uint64, error) {
+	return fscommon.GetValueByKey(path, "memory.events", "oom_kill")
+}
+
+func (m *manager) OOMKillCount() (uint64, error) {
+	return OOMKillCount(m.dirPath)
+}
diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go
index 18cd411c..feddc7aa 100644
--- a/libcontainer/cgroups/fs2/hugetlb.go
+++ b/libcontainer/cgroups/fs2/hugetlb.go
@@ -44,14 +44,10 @@ func statHugeTlb(dirPath string, stats *cgroups.Stats) error {
 		hugetlbStats.Usage = value
 
 		fileName := "hugetlb." + pagesize + ".events"
-		contents, err := fscommon.ReadFile(dirPath, fileName)
+		value, err = fscommon.GetValueByKey(dirPath, fileName, "max")
 		if err != nil {
 			return errors.Wrap(err, "failed to read stats")
 		}
-		_, value, err = fscommon.GetCgroupParamKeyValue(contents)
-		if err != nil {
-			return errors.Wrap(err, "failed to parse "+fileName)
-		}
 		hugetlbStats.Failcnt = value
 
 		stats.HugetlbStats[pagesize] = hugetlbStats
diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go
index 81b28df7..cc8eaeb2 100644
--- a/libcontainer/cgroups/fs2/memory.go
+++ b/libcontainer/cgroups/fs2/memory.go
@@ -82,7 +82,7 @@ func statMemory(dirPath string, stats *cgroups.Stats) error {
 
 	sc := bufio.NewScanner(statsFile)
 	for sc.Scan() {
-		t, v, err := fscommon.GetCgroupParamKeyValue(sc.Text())
+		t, v, err := fscommon.ParseKeyValue(sc.Text())
 		if err != nil {
 			return errors.Wrapf(err, "failed to parse memory.stat (%q)", sc.Text())
 		}
diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go
index 2e4e837f..db0caded 100644
--- a/libcontainer/cgroups/fscommon/utils.go
+++ b/libcontainer/cgroups/fscommon/utils.go
@@ -35,22 +35,42 @@ func ParseUint(s string, base, bitSize int) (uint64, error) {
 	return value, nil
 }
 
-// GetCgroupParamKeyValue parses a space-separated "name value" kind of cgroup
-// parameter and returns its components. For example, "io_service_bytes 1234"
-// will return as "io_service_bytes", 1234.
-func GetCgroupParamKeyValue(t string) (string, uint64, error) {
-	parts := strings.Fields(t)
-	switch len(parts) {
-	case 2:
-		value, err := ParseUint(parts[1], 10, 64)
-		if err != nil {
-			return "", 0, fmt.Errorf("unable to convert to uint64: %v", err)
-		}
+// ParseKeyValue parses a space-separated "name value" kind of cgroup
+// parameter and returns its key as a string, and its value as uint64
+// (ParseUint is used to convert the value). For example,
+// "io_service_bytes 1234" will be returned as "io_service_bytes", 1234.
+func ParseKeyValue(t string) (string, uint64, error) {
+	parts := strings.SplitN(t, " ", 3)
+	if len(parts) != 2 {
+		return "", 0, fmt.Errorf("line %q is not in key value format", t)
+	}
 
-		return parts[0], value, nil
-	default:
-		return "", 0, ErrNotValidFormat
+	value, err := ParseUint(parts[1], 10, 64)
+	if err != nil {
+		return "", 0, fmt.Errorf("unable to convert to uint64: %v", err)
 	}
+
+	return parts[0], value, nil
+}
+
+// GetValueByKey reads a key-value pairs from the specified cgroup file,
+// and returns a value of the specified key. ParseUint is used for value
+// conversion.
+func GetValueByKey(path, file, key string) (uint64, error) {
+	content, err := ReadFile(path, file)
+	if err != nil {
+		return 0, err
+	}
+
+	lines := strings.Split(string(content), "\n")
+	for _, line := range lines {
+		arr := strings.Split(line, " ")
+		if len(arr) == 2 && arr[0] == key {
+			return ParseUint(arr[1], 10, 64)
+		}
+	}
+
+	return 0, nil
 }
 
 // GetCgroupParamUint reads a single uint64 value from the specified cgroup file.
diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
index 2ad1347e..59a8b1e7 100644
--- a/libcontainer/cgroups/systemd/v1.go
+++ b/libcontainer/cgroups/systemd/v1.go
@@ -450,3 +450,7 @@ func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) {
 func (m *legacyManager) Exists() bool {
 	return cgroups.PathExists(m.Path("devices"))
 }
+
+func (m *legacyManager) OOMKillCount() (uint64, error) {
+	return fs.OOMKillCount(m.Path("memory"))
+}
diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go
index 6f142265..65abc059 100644
--- a/libcontainer/cgroups/systemd/v2.go
+++ b/libcontainer/cgroups/systemd/v2.go
@@ -495,3 +495,7 @@ func (m *unifiedManager) GetFreezerState() (configs.FreezerState, error) {
 func (m *unifiedManager) Exists() bool {
 	return cgroups.PathExists(m.path)
 }
+
+func (m *unifiedManager) OOMKillCount() (uint64, error) {
+	return fs2.OOMKillCount(m.path)
+}
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 3dca29e4..65e2eace 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -570,6 +570,7 @@ func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, messageSockP
 		intelRdtPath:    state.IntelRdtPath,
 		messageSockPair: messageSockPair,
 		logFilePair:     logFilePair,
+		manager:         c.cgroupManager,
 		config:          c.newInitConfig(p),
 		process:         p,
 		bootstrapData:   data,
diff --git a/libcontainer/notify_linux_v2.go b/libcontainer/notify_linux_v2.go
index cdab10ed..dd0ec290 100644
--- a/libcontainer/notify_linux_v2.go
+++ b/libcontainer/notify_linux_v2.go
@@ -3,48 +3,28 @@
 package libcontainer
 
 import (
-	"io/ioutil"
 	"path/filepath"
-	"strconv"
-	"strings"
 	"unsafe"
 
+	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
 
-func getValueFromCgroup(path, key string) (int, error) {
-	content, err := ioutil.ReadFile(path)
-	if err != nil {
-		return 0, err
-	}
-
-	lines := strings.Split(string(content), "\n")
-	for _, line := range lines {
-		arr := strings.Split(line, " ")
-		if len(arr) == 2 && arr[0] == key {
-			return strconv.Atoi(arr[1])
-		}
-	}
-	return 0, nil
-}
-
 func registerMemoryEventV2(cgDir, evName, cgEvName string) (<-chan struct{}, error) {
-	eventControlPath := filepath.Join(cgDir, evName)
-	cgEvPath := filepath.Join(cgDir, cgEvName)
 	fd, err := unix.InotifyInit()
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to init inotify")
 	}
 	// watching oom kill
-	evFd, err := unix.InotifyAddWatch(fd, eventControlPath, unix.IN_MODIFY)
+	evFd, err := unix.InotifyAddWatch(fd, filepath.Join(cgDir, evName), unix.IN_MODIFY)
 	if err != nil {
 		unix.Close(fd)
 		return nil, errors.Wrap(err, "unable to add inotify watch")
 	}
 	// Because no `unix.IN_DELETE|unix.IN_DELETE_SELF` event for cgroup file system, so watching all process exited
-	cgFd, err := unix.InotifyAddWatch(fd, cgEvPath, unix.IN_MODIFY)
+	cgFd, err := unix.InotifyAddWatch(fd, filepath.Join(cgDir, cgEvName), unix.IN_MODIFY)
 	if err != nil {
 		unix.Close(fd)
 		return nil, errors.Wrap(err, "unable to add inotify watch")
@@ -79,12 +59,12 @@ func registerMemoryEventV2(cgDir, evName, cgEvName string) (<-chan struct{}, err
 				}
 				switch int(rawEvent.Wd) {
 				case evFd:
-					oom, err := getValueFromCgroup(eventControlPath, "oom_kill")
+					oom, err := fscommon.GetValueByKey(cgDir, evName, "oom_kill")
 					if err != nil || oom > 0 {
 						ch <- struct{}{}
 					}
 				case cgFd:
-					pids, err := getValueFromCgroup(cgEvPath, "populated")
+					pids, err := fscommon.GetValueByKey(cgDir, cgEvName, "populated")
 					if err != nil || pids == 0 {
 						return
 					}
diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 834268b9..6f282877 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -65,6 +65,7 @@ type setnsProcess struct {
 	logFilePair     filePair
 	cgroupPaths     map[string]string
 	rootlessCgroups bool
+	manager         cgroups.Manager
 	intelRdtPath    string
 	config          *initConfig
 	fds             []string
@@ -88,6 +89,8 @@ func (p *setnsProcess) signal(sig os.Signal) error {
 
 func (p *setnsProcess) start() (retErr error) {
 	defer p.messageSockPair.parent.Close()
+	// get the "before" value of oom kill count
+	oom, _ := p.manager.OOMKillCount()
 	err := p.cmd.Start()
 	// close the write-side of the pipes (controlled by child)
 	p.messageSockPair.child.Close()
@@ -97,6 +100,10 @@ func (p *setnsProcess) start() (retErr error) {
 	}
 	defer func() {
 		if retErr != nil {
+			if newOom, err := p.manager.OOMKillCount(); err == nil && newOom != oom {
+				// Someone in this cgroup was killed, this _might_ be us.
+				retErr = newSystemErrorWithCause(retErr, "possibly OOM-killed")
+			}
 			err := ignoreTerminateErrors(p.terminate())
 			if err != nil {
 				logrus.WithError(err).Warn("unable to terminate setnsProcess")
@@ -321,6 +328,24 @@ func (p *initProcess) start() (retErr error) {
 	}
 	defer func() {
 		if retErr != nil {
+			// init might be killed by the kernel's OOM killer.
+			oom, err := p.manager.OOMKillCount()
+			if err != nil {
+				logrus.WithError(err).Warn("unable to get oom kill count")
+			} else if oom > 0 {
+				// Does not matter what the particular error was,
+				// its cause is most probably OOM, so report that.
+				const oomError = "container init was OOM-killed (memory limit too low?)"
+
+				if logrus.GetLevel() >= logrus.DebugLevel {
+					// Only show the original error if debug is set,
+					// as it is not generally very useful.
+					retErr = newSystemErrorWithCause(retErr, oomError)
+				} else {
+					retErr = newSystemError(errors.New(oomError))
+				}
+			}
+
 			// terminate the process to ensure we can remove cgroups
 			if err := ignoreTerminateErrors(p.terminate()); err != nil {
 				logrus.WithError(err).Warn("unable to terminate initProcess")


Test patch:
diff --git a/libcontainer/cgroups/fs/cpu_test.go b/libcontainer/cgroups/fs/cpu_test.go
index 4a8ecf99..9adc9b31 100644
--- a/libcontainer/cgroups/fs/cpu_test.go
+++ b/libcontainer/cgroups/fs/cpu_test.go
@@ -112,7 +112,7 @@ func TestCpuStats(t *testing.T) {
 		throttledTime = uint64(18446744073709551615)
 	)
 
-	cpuStatContent := fmt.Sprintf("nr_periods %d\n nr_throttled %d\n throttled_time %d\n",
+	cpuStatContent := fmt.Sprintf("nr_periods %d\nnr_throttled %d\nthrottled_time %d\n",
 		nrPeriods, nrThrottled, throttledTime)
 	helper.writeFileContents(map[string]string{
 		"cpu.stat": cpuStatContent,
diff --git a/libcontainer/cgroups/fs/memory_test.go b/libcontainer/cgroups/fs/memory_test.go
index 00fb338b..44f1c9a9 100644
--- a/libcontainer/cgroups/fs/memory_test.go
+++ b/libcontainer/cgroups/fs/memory_test.go
@@ -165,11 +165,6 @@ func TestMemorySetSwapSmallerThanMemory(t *testing.T) {
 	helper.writeFileContents(map[string]string{
 		"memory.limit_in_bytes":       strconv.Itoa(memoryBefore),
 		"memory.memsw.limit_in_bytes": strconv.Itoa(memoryswapBefore),
-		// Set will call getMemoryData when memory and swap memory are
-		// both set, fake these fields so we don't get error.
-		"memory.usage_in_bytes":     "0",
-		"memory.max_usage_in_bytes": "0",
-		"memory.failcnt":            "0",
 	})
 
 	helper.CgroupData.config.Resources.Memory = memoryAfter
@@ -184,14 +179,14 @@ func TestMemorySetSwapSmallerThanMemory(t *testing.T) {
 		t.Fatalf("Failed to parse memory.limit_in_bytes - %s", err)
 	}
 	if value != memoryAfter {
-		t.Fatal("Got the wrong value, set memory.limit_in_bytes failed.")
+		t.Fatalf("Got the wrong value (%d != %d), set memory.limit_in_bytes failed", value, memoryAfter)
 	}
 	value, err = fscommon.GetCgroupParamUint(helper.CgroupPath, "memory.memsw.limit_in_bytes")
 	if err != nil {
 		t.Fatalf("Failed to parse memory.memsw.limit_in_bytes - %s", err)
 	}
 	if value != memoryswapAfter {
-		t.Fatal("Got the wrong value, set memory.memsw.limit_in_bytes failed.")
+		t.Fatalf("Got the wrong value (%d != %d), set memory.memsw.limit_in_bytes failed", value, memoryswapAfter)
 	}
 }
 
diff --git a/libcontainer/container_linux_test.go b/libcontainer/container_linux_test.go
index 73915997..2a9af6ad 100644
--- a/libcontainer/container_linux_test.go
+++ b/libcontainer/container_linux_test.go
@@ -55,6 +55,10 @@ func (m *mockCgroupManager) Exists() bool {
 	return err == nil
 }
 
+func (m *mockCgroupManager) OOMKillCount() (uint64, error) {
+	return 0, nil
+}
+
 func (m *mockCgroupManager) GetPaths() map[string]string {
 	return m.paths
 }
diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats
index a319ed04..38d06304 100644
--- a/tests/integration/cgroups.bats
+++ b/tests/integration/cgroups.bats
@@ -79,7 +79,7 @@ function setup() {
 
 	runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_permissions
 	[ "$status" -eq 1 ]
-	[[ ${lines[0]} == *"permission denied"* ]]
+	[[ "$output" == *"applying cgroup configuration"*"permission denied"* ]]
 }
 
 @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" {
@@ -92,7 +92,8 @@ function setup() {
 
 	runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_permissions
 	[ "$status" -eq 1 ]
-	[[ ${lines[0]} == *"rootless needs no limits + no cgrouppath when no permission is granted for cgroups"* ]] || [[ ${lines[0]} == *"cannot set pids limit: container could not join or create cgroup"* ]]
+	[[ "$output" == *"rootless needs no limits + no cgrouppath when no permission is granted for cgroups"* ]] ||
+		[[ "$output" == *"cannot set pids limit: container could not join or create cgroup"* ]]
 }
 
 @test "runc create (limits + cgrouppath + permission on the cgroup dir) succeeds" {
diff --git a/tests/integration/update.bats b/tests/integration/update.bats
index c03e7c6d..5e6377d0 100644
--- a/tests/integration/update.bats
+++ b/tests/integration/update.bats
@@ -238,6 +238,42 @@ EOF
 
 	check_cgroup_value "pids.max" 20
 	check_systemd_value "TasksMax" 20
+
+	if [ "$HAVE_SWAP" = "yes" ]; then
+		# Test case for https://github.com/opencontainers/runc/pull/592,
+		# checking libcontainer/cgroups/fs/memory.go:setMemoryAndSwap.
+
+		runc update test_update --memory 30M --memory-swap 50M
+		[ "$status" -eq 0 ]
+
+		check_cgroup_value $MEM_LIMIT $((30 * 1024 * 1024))
+		check_systemd_value $SD_MEM_LIMIT $((30 * 1024 * 1024))
+
+		if [ "$CGROUP_UNIFIED" = "yes" ]; then
+			# for cgroupv2, swap does not include mem
+			check_cgroup_value "$MEM_SWAP" $((20 * 1024 * 1024))
+			check_systemd_value "$SD_MEM_SWAP" $((20 * 1024 * 1024))
+		else
+			check_cgroup_value "$MEM_SWAP" $((50 * 1024 * 1024))
+			check_systemd_value "$SD_MEM_SWAP" $((50 * 1024 * 1024))
+		fi
+
+		# Now, set new memory to more than old swap
+		runc update test_update --memory 60M --memory-swap 80M
+		[ "$status" -eq 0 ]
+
+		check_cgroup_value $MEM_LIMIT $((60 * 1024 * 1024))
+		check_systemd_value $SD_MEM_LIMIT $((60 * 1024 * 1024))
+
+		if [ "$CGROUP_UNIFIED" = "yes" ]; then
+			# for cgroupv2, swap does not include mem
+			check_cgroup_value "$MEM_SWAP" $((20 * 1024 * 1024))
+			check_systemd_value "$SD_MEM_SWAP" $((20 * 1024 * 1024))
+		else
+			check_cgroup_value "$MEM_SWAP" $((80 * 1024 * 1024))
+			check_systemd_value "$SD_MEM_SWAP" $((80 * 1024 * 1024))
+		fi
+	fi
 }
 
 @test "update cgroup cpu limits" {