Fixes to cgroups

There were a few problems with cgroups:
- cleanup loop what breaking too early
- parse of /proc/[pid]/cgroups was skipping "name=systemd"
  because "name=" was not being removed from name.
- When no limits are specified, fillFromAncestor was not being
  called, causing a failure to set cpuset.mems

Updates #4536

PiperOrigin-RevId: 337947356

3 files changed, 26 insertions, 24 deletions

diff --git a/runsc/cgroup/cgroup.go b/runsc/cgroup/cgroup.go
index 8fbc3887a..56da21584 100644
--- a/runsc/cgroup/cgroup.go
+++ b/runsc/cgroup/cgroup.go
@@ -201,13 +201,15 @@ func LoadPaths(pid string) (map[string]string, error) {
 	paths := make(map[string]string)
 	scanner := bufio.NewScanner(f)
 	for scanner.Scan() {
-		// Format: ID:controller1,controller2:path
+		// Format: ID:[name=]controller1,controller2:path
 		// Example: 2:cpu,cpuacct:/user.slice
 		tokens := strings.Split(scanner.Text(), ":")
 		if len(tokens) != 3 {
 			return nil, fmt.Errorf("invalid cgroups file, line: %q", scanner.Text())
 		}
 		for _, ctrlr := range strings.Split(tokens[1], ",") {
+			// Remove prefix for cgroups with no controller, eg. systemd.
+			ctrlr = strings.TrimPrefix(ctrlr, "name=")
 			paths[ctrlr] = tokens[2]
 		}
 	}
@@ -237,7 +239,7 @@ func New(spec *specs.Spec) (*Cgroup, error) {
 		var err error
 		parents, err = LoadPaths("self")
 		if err != nil {
-			return nil, fmt.Errorf("finding current cgroups: %v", err)
+			return nil, fmt.Errorf("finding current cgroups: %w", err)
 		}
 	}
 	return &Cgroup{
@@ -276,10 +278,8 @@ func (c *Cgroup) Install(res *specs.LinuxResources) error {
 			}
 			return err
 		}
-		if res != nil {
-			if err := cfg.ctrlr.set(res, path); err != nil {
-				return err
-			}
+		if err := cfg.ctrlr.set(res, path); err != nil {
+			return err
 		}
 	}
 	clean.Release()
@@ -304,14 +304,15 @@ func (c *Cgroup) Uninstall() error {
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 		b := backoff.WithContext(backoff.NewConstantBackOff(100*time.Millisecond), ctx)
-		if err := backoff.Retry(func() error {
+		fn := func() error {
 			err := syscall.Rmdir(path)
 			if os.IsNotExist(err) {
 				return nil
 			}
 			return err
-		}, b); err != nil {
-			return fmt.Errorf("removing cgroup path %q: %v", path, err)
+		}
+		if err := backoff.Retry(fn, b); err != nil {
+			return fmt.Errorf("removing cgroup path %q: %w", path, err)
 		}
 	}
 	return nil
@@ -332,7 +333,6 @@ func (c *Cgroup) Join() (func(), error) {
 		if _, ok := controllers[ctrlr]; ok {
 			fullPath := filepath.Join(cgroupRoot, ctrlr, path)
 			undoPaths = append(undoPaths, fullPath)
-			break
 		}
 	}
 
@@ -422,7 +422,7 @@ func (*noop) set(*specs.LinuxResources, string) error {
 type memory struct{}
 
 func (*memory) set(spec *specs.LinuxResources, path string) error {
-	if spec.Memory == nil {
+	if spec == nil || spec.Memory == nil {
 		return nil
 	}
 	if err := setOptionalValueInt(path, "memory.limit_in_bytes", spec.Memory.Limit); err != nil {
@@ -455,7 +455,7 @@ func (*memory) set(spec *specs.LinuxResources, path string) error {
 type cpu struct{}
 
 func (*cpu) set(spec *specs.LinuxResources, path string) error {
-	if spec.CPU == nil {
+	if spec == nil || spec.CPU == nil {
 		return nil
 	}
 	if err := setOptionalValueUint(path, "cpu.shares", spec.CPU.Shares); err != nil {
@@ -478,7 +478,7 @@ type cpuSet struct{}
 func (*cpuSet) set(spec *specs.LinuxResources, path string) error {
 	// cpuset.cpus and mems are required fields, but are not set on a new cgroup.
 	// If not set in the spec, get it from one of the ancestors cgroup.
-	if spec.CPU == nil || spec.CPU.Cpus == "" {
+	if spec == nil || spec.CPU == nil || spec.CPU.Cpus == "" {
 		if _, err := fillFromAncestor(filepath.Join(path, "cpuset.cpus")); err != nil {
 			return err
 		}
@@ -488,18 +488,17 @@ func (*cpuSet) set(spec *specs.LinuxResources, path string) error {
 		}
 	}
 
-	if spec.CPU == nil || spec.CPU.Mems == "" {
+	if spec == nil || spec.CPU == nil || spec.CPU.Mems == "" {
 		_, err := fillFromAncestor(filepath.Join(path, "cpuset.mems"))
 		return err
 	}
-	mems := spec.CPU.Mems
-	return setValue(path, "cpuset.mems", mems)
+	return setValue(path, "cpuset.mems", spec.CPU.Mems)
 }
 
 type blockIO struct{}
 
 func (*blockIO) set(spec *specs.LinuxResources, path string) error {
-	if spec.BlockIO == nil {
+	if spec == nil || spec.BlockIO == nil {
 		return nil
 	}
 
@@ -549,7 +548,7 @@ func setThrottle(path, name string, devs []specs.LinuxThrottleDevice) error {
 type networkClass struct{}
 
 func (*networkClass) set(spec *specs.LinuxResources, path string) error {
-	if spec.Network == nil {
+	if spec == nil || spec.Network == nil {
 		return nil
 	}
 	return setOptionalValueUint32(path, "net_cls.classid", spec.Network.ClassID)
@@ -558,7 +557,7 @@ func (*networkClass) set(spec *specs.LinuxResources, path string) error {
 type networkPrio struct{}
 
 func (*networkPrio) set(spec *specs.LinuxResources, path string) error {
-	if spec.Network == nil {
+	if spec == nil || spec.Network == nil {
 		return nil
 	}
 	for _, prio := range spec.Network.Priorities {
@@ -573,7 +572,7 @@ func (*networkPrio) set(spec *specs.LinuxResources, path string) error {
 type pids struct{}
 
 func (*pids) set(spec *specs.LinuxResources, path string) error {
-	if spec.Pids == nil || spec.Pids.Limit <= 0 {
+	if spec == nil || spec.Pids == nil || spec.Pids.Limit <= 0 {
 		return nil
 	}
 	val := strconv.FormatInt(spec.Pids.Limit, 10)
@@ -583,6 +582,9 @@ func (*pids) set(spec *specs.LinuxResources, path string) error {
 type hugeTLB struct{}
 
 func (*hugeTLB) set(spec *specs.LinuxResources, path string) error {
+	if spec == nil {
+		return nil
+	}
 	for _, limit := range spec.HugepageLimits {
 		name := fmt.Sprintf("hugetlb.%s.limit_in_bytes", limit.Pagesize)
 		val := strconv.FormatUint(limit.Limit, 10)
diff --git a/runsc/container/container.go b/runsc/container/container.go
index 63478ba8c..63f64ce6e 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -985,7 +985,7 @@ func (c *Container) createGoferProcess(spec *specs.Spec, conf *config.Config, bu
 	// Start the gofer in the given namespace.
 	log.Debugf("Starting gofer: %s %v", binPath, args)
 	if err := specutils.StartInNS(cmd, nss); err != nil {
-		return nil, nil, fmt.Errorf("Gofer: %v", err)
+		return nil, nil, fmt.Errorf("gofer: %v", err)
 	}
 	log.Infof("Gofer started, PID: %d", cmd.Process.Pid)
 	c.GoferPid = cmd.Process.Pid
diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go
index 1f8e277cc..cc188f45b 100644
--- a/runsc/container/container_test.go
+++ b/runsc/container/container_test.go
@@ -2362,12 +2362,12 @@ func executeCombinedOutput(cont *Container, name string, arg ...string) ([]byte,
 }
 
 // executeSync synchronously executes a new process.
-func (cont *Container) executeSync(args *control.ExecArgs) (syscall.WaitStatus, error) {
-	pid, err := cont.Execute(args)
+func (c *Container) executeSync(args *control.ExecArgs) (syscall.WaitStatus, error) {
+	pid, err := c.Execute(args)
 	if err != nil {
 		return 0, fmt.Errorf("error executing: %v", err)
 	}
-	ws, err := cont.WaitPID(pid)
+	ws, err := c.WaitPID(pid)
 	if err != nil {
 		return 0, fmt.Errorf("error waiting: %v", err)
 	}