Merge pull request #80320 from wk8/wk8/gmsa_cleanup

Make container removal fail if platform-specific containers fail
This commit is contained in:
Kubernetes Prow Robot 2019-08-27 22:41:29 -07:00 committed by GitHub
commit 6cf7f3c342
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 27 additions and 38 deletions

View File

@ -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
}

View File

@ -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
}

View File

@ -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)

View File

@ -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