Merge pull request #12857 from giuseppe/fix-rm-dir-not-empty

exec: retry rm -rf on ENOTEMPTY and EBUSY

4 files changed, 59 insertions, 25 deletions

diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index 7d4e28d5d..d1c190905 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -14,6 +14,7 @@ import (
 	"github.com/containers/storage/pkg/stringid"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
 )
 
 // ExecConfig contains the configuration of an exec session
@@ -774,13 +775,40 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
 	return exitCode, nil
 }
 
-// cleanup an exec session after its done
-func (c *Container) cleanupExecBundle(sessionID string) error {
-	if err := os.RemoveAll(c.execBundlePath(sessionID)); err != nil && !os.IsNotExist(err) {
-		return err
+// cleanupExecBundle cleanups an exec session after its done
+// Please be careful when using this function since it might temporarily unlock
+// the container when os.RemoveAll($bundlePath) fails with ENOTEMPTY or EBUSY
+// errors.
+func (c *Container) cleanupExecBundle(sessionID string) (Err error) {
+	path := c.execBundlePath(sessionID)
+	for attempts := 0; attempts < 50; attempts++ {
+		Err = os.RemoveAll(path)
+		if Err == nil || os.IsNotExist(Err) {
+			return nil
+		}
+		if pathErr, ok := Err.(*os.PathError); ok {
+			Err = pathErr.Err
+			if errors.Cause(Err) == unix.ENOTEMPTY || errors.Cause(Err) == unix.EBUSY {
+				// give other processes a chance to use the container
+				if !c.batched {
+					if err := c.save(); err != nil {
+						return err
+					}
+					c.lock.Unlock()
+				}
+				time.Sleep(time.Millisecond * 100)
+				if !c.batched {
+					c.lock.Lock()
+					if err := c.syncContainer(); err != nil {
+						return err
+					}
+				}
+				continue
+			}
+		}
+		return
 	}
-
-	return nil
+	return
 }
 
 // the path to a containers exec session bundle
diff --git a/libpod/oci_attach_linux.go b/libpod/oci_attach_linux.go
index 1c15d567c..1ee664e81 100644
--- a/libpod/oci_attach_linux.go
+++ b/libpod/oci_attach_linux.go
@@ -273,9 +273,11 @@ func readStdio(conn *net.UnixConn, streams *define.AttachStreams, receiveStdoutE
 	var err error
 	select {
 	case err = <-receiveStdoutError:
+		conn.CloseWrite()
 		return err
 	case err = <-stdinDone:
 		if err == define.ErrDetach {
+			conn.CloseWrite()
 			return err
 		}
 		if err == nil {
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index 29c600109..04deaac83 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -389,6 +389,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 	if err != nil {
 		return nil, nil, err
 	}
+	defer processFile.Close()
 
 	args := r.sharedConmonArgs(c, sessionID, c.execBundlePath(sessionID), c.execPidPath(sessionID), c.execLogPath(sessionID), c.execExitFileDir(sessionID), ociLog, define.NoLogging, "")
 
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index 9ab12732f..3799b463f 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -644,6 +644,20 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
 		}
 	}
 
+	// Check that no other containers depend on the container.
+	// Only used if not removing a pod - pods guarantee that all
+	// deps will be evicted at the same time.
+	if !removePod {
+		deps, err := r.state.ContainerInUse(c)
+		if err != nil {
+			return err
+		}
+		if len(deps) != 0 {
+			depsStr := strings.Join(deps, ", ")
+			return errors.Wrapf(define.ErrCtrExists, "container %s has dependent containers which must be removed before it: %s", c.ID(), depsStr)
+		}
+	}
+
 	// Check that the container's in a good state to be removed.
 	if c.state.State == define.ContainerStateRunning {
 		time := c.StopTimeout()
@@ -666,25 +680,6 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
 		}
 	}
 
-	// Remove all active exec sessions
-	if err := c.removeAllExecSessions(); err != nil {
-		return err
-	}
-
-	// Check that no other containers depend on the container.
-	// Only used if not removing a pod - pods guarantee that all
-	// deps will be evicted at the same time.
-	if !removePod {
-		deps, err := r.state.ContainerInUse(c)
-		if err != nil {
-			return err
-		}
-		if len(deps) != 0 {
-			depsStr := strings.Join(deps, ", ")
-			return errors.Wrapf(define.ErrCtrExists, "container %s has dependent containers which must be removed before it: %s", c.ID(), depsStr)
-		}
-	}
-
 	var cleanupErr error
 
 	// Clean up network namespace, cgroups, mounts.
@@ -704,6 +699,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
 		return errors.Wrapf(err, "unable to set container %s removing state in database", c.ID())
 	}
 
+	// Remove all active exec sessions
+	// removing the exec sessions might temporarily unlock the container's lock.  Using it
+	// after setting the state to ContainerStateRemoving will prevent that the container is
+	// restarted
+	if err := c.removeAllExecSessions(); err != nil {
+		return err
+	}
+
 	// Stop the container's storage
 	if err := c.teardownStorage(); err != nil {
 		if cleanupErr == nil {