kubernetes · k8s-ci-robot · Aug 28, 2019 · Aug 23, 2019 · Random-Liu · Aug 23, 2019
diff --git 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,19 +292,22 @@ 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.
 	err := ds.removeContainerLogSymlink(r.ContainerId)
 	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
@@ -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
@@ -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
@@ -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