diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 6cb0144c462..7bf5622d807 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -182,14 +182,14 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create if cleanupInfo != nil { // we don't perform the clean up just yet at that could destroy information // needed for the container to start (e.g. Windows credentials stored in - // registry keys); instead, we'll clean up after the container successfully - // starts or gets removed + // registry keys); instead, we'll clean up when the container gets removed ds.containerCleanupInfos[containerID] = cleanupInfo } return &runtimeapi.CreateContainerResponse{ContainerId: containerID}, nil } - // the creation failed, let's clean up right away + // the creation failed, let's clean up right away - we ignore any errors though, + // this is best effort ds.performPlatformSpecificContainerCleanupAndLogErrors(containerName, cleanupInfo) return nil, createErr @@ -278,8 +278,6 @@ func (ds *dockerService) StartContainer(_ context.Context, r *runtimeapi.StartCo return nil, fmt.Errorf("failed to start container %q: %v", r.ContainerId, err) } - ds.performPlatformSpecificContainerForContainer(r.ContainerId) - return &runtimeapi.StartContainerResponse{}, nil } @@ -294,8 +292,6 @@ func (ds *dockerService) StopContainer(_ context.Context, r *runtimeapi.StopCont // RemoveContainer removes the container. func (ds *dockerService) RemoveContainer(_ context.Context, r *runtimeapi.RemoveContainerRequest) (*runtimeapi.RemoveContainerResponse, error) { - ds.performPlatformSpecificContainerForContainer(r.ContainerId) - // Ideally, log lifecycle should be independent of container lifecycle. // However, docker will remove container log after container is removed, // we can't prevent that now, so we also clean up the symlink here. @@ -303,10 +299,15 @@ func (ds *dockerService) RemoveContainer(_ context.Context, r *runtimeapi.Remove if err != nil { return nil, err } + errors := ds.performPlatformSpecificContainerForContainer(r.ContainerId) + if len(errors) != 0 { + return nil, fmt.Errorf("failed to run platform-specific clean ups for container %q: %v", r.ContainerId, errors) + } err = ds.client.RemoveContainer(r.ContainerId, dockertypes.ContainerRemoveOptions{RemoveVolumes: true, Force: true}) if err != nil { return nil, fmt.Errorf("failed to remove container %q: %v", r.ContainerId, err) } + return &runtimeapi.RemoveContainerResponse{}, nil } @@ -454,19 +455,27 @@ func (ds *dockerService) UpdateContainerResources(_ context.Context, r *runtimea return &runtimeapi.UpdateContainerResourcesResponse{}, nil } -func (ds *dockerService) performPlatformSpecificContainerForContainer(containerID string) { +func (ds *dockerService) performPlatformSpecificContainerForContainer(containerID string) (errors []error) { if cleanupInfo, present := ds.containerCleanupInfos[containerID]; present { - ds.performPlatformSpecificContainerCleanupAndLogErrors(containerID, cleanupInfo) - delete(ds.containerCleanupInfos, containerID) + errors = ds.performPlatformSpecificContainerCleanupAndLogErrors(containerID, cleanupInfo) + + if len(errors) == 0 { + delete(ds.containerCleanupInfos, containerID) + } } + + return } -func (ds *dockerService) performPlatformSpecificContainerCleanupAndLogErrors(containerNameOrID string, cleanupInfo *containerCleanupInfo) { +func (ds *dockerService) performPlatformSpecificContainerCleanupAndLogErrors(containerNameOrID string, cleanupInfo *containerCleanupInfo) []error { if cleanupInfo == nil { - return + return nil } - for _, err := range ds.performPlatformSpecificContainerCleanup(cleanupInfo) { + errors := ds.performPlatformSpecificContainerCleanup(cleanupInfo) + for _, err := range errors { klog.Warningf("error when cleaning up after container %q: %v", containerNameOrID, err) } + + return errors } diff --git a/pkg/kubelet/dockershim/docker_container_unsupported.go b/pkg/kubelet/dockershim/docker_container_unsupported.go index 8ba3278520d..e3848320ba1 100644 --- a/pkg/kubelet/dockershim/docker_container_unsupported.go +++ b/pkg/kubelet/dockershim/docker_container_unsupported.go @@ -27,23 +27,13 @@ type containerCleanupInfo struct{} // applyPlatformSpecificDockerConfig applies platform-specific configurations to a dockertypes.ContainerCreateConfig struct. // The containerCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCleanup -// after either: -// * the container creation has failed -// * the container has been successfully started -// * the container has been removed -// whichever happens first. +// after either the container creation has failed or the container has been removed. func (ds *dockerService) applyPlatformSpecificDockerConfig(*runtimeapi.CreateContainerRequest, *dockertypes.ContainerCreateConfig) (*containerCleanupInfo, error) { return nil, nil } // performPlatformSpecificContainerCleanup is responsible for doing any platform-specific cleanup -// after either: -// * the container creation has failed -// * the container has been successfully started -// * the container has been removed -// whichever happens first. -// Any errors it returns are simply logged, but do not prevent the container from being started or -// removed. +// after either the container creation has failed or the container has been removed. func (ds *dockerService) performPlatformSpecificContainerCleanup(cleanupInfo *containerCleanupInfo) (errors []error) { return } diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index 4276d7c531f..17b01c9c79d 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -38,11 +38,7 @@ type containerCleanupInfo struct { // applyPlatformSpecificDockerConfig applies platform-specific configurations to a dockertypes.ContainerCreateConfig struct. // The containerCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCleanup -// after either: -// * the container creation has failed -// * the container has been successfully started -// * the container has been removed -// whichever happens first. +// after either the container creation has failed or the container has been removed. func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig) (*containerCleanupInfo, error) { cleanupInfo := &containerCleanupInfo{} @@ -163,13 +159,7 @@ func randomString(length int) (string, error) { } // performPlatformSpecificContainerCleanup is responsible for doing any platform-specific cleanup -// after either: -// * the container creation has failed -// * the container has been successfully started -// * the container has been removed -// whichever happens first. -// Any errors it returns are simply logged, but do not prevent the container from being started or -// removed. +// after either the container creation has failed or the container has been removed. func (ds *dockerService) performPlatformSpecificContainerCleanup(cleanupInfo *containerCleanupInfo) (errors []error) { if err := removeGMSARegistryValue(cleanupInfo); err != nil { errors = append(errors, err) diff --git a/pkg/kubelet/dockershim/docker_service.go b/pkg/kubelet/dockershim/docker_service.go index ce40e9ff5b0..5d238639762 100644 --- a/pkg/kubelet/dockershim/docker_service.go +++ b/pkg/kubelet/dockershim/docker_service.go @@ -311,7 +311,7 @@ type dockerService struct { startLocalStreamingServer bool // containerCleanupInfos maps container IDs to the `containerCleanupInfo` structs - // needed to clean up after containers have been started or removed. + // needed to clean up after containers have been removed. // (see `applyPlatformSpecificDockerConfig` and `performPlatformSpecificContainerCleanup` // methods for more info). containerCleanupInfos map[string]*containerCleanupInfo