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

Commit 5e759f5

Browse files
Random-LiuRandom-Liu
committed
Remove container lifecycle image ref dependency.
Signed-off-by: Lantao Liu <[email protected]>
1 parent 89aaac8 commit 5e759f5

5 files changed

Lines changed: 61 additions & 35 deletions

File tree

‎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)