From 3bd60e49d41a9332efb6d72de7ec562d96597cfe Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Wed, 6 Mar 2024 11:31:45 +0100 Subject: [PATCH] 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: https://github.com/kubernetes/kubernetes/issues/123631 Refers to https://github.com/kubernetes/kubernetes/pull/123711 as well. Signed-off-by: Sascha Grunert --- internal/storage/image.go | 17 +++++++++++++++++ internal/storage/runtime.go | 11 +++++++++-- internal/storage/runtime_test.go | 10 +++++----- server/container_create_linux.go | 6 ++++++ server/sandbox_run_freebsd.go | 1 + server/sandbox_run_linux.go | 1 + server/sandbox_run_test.go | 6 ++++-- test/mocks/criostorage/criostorage.go | 23 +++++++++++++++++++---- 8 files changed, 62 insertions(+), 13 deletions(-) diff --git a/internal/storage/image.go b/internal/storage/image.go index 64d7ab5195b..6f288665aec 100644 --- a/internal/storage/image.go +++ b/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) } diff --git a/internal/storage/runtime.go b/internal/storage/runtime.go index 27f9dac7467..9ebb2be45c5 100644 --- a/internal/storage/runtime.go +++ b/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, diff --git a/internal/storage/runtime_test.go b/internal/storage/runtime_test.go index cd49de08d9e..0ea5487769a 100644 --- a/internal/storage/runtime_test.go +++ b/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, ) }) diff --git a/server/container_create_linux.go b/server/container_create_linux.go index e8eb90b8d32..3a30ce3eb76 100644 --- a/server/container_create_linux.go +++ b/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] diff --git a/server/sandbox_run_freebsd.go b/server/sandbox_run_freebsd.go index 4343666a13e..9d31307815b 100644 --- a/server/sandbox_run_freebsd.go +++ b/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()) diff --git a/server/sandbox_run_linux.go b/server/sandbox_run_linux.go index 6b858e94338..eb5edd977f4 100644 --- a/server/sandbox_run_linux.go +++ b/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()) diff --git a/server/sandbox_run_test.go b/server/sandbox_run_test.go index fde710cc8c6..585b28f89de 100644 --- a/server/sandbox_run_test.go +++ b/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), diff --git a/test/mocks/criostorage/criostorage.go b/test/mocks/criostorage/criostorage.go index 630f1bcb660..32f4386f906 100644 --- a/test/mocks/criostorage/criostorage.go +++ b/test/mocks/criostorage/criostorage.go @@ -7,6 +7,7 @@ package criostoragemock import ( context "context" reflect "reflect" + sync "sync" types "github.com/containers/image/v5/types" storage "github.com/containers/storage" @@ -96,6 +97,20 @@ func (mr *MockImageServerMockRecorder) HeuristicallyTryResolvingStringAsIDPrefix return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HeuristicallyTryResolvingStringAsIDPrefix", reflect.TypeOf((*MockImageServer)(nil).HeuristicallyTryResolvingStringAsIDPrefix), arg0) } +// ImageRemovalLock mocks base method. +func (m *MockImageServer) ImageRemovalLock(arg0 storage0.StorageImageID) *sync.RWMutex { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ImageRemovalLock", arg0) + ret0, _ := ret[0].(*sync.RWMutex) + return ret0 +} + +// ImageRemovalLock indicates an expected call of ImageRemovalLock. +func (mr *MockImageServerMockRecorder) ImageRemovalLock(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ImageRemovalLock", reflect.TypeOf((*MockImageServer)(nil).ImageRemovalLock), arg0) +} + // ImageStatusByID mocks base method. func (m *MockImageServer) ImageStatusByID(arg0 *types.SystemContext, arg1 storage0.StorageImageID) (*storage0.ImageResult, error) { m.ctrl.T.Helper() @@ -224,18 +239,18 @@ func (mr *MockRuntimeServerMockRecorder) CreateContainer(arg0, arg1, arg2, arg3, } // CreatePodSandbox mocks base method. -func (m *MockRuntimeServer) CreatePodSandbox(arg0 *types.SystemContext, arg1, arg2 string, arg3 references.RegistryImageReference, arg4, arg5, arg6, arg7, arg8 string, arg9 uint32, arg10 *types0.IDMappingOptions, arg11 []string, arg12 bool) (storage0.ContainerInfo, error) { +func (m *MockRuntimeServer) CreatePodSandbox(arg0 *types.SystemContext, arg1, arg2 string, arg3 references.RegistryImageReference, arg4, arg5, arg6, arg7, arg8 string, arg9 uint32, arg10 *types0.IDMappingOptions, arg11 []string, arg12 bool, arg13 storage0.ImageServer) (storage0.ContainerInfo, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreatePodSandbox", arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12) + ret := m.ctrl.Call(m, "CreatePodSandbox", arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12, arg13) ret0, _ := ret[0].(storage0.ContainerInfo) ret1, _ := ret[1].(error) return ret0, ret1 } // CreatePodSandbox indicates an expected call of CreatePodSandbox. -func (mr *MockRuntimeServerMockRecorder) CreatePodSandbox(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12 interface{}) *gomock.Call { +func (mr *MockRuntimeServerMockRecorder) CreatePodSandbox(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12, arg13 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreatePodSandbox", reflect.TypeOf((*MockRuntimeServer)(nil).CreatePodSandbox), arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreatePodSandbox", reflect.TypeOf((*MockRuntimeServer)(nil).CreatePodSandbox), arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12, arg13) } // DeleteContainer mocks base method.