Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit 5766ef2

Browse files
mikebrowmikebrow
authored
Merge pull request #995 from Random-Liu/cherrypick-#991-release-1.0
Cherrypick #991 release 1.0
2 parents a8b8525 + 0ac8363 commit 5766ef2

7 files changed

Lines changed: 137 additions & 36 deletions

File tree

‎integration/container_without_image_ref_test.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/*
2+
Copyright 2018 The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package integration
18+
19+
import (
20+
"testing"
21+
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
25+
)
26+
27+
// Test container lifecycle can work without image references.
28+
func TestContainerLifecycleWithoutImageRef(t *testing.T) {
29+
t.Log("Create a sandbox")
30+
sbConfig := PodSandboxConfig("sandbox", "container-lifecycle-without-image-ref")
31+
sb, err := runtimeService.RunPodSandbox(sbConfig)
32+
require.NoError(t, err)
33+
defer func() {
34+
assert.NoError(t, runtimeService.StopPodSandbox(sb))
35+
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
36+
}()
37+
38+
const (
39+
testImage = "busybox"
40+
containerName = "test-container"
41+
)
42+
t.Log("Pull test image")
43+
img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil)
44+
require.NoError(t, err)
45+
defer func() {
46+
assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
47+
}()
48+
49+
t.Log("Create test container")
50+
cnConfig := ContainerConfig(
51+
containerName,
52+
testImage,
53+
WithCommand("sleep", "1000"),
54+
)
55+
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
56+
require.NoError(t, err)
57+
require.NoError(t, runtimeService.StartContainer(cn))
58+
59+
t.Log("Remove test image")
60+
assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
61+
62+
t.Log("Container status should be running")
63+
status, err := runtimeService.ContainerStatus(cn)
64+
require.NoError(t, err)
65+
assert.Equal(t, status.GetState(), runtime.ContainerState_CONTAINER_RUNNING)
66+
67+
t.Logf("Stop container")
68+
err = runtimeService.StopContainer(cn, 1)
69+
assert.NoError(t, err)
70+
71+
t.Log("Container status should be exited")
72+
status, err = runtimeService.ContainerStatus(cn)
73+
require.NoError(t, err)
74+
assert.Equal(t, status.GetState(), runtime.ContainerState_CONTAINER_EXITED)
75+
}

‎integration/image_load_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestImageLoad(t *testing.T) {
3636
loadedImage := "docker.io/library/" + testImage
3737
_, err := exec.LookPath("docker")
3838
if err != nil {
39-
t.Skip("Docker is not available: %v", err)
39+
t.Skipf("Docker is not available: %v", err)
4040
}
4141
t.Logf("docker save image into tarball")
4242
output, err := exec.Command("docker", "pull", testImage).CombinedOutput()

‎pkg/server/container_create.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
197197
opts = append(opts, customopts.WithVolumes(mountMap))
198198
}
199199
meta.ImageRef = image.ID
200+
meta.StopSignal = image.ImageSpec.Config.StopSignal
200201

201202
// Get container log path.
202203
if config.GetLogPath() != "" {

‎pkg/server/container_status.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
2626

2727
criconfig "github.com/containerd/cri/pkg/config"
28+
"github.com/containerd/cri/pkg/store"
2829
containerstore "github.com/containerd/cri/pkg/store/container"
2930
)
3031

@@ -44,16 +45,19 @@ func (c *criService) ContainerStatus(ctx context.Context, r *runtime.ContainerSt
4445
imageRef := container.ImageRef
4546
image, err := c.imageStore.Get(imageRef)
4647
if err != nil {
47-
return nil, errors.Wrapf(err, "failed to get image %q", imageRef)
48-
}
49-
if len(image.RepoTags) > 0 {
50-
// Based on current behavior of dockershim, this field should be
51-
// image tag.
52-
spec = &runtime.ImageSpec{Image: image.RepoTags[0]}
53-
}
54-
if len(image.RepoDigests) > 0 {
55-
// Based on the CRI definition, this field will be consumed by user.
56-
imageRef = image.RepoDigests[0]
48+
if err != store.ErrNotExist {
49+
return nil, errors.Wrapf(err, "failed to get image %q", imageRef)
50+
}
51+
} else {
52+
if len(image.RepoTags) > 0 {
53+
// Based on current behavior of dockershim, this field should be
54+
// image tag.
55+
spec = &runtime.ImageSpec{Image: image.RepoTags[0]}
56+
}
57+
if len(image.RepoDigests) > 0 {
58+
// Based on the CRI definition, this field will be consumed by user.
59+
imageRef = image.RepoDigests[0]
60+
}
5761
}
5862
status := toCRIContainerStatus(container, spec, imageRef)
5963
info, err := toCRIContainerInfo(ctx, container, r.GetVerbose())

‎pkg/server/container_stop.go‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"golang.org/x/sys/unix"
2929
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
3030

31+
"github.com/containerd/cri/pkg/store"
3132
containerstore "github.com/containerd/cri/pkg/store/container"
3233
)
3334

@@ -77,24 +78,36 @@ func (c *criService) stopContainer(ctx context.Context, container containerstore
7778
// We only need to kill the task. The event handler will Delete the
7879
// task from containerd after it handles the Exited event.
7980
if timeout > 0 {
80-
stopSignal := unix.SIGTERM
81-
image, err := c.imageStore.Get(container.ImageRef)
82-
if err != nil {
83-
// NOTE(random-liu): It's possible that the container is stopped,
84-
// deleted and image is garbage collected before this point. However,
85-
// the chance is really slim, even it happens, it's still fine to return
86-
// an error here.
87-
return errors.Wrapf(err, "failed to get image metadata %q", container.ImageRef)
88-
}
89-
if image.ImageSpec.Config.StopSignal != "" {
90-
stopSignal, err = signal.ParseSignal(image.ImageSpec.Config.StopSignal)
81+
stopSignal := "SIGTERM"
82+
if container.StopSignal != "" {
83+
stopSignal = container.StopSignal
84+
} else {
85+
// The image may have been deleted, and the `StopSignal` field is
86+
// just introduced to handle that.
87+
// However, for containers created before the `StopSignal` field is
88+
// introduced, still try to get the stop signal from the image config.
89+
// If the image has been deleted, logging an error and using the
90+
// default SIGTERM is still better than returning error and leaving
91+
// the container unstoppable. (See issue #990)
92+
// TODO(random-liu): Remove this logic when containerd 1.2 is deprecated.
93+
image, err := c.imageStore.Get(container.ImageRef)
9194
if err != nil {
92-
return errors.Wrapf(err, "failed to parse stop signal %q",
93-
image.ImageSpec.Config.StopSignal)
95+
if err != store.ErrNotExist {
96+
return errors.Wrapf(err, "failed to get image %q", container.ImageRef)
97+
}
98+
logrus.Warningf("Image %q not found, stop container with signal %q", container.ImageRef, stopSignal)
99+
} else {
100+
if image.ImageSpec.Config.StopSignal != "" {
101+
stopSignal = image.ImageSpec.Config.StopSignal
102+
}
94103
}
95104
}
96-
logrus.Infof("Stop container %q with signal %v", id, stopSignal)
97-
if err = task.Kill(ctx, stopSignal); err != nil && !errdefs.IsNotFound(err) {
105+
sig, err := signal.ParseSignal(stopSignal)
106+
if err != nil {
107+
return errors.Wrapf(err, "failed to parse stop signal %q", stopSignal)
108+
}
109+
logrus.Infof("Stop container %q with signal %v", id, sig)
110+
if err = task.Kill(ctx, sig); err != nil && !errdefs.IsNotFound(err) {
98111
return errors.Wrapf(err, "failed to stop container %q", id)
99112
}
100113

‎pkg/store/container/container_test.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,9 @@ func TestContainerStore(t *testing.T) {
3939
Attempt: 1,
4040
},
4141
},
42-
ImageRef: "TestImage-1",
43-
LogPath: "/test/log/path/1",
42+
ImageRef: "TestImage-1",
43+
StopSignal: "SIGTERM",
44+
LogPath: "/test/log/path/1",
4445
},
4546
"2abcd": {
4647
ID: "2abcd",
@@ -52,8 +53,9 @@ func TestContainerStore(t *testing.T) {
5253
Attempt: 2,
5354
},
5455
},
55-
ImageRef: "TestImage-2",
56-
LogPath: "/test/log/path/2",
56+
StopSignal: "SIGTERM",
57+
ImageRef: "TestImage-2",
58+
LogPath: "/test/log/path/2",
5759
},
5860
"4a333": {
5961
ID: "4a333",
@@ -65,8 +67,9 @@ func TestContainerStore(t *testing.T) {
6567
Attempt: 3,
6668
},
6769
},
68-
ImageRef: "TestImage-3",
69-
LogPath: "/test/log/path/3",
70+
StopSignal: "SIGTERM",
71+
ImageRef: "TestImage-3",
72+
LogPath: "/test/log/path/3",
7073
},
7174
"4abcd": {
7275
ID: "4abcd",
@@ -78,7 +81,8 @@ func TestContainerStore(t *testing.T) {
7881
Attempt: 1,
7982
},
8083
},
81-
ImageRef: "TestImage-4abcd",
84+
StopSignal: "SIGTERM",
85+
ImageRef: "TestImage-4abcd",
8286
},
8387
}
8488
statuses := map[string]Status{
@@ -182,8 +186,9 @@ func TestWithContainerIO(t *testing.T) {
182186
Attempt: 1,
183187
},
184188
},
185-
ImageRef: "TestImage-1",
186-
LogPath: "/test/log/path",
189+
ImageRef: "TestImage-1",
190+
StopSignal: "SIGTERM",
191+
LogPath: "/test/log/path",
187192
}
188193
status := Status{
189194
Pid: 1,

‎pkg/store/container/metadata.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import (
2727
// 1) Metadata is immutable after created.
2828
// 2) Metadata is checkpointed as containerd container label.
2929

30-
// metadataVersion is current version of container metadata.
30+
// metadataVersion is current version of container metadata.
3131
const metadataVersion = "v1" // nolint
3232

3333
// versionedMetadata is the internal versioned container metadata.
@@ -58,6 +58,9 @@ type Metadata struct {
5858
ImageRef string
5959
// LogPath is the container log path.
6060
LogPath string
61+
// StopSignal is the system call signal that will be sent to the container to exit.
62+
// TODO(random-liu): Add integration test for stop signal.
63+
StopSignal string
6164
}
6265

6366
// MarshalJSON encodes Metadata into bytes in json format.

0 commit comments

Comments
 (0)