diff --git a/integration/container_without_image_ref_test.go b/integration/container_without_image_ref_test.go new file mode 100644 index 000000000..ec7cd4e34 --- /dev/null +++ b/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) +} diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index e29cb40f8..782626878 100644 --- a/pkg/server/container_create.go +++ b/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() != "" { diff --git a/pkg/server/container_status.go b/pkg/server/container_status.go index a02454690..e4200acab 100644 --- a/pkg/server/container_status.go +++ b/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()) diff --git a/pkg/server/container_stop.go b/pkg/server/container_stop.go index 198c9de2c..ddee397ae 100644 --- a/pkg/server/container_stop.go +++ b/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) } diff --git a/pkg/store/container/container_test.go b/pkg/store/container/container_test.go index 3ea52804f..44cff4f08 100644 --- a/pkg/store/container/container_test.go +++ b/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, diff --git a/pkg/store/container/metadata.go b/pkg/store/container/metadata.go index cdfa883db..6dc7d5233 100644 --- a/pkg/store/container/metadata.go +++ b/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.