From 1cf5f8ee3bf24caf68404e6f53d6eac00016f6e1 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 12:28:06 -0700 Subject: [PATCH 01/12] container: Don't call OCI runtime status We get notified of container exits by inotify so we already have updated status of the container in memory state. Signed-off-by: Mrunal Patel --- server/container_status.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/server/container_status.go b/server/container_status.go index a59d9630..1e806812 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -26,11 +26,6 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq return nil, err } - if err = s.Runtime().UpdateStatus(c); err != nil { - return nil, err - } - s.ContainerStateToDisk(c) - containerID := c.ID() resp := &pb.ContainerStatusResponse{ Status: &pb.ContainerStatus{ From bfcebcdb003883a2895b52d9e955a306732da2bb Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 12:29:53 -0700 Subject: [PATCH 02/12] Store imageName and imageRef for containers We calculate these values at container creation time and store them in the container object as they are requested during container status. This avoids re-calculation and speeds up container status. Signed-off-by: Mrunal Patel --- libkpod/container_server.go | 14 ++++++++++-- oci/container.go | 16 +++++++++++++- pkg/annotations/annotations.go | 6 ++++++ server/container_create.go | 38 ++++++++++++++++++++++++++++++++- server/container_status.go | 39 +++------------------------------- server/sandbox_run.go | 2 +- 6 files changed, 74 insertions(+), 41 deletions(-) diff --git a/libkpod/container_server.go b/libkpod/container_server.go index b4c333c3..dde2b5ba 100644 --- a/libkpod/container_server.go +++ b/libkpod/container_server.go @@ -350,7 +350,7 @@ func (c *ContainerServer) LoadSandbox(id string) error { return err } - scontainer, err := oci.NewContainer(m.Annotations[annotations.ContainerID], cname, sandboxPath, m.Annotations[annotations.LogPath], sb.NetNs(), labels, kubeAnnotations, "", nil, id, false, false, false, privileged, trusted, sandboxDir, created, m.Annotations["org.opencontainers.image.stopSignal"]) + scontainer, err := oci.NewContainer(m.Annotations[annotations.ContainerID], cname, sandboxPath, m.Annotations[annotations.LogPath], sb.NetNs(), labels, kubeAnnotations, "", "", "", nil, id, false, false, false, privileged, trusted, sandboxDir, created, m.Annotations["org.opencontainers.image.stopSignal"]) if err != nil { return err } @@ -440,6 +440,16 @@ func (c *ContainerServer) LoadContainer(id string) error { img = "" } + imgName, ok := m.Annotations[annotations.ImageName] + if !ok { + imgName = "" + } + + imgRef, ok := m.Annotations[annotations.ImageRef] + if !ok { + imgRef = "" + } + kubeAnnotations := make(map[string]string) if err = json.Unmarshal([]byte(m.Annotations[annotations.Annotations]), &kubeAnnotations); err != nil { return err @@ -450,7 +460,7 @@ func (c *ContainerServer) LoadContainer(id string) error { return err } - ctr, err := oci.NewContainer(id, name, containerPath, m.Annotations[annotations.LogPath], sb.NetNs(), labels, kubeAnnotations, img, &metadata, sb.ID(), tty, stdin, stdinOnce, sb.Privileged(), sb.Trusted(), containerDir, created, m.Annotations["org.opencontainers.image.stopSignal"]) + ctr, err := oci.NewContainer(id, name, containerPath, m.Annotations[annotations.LogPath], sb.NetNs(), labels, kubeAnnotations, img, imgName, imgRef, &metadata, sb.ID(), tty, stdin, stdinOnce, sb.Privileged(), sb.Trusted(), containerDir, created, m.Annotations["org.opencontainers.image.stopSignal"]) if err != nil { return err } diff --git a/oci/container.go b/oci/container.go index 7907856f..dddc7987 100644 --- a/oci/container.go +++ b/oci/container.go @@ -43,6 +43,8 @@ type Container struct { // this is the /var/lib/storage/... directory dir string stopSignal string + imageName string + imageRef string } // ContainerState represents the status of a container. @@ -57,7 +59,7 @@ type ContainerState struct { } // NewContainer creates a container object. -func NewContainer(id string, name string, bundlePath string, logPath string, netns ns.NetNS, labels map[string]string, annotations map[string]string, image string, metadata *pb.ContainerMetadata, sandbox string, terminal bool, stdin bool, stdinOnce bool, privileged bool, trusted bool, dir string, created time.Time, stopSignal string) (*Container, error) { +func NewContainer(id string, name string, bundlePath string, logPath string, netns ns.NetNS, labels map[string]string, annotations map[string]string, image string, imageName string, imageRef string, metadata *pb.ContainerMetadata, sandbox string, terminal bool, stdin bool, stdinOnce bool, privileged bool, trusted bool, dir string, created time.Time, stopSignal string) (*Container, error) { state := &ContainerState{} state.Created = created c := &Container{ @@ -76,6 +78,8 @@ func NewContainer(id string, name string, bundlePath string, logPath string, net metadata: metadata, annotations: annotations, image: image, + imageName: imageName, + imageRef: imageRef, dir: dir, state: state, stopSignal: stopSignal, @@ -155,6 +159,16 @@ func (c *Container) Image() string { return c.image } +// ImageName returns the image name of the container. +func (c *Container) ImageName() string { + return c.imageName +} + +// ImageRef returns the image ref of the container. +func (c *Container) ImageRef() string { + return c.imageRef +} + // Sandbox returns the sandbox name of the container. func (c *Container) Sandbox() string { return c.sandbox diff --git a/pkg/annotations/annotations.go b/pkg/annotations/annotations.go index 26ead571..0cdba43c 100644 --- a/pkg/annotations/annotations.go +++ b/pkg/annotations/annotations.go @@ -22,6 +22,12 @@ const ( // Image is the container image ID annotation Image = "io.kubernetes.cri-o.Image" + // ImageName is the container image name annotation + ImageName = "io.kubernetes.cri-o.ImageName" + + // ImageRef is the container image ref annotation + ImageRef = "io.kubernetes.cri-o.ImageRef" + // KubeName is the kubernetes name annotation KubeName = "io.kubernetes.cri-o.KubeName" diff --git a/server/container_create.go b/server/container_create.go index d85e9e64..3c17be6a 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/docker/distribution/reference" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/pkg/symlink" "github.com/kubernetes-incubator/cri-o/libkpod" @@ -565,6 +566,41 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string, } image = images[0] + // Get imageName and imageRef that are requested in container status + imageName := image + status, err := s.StorageImageServer().ImageStatus(s.ImageContext(), image) + if err != nil { + return nil, err + } + + imageRef := status.ID + // + // TODO: https://github.com/kubernetes-incubator/cri-o/issues/531 + // + //for _, n := range status.Names { + //r, err := reference.ParseNormalizedNamed(n) + //if err != nil { + //return nil, fmt.Errorf("failed to normalize image name for ImageRef: %v", err) + //} + //if digested, isDigested := r.(reference.Canonical); isDigested { + //imageRef = reference.FamiliarString(digested) + //break + //} + //} + for _, n := range status.Names { + r, err := reference.ParseNormalizedNamed(n) + if err != nil { + return nil, fmt.Errorf("failed to normalize image name for Image: %v", err) + } + if tagged, isTagged := r.(reference.Tagged); isTagged { + imageName = reference.FamiliarString(tagged) + break + } + } + + specgen.AddAnnotation(annotations.ImageName, imageName) + specgen.AddAnnotation(annotations.ImageRef, imageRef) + // bind mount the pod shm specgen.AddBindMount(sb.ShmPath(), "/dev/shm", []string{"rw"}) @@ -727,7 +763,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string, return nil, err } - container, err := oci.NewContainer(containerID, containerName, containerInfo.RunDir, logPath, sb.NetNs(), labels, kubeAnnotations, image, metadata, sb.ID(), containerConfig.Tty, containerConfig.Stdin, containerConfig.StdinOnce, sb.Privileged(), sb.Trusted(), containerInfo.Dir, created, containerImageConfig.Config.StopSignal) + container, err := oci.NewContainer(containerID, containerName, containerInfo.RunDir, logPath, sb.NetNs(), labels, kubeAnnotations, image, imageName, imageRef, metadata, sb.ID(), containerConfig.Tty, containerConfig.Stdin, containerConfig.StdinOnce, sb.Privileged(), sb.Trusted(), containerInfo.Dir, created, containerImageConfig.Config.StopSignal) if err != nil { return nil, err } diff --git a/server/container_status.go b/server/container_status.go index 1e806812..9fb80022 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -2,9 +2,7 @@ package server import ( "encoding/json" - "fmt" - "github.com/docker/distribution/reference" "github.com/kubernetes-incubator/cri-o/oci" rspec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" @@ -33,49 +31,18 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq Metadata: c.Metadata(), Labels: c.Labels(), Annotations: c.Annotations(), + ImageRef: c.ImageRef(), }, } + resp.Status.Image = &pb.ImageSpec{Image: c.ImageName()} mounts, err := s.getMounts(containerID) if err != nil { return nil, err } + resp.Status.Mounts = mounts - imageName := c.Image() - status, err := s.StorageImageServer().ImageStatus(s.ImageContext(), imageName) - if err != nil { - return nil, err - } - - imageRef := status.ID - // - // TODO: https://github.com/kubernetes-incubator/cri-o/issues/531 - // - //for _, n := range status.Names { - //r, err := reference.ParseNormalizedNamed(n) - //if err != nil { - //return nil, fmt.Errorf("failed to normalize image name for ImageRef: %v", err) - //} - //if digested, isDigested := r.(reference.Canonical); isDigested { - //imageRef = reference.FamiliarString(digested) - //break - //} - //} - resp.Status.ImageRef = imageRef - - for _, n := range status.Names { - r, err := reference.ParseNormalizedNamed(n) - if err != nil { - return nil, fmt.Errorf("failed to normalize image name for Image: %v", err) - } - if tagged, isTagged := r.(reference.Tagged); isTagged { - imageName = reference.FamiliarString(tagged) - break - } - } - resp.Status.Image = &pb.ImageSpec{Image: imageName} - cState := s.Runtime().ContainerStatus(c) rStatus := pb.ContainerState_CONTAINER_UNKNOWN diff --git a/server/sandbox_run.go b/server/sandbox_run.go index 8ee1e054..a26b4bb1 100644 --- a/server/sandbox_run.go +++ b/server/sandbox_run.go @@ -461,7 +461,7 @@ func (s *Server) RunPodSandbox(ctx context.Context, req *pb.RunPodSandboxRequest return nil, fmt.Errorf("failed to write runtime configuration for pod sandbox %s(%s): %v", sb.Name(), id, err) } - container, err := oci.NewContainer(id, containerName, podContainer.RunDir, logPath, sb.NetNs(), labels, kubeAnnotations, "", nil, id, false, false, false, sb.Privileged(), sb.Trusted(), podContainer.Dir, created, podContainer.Config.Config.StopSignal) + container, err := oci.NewContainer(id, containerName, podContainer.RunDir, logPath, sb.NetNs(), labels, kubeAnnotations, "", "", "", nil, id, false, false, false, sb.Privileged(), sb.Trusted(), podContainer.Dir, created, podContainer.Config.Config.StopSignal) if err != nil { return nil, err } From 5ab6ec304656e539f97a8f1ddab9ab2b0076fb4a Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 12:43:56 -0700 Subject: [PATCH 03/12] oci: Add volumes field to Container We add a ContainerVolume struct and store a list of volumes in the Container object for quick retrieval. Signed-off-by: Mrunal Patel --- oci/container.go | 19 +++++++++++++++++++ pkg/annotations/annotations.go | 3 +++ 2 files changed, 22 insertions(+) diff --git a/oci/container.go b/oci/container.go index dddc7987..8eaf292b 100644 --- a/oci/container.go +++ b/oci/container.go @@ -45,6 +45,14 @@ type Container struct { stopSignal string imageName string imageRef string + volumes []ContainerVolume +} + +// ContainerVolume is a bind mount for the container. +type ContainerVolume struct { + ContainerPath string `json:"container_path"` + HostPath string `json:"host_path"` + Readonly bool `json:"readonly"` } // ContainerState represents the status of a container. @@ -198,3 +206,14 @@ func (c *Container) State() *ContainerState { defer c.opLock.Unlock() return c.state } + +// AddVolume adds a volume to list of container volumes. +func (c *Container) AddVolume(v ContainerVolume) { + c.volumes = append(c.volumes, v) +} + +// Volumes returns the list of container volumes. +func (c *Container) Volumes() []ContainerVolume { + return c.volumes + +} diff --git a/pkg/annotations/annotations.go b/pkg/annotations/annotations.go index 0cdba43c..0b8a5ea1 100644 --- a/pkg/annotations/annotations.go +++ b/pkg/annotations/annotations.go @@ -69,6 +69,9 @@ const ( // StdinOnce is the stdin_once annotation StdinOnce = "io.kubernetes.cri-o.StdinOnce" + + // Volumes is the volumes annotatoin + Volumes = "io.kubernetes.cri-o.Volumes" ) // ContainerType values From fa317b41fdcc257b242575c54d248f0574975b62 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 12:52:25 -0700 Subject: [PATCH 04/12] Add volumes to container object at container create time Signed-off-by: Mrunal Patel --- libkpod/container_server.go | 12 ++++++++++++ server/container_create.go | 32 +++++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/libkpod/container_server.go b/libkpod/container_server.go index dde2b5ba..344ebf2c 100644 --- a/libkpod/container_server.go +++ b/libkpod/container_server.go @@ -355,6 +355,18 @@ func (c *ContainerServer) LoadSandbox(id string) error { return err } + if m.Annotations[annotations.Volumes] != "" { + containerVolumes := []oci.ContainerVolume{} + if err = json.Unmarshal([]byte(m.Annotations[annotations.Volumes]), &containerVolumes); err != nil { + return fmt.Errorf("failed to unmarshal container volumes: %v", err) + } + if containerVolumes != nil { + for _, cv := range containerVolumes { + scontainer.AddVolume(cv) + } + } + } + c.ContainerStateFromDisk(scontainer) if err = label.ReserveLabel(processLabel); err != nil { diff --git a/server/container_create.go b/server/container_create.go index 3c17be6a..2e23445b 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -40,22 +40,23 @@ const ( seccompLocalhostPrefix = "localhost/" ) -func addOCIBindMounts(sb *sandbox.Sandbox, containerConfig *pb.ContainerConfig, specgen *generate.Generator) error { +func addOCIBindMounts(sb *sandbox.Sandbox, containerConfig *pb.ContainerConfig, specgen *generate.Generator) ([]oci.ContainerVolume, error) { + volumes := []oci.ContainerVolume{} mounts := containerConfig.GetMounts() for _, mount := range mounts { dest := mount.ContainerPath if dest == "" { - return fmt.Errorf("Mount.ContainerPath is empty") + return nil, fmt.Errorf("Mount.ContainerPath is empty") } src := mount.HostPath if src == "" { - return fmt.Errorf("Mount.HostPath is empty") + return nil, fmt.Errorf("Mount.HostPath is empty") } if _, err := os.Stat(src); err != nil && os.IsNotExist(err) { if err1 := os.MkdirAll(src, 0644); err1 != nil { - return fmt.Errorf("Failed to mkdir %s: %s", src, err) + return nil, fmt.Errorf("Failed to mkdir %s: %s", src, err) } } @@ -68,14 +69,20 @@ func addOCIBindMounts(sb *sandbox.Sandbox, containerConfig *pb.ContainerConfig, if mount.SelinuxRelabel { // Need a way in kubernetes to determine if the volume is shared or private if err := label.Relabel(src, sb.MountLabel(), true); err != nil && err != unix.ENOTSUP { - return fmt.Errorf("relabel failed %s: %v", src, err) + return nil, fmt.Errorf("relabel failed %s: %v", src, err) } } + volumes = append(volumes, oci.ContainerVolume{ + ContainerPath: dest, + HostPath: src, + Readonly: mount.Readonly, + }) + specgen.AddBindMount(src, dest, options) } - return nil + return volumes, nil } func addImageVolumes(rootfs string, s *Server, containerInfo *storage.ContainerInfo, specgen *generate.Generator, mountLabel string) error { @@ -361,10 +368,17 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string, specgen.HostSpecific = true specgen.ClearProcessRlimits() - if err := addOCIBindMounts(sb, containerConfig, &specgen); err != nil { + containerVolumes, err := addOCIBindMounts(sb, containerConfig, &specgen) + if err != nil { return nil, err } + volumesJSON, err := json.Marshal(containerVolumes) + if err != nil { + return nil, err + } + specgen.AddAnnotation(annotations.Volumes, string(volumesJSON)) + // Add cgroup mount so container process can introspect its own limits specgen.AddCgroupsMount("ro") @@ -768,6 +782,10 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string, return nil, err } + for _, cv := range containerVolumes { + container.AddVolume(cv) + } + return container, nil } From 3f1b42ee9e81dd4be75957e7bb1aa0f7c961cf7b Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 12:58:38 -0700 Subject: [PATCH 05/12] Return container mounts in status from stored list Signed-off-by: Mrunal Patel --- server/container_status.go | 57 +++++--------------------------------- 1 file changed, 7 insertions(+), 50 deletions(-) diff --git a/server/container_status.go b/server/container_status.go index 9fb80022..1bfa9091 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -1,10 +1,7 @@ package server import ( - "encoding/json" - "github.com/kubernetes-incubator/cri-o/oci" - rspec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "golang.org/x/net/context" pb "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" @@ -36,11 +33,14 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq } resp.Status.Image = &pb.ImageSpec{Image: c.ImageName()} - mounts, err := s.getMounts(containerID) - if err != nil { - return nil, err + mounts := []*pb.Mount{} + for _, cv := range c.Volumes() { + mounts = append(mounts, &pb.Mount{ + ContainerPath: cv.ContainerPath, + HostPath: cv.HostPath, + Readonly: cv.Readonly, + }) } - resp.Status.Mounts = mounts cState := s.Runtime().ContainerStatus(c) @@ -82,46 +82,3 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq logrus.Debugf("ContainerStatusResponse: %+v", resp) return resp, nil } - -func (s *Server) getMounts(id string) ([]*pb.Mount, error) { - config, err := s.Store().FromContainerDirectory(id, "config.json") - if err != nil { - return nil, err - } - var m rspec.Spec - if err = json.Unmarshal(config, &m); err != nil { - return nil, err - } - isRO := func(m rspec.Mount) bool { - var ro bool - for _, o := range m.Options { - if o == "ro" { - ro = true - break - } - } - return ro - } - isBind := func(m rspec.Mount) bool { - var bind bool - for _, o := range m.Options { - if o == "bind" || o == "rbind" { - bind = true - break - } - } - return bind - } - mounts := []*pb.Mount{} - for _, b := range m.Mounts { - if !isBind(b) { - continue - } - mounts = append(mounts, &pb.Mount{ - ContainerPath: b.Destination, - HostPath: b.Source, - Readonly: isRO(b), - }) - } - return mounts, nil -} From ea4b6fa55d381ba81cb525010bba8aee6245862c Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 16:32:27 -0700 Subject: [PATCH 06/12] container: Reduce number of calls to UpdateStatus Signed-off-by: Mrunal Patel --- server/container_create.go | 4 ---- server/container_remove.go | 4 ---- server/container_stop.go | 3 --- 3 files changed, 11 deletions(-) diff --git a/server/container_create.go b/server/container_create.go index 2e23445b..a7e1f414 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -334,10 +334,6 @@ func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerReq return nil, err } - if err = s.Runtime().UpdateStatus(container); err != nil { - return nil, err - } - s.addContainer(container) if err = s.CtrIDIndex().Add(containerID); err != nil { diff --git a/server/container_remove.go b/server/container_remove.go index 08fe8e40..1464dade 100644 --- a/server/container_remove.go +++ b/server/container_remove.go @@ -20,10 +20,6 @@ func (s *Server) RemoveContainer(ctx context.Context, req *pb.RemoveContainerReq return nil, err } - if err := s.Runtime().UpdateStatus(c); err != nil { - return nil, fmt.Errorf("failed to update container state: %v", err) - } - cState := s.Runtime().ContainerStatus(c) if cState.Status == oci.ContainerStateCreated || cState.Status == oci.ContainerStateRunning { if err := s.Runtime().StopContainer(c, -1); err != nil { diff --git a/server/container_stop.go b/server/container_stop.go index cff22363..9a528f61 100644 --- a/server/container_stop.go +++ b/server/container_stop.go @@ -17,9 +17,6 @@ func (s *Server) StopContainer(ctx context.Context, req *pb.StopContainerRequest return nil, err } - if err := s.Runtime().UpdateStatus(c); err != nil { - return nil, err - } cStatus := s.Runtime().ContainerStatus(c) if cStatus.Status != oci.ContainerStateStopped { if err := s.Runtime().StopContainer(c, req.Timeout); err != nil { From ce17c5214dd81cc3b96703a154a5610eac56b7c0 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Mon, 14 Aug 2017 16:33:10 -0700 Subject: [PATCH 07/12] sandbox: Reduce number of calls to UpdateStatus Also, we distinguish between container and a pod infra container in the exit monitor as pod infra containers aren't stored in the main container index. Signed-off-by: Mrunal Patel --- server/sandbox_list.go | 4 ---- server/sandbox_remove.go | 4 ---- server/sandbox_run.go | 10 ---------- server/sandbox_status.go | 5 ----- server/sandbox_stop.go | 3 --- server/server.go | 15 ++++++++++++++- 6 files changed, 14 insertions(+), 27 deletions(-) diff --git a/server/sandbox_list.go b/server/sandbox_list.go index 3e0044cf..c70e6fd2 100644 --- a/server/sandbox_list.go +++ b/server/sandbox_list.go @@ -60,10 +60,6 @@ func (s *Server) ListPodSandbox(ctx context.Context, req *pb.ListPodSandboxReque // it's better not to panic continue } - if err := s.Runtime().UpdateStatus(podInfraContainer); err != nil { - return nil, err - } - cState := s.Runtime().ContainerStatus(podInfraContainer) created := cState.Created.UnixNano() rStatus := pb.PodSandboxState_SANDBOX_NOTREADY diff --git a/server/sandbox_remove.go b/server/sandbox_remove.go index e72e1b26..d94470c4 100644 --- a/server/sandbox_remove.go +++ b/server/sandbox_remove.go @@ -38,10 +38,6 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR // Delete all the containers in the sandbox for _, c := range containers { - if err := s.Runtime().UpdateStatus(c); err != nil { - return nil, fmt.Errorf("failed to update container state: %v", err) - } - cState := s.Runtime().ContainerStatus(c) if cState.Status == oci.ContainerStateCreated || cState.Status == oci.ContainerStateRunning { if err := s.Runtime().StopContainer(c, -1); err != nil { diff --git a/server/sandbox_run.go b/server/sandbox_run.go index a26b4bb1..f1159832 100644 --- a/server/sandbox_run.go +++ b/server/sandbox_run.go @@ -78,19 +78,9 @@ func (s *Server) runContainer(container *oci.Container, cgroupParent string) err if err := s.Runtime().CreateContainer(container, cgroupParent); err != nil { return err } - - if err := s.Runtime().UpdateStatus(container); err != nil { - return err - } - if err := s.Runtime().StartContainer(container); err != nil { return err } - - if err := s.Runtime().UpdateStatus(container); err != nil { - return err - } - return nil } diff --git a/server/sandbox_status.go b/server/sandbox_status.go index 17db0a98..be37862f 100644 --- a/server/sandbox_status.go +++ b/server/sandbox_status.go @@ -16,11 +16,6 @@ func (s *Server) PodSandboxStatus(ctx context.Context, req *pb.PodSandboxStatusR } podInfraContainer := sb.InfraContainer() - if err = s.Runtime().UpdateStatus(podInfraContainer); err != nil { - return nil, err - } - s.ContainerStateToDisk(podInfraContainer) - cState := s.Runtime().ContainerStatus(podInfraContainer) netNsPath, err := podInfraContainer.NetNsPath() diff --git a/server/sandbox_stop.go b/server/sandbox_stop.go index 7687272b..ecff1cfe 100644 --- a/server/sandbox_stop.go +++ b/server/sandbox_stop.go @@ -70,9 +70,6 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque containers = append(containers, podInfraContainer) for _, c := range containers { - if err := s.Runtime().UpdateStatus(c); err != nil { - return nil, err - } cStatus := s.Runtime().ContainerStatus(c) if cStatus.Status != oci.ContainerStateStopped { if err := s.Runtime().StopContainer(c, -1); err != nil { diff --git a/server/server.go b/server/server.go index 141eccbc..601eec89 100644 --- a/server/server.go +++ b/server/server.go @@ -313,15 +313,28 @@ func (s *Server) StartExitMonitor() { logrus.Debugf("event: %v", event) if event.Op&fsnotify.Create == fsnotify.Create { containerID := filepath.Base(event.Name) - logrus.Debugf("container exited: %v", containerID) + logrus.Debugf("container or sandbox exited: %v", containerID) c := s.GetContainer(containerID) if c != nil { + logrus.Debugf("container exited and found: %v", containerID) err := s.Runtime().UpdateStatus(c) if err != nil { logrus.Warnf("Failed to update container status %s: %v", c, err) } else { s.ContainerStateToDisk(c) } + } else { + sb := s.GetSandbox(containerID) + if sb != nil { + c := sb.InfraContainer() + logrus.Debugf("sandbox exited and found: %v", containerID) + err := s.Runtime().UpdateStatus(c) + if err != nil { + logrus.Warnf("Failed to update sandbox infra container status %s: %v", c, err) + } else { + s.ContainerStateToDisk(c) + } + } } } case err := <-watcher.Errors: From 8d58f227cd15869d687dc875ddcef10719cc7026 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Wed, 16 Aug 2017 15:42:20 -0700 Subject: [PATCH 08/12] sandbox_stop: Store stopped status This allows us to respond to kubelet quickly if the pod was already stopped successfully earlier. Signed-off-by: Mrunal Patel --- libkpod/sandbox/sandbox.go | 14 ++++++++++++++ server/sandbox_stop.go | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/libkpod/sandbox/sandbox.go b/libkpod/sandbox/sandbox.go index b19ff819..51097c2e 100644 --- a/libkpod/sandbox/sandbox.go +++ b/libkpod/sandbox/sandbox.go @@ -153,6 +153,7 @@ type Sandbox struct { resolvPath string hostname string portMappings []*hostport.PortMapping + stopped bool } const ( @@ -381,6 +382,19 @@ func (s *Sandbox) NetNsCreate() error { return nil } +// SetStopped sets the sandbox state to stopped. +// This should be set after a stop operation succeeds +// so that subsequent stops can return fast. +func (s *Sandbox) SetStopped() { + s.stopped = true +} + +// Stopped returns whether the sandbox state has been +// set to stopped. +func (s *Sandbox) Stopped() bool { + return s.stopped +} + // NetNsJoin attempts to join the sandbox to an existing network namespace // This will fail if the sandbox is already part of a network namespace func (s *Sandbox) NetNsJoin(nspath, name string) error { diff --git a/server/sandbox_stop.go b/server/sandbox_stop.go index ecff1cfe..2332c6cc 100644 --- a/server/sandbox_stop.go +++ b/server/sandbox_stop.go @@ -37,6 +37,10 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque return resp, nil } + if sb.Stopped() { + return &pb.StopPodSandboxResponse{}, nil + } + podInfraContainer := sb.InfraContainer() netnsPath, err := podInfraContainer.NetNsPath() if err != nil { @@ -110,6 +114,7 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque logrus.Warnf("failed to stop sandbox container in pod sandbox %s: %v", sb.ID(), err) } + sb.SetStopped() resp := &pb.StopPodSandboxResponse{} logrus.Debugf("StopPodSandboxResponse: %+v", resp) return resp, nil From cab08602572f6709d654d4cfb43ec01998bd01f0 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 17 Aug 2017 11:28:18 -0700 Subject: [PATCH 09/12] sandbox_remove: Don't stop containers if sandbox is stopped already Signed-off-by: Mrunal Patel --- server/sandbox_remove.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/server/sandbox_remove.go b/server/sandbox_remove.go index d94470c4..1d96f4fe 100644 --- a/server/sandbox_remove.go +++ b/server/sandbox_remove.go @@ -38,11 +38,13 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR // Delete all the containers in the sandbox for _, c := range containers { - cState := s.Runtime().ContainerStatus(c) - if cState.Status == oci.ContainerStateCreated || cState.Status == oci.ContainerStateRunning { - if err := s.Runtime().StopContainer(c, -1); err != nil { - // Assume container is already stopped - logrus.Warnf("failed to stop container %s: %v", c.Name(), err) + if !sb.Stopped() { + cState := s.Runtime().ContainerStatus(c) + if cState.Status == oci.ContainerStateCreated || cState.Status == oci.ContainerStateRunning { + if err := s.Runtime().StopContainer(c, -1); err != nil { + // Assume container is already stopped + logrus.Warnf("failed to stop container %s: %v", c.Name(), err) + } } } From 908b3fcbbc4539135c57cb5609db71f09c5d1c53 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 17 Aug 2017 14:27:29 -0700 Subject: [PATCH 10/12] Add container/sandbox id to response debugs Signed-off-by: Mrunal Patel --- server/container_stop.go | 2 +- server/sandbox_stop.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/server/container_stop.go b/server/container_stop.go index 9a528f61..d1b9a8e7 100644 --- a/server/container_stop.go +++ b/server/container_stop.go @@ -30,6 +30,6 @@ func (s *Server) StopContainer(ctx context.Context, req *pb.StopContainerRequest s.ContainerStateToDisk(c) resp := &pb.StopContainerResponse{} - logrus.Debugf("StopContainerResponse: %+v", resp) + logrus.Debugf("StopContainerResponse %s: %+v", c.ID(), resp) return resp, nil } diff --git a/server/sandbox_stop.go b/server/sandbox_stop.go index 2332c6cc..b4d7f02b 100644 --- a/server/sandbox_stop.go +++ b/server/sandbox_stop.go @@ -34,11 +34,14 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque resp := &pb.StopPodSandboxResponse{} logrus.Warnf("could not get sandbox %s, it's probably been stopped already: %v", req.PodSandboxId, err) + logrus.Debugf("StopPodSandboxResponse %s: %+v", req.PodSandboxId, resp) return resp, nil } if sb.Stopped() { - return &pb.StopPodSandboxResponse{}, nil + resp := &pb.StopPodSandboxResponse{} + logrus.Debugf("StopPodSandboxResponse %s: %+v", sb.ID(), resp) + return resp, nil } podInfraContainer := sb.InfraContainer() @@ -116,7 +119,7 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque sb.SetStopped() resp := &pb.StopPodSandboxResponse{} - logrus.Debugf("StopPodSandboxResponse: %+v", resp) + logrus.Debugf("StopPodSandboxResponse %s: %+v", sb.ID(), resp) return resp, nil } From 37edc50c1d91f2e96bc4abae23f2d3da75abbf1a Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 17 Aug 2017 16:43:41 -0700 Subject: [PATCH 11/12] oci: Check if process exists before trying to kill it Signed-off-by: Mrunal Patel --- oci/oci.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/oci/oci.go b/oci/oci.go index 2eb14f0c..3243e593 100644 --- a/oci/oci.go +++ b/oci/oci.go @@ -496,8 +496,16 @@ func (r *Runtime) ExecSync(c *Container, command []string, timeout int64) (resp func (r *Runtime) StopContainer(c *Container, timeout int64) error { c.opLock.Lock() defer c.opLock.Unlock() + + // Check if the process is around before sending a signal + err := unix.Kill(c.state.Pid, 0) + if err == unix.ESRCH { + c.state.Finished = time.Now() + return nil + } + if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, os.Stderr, r.Path(c), "kill", c.id, c.GetStopSignal()); err != nil { - return err + return fmt.Errorf("failed to stop container %s, %v", c.id, err) } if timeout == -1 { // default 10 seconds delay From 701e7ff63f2ee786c643a51479999835b04f1658 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 17 Aug 2017 21:13:39 -0700 Subject: [PATCH 12/12] container_status: Get latest container status if exit code is -1 Signed-off-by: Mrunal Patel --- server/container_status.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/server/container_status.go b/server/container_status.go index 1bfa9091..3a316435 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -46,6 +46,17 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq cState := s.Runtime().ContainerStatus(c) rStatus := pb.ContainerState_CONTAINER_UNKNOWN + // If we defaulted to exit code -1 earlier then we attempt to + // get the exit code from the exit file again. + // TODO: We could wait in UpdateStatus for exit file to show up. + if cState.ExitCode == -1 { + err := s.Runtime().UpdateStatus(c) + if err != nil { + logrus.Warnf("Failed to UpdateStatus of container %s: %v", c.ID(), err) + } + cState = s.Runtime().ContainerStatus(c) + } + switch cState.Status { case oci.ContainerStateCreated: rStatus = pb.ContainerState_CONTAINER_CREATED