From a0916b1044345f95229c3779f7aa233d6d31949f Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sat, 20 May 2017 15:14:51 +0200 Subject: [PATCH 1/4] server: sandbox_stop: ignore not found sandboxes This patch matches dockershim behavior Signed-off-by: Antonio Murdaca --- server/sandbox_stop.go | 12 +++++++++++- test/pod.bats | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/server/sandbox_stop.go b/server/sandbox_stop.go index d18cf51f..fb998b1d 100644 --- a/server/sandbox_stop.go +++ b/server/sandbox_stop.go @@ -16,7 +16,17 @@ func (s *Server) StopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque logrus.Debugf("StopPodSandboxRequest %+v", req) sb, err := s.getPodSandboxFromRequest(req.PodSandboxId) if err != nil { - return nil, err + if err == errSandboxIDEmpty { + return nil, err + } + + // If the sandbox isn't found we just return an empty response to adhere + // the the CRI interface which expects to not error out in not found + // cases. + + resp := &pb.StopPodSandboxResponse{} + logrus.Warnf("could not get sandbox %s, it's probably been stopped already: %v", req.PodSandboxId, err) + return resp, nil } podInfraContainer := sb.infraContainer diff --git a/test/pod.bats b/test/pod.bats index 4a4ce3f4..0ab7fb77 100644 --- a/test/pod.bats +++ b/test/pod.bats @@ -56,6 +56,29 @@ function teardown() { stop_crio } +@test "pod stop ignores not found sandboxes" { + start_crio + + run crioctl pod run --config "$TESTDATA"/sandbox_config.json + echo "$output" + [ "$status" -eq 0 ] + pod_id="$output" + run crioctl pod stop --id "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + run crioctl pod remove --id "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + + run crioctl pod stop --id "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + + cleanup_ctrs + cleanup_pods + stop_crio +} + @test "pod list filtering" { start_crio run crioctl pod run --config "$TESTDATA"/sandbox_config.json -name pod1 --label "a=b" --label "c=d" --label "e=f" From 22d055869dca053303a205ac0d94529c7714f9da Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sat, 20 May 2017 15:16:58 +0200 Subject: [PATCH 2/4] server: container_status: return image name if available If we create a container using the image ID like 771cd5947d5ea4bf8e8f4900dd357dbb67e7b16486c270f8274087d182d457c6, then a call to container_status will return that same ID for the "Image" field in ContainerStatusResponse. This patch matches dockershim behavior and return the first tagged name if available from the image store. This is also needed to fix a failure in k8s e2d tests. Reference: https://github.com/kubernetes/kubernetes/pull/39298/files#diff-c7dd39479fd733354254e70845075db5R369 Reference: https://github.com/kubernetes/kubernetes/blob/67a5bf8454bb839ee80a4245e9e09fc8f700fa84/test/e2e/framework/util.go#L1941 Signed-off-by: Antonio Murdaca --- cmd/crioctl/container.go | 3 +++ server/container_status.go | 21 ++++++++++++++++++--- test/image.bats | 25 +++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/cmd/crioctl/container.go b/cmd/crioctl/container.go index 693804d0..20ba367b 100644 --- a/cmd/crioctl/container.go +++ b/cmd/crioctl/container.go @@ -460,6 +460,9 @@ func ContainerStatus(client pb.RuntimeServiceClient, ID string) error { ftm := time.Unix(0, r.Status.FinishedAt) fmt.Printf("Finished: %v\n", ftm) fmt.Printf("Exit Code: %v\n", r.Status.ExitCode) + if r.Status.Image != nil { + fmt.Printf("Image: %v\n", r.Status.Image.Image) + } return nil } diff --git a/server/container_status.go b/server/container_status.go index febe7060..5e7b2b22 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -2,8 +2,10 @@ package server import ( "encoding/json" + "fmt" "github.com/Sirupsen/logrus" + "github.com/docker/distribution/reference" "github.com/kubernetes-incubator/cri-o/oci" rspec "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/net/context" @@ -24,14 +26,12 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq s.containerStateToDisk(c) containerID := c.ID() - image := c.Image() resp := &pb.ContainerStatusResponse{ Status: &pb.ContainerStatus{ Id: containerID, Metadata: c.Metadata(), Labels: c.Labels(), Annotations: c.Annotations(), - Image: image, }, } @@ -41,13 +41,28 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq } resp.Status.Mounts = mounts - status, err := s.storageImageServer.ImageStatus(s.imageContext, image.Image) + imageName := c.Image().Image + status, err := s.storageImageServer.ImageStatus(s.imageContext, imageName) if err != nil { return nil, err } + // TODO: use status.ID only if no digested names!!! + // need to modify ImageStatus to split tagged and digested! resp.Status.ImageRef = status.ID + 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/test/image.bats b/test/image.bats index ebad58fc..3dc50416 100644 --- a/test/image.bats +++ b/test/image.bats @@ -23,6 +23,31 @@ function teardown() { stop_crio } +@test "container status return image:tag if created by image ID" { + start_crio + + run crioctl pod run --config "$TESTDATA"/sandbox_config.json + echo "$output" + [ "$status" -eq 0 ] + pod_id="$output" + + sed -e "s/%VALUE%/$REDIS_IMAGEID/g" "$TESTDATA"/container_config_by_imageid.json > "$TESTDIR"/ctr_by_imageid.json + + run crioctl ctr create --config "$TESTDIR"/ctr_by_imageid.json --pod "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + ctr_id="$output" + + run crioctl ctr status --id "$ctr_id" + echo "$output" + [ "$status" -eq 0 ] + [[ "$output" =~ "Image: redis:alpine" ]] + + cleanup_ctrs + cleanup_pods + stop_crio +} + @test "image pull" { start_crio "" "" --no-pause-image run crioctl image pull "$IMAGE" From d099e3a98846264059d6371846a3e4b7febfaa44 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sat, 20 May 2017 17:08:00 +0200 Subject: [PATCH 3/4] server: container_status: we should return digested references in imageRef currently blocked on https://github.com/kubernetes-incubator/cri-o/issues/531 Signed-off-by: Antonio Murdaca --- cmd/crioctl/container.go | 4 ++++ server/container_status.go | 18 +++++++++++++++--- server/image_status.go | 1 + test/helpers.bash | 20 +++++++++++++++++++- test/image.bats | 27 +++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 4 deletions(-) diff --git a/cmd/crioctl/container.go b/cmd/crioctl/container.go index 20ba367b..7788d547 100644 --- a/cmd/crioctl/container.go +++ b/cmd/crioctl/container.go @@ -463,6 +463,10 @@ func ContainerStatus(client pb.RuntimeServiceClient, ID string) error { if r.Status.Image != nil { fmt.Printf("Image: %v\n", r.Status.Image.Image) } + // + // TODO: https://github.com/kubernetes-incubator/cri-o/issues/531 + // + //fmt.Printf("ImageRef: %v\n", r.Status.ImageRef) return nil } diff --git a/server/container_status.go b/server/container_status.go index 5e7b2b22..d2a500d3 100644 --- a/server/container_status.go +++ b/server/container_status.go @@ -47,9 +47,21 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq return nil, err } - // TODO: use status.ID only if no digested names!!! - // need to modify ImageStatus to split tagged and digested! - resp.Status.ImageRef = status.ID + 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) diff --git a/server/image_status.go b/server/image_status.go index d2cf9d4f..0841c702 100644 --- a/server/image_status.go +++ b/server/image_status.go @@ -32,6 +32,7 @@ func (s *Server) ImageStatus(ctx context.Context, req *pb.ImageStatusRequest) (* Id: status.ID, RepoTags: status.Names, Size_: *status.Size, + // TODO: https://github.com/kubernetes-incubator/cri-o/issues/531 }, } logrus.Debugf("ImageStatusResponse: %+v", resp) diff --git a/test/helpers.bash b/test/helpers.bash index 873c2f67..7658357a 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -147,7 +147,7 @@ function start_crio() { if ! [ "$3" = "--no-pause-image" ] ; then "$BIN2IMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --source-binary "$PAUSE_BINARY" fi - "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --add-name=docker://docker.io/library/redis:alpine --signature-policy="$INTEGRATION_ROOT"/policy.json + "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --add-name=docker.io/library/redis:alpine --signature-policy="$INTEGRATION_ROOT"/policy.json "$CRIO_BINARY" --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --signature-policy "$INTEGRATION_ROOT"/policy.json --config /dev/null config >$CRIO_CONFIG # Prepare the CNI configuration files, we're running with non host networking by default @@ -170,6 +170,24 @@ function start_crio() { if [ "$status" -ne 0 ] ; then crioctl image pull busybox:latest fi + # + # + # + # TODO: remove the code below for redis digested image id when + # https://github.com/kubernetes-incubator/cri-o/issues/531 is complete + # as the digested reference will be auto-stored when pulling the tag + # above + # + # + # + REDIS_IMAGEID_DIGESTED="redis@sha256:03789f402b2ecfb98184bf128d180f398f81c63364948ff1454583b02442f73b" + run crioctl image status --id $REDIS_IMAGEID_DIGESTED + if [ "$status" -ne 0 ]; then + crioctl image pull $REDIS_IMAGEID_DIGESTED + fi + # + # + # BUSYBOX_IMAGEID=$(crioctl image status --id=busybox | head -1 | sed -e "s/ID: //g") } diff --git a/test/image.bats b/test/image.bats index 3dc50416..cb2bc155 100644 --- a/test/image.bats +++ b/test/image.bats @@ -48,6 +48,33 @@ function teardown() { stop_crio } +@test "container status return image@digest if created by image ID and digest available" { + skip "depends on https://github.com/kubernetes-incubator/cri-o/issues/531" + + start_crio + + run crioctl pod run --config "$TESTDATA"/sandbox_config.json + echo "$output" + [ "$status" -eq 0 ] + pod_id="$output" + + sed -e "s/%VALUE%/$REDIS_IMAGEID_DIGESTED/g" "$TESTDATA"/container_config_by_imageid.json > "$TESTDIR"/ctr_by_imageid.json + + run crioctl ctr create --config "$TESTDIR"/ctr_by_imageid.json --pod "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + ctr_id="$output" + + run crioctl ctr status --id "$ctr_id" + echo "$output" + [ "$status" -eq 0 ] + [[ "$output" =~ "ImageRef: redis@sha256:03789f402b2ecfb98184bf128d180f398f81c63364948ff1454583b02442f73b" ]] + + cleanup_ctrs + cleanup_pods + stop_crio +} + @test "image pull" { start_crio "" "" --no-pause-image run crioctl image pull "$IMAGE" From 5f53416611811f0e0e65239f45abe753c832b05d Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Mon, 22 May 2017 16:38:10 +0200 Subject: [PATCH 4/4] server: sandbox_remove: add comment on sandbox not found empty response Signed-off-by: Antonio Murdaca --- server/sandbox_remove.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/sandbox_remove.go b/server/sandbox_remove.go index b4a5b66e..6f23b9ed 100644 --- a/server/sandbox_remove.go +++ b/server/sandbox_remove.go @@ -21,6 +21,10 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR return nil, err } + // If the sandbox isn't found we just return an empty response to adhere + // the the CRI interface which expects to not error out in not found + // cases. + resp := &pb.RemovePodSandboxResponse{} logrus.Warnf("could not get sandbox %s, it's probably been removed already: %v", req.PodSandboxId, err) return resp, nil