SQUASHME Address comments by alexlarsson

Trivial fixes to error returns
Changes to error handling in sandbox create/return
Error handling functions added to state, used when listing containers
Log correct error in a defer
Rework of error handling code in state

Signed-off-by: Matthew Heon <mheon@redhat.com>

server/container_create.go

@@ -239,7 +239,7 @@ func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerReq
 
 	sb, err := s.state.LookupSandboxByID(sbID)
 	if err != nil {
-		return nil, fmt.Errorf("error retrieving PodSandbox with ID starting with %s: %v", sbID, err)
+		return nil, err
 	}
 
 	// The config of the container
@@ -506,7 +506,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 	}
 	if sb.ResolvPath() != "" {
 		// bind mount the pod resolver file
-		specgen.AddBindMount(sb.ResolvPath(), "/etc/resolv.conf", []string{"ro"})
+		specgen.AddBindMount(sb.ResolvPath(), "/etc/resolv.conf", options)
 	}
 
 	// Bind mount /etc/hosts for host networking containers

server/container_list.go

@@ -3,6 +3,7 @@ package server
 import (
 	"github.com/Sirupsen/logrus"
 	"github.com/kubernetes-incubator/cri-o/oci"
+	"github.com/kubernetes-incubator/cri-o/server/state"
 	"golang.org/x/net/context"
 	"k8s.io/apimachinery/pkg/fields"
 	pb "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
@@ -38,23 +39,31 @@ func (s *Server) ListContainers(ctx context.Context, req *pb.ListContainersReque
 		if filter.Id != "" {
 			c, err := s.state.LookupContainerByID(filter.Id)
 			if err != nil {
-				return nil, err
+				if !state.IsCtrNotExist(err) {
-			}
+					return nil, err
-			if filter.PodSandboxId != "" {
-				if c.Sandbox() == filter.PodSandboxId {
-					ctrList = []*oci.Container{c}
-				} else {
-					ctrList = []*oci.Container{}
 				}
 
+				ctrList = []*oci.Container{}
 			} else {
-				ctrList = []*oci.Container{c}
+				if filter.PodSandboxId != "" {
+					if c.Sandbox() == filter.PodSandboxId {
+						ctrList = []*oci.Container{c}
+					} else {
+						ctrList = []*oci.Container{}
+					}
+
+				} else {
+					ctrList = []*oci.Container{c}
+				}
 			}
 		} else {
 			if filter.PodSandboxId != "" {
 				pod, err := s.state.GetSandbox(filter.PodSandboxId)
-				// TODO check if this is a pod not found error, if not we should error out here
 				if err != nil {
+					if !state.IsSandboxNotExist(err) {
+						return nil, err
+					}
+
 					ctrList = []*oci.Container{}
 				} else {
 					ctrList = pod.Containers()

server/sandbox/sandbox.go

@@ -287,6 +287,7 @@ func (s *Sandbox) Hostname() string {
 	return s.hostname
 }
 
+// PortMappings gets the sandbox's host port mappings
 func (s *Sandbox) PortMappings() []*hostport.PortMapping {
 	return s.portMappings
 }

server/sandbox_list.go

@@ -6,6 +6,7 @@ import (
 	"github.com/Sirupsen/logrus"
 	"github.com/kubernetes-incubator/cri-o/oci"
 	"github.com/kubernetes-incubator/cri-o/server/sandbox"
+	"github.com/kubernetes-incubator/cri-o/server/state"
 	"golang.org/x/net/context"
 	"k8s.io/apimachinery/pkg/fields"
 	pb "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
@@ -47,8 +48,11 @@ func (s *Server) ListPodSandbox(ctx context.Context, req *pb.ListPodSandboxReque
 	if filter != nil {
 		if filter.Id != "" {
 			sb, err := s.state.LookupSandboxByID(filter.Id)
-			// TODO if we return something other than a No Such Sandbox should we throw an error instead?
 			if err != nil {
+				if !state.IsCtrNotExist(err) {
+					return nil, err
+				}
+
 				podList = []*sandbox.Sandbox{}
 			} else {
 				podList = []*sandbox.Sandbox{sb}

server/sandbox_remove.go

@@ -66,7 +66,7 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR
 		}
 
 		if err := s.removeContainer(c); err != nil {
-			return nil, fmt.Errorf("failed to delete container %s in pod sandbox %s: %v", c.Name(), sb.ID(), err)
+			return nil, err
 		}
 	}
 

server/server.go

@@ -614,7 +614,7 @@ func (s *Server) getPodSandboxFromRequest(podSandboxID string) (*sandbox.Sandbox
 
 	sb, err := s.state.LookupSandboxByID(podSandboxID)
 	if err != nil {
-		return nil, fmt.Errorf("could not retrieve pod sandbox with ID starting with %v: %v", podSandboxID, err)
+		return nil, err
 	}
 
 	return sb, nil

server/state/error.go

@@ -0,0 +1,98 @@
+package state
+
+import (
+	"fmt"
+)
+
+// NoSuchSandboxError is an error occurring when requested sandbox does not exist
+type NoSuchSandboxError struct {
+	id    string
+	name  string
+	inner error
+}
+
+// Error produces an human-readable error message
+func (e NoSuchSandboxError) Error() string {
+	if e.id != "" {
+		if e.inner == nil {
+			return fmt.Sprintf("no sandbox found with ID %s", e.id)
+		}
+
+		return fmt.Sprintf("no sandbox found with ID %s: %v", e.id, e.inner)
+	} else if e.name != "" {
+		if e.inner == nil {
+			return fmt.Sprintf("no sandbox found with name %s", e.name)
+		}
+
+		return fmt.Sprintf("no sandbox found with name %s: %v", e.name, e.inner)
+	} else if e.inner != nil {
+		return fmt.Sprintf("no such sandbox: %v", e.inner)
+	}
+
+	return "no such sandbox"
+}
+
+// NoSuchCtrError is an error occurring when requested container does not exist
+type NoSuchCtrError struct {
+	id      string
+	name    string
+	sandbox string
+	inner   error
+}
+
+// Error produces a human-readable error message
+func (e NoSuchCtrError) Error() string {
+	if e.id != "" {
+		if e.sandbox != "" {
+			if e.inner == nil {
+				return fmt.Sprintf("no container found with ID %s in sandbox %s", e.id, e.sandbox)
+			}
+
+			return fmt.Sprintf("no container found with ID %s in sandbox %s: %v", e.id, e.sandbox, e.inner)
+		}
+		if e.inner == nil {
+			return fmt.Sprintf("no container found with ID %s", e.id)
+		}
+
+		return fmt.Sprintf("no container found with ID %s: %v", e.id, e.inner)
+	} else if e.name != "" {
+		if e.sandbox != "" {
+			if e.inner == nil {
+				return fmt.Sprintf("no container found with name %s in sandbox %s", e.name, e.sandbox)
+			}
+
+			return fmt.Sprintf("no container found with name %s in sandbox %s: %v", e.name, e.sandbox, e.inner)
+		}
+		if e.inner == nil {
+			return fmt.Sprintf("no container found with name %s", e.name)
+		}
+
+		return fmt.Sprintf("no container found with name %s: %v", e.name, e.inner)
+	} else if e.inner != nil {
+		return fmt.Sprintf("no such container: %v", e.inner)
+	}
+
+	return "no such container"
+}
+
+// Functions for verifying errors
+
+// IsSandboxNotExist checks if an error indicated that given sandbox does not exist
+func IsSandboxNotExist(err error) bool {
+	switch err.(type) {
+	case *NoSuchSandboxError:
+		return true
+	default:
+		return false
+	}
+}
+
+// IsCtrNotExist checks if an error indicates that given container does not exist
+func IsCtrNotExist(err error) bool {
+	switch err.(type) {
+	case *NoSuchCtrError:
+		return true
+	default:
+		return false
+	}
+}

server/state/in_memory_state.go

@@ -47,12 +47,9 @@ func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) (err error) {
 		return fmt.Errorf("nil passed as sandbox to AddSandbox")
 	}
 
-	if _, exist := s.sandboxes[sandbox.ID()]; exist {
-		return fmt.Errorf("sandbox with ID %v already exists", sandbox.ID())
-	}
-
 	// We shouldn't share ID with any containers
 	// Our pod infra container will share our ID and we don't want it to conflict with anything
+	// An equivalent check for sandbox IDs is done in addSandboxMappings()
 	if _, exist := s.containers[sandbox.ID()]; exist {
 		return fmt.Errorf("requested sandbox ID %v conflicts with existing container ID", sandbox.ID())
 	}
@@ -63,28 +60,25 @@ func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) (err error) {
 	defer func() {
 		if err != nil {
 			if err2 := s.deleteSandboxMappings(sandbox); err2 != nil {
-				logrus.Errorf("Error removing mappings for incompletely-added sandbox %s: %v", sandbox.ID(), err)
+				logrus.Errorf("Error removing mappings for incompletely-added sandbox %s: %v", sandbox.ID(), err2)
 			}
 		}
 	}()
 
 	// If there are containers in the sandbox add them to the mapping
-	addedCtrs := make([]*oci.Container, 0, len(sandbox.Containers()))
 	for _, ctr := range sandbox.Containers() {
 		if err := s.addContainerMappings(ctr, true); err != nil {
 			return fmt.Errorf("error adding container %v mappings in sandbox %v", ctr.ID(), sandbox.ID())
 		}
-		addedCtrs = append(addedCtrs, ctr)
+
-	}
+		defer func(c *oci.Container) {
-	defer func() {
+			if err != nil {
-		if err != nil {
+				if err2 := s.deleteContainerMappings(c, true); err2 != nil {
-			for _, ctr := range addedCtrs {
+					logrus.Errorf("Error removing container %s mappings: %v", c.ID(), err2)
-				if err2 := s.deleteContainerMappings(ctr, true); err2 != nil {
-					logrus.Errorf("Error removing container %s mappings: %v", ctr.ID(), err2)
 				}
 			}
-		}
+		}(ctr)
-	}()
+	}
 
 	// Add the pod infrastructure container to mappings
 	// TODO: Right now, we don't add it to the all containers listing. We may want to change this.
@@ -129,7 +123,7 @@ func (s *InMemoryState) DeleteSandbox(id string) (err error) {
 	defer s.lock.Unlock()
 
 	if _, exist := s.sandboxes[id]; !exist {
-		return errNoSuchSandbox(id)
+		return &NoSuchSandboxError{id: id}
 	}
 
 	sb := s.sandboxes[id]
@@ -145,23 +139,20 @@ func (s *InMemoryState) DeleteSandbox(id string) (err error) {
 		}
 	}()
 
-	removedCtrs := make([]*oci.Container, 0, len(sb.Containers()))
 	// If there are containers left in the sandbox delete them from the mappings
 	for _, ctr := range sb.Containers() {
 		if err := s.deleteContainerMappings(ctr, true); err != nil {
 			return fmt.Errorf("error removing container %v mappings: %v", ctr.ID(), err)
 		}
-		removedCtrs = append(removedCtrs, ctr)
+
-	}
+		defer func(c *oci.Container) {
-	defer func() {
+			if err != nil {
-		if err != nil {
+				if err2 := s.addContainerMappings(c, true); err2 != nil {
-			for _, ctr := range removedCtrs {
+					logrus.Errorf("Error re-adding mappings for container %s: %v", c.ID(), err2)
-				if err2 := s.addContainerMappings(ctr, true); err2 != nil {
-					logrus.Errorf("Error re-adding mappings for container %s: %v", ctr.ID(), err2)
 				}
 			}
-		}
+		}(ctr)
-	}()
+	}
 
 	// Delete infra container from mappings
 	if err := s.deleteContainerMappings(sb.InfraContainer(), false); err != nil {
@@ -193,7 +184,7 @@ func (s *InMemoryState) GetSandbox(id string) (*sandbox.Sandbox, error) {
 
 	sandbox, ok := s.sandboxes[id]
 	if !ok {
-		return nil, errNoSuchSandbox(id)
+		return nil, &NoSuchSandboxError{id: id}
 	}
 
 	return sandbox, nil
@@ -206,7 +197,10 @@ func (s *InMemoryState) LookupSandboxByName(name string) (*sandbox.Sandbox, erro
 
 	id, err := s.podNameIndex.Get(name)
 	if err != nil {
-		return nil, fmt.Errorf("could not resolve sandbox name %v: %v", name, err)
+		return nil, &NoSuchSandboxError{
+			name:  name,
+			inner: err,
+		}
 	}
 
 	sandbox, ok := s.sandboxes[id]
@@ -226,7 +220,10 @@ func (s *InMemoryState) LookupSandboxByID(id string) (*sandbox.Sandbox, error) {
 
 	fullID, err := s.podIDIndex.Get(id)
 	if err != nil {
-		return nil, fmt.Errorf("could not resolve sandbox id %v: %v", id, err)
+		return nil, &NoSuchSandboxError{
+			id:    id,
+			inner: err,
+		}
 	}
 
 	sandbox, ok := s.sandboxes[fullID]
@@ -262,7 +259,7 @@ func (s *InMemoryState) AddContainer(c *oci.Container) error {
 
 	sandbox, ok := s.sandboxes[c.Sandbox()]
 	if !ok {
-		return errNoSuchSandbox(c.Sandbox())
+		return &NoSuchSandboxError{id: c.Sandbox()}
 	}
 
 	if ctr := sandbox.GetContainer(c.ID()); ctr != nil {
@@ -331,12 +328,15 @@ func (s *InMemoryState) DeleteContainer(id, sandboxID string) error {
 
 	sandbox, ok := s.sandboxes[sandboxID]
 	if !ok {
-		return errNoSuchSandbox(sandboxID)
+		return &NoSuchSandboxError{id: sandboxID}
 	}
 
 	ctr := sandbox.GetContainer(id)
 	if ctr == nil {
-		return errNoSuchContainerInSandbox(id, sandboxID)
+		return &NoSuchCtrError{
+			id:      id,
+			sandbox: id,
+		}
 	}
 
 	if err := s.deleteContainerMappings(ctr, true); err != nil {
@@ -385,7 +385,7 @@ func (s *InMemoryState) GetContainerSandbox(id string) (string, error) {
 
 	ctr, exist := s.containers[id]
 	if !exist {
-		return "", errNoSuchContainer(id)
+		return "", &NoSuchCtrError{id: id}
 	}
 
 	return ctr.Sandbox(), nil
@@ -398,7 +398,10 @@ func (s *InMemoryState) LookupContainerByName(name string) (*oci.Container, erro
 
 	fullID, err := s.ctrNameIndex.Get(name)
 	if err != nil {
-		return nil, fmt.Errorf("cannot resolve container name %v: %v", name, err)
+		return nil, &NoSuchCtrError{
+			name:  name,
+			inner: err,
+		}
 	}
 
 	return s.getContainer(fullID)
@@ -412,7 +415,10 @@ func (s *InMemoryState) LookupContainerByID(id string) (*oci.Container, error) {
 
 	fullID, err := s.ctrIDIndex.Get(id)
 	if err != nil {
-		return nil, fmt.Errorf("cannot resolve container ID %v: %v", id, err)
+		return nil, &NoSuchCtrError{
+			id:    id,
+			inner: err,
+		}
 	}
 
 	return s.getContainer(fullID)
@@ -437,7 +443,7 @@ func (s *InMemoryState) GetAllContainers() ([]*oci.Container, error) {
 func (s *InMemoryState) getContainer(id string) (*oci.Container, error) {
 	ctr, exist := s.containers[id]
 	if !exist {
-		return nil, errNoSuchContainer(id)
+		return nil, &NoSuchCtrError{id: id}
 	}
 
 	return s.getContainerFromSandbox(id, ctr.Sandbox())
@@ -448,25 +454,16 @@ func (s *InMemoryState) getContainer(id string) (*oci.Container, error) {
 func (s *InMemoryState) getContainerFromSandbox(id, sandboxID string) (*oci.Container, error) {
 	sandbox, ok := s.sandboxes[sandboxID]
 	if !ok {
-		return nil, errNoSuchSandbox(sandboxID)
+		return nil, &NoSuchSandboxError{id: sandboxID}
 	}
 
 	ctr := sandbox.GetContainer(id)
 	if ctr == nil {
-		return nil, errNoSuchContainerInSandbox(id, sandboxID)
+		return nil, &NoSuchCtrError{
+			id:      id,
+			sandbox: sandboxID,
+		}
 	}
 
 	return ctr, nil
 }
-
-func errNoSuchSandbox(id string) error {
-	return fmt.Errorf("no sandbox with ID %s found", id)
-}
-
-func errNoSuchContainer(id string) error {
-	return fmt.Errorf("no container with ID %s found", id)
-}
-
-func errNoSuchContainerInSandbox(id string, sandboxID string) error {
-	return fmt.Errorf("no container with ID %s in sandbox %s", id, sandboxID)
-}

test/ctr.bats

@@ -11,7 +11,7 @@ function teardown() {
 	run crioctl ctr status --id randomid
 	echo "$output"
 	[ "$status" -eq 1 ]
-	[[ "$output" =~ "container with ID starting with randomid could not be retrieved: cannot resolve container ID randomid: ID does not exist" ]]
+	[[ "$output" =~ "container with ID starting with randomid could not be retrieved: no container found with ID randomid: ID does not exist" ]]
 
 	stop_crio
 }

test/restore.bats

@@ -166,7 +166,7 @@ function teardown() {
 	run crioctl ctr stop --id "$ctr_id"
 	echo "$output"
 	[ "$status" -eq 1 ]
-	[[ "${output}" =~ "not found" ]]
+	[[ "${output}" =~ "no container found" ]]
 
 	cleanup_ctrs
 	cleanup_pods