Merge release-20210927.0-61-g98a694eeb (automated)

2 files changed, 114 insertions, 35 deletions

diff --git a/pkg/sync/wait.go b/pkg/sync/wait.go
new file mode 100644
index 000000000..f8e7742a5
--- /dev/null
+++ b/pkg/sync/wait.go
@@ -0,0 +1,58 @@
+// Copyright 2021 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sync
+
+// WaitGroupErr is similar to WaitGroup but allows goroutines to report error.
+// Only the first error is retained and reported back.
+//
+// Example usage:
+// 	wg := WaitGroupErr{}
+// 	wg.Add(1)
+// 	go func() {
+//			defer wg.Done()
+//			if err := ...; err != nil {
+//				wg.ReportError(err)
+//				return
+// 			}
+// 	}()
+//	return wg.Error()
+//
+type WaitGroupErr struct {
+	WaitGroup
+
+	// mu protects firstErr.
+	mu Mutex
+
+	// firstErr holds the first error reported. nil is no error occurred.
+	firstErr error
+}
+
+// ReportError reports an error. Note it does not call Done().
+func (w *WaitGroupErr) ReportError(err error) {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	if w.firstErr == nil {
+		w.firstErr = err
+	}
+}
+
+// Error waits for the counter to reach 0 and returns the first reported error
+// if any.
+func (w *WaitGroupErr) Error() error {
+	w.Wait()
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	return w.firstErr
+}
diff --git a/runsc/cgroup/cgroup.go b/runsc/cgroup/cgroup.go
index 7280a52fc..7a0f0694f 100644
--- a/runsc/cgroup/cgroup.go
+++ b/runsc/cgroup/cgroup.go
@@ -19,7 +19,6 @@ package cgroup
 import (
 	"bufio"
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -34,6 +33,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/log"
+	"gvisor.dev/gvisor/pkg/sync"
 )
 
 const (
@@ -104,17 +104,21 @@ func setOptionalValueUint16(path, name string, val *uint16) error {
 
 func setValue(path, name, data string) error {
 	fullpath := filepath.Join(path, name)
+	log.Debugf("Setting %q to %q", fullpath, data)
+	return writeFile(fullpath, []byte(data), 0700)
+}
 
-	// Retry writes on EINTR; see:
-	//    https://github.com/golang/go/issues/38033
-	for {
-		err := ioutil.WriteFile(fullpath, []byte(data), 0700)
-		if err == nil {
-			return nil
-		} else if !errors.Is(err, unix.EINTR) {
-			return err
-		}
+// writeFile is similar to ioutil.WriteFile() but doesn't create the file if it
+// doesn't exist.
+func writeFile(path string, data []byte, perm os.FileMode) error {
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, perm)
+	if err != nil {
+		return err
 	}
+	defer f.Close()
+
+	_, err = f.Write(data)
+	return err
 }
 
 func getValue(path, name string) (string, error) {
@@ -155,15 +159,8 @@ func fillFromAncestor(path string) (string, error) {
 		return "", err
 	}
 
-	// Retry writes on EINTR; see:
-	//    https://github.com/golang/go/issues/38033
-	for {
-		err := ioutil.WriteFile(path, []byte(val), 0700)
-		if err == nil {
-			break
-		} else if !errors.Is(err, unix.EINTR) {
-			return "", err
-		}
+	if err := writeFile(path, []byte(val), 0700); err != nil {
+		return "", nil
 	}
 	return val, nil
 }
@@ -371,21 +368,20 @@ func (c *Cgroup) Install(res *specs.LinuxResources) error {
 	}
 	for _, key := range missing {
 		ctrlr := controllers[key]
-		path := c.MakePath(key)
-		log.Debugf("Creating cgroup %q: %q", key, path)
-		if err := os.MkdirAll(path, 0755); err != nil {
-			if ctrlr.optional() && errors.Is(err, unix.EROFS) {
-				if err := ctrlr.skip(res); err != nil {
-					return err
-				}
-				log.Infof("Skipping cgroup %q", key)
-				continue
+
+		if skip, err := c.createController(key); skip && ctrlr.optional() {
+			if err := ctrlr.skip(res); err != nil {
+				return err
 			}
+			log.Infof("Skipping cgroup %q, err: %v", key, err)
+			continue
+		} else if err != nil {
 			return err
 		}
 
 		// Only set controllers that were created by me.
 		c.Own[key] = true
+		path := c.MakePath(key)
 		if err := ctrlr.set(res, path); err != nil {
 			return err
 		}
@@ -394,10 +390,29 @@ func (c *Cgroup) Install(res *specs.LinuxResources) error {
 	return nil
 }
 
+// createController creates the controller directory, checking that the
+// controller is enabled in the system. It returns a boolean indicating whether
+// the controller should be skipped (e.g. controller is disabled). In case it
+// should be skipped, it also returns the error it got.
+func (c *Cgroup) createController(name string) (bool, error) {
+	ctrlrPath := filepath.Join(cgroupRoot, name)
+	if _, err := os.Stat(ctrlrPath); err != nil {
+		return os.IsNotExist(err), err
+	}
+
+	path := c.MakePath(name)
+	log.Debugf("Creating cgroup %q: %q", name, path)
+	if err := os.MkdirAll(path, 0755); err != nil {
+		return false, err
+	}
+	return false, nil
+}
+
 // Uninstall removes the settings done in Install(). If cgroup path already
 // existed when Install() was called, Uninstall is a noop.
 func (c *Cgroup) Uninstall() error {
 	log.Debugf("Deleting cgroup %q", c.Name)
+	wait := sync.WaitGroupErr{}
 	for key := range controllers {
 		if !c.Own[key] {
 			// cgroup is managed by caller, don't touch it.
@@ -406,9 +421,8 @@ func (c *Cgroup) Uninstall() error {
 		path := c.MakePath(key)
 		log.Debugf("Removing cgroup controller for key=%q path=%q", key, path)
 
-		// If we try to remove the cgroup too soon after killing the
-		// sandbox we might get EBUSY, so we retry for a few seconds
-		// until it succeeds.
+		// If we try to remove the cgroup too soon after killing the sandbox we
+		// might get EBUSY, so we retry for a few seconds until it succeeds.
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 		b := backoff.WithContext(backoff.NewConstantBackOff(100*time.Millisecond), ctx)
@@ -419,11 +433,18 @@ func (c *Cgroup) Uninstall() error {
 			}
 			return err
 		}
-		if err := backoff.Retry(fn, b); err != nil {
-			return fmt.Errorf("removing cgroup path %q: %w", path, err)
-		}
+		// Run deletions in parallel to remove all directories even if there are
+		// failures/timeouts in other directories.
+		wait.Add(1)
+		go func() {
+			defer wait.Done()
+			if err := backoff.Retry(fn, b); err != nil {
+				wait.ReportError(fmt.Errorf("removing cgroup path %q: %w", path, err))
+				return
+			}
+		}()
 	}
-	return nil
+	return wait.Error()
 }
 
 // Join adds the current process to the all controllers. Returns function that