Merge pull request #991 from Random-Liu/remove-container-lifecycle-image-dependency

Remove container lifecycle image dependency

integration/container_without_image_ref_test.go

@@ -0,0 +1,75 @@
+/*
+Copyright 2018 The containerd Authors.
+
+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 integration
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
+)
+
+// Test container lifecycle can work without image references.
+func TestContainerLifecycleWithoutImageRef(t *testing.T) {
+	t.Log("Create a sandbox")
+	sbConfig := PodSandboxConfig("sandbox", "container-lifecycle-without-image-ref")
+	sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, runtimeService.StopPodSandbox(sb))
+		assert.NoError(t, runtimeService.RemovePodSandbox(sb))
+	}()
+
+	const (
+		testImage     = "busybox"
+		containerName = "test-container"
+	)
+	t.Log("Pull test image")
+	img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
+	}()
+
+	t.Log("Create test container")
+	cnConfig := ContainerConfig(
+		containerName,
+		testImage,
+		WithCommand("sleep", "1000"),
+	)
+	cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
+	require.NoError(t, err)
+	require.NoError(t, runtimeService.StartContainer(cn))
+
+	t.Log("Remove test image")
+	assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
+
+	t.Log("Container status should be running")
+	status, err := runtimeService.ContainerStatus(cn)
+	require.NoError(t, err)
+	assert.Equal(t, status.GetState(), runtime.ContainerState_CONTAINER_RUNNING)
+
+	t.Logf("Stop container")
+	err = runtimeService.StopContainer(cn, 1)
+	assert.NoError(t, err)
+
+	t.Log("Container status should be exited")
+	status, err = runtimeService.ContainerStatus(cn)
+	require.NoError(t, err)
+	assert.Equal(t, status.GetState(), runtime.ContainerState_CONTAINER_EXITED)
+}

pkg/server/container_create.go

@@ -187,6 +187,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		opts = append(opts, customopts.WithVolumes(mountMap))
 	}
 	meta.ImageRef = image.ID
+	meta.StopSignal = image.ImageSpec.Config.StopSignal
 
 	// Get container log path.
 	if config.GetLogPath() != "" {

pkg/server/container_status.go

@@ -24,6 +24,7 @@ import (
 	"golang.org/x/net/context"
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
 
+	"github.com/containerd/cri/pkg/store"
 	containerstore "github.com/containerd/cri/pkg/store/container"
 )
 
@@ -43,17 +44,20 @@ func (c *criService) ContainerStatus(ctx context.Context, r *runtime.ContainerSt
 	imageRef := container.ImageRef
 	image, err := c.imageStore.Get(imageRef)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to get image %q", imageRef)
-	}
-	repoTags, repoDigests := parseImageReferences(image.References)
-	if len(repoTags) > 0 {
-		// Based on current behavior of dockershim, this field should be
-		// image tag.
-		spec = &runtime.ImageSpec{Image: repoTags[0]}
-	}
-	if len(repoDigests) > 0 {
-		// Based on the CRI definition, this field will be consumed by user.
-		imageRef = repoDigests[0]
+		if err != store.ErrNotExist {
+			return nil, errors.Wrapf(err, "failed to get image %q", imageRef)
+		}
+	} else {
+		repoTags, repoDigests := parseImageReferences(image.References)
+		if len(repoTags) > 0 {
+			// Based on current behavior of dockershim, this field should be
+			// image tag.
+			spec = &runtime.ImageSpec{Image: repoTags[0]}
+		}
+		if len(repoDigests) > 0 {
+			// Based on the CRI definition, this field will be consumed by user.
+			imageRef = repoDigests[0]
+		}
 	}
 	status := toCRIContainerStatus(container, spec, imageRef)
 	info, err := toCRIContainerInfo(ctx, container, r.GetVerbose())

pkg/server/container_stop.go

@@ -27,6 +27,7 @@ import (
 	"golang.org/x/sys/unix"
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
 
+	"github.com/containerd/cri/pkg/store"
 	containerstore "github.com/containerd/cri/pkg/store/container"
 )
 
@@ -76,24 +77,36 @@ func (c *criService) stopContainer(ctx context.Context, container containerstore
 	// We only need to kill the task. The event handler will Delete the
 	// task from containerd after it handles the Exited event.
 	if timeout > 0 {
-		stopSignal := unix.SIGTERM
-		image, err := c.imageStore.Get(container.ImageRef)
-		if err != nil {
-			// NOTE(random-liu): It's possible that the container is stopped,
-			// deleted and image is garbage collected before this point. However,
-			// the chance is really slim, even it happens, it's still fine to return
-			// an error here.
-			return errors.Wrapf(err, "failed to get image metadata %q", container.ImageRef)
-		}
-		if image.ImageSpec.Config.StopSignal != "" {
-			stopSignal, err = signal.ParseSignal(image.ImageSpec.Config.StopSignal)
+		stopSignal := "SIGTERM"
+		if container.StopSignal != "" {
+			stopSignal = container.StopSignal
+		} else {
+			// The image may have been deleted, and the `StopSignal` field is
+			// just introduced to handle that.
+			// However, for containers created before the `StopSignal` field is
+			// introduced, still try to get the stop signal from the image config.
+			// If the image has been deleted, logging an error and using the
+			// default SIGTERM is still better than returning error and leaving
+			// the container unstoppable. (See issue #990)
+			// TODO(random-liu): Remove this logic when containerd 1.2 is deprecated.
+			image, err := c.imageStore.Get(container.ImageRef)
 			if err != nil {
-				return errors.Wrapf(err, "failed to parse stop signal %q",
-					image.ImageSpec.Config.StopSignal)
+				if err != store.ErrNotExist {
+					return errors.Wrapf(err, "failed to get image %q", container.ImageRef)
+				}
+				logrus.Warningf("Image %q not found, stop container with signal %q", container.ImageRef, stopSignal)
+			} else {
+				if image.ImageSpec.Config.StopSignal != "" {
+					stopSignal = image.ImageSpec.Config.StopSignal
+				}
 			}
 		}
-		logrus.Infof("Stop container %q with signal %v", id, stopSignal)
-		if err = task.Kill(ctx, stopSignal); err != nil && !errdefs.IsNotFound(err) {
+		sig, err := signal.ParseSignal(stopSignal)
+		if err != nil {
+			return errors.Wrapf(err, "failed to parse stop signal %q", stopSignal)
+		}
+		logrus.Infof("Stop container %q with signal %v", id, sig)
+		if err = task.Kill(ctx, sig); err != nil && !errdefs.IsNotFound(err) {
 			return errors.Wrapf(err, "failed to stop container %q", id)
 		}
 

pkg/store/container/container_test.go

@@ -39,8 +39,9 @@ func TestContainerStore(t *testing.T) {
 					Attempt: 1,
 				},
 			},
-			ImageRef: "TestImage-1",
-			LogPath:  "/test/log/path/1",
+			ImageRef:   "TestImage-1",
+			StopSignal: "SIGTERM",
+			LogPath:    "/test/log/path/1",
 		},
 		"2abcd": {
 			ID:        "2abcd",
@@ -52,8 +53,9 @@ func TestContainerStore(t *testing.T) {
 					Attempt: 2,
 				},
 			},
-			ImageRef: "TestImage-2",
-			LogPath:  "/test/log/path/2",
+			StopSignal: "SIGTERM",
+			ImageRef:   "TestImage-2",
+			LogPath:    "/test/log/path/2",
 		},
 		"4a333": {
 			ID:        "4a333",
@@ -65,8 +67,9 @@ func TestContainerStore(t *testing.T) {
 					Attempt: 3,
 				},
 			},
-			ImageRef: "TestImage-3",
-			LogPath:  "/test/log/path/3",
+			StopSignal: "SIGTERM",
+			ImageRef:   "TestImage-3",
+			LogPath:    "/test/log/path/3",
 		},
 		"4abcd": {
 			ID:        "4abcd",
@@ -78,7 +81,8 @@ func TestContainerStore(t *testing.T) {
 					Attempt: 1,
 				},
 			},
-			ImageRef: "TestImage-4abcd",
+			StopSignal: "SIGTERM",
+			ImageRef:   "TestImage-4abcd",
 		},
 	}
 	statuses := map[string]Status{
@@ -182,8 +186,9 @@ func TestWithContainerIO(t *testing.T) {
 				Attempt: 1,
 			},
 		},
-		ImageRef: "TestImage-1",
-		LogPath:  "/test/log/path",
+		ImageRef:   "TestImage-1",
+		StopSignal: "SIGTERM",
+		LogPath:    "/test/log/path",
 	}
 	status := Status{
 		Pid:        1,

pkg/store/container/metadata.go

@@ -27,7 +27,7 @@ import (
 // 1) Metadata is immutable after created.
 // 2) Metadata is checkpointed as containerd container label.
 
-// metadataVersion  is current version of container metadata.
+// metadataVersion is current version of container metadata.
 const metadataVersion = "v1" // nolint
 
 // versionedMetadata is the internal versioned container metadata.
@@ -58,6 +58,9 @@ type Metadata struct {
 	ImageRef string
 	// LogPath is the container log path.
 	LogPath string
+	// StopSignal is the system call signal that will be sent to the container to exit.
+	// TODO(random-liu): Add integration test for stop signal.
+	StopSignal string
 }
 
 // MarshalJSON encodes Metadata into bytes in json format.