Container life cycle should not rely on the existence of other image references.

[Random-Liu]

Containerd manages images based on references. Image names (tag, id etc.), container references are all valid independent references.

They are independent, means that you can remove all image references, and the container can still run happily because the snapshots are still referenced. This also means that we can't assume image references (and the content) exist across the whole life cycle of a container using the image.

However, we are doing so in some container operations:

• ContainerStop: https://github.com/containerd/cri/blob/master/pkg/server/container_stop.go#L80
• ContainerStatus: https://github.com/containerd/cri/blob/master/pkg/server/container_status.go#L44

This causes issue like containerd/containerd#2858.

There are 2 options to fix this:

• Option 1: Add logic in the cri plugin to avoid deleting image references when container is still using an image.
  • Problem 1: This is racy. It is always possible that after you check and make sure that an image is not in use, a new container is created referencing the image. Need to add synchronization for this case.
  • Problem 2: Users can always delete image references by talking with containerd directly. We don't have any control to that.
• Option 2: Always assume that image reference may be gone after container creation, and never rely on it in the container lifecycle.

Given the problem of option 1, I prefer option 2.

For option 2:

1. For StopContainer, only StopSignal is needed, we should add it into container metadata during container creation;
2. For ContainerStatus, the image id to tag conversion is best effort anyway, just don't return error.

Note that adding StopSignal into metadata is adding a new field into our metadata checkpoint. We should handle in place upgrade properly.

[mikebrow]

agree option 2

[Random-Liu]

At a second thought, I think we need both option 1 and 2.

We need option 1, because that is Docker's behavior, I think people expect that in crictl. And even for Kubernetes garbage collection, it is an important piece to avoid race condition:

1. Kubelet compares container and image list, and find an unused image;
2. New container created using the image;
3. Kubelet garbage collects the image.

Currently, for Docker, 3) will fail; but for containerd 3) will remove the image.

We still need option 2, because users may still use ctr to remove the image. That is not expected behavior, but we should live with it, at least container lifecycle shouldn't be affected. And because that is caused by unexpected behavior, I think it is fine to not handle #991 (comment).

And I think we should cherry-pick option 2 into supported branches, because it is low risk and fixes issues in most cases. We can only keep option 1 in master.

[mikebrow]

ok, thx for investigating! agree option 1 with tolerance for option 2.

[mikebrow]

suggest adding an issue to enable the garbage collect of the in use image.. (breaking the behavior exhibited by docker in refusing to remove images in use)