Supress error when deleting non-existing container with --force

This addresses the first issue reported in #59. CRI-O expects runsc to
return success to delete when --force is used with a non-existing container.

PiperOrigin-RevId: 198487418
Change-Id: If7660e8fdab1eb29549d0a7a45ea82e20a1d4f4a

runsc/cmd/BUILD

@@ -44,13 +44,17 @@ go_library(
 go_test(
     name = "cmd_test",
     size = "small",
-    srcs = ["exec_test.go"],
+    srcs = [
+        "delete_test.go",
+        "exec_test.go",
+    ],
     embed = [":cmd"],
     deps = [
         "//pkg/abi/linux",
         "//pkg/sentry/control",
         "//pkg/sentry/kernel/auth",
         "//pkg/urpc",
+        "//runsc/boot",
         "@com_github_google_go-cmp//cmp:go_default_library",
         "@com_github_google_go-cmp//cmp/cmpopts:go_default_library",
         "@com_github_opencontainers_runtime-spec//specs-go:go_default_library",

runsc/cmd/delete.go

@@ -15,9 +15,13 @@
 package cmd
 
 import (
+	"fmt"
+	"os"
+
 	"context"
 	"flag"
 	"github.com/google/subcommands"
+	"gvisor.googlesource.com/gvisor/pkg/log"
 	"gvisor.googlesource.com/gvisor/runsc/boot"
 	"gvisor.googlesource.com/gvisor/runsc/container"
 )
@@ -56,19 +60,28 @@ func (d *Delete) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}
 	}
 
 	conf := args[0].(*boot.Config)
-
-	for i := 0; i < f.NArg(); i++ {
-		id := f.Arg(i)
-		c, err := container.Load(conf.RootDir, id)
-		if err != nil {
-			Fatalf("error loading container %q: %v", id, err)
-		}
-		if !d.force && (c.Status == container.Running) {
-			Fatalf("cannot stop running container without --force flag")
-		}
-		if err := c.Destroy(); err != nil {
-			Fatalf("error destroying container: %v", err)
-		}
+	if err := d.execute(f.Args(), conf); err != nil {
+		Fatalf("%v", err)
 	}
 	return subcommands.ExitSuccess
 }
+
+func (d *Delete) execute(ids []string, conf *boot.Config) error {
+	for _, id := range ids {
+		c, err := container.Load(conf.RootDir, id)
+		if err != nil {
+			if os.IsNotExist(err) && d.force {
+				log.Warningf("couldn't find container %q: %v", id, err)
+				return nil
+			}
+			return fmt.Errorf("error loading container %q: %v", id, err)
+		}
+		if !d.force && (c.Status == container.Running) {
+			return fmt.Errorf("cannot stop running container without --force flag")
+		}
+		if err := c.Destroy(); err != nil {
+			return fmt.Errorf("error destroying container: %v", err)
+		}
+	}
+	return nil
+}

runsc/cmd/delete_test.go

@@ -0,0 +1,41 @@
+// Copyright 2018 Google Inc.
+//
+// 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 cmd
+
+import (
+	"io/ioutil"
+	"testing"
+
+	"gvisor.googlesource.com/gvisor/runsc/boot"
+)
+
+func TestNotFound(t *testing.T) {
+	ids := []string{"123"}
+	dir, err := ioutil.TempDir("", "metadata")
+	if err != nil {
+		t.Fatalf("error creating dir: %v", err)
+	}
+	conf := &boot.Config{RootDir: dir}
+
+	d := Delete{}
+	if err := d.execute(ids, conf); err == nil {
+		t.Error("Deleting non-existend container should have failed")
+	}
+
+	d = Delete{force: true}
+	if err := d.execute(ids, conf); err != nil {
+		t.Errorf("Deleting non-existend container with --force should NOT have failed: %v", err)
+	}
+}

runsc/container/container.go

@@ -93,21 +93,19 @@ type Container struct {
 }
 
 // Load loads a container with the given id from a metadata file.
+// Returns ErrNotExist if container doesn't exits.
 func Load(rootDir, id string) (*Container, error) {
 	log.Debugf("Load container %q %q", rootDir, id)
 	if err := validateID(id); err != nil {
 		return nil, err
 	}
-	cRoot := filepath.Join(rootDir, id)
-	if !exists(cRoot) {
-		return nil, fmt.Errorf("container with id %q does not exist", id)
-	}
-	metaFile := filepath.Join(cRoot, metadataFilename)
-	if !exists(metaFile) {
-		return nil, fmt.Errorf("container with id %q does not have metadata file %q", id, metaFile)
-	}
+	metaFile := filepath.Join(rootDir, id, metadataFilename)
 	metaBytes, err := ioutil.ReadFile(metaFile)
 	if err != nil {
+		if os.IsNotExist(err) {
+			// Preserve error so that callers can distinguish 'not found' errors.
+			return nil, err
+		}
 		return nil, fmt.Errorf("error reading container metadata file %q: %v", metaFile, err)
 	}
 	var c Container
@@ -161,8 +159,10 @@ func Create(id string, spec *specs.Spec, conf *boot.Config, bundleDir, consoleSo
 	}
 
 	containerRoot := filepath.Join(conf.RootDir, id)
-	if exists(containerRoot) {
-		return nil, fmt.Errorf("container with id %q already exists: %q ", id, containerRoot)
+	if _, err := os.Stat(containerRoot); err == nil {
+		return nil, fmt.Errorf("container with id %q already exists: %q", id, containerRoot)
+	} else if !os.IsNotExist(err) {
+		return nil, fmt.Errorf("error looking for existing container in %q: %v", containerRoot, err)
 	}
 
 	c := &Container{
@@ -328,11 +328,6 @@ func (c *Container) Destroy() error {
 		return err
 	}
 
-	// Then destroy all the metadata.
-	if err := os.RemoveAll(c.Root); err != nil {
-		log.Warningf("Failed to delete container root directory %q, err: %v", c.Root, err)
-	}
-
 	// "If any poststop hook fails, the runtime MUST log a warning, but the
 	// remaining hooks and lifecycle continue as if the hook had succeeded".
 	if c.Spec.Hooks != nil && (c.Status == Created || c.Status == Running) {
@@ -372,13 +367,3 @@ func (c *Container) save() error {
 	}
 	return nil
 }
-
-// exists returns true if the given file exists.
-func exists(f string) bool {
-	if _, err := os.Stat(f); err == nil {
-		return true
-	} else if !os.IsNotExist(err) {
-		log.Warningf("error checking for file %q: %v", f, err)
-	}
-	return false
-}