From 1775a0e11e56ee619a35b46d3d1561d99095a01c Mon Sep 17 00:00:00 2001
From: Nicolas Lacasse <nlacasse@google.com>
Date: Thu, 13 Dec 2018 15:36:59 -0800
Subject: [PATCH] container.Destroy should clean up container metadata even if
 other cleanups fail

If the sandbox process is dead (because of a panic or some other problem),
container.Destroy will never remove the container metadata file, since it will
always fail when calling container.stop().

This CL changes container.Destroy() to always perform the three necessary
cleanup operations:
* Stop the sandbox and gofer processes.
* Remove the container fs on the host.
* Delete the container metadata directory.

Errors from these three operations will be concatenated and returned from
Destroy().

PiperOrigin-RevId: 225448164
Change-Id: I99c6311b2e4fe5f6e2ca991424edf1ebeae9df32
---
 runsc/container/container.go | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/runsc/container/container.go b/runsc/container/container.go
index 80a27df4a..07924d23a 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -626,20 +626,36 @@ func (c *Container) Processes() ([]*control.Process, error) {
 }
 
 // Destroy stops all processes and frees all resources associated with the
-// container. It fails fast and is idempotent.
+// container.
 func (c *Container) Destroy() error {
 	log.Debugf("Destroy container %q", c.ID)
 
+	// We must perform the following cleanup steps:
+	// * stop the container and gofer processes,
+	// * remove the container filesystem on the host, and
+	// * delete the container metadata directory.
+	//
+	// It's possible for one or more of these steps to fail, but we should
+	// do our best to perform all of the cleanups. Hence, we keep a slice
+	// of errors return their concatenation.
+	var errs []string
+
 	if err := c.stop(); err != nil {
-		return fmt.Errorf("error stopping container: %v", err)
+		err = fmt.Errorf("error stopping container: %v", err)
+		log.Warningf("%v", err)
+		errs = append(errs, err.Error())
 	}
 
 	if err := destroyFS(c.Spec); err != nil {
-		return fmt.Errorf("error destroying container fs: %v", err)
+		err = fmt.Errorf("error destroying container fs: %v", err)
+		log.Warningf("%v", err)
+		errs = append(errs, err.Error())
 	}
 
 	if err := os.RemoveAll(c.Root); err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("error deleting container root directory %q: %v", c.Root, err)
+		err = fmt.Errorf("error deleting container root directory %q: %v", c.Root, err)
+		log.Warningf("%v", err)
+		errs = append(errs, err.Error())
 	}
 
 	// "If any poststop hook fails, the runtime MUST log a warning, but the
@@ -655,7 +671,11 @@ func (c *Container) Destroy() error {
 	}
 
 	c.changeStatus(Stopped)
-	return nil
+
+	if len(errs) == 0 {
+		return nil
+	}
+	return fmt.Errorf(strings.Join(errs, "\n"))
 }
 
 // save saves the container metadata to a file.