Add image removal lock to image server

We now avoid removing images when a sandbox or container creation is in
progress. This should close the gap in the outlined corner case in:

kubernetes/kubernetes#123631

Refers to kubernetes/kubernetes#123711 as well.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

internal/storage/image.go

@@ -94,6 +94,7 @@ type imageService struct {
 	ctx                  context.Context
 	config               *config.Config
 	regexForPinnedImages []*regexp.Regexp
+	imageRemovalLock     sync.Map
 }
 
 // ImageBeingPulled map[string]bool to keep track of the images haven't done pulling.
@@ -151,6 +152,14 @@ type ImageServer interface {
 	// CandidatesForPotentiallyShortImageName resolves an image name into a set of fully-qualified image names (domain/repo/image:tag|@digest).
 	// It will only return an empty slice if err != nil.
 	CandidatesForPotentiallyShortImageName(systemContext *types.SystemContext, imageName string) ([]RegistryImageReference, error)
+	// ImageRemovalLock returns the removal lock for the specified image storage reference.
+	ImageRemovalLock(id StorageImageID) *sync.RWMutex
+}
+
+// ImageRemovalLock returns the removal lock for the specified image storage reference.
+func (svc *imageService) ImageRemovalLock(id StorageImageID) *sync.RWMutex {
+	value, _ := svc.imageRemovalLock.LoadOrStore(id, &sync.RWMutex{})
+	return value.(*sync.RWMutex) // nolint:errcheck,forcetypeassert // type matches line above
 }
 
 func parseImageNames(image *storage.Image) (someName *RegistryImageReference, tags []reference.NamedTagged, digests []reference.Canonical, err error) {
@@ -726,6 +735,14 @@ func (svc *imageService) DeleteImage(systemContext *types.SystemContext, id Stor
 		return err
 	}
 
+	// Avoid removing images which are currently used for container creation
+	imageRemovalLock := svc.ImageRemovalLock(id)
+	imageRemovalLock.Lock()
+	defer imageRemovalLock.Unlock()
+
+	// Cleanup memory when the image got removed
+	defer svc.imageRemovalLock.Delete(id)
+
 	return ref.DeleteImage(svc.ctx, systemContext)
 }
 

internal/storage/runtime.go

@@ -66,7 +66,7 @@ type RuntimeServer interface {
 	// with the pod's infrastructure container having the same value for
 	// both its pod's ID and its container ID.
 	// Pointer arguments can be nil.  All other arguments are required.
-	CreatePodSandbox(systemContext *types.SystemContext, podName, podID string, pauseImage RegistryImageReference, imageAuthFile, containerName, metadataName, uid, namespace string, attempt uint32, idMappingsOptions *storage.IDMappingOptions, labelOptions []string, privileged bool) (ContainerInfo, error)
+	CreatePodSandbox(systemContext *types.SystemContext, podName, podID string, pauseImage RegistryImageReference, imageAuthFile, containerName, metadataName, uid, namespace string, attempt uint32, idMappingsOptions *storage.IDMappingOptions, labelOptions []string, privileged bool, imageServer ImageServer) (ContainerInfo, error)
 
 	// GetContainerMetadata returns the metadata we've stored for a container.
 	GetContainerMetadata(idOrName string) (RuntimeContainerMetadata, error)
@@ -298,7 +298,7 @@ func (r *runtimeService) createContainerOrPodSandbox(systemContext *types.System
 	}, nil
 }
 
-func (r *runtimeService) CreatePodSandbox(systemContext *types.SystemContext, podName, podID string, pauseImage RegistryImageReference, imageAuthFile, containerName, metadataName, uid, namespace string, attempt uint32, idMappingsOptions *storage.IDMappingOptions, labelOptions []string, privileged bool) (ContainerInfo, error) {
+func (r *runtimeService) CreatePodSandbox(systemContext *types.SystemContext, podName, podID string, pauseImage RegistryImageReference, imageAuthFile, containerName, metadataName, uid, namespace string, attempt uint32, idMappingsOptions *storage.IDMappingOptions, labelOptions []string, privileged bool, imageServer ImageServer) (ContainerInfo, error) {
 	// Check if we have the specified image.
 	var ref types.ImageReference
 	ref, err := istorage.Transport.NewStoreReference(r.storageImageServer.GetStore(), pauseImage.Raw(), "")
@@ -338,6 +338,13 @@ func (r *runtimeService) CreatePodSandbox(systemContext *types.SystemContext, po
 	// Resolve the image ID.
 	imageID := storageImageIDFromImage(img)
 
+	if imageServer != nil {
+		// Avoid image removal during sandbox creation
+		imageRemovalLock := imageServer.ImageRemovalLock(imageID)
+		imageRemovalLock.RLock()
+		defer imageRemovalLock.RUnlock()
+	}
+
 	return r.createContainerOrPodSandbox(systemContext, podID, &runtimeContainerMetadataTemplate{
 		podName:            podName,
 		podID:              podID,

internal/storage/runtime_test.go

@@ -539,7 +539,7 @@ var _ = t.Describe("Runtime", func() {
 				info, err = sut.CreatePodSandbox(&types.SystemContext{},
 					"podName", "podID", pauseImage, "",
 					"containerName", "metadataName",
-					"uid", "namespace", 0, nil, []string{"mountLabel"}, false,
+					"uid", "namespace", 0, nil, []string{"mountLabel"}, false, nil,
 				)
 			})
 
@@ -675,7 +675,7 @@ var _ = t.Describe("Runtime", func() {
 			_, err = sut.CreatePodSandbox(&types.SystemContext{},
 				"podName", "podID", pauseImage, "",
 				"containerName", "metadataName",
-				"uid", "namespace", 0, nil, []string{"mountLabel"}, false,
+				"uid", "namespace", 0, nil, []string{"mountLabel"}, false, nil,
 			)
 
 			// Then
@@ -697,7 +697,7 @@ var _ = t.Describe("Runtime", func() {
 			_, err = sut.CreatePodSandbox(&types.SystemContext{},
 				"podName", "podID", pauseImage, "",
 				"containerName", "metadataName",
-				"uid", "namespace", 0, nil, []string{"mountLabel"}, false,
+				"uid", "namespace", 0, nil, []string{"mountLabel"}, false, nil,
 			)
 
 			// Then
@@ -801,7 +801,7 @@ var _ = t.Describe("Runtime", func() {
 			info, err = sut.CreatePodSandbox(&types.SystemContext{},
 				"podName", "podID", pauseImage, "",
 				"containerName", "metadataName",
-				"uid", "namespace", 0, nil, []string{"mountLabel"}, false,
+				"uid", "namespace", 0, nil, []string{"mountLabel"}, false, nil,
 			)
 		})
 
@@ -820,7 +820,7 @@ var _ = t.Describe("Runtime", func() {
 			info, err = sut.CreatePodSandbox(&types.SystemContext{},
 				"podName", "podID", pauseImage, "/var/non-default/credentials.json",
 				"containerName", "metadataName",
-				"uid", "namespace", 0, nil, []string{"mountLabel"}, false,
+				"uid", "namespace", 0, nil, []string{"mountLabel"}, false, nil,
 			)
 		})
 

server/container_create_linux.go

@@ -245,6 +245,12 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr ctrfactory.Cont
 
 	imageName := imgResult.SomeNameOfThisImage
 	imageID := imgResult.ID
+
+	// Avoid image removal during container creation
+	imageRemovalLock := s.StorageImageServer().ImageRemovalLock(imageID)
+	imageRemovalLock.RLock()
+	defer imageRemovalLock.RUnlock()
+
 	someRepoDigest := ""
 	if len(imgResult.RepoDigests) > 0 {
 		someRepoDigest = imgResult.RepoDigests[0]

server/sandbox_run_freebsd.go

@@ -157,6 +157,7 @@ func (s *Server) runPodSandbox(ctx context.Context, req *types.RunPodSandboxRequ
 		nil,
 		labelOptions,
 		privileged,
+		s.ContainerServer.StorageImageServer(),
 	)
 	if errors.Is(err, storage.ErrDuplicateName) {
 		return nil, fmt.Errorf("pod sandbox with name %q already exists", sbox.Name())

server/sandbox_run_linux.go

@@ -469,6 +469,7 @@ func (s *Server) runPodSandbox(ctx context.Context, req *types.RunPodSandboxRequ
 		idMappingsOptions,
 		labelOptions,
 		privileged,
+		s.ContainerServer.StorageImageServer(),
 	)
 	if errors.Is(err, storage.ErrDuplicateName) {
 		return nil, fmt.Errorf("pod sandbox with name %q already exists", sbox.Name())

server/sandbox_run_test.go

@@ -30,7 +30,8 @@ var _ = t.Describe("RunPodSandbox", func() {
 				runtimeServerMock.EXPECT().CreatePodSandbox(gomock.Any(),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
+					gomock.Any()).
 					Return(storage.ContainerInfo{
 						RunDir: "/tmp",
 						Config: &v1.Image{Config: v1.ImageConfig{}},
@@ -111,7 +112,8 @@ var _ = t.Describe("RunPodSandbox", func() {
 				runtimeServerMock.EXPECT().CreatePodSandbox(gomock.Any(),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
+					gomock.Any()).
 					Return(storage.ContainerInfo{}, nil),
 				runtimeServerMock.EXPECT().DeleteContainer(gomock.Any(), gomock.Any()).
 					Return(nil),