diff --git a/server/container_create.go b/server/container_create.go index 2e2face9..53a2a34c 100644 --- a/server/container_create.go +++ b/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 diff --git a/server/container_list.go b/server/container_list.go index 2d1577ea..5d76af55 100644 --- a/server/container_list.go +++ b/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 filter.PodSandboxId != "" { - if c.Sandbox() == filter.PodSandboxId { - ctrList = []*oci.Container{c} - } else { - ctrList = []*oci.Container{} + if !state.IsCtrNotExist(err) { + return nil, err } + 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() diff --git a/server/sandbox/sandbox.go b/server/sandbox/sandbox.go index 7608dcc8..a9f37469 100644 --- a/server/sandbox/sandbox.go +++ b/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 } diff --git a/server/sandbox_list.go b/server/sandbox_list.go index 8e70b018..c01ac808 100644 --- a/server/sandbox_list.go +++ b/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} diff --git a/server/sandbox_remove.go b/server/sandbox_remove.go index 696f00f0..58f5ef9d 100644 --- a/server/sandbox_remove.go +++ b/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 } } diff --git a/server/server.go b/server/server.go index 2ed640b4..c97a248c 100644 --- a/server/server.go +++ b/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 diff --git a/server/state/error.go b/server/state/error.go new file mode 100644 index 00000000..ffbf04f1 --- /dev/null +++ b/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 + } +} diff --git a/server/state/in_memory_state.go b/server/state/in_memory_state.go index 1b301528..2259673d 100644 --- a/server/state/in_memory_state.go +++ b/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() { - if err != nil { - for _, ctr := range addedCtrs { - if err2 := s.deleteContainerMappings(ctr, true); err2 != nil { - logrus.Errorf("Error removing container %s mappings: %v", ctr.ID(), err2) + + defer func(c *oci.Container) { + if err != nil { + if err2 := s.deleteContainerMappings(c, true); err2 != nil { + logrus.Errorf("Error removing container %s mappings: %v", c.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() { - if err != nil { - for _, ctr := range removedCtrs { - if err2 := s.addContainerMappings(ctr, true); err2 != nil { - logrus.Errorf("Error re-adding mappings for container %s: %v", ctr.ID(), err2) + + defer func(c *oci.Container) { + if err != nil { + if err2 := s.addContainerMappings(c, true); err2 != nil { + logrus.Errorf("Error re-adding mappings for container %s: %v", c.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) -} diff --git a/test/ctr.bats b/test/ctr.bats index 7a437653..02203a56 100644 --- a/test/ctr.bats +++ b/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 } diff --git a/test/restore.bats b/test/restore.bats index 264096ed..d3ef5a41 100644 --- a/test/restore.bats +++ b/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