Skip to content

feat: add retry to remove docker image #1739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add retry to remove docker image #1739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions ...ass/integrationtests/componentmanager/plugins/docker/DockerImageArtifactDownloadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@
import com.aws.greengrass.componentmanager.ComponentManager;
import com.aws.greengrass.componentmanager.ComponentStore;
import com.aws.greengrass.componentmanager.converter.RecipeLoader;
import com.aws.greengrass.componentmanager.models.ComponentArtifact;
import com.aws.greengrass.componentmanager.models.ComponentIdentifier;
import com.aws.greengrass.componentmanager.plugins.docker.DefaultDockerClient;
import com.aws.greengrass.componentmanager.plugins.docker.DockerImageDownloader;
import com.aws.greengrass.componentmanager.plugins.docker.EcrAccessor;
import com.aws.greengrass.componentmanager.plugins.docker.Image;
import com.aws.greengrass.componentmanager.plugins.docker.Registry;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerImageDeleteException;
import com.aws.greengrass.config.PlatformResolver;
import com.aws.greengrass.config.Topic;
import com.aws.greengrass.deployment.DeviceConfiguration;
Expand All @@ -23,6 +26,7 @@
import com.aws.greengrass.tes.LazyCredentialProvider;
import com.aws.greengrass.testcommons.testutilities.GGExtension;
import com.aws.greengrass.util.NucleusPaths;
import com.aws.greengrass.util.RetryUtils;
import com.vdurmont.semver4j.Semver;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand All @@ -36,15 +40,18 @@
import software.amazon.awssdk.services.ecr.model.GetAuthorizationTokenResponse;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.Base64;
import java.util.Collections;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.never;
Expand Down Expand Up @@ -149,6 +156,52 @@ void GIVEN_component_with_dockerhub_image_as_artifact_WHEN_prepare_packages_THEN
verify(dockerClient, times(3)).pullImage(any(Image.class));
}

@Test
void GIVEN_docker_image_downloader_WHEN_cleanup_succeeds_THEN_no_retries() throws Exception {
DockerImageDownloader downloader = createDownloaderWithRetryConfig();

downloader.cleanup();

verify(dockerClient, times(1)).deleteImage(any(Image.class));
}

@Test
void GIVEN_docker_image_downloader_WHEN_cleanup_with_retry_config_THEN_retry_mechanism_available() throws Exception {
DockerImageDownloader downloader = createDownloaderWithRetryConfig();

// Verify that the retry configuration was set successfully via reflection
java.lang.reflect.Field field = DockerImageDownloader.class.getDeclaredField("finiteDeleteAttemptsRetryConfig");
field.setAccessible(true);
RetryUtils.RetryConfig config = (RetryUtils.RetryConfig) field.get(downloader);

// Verify the retry config is properly set
assertEquals(3, config.getMaxAttempt());
assertEquals(Duration.ofMillis(50), config.getInitialRetryInterval());
}

private DockerImageDownloader createDownloaderWithRetryConfig() throws Exception {
RetryUtils.RetryConfig retryConfig = RetryUtils.RetryConfig.builder()
.initialRetryInterval(Duration.ofMillis(50))
.maxRetryInterval(Duration.ofMillis(50))
.maxAttempt(3)
.retryableExceptions(Collections.singletonList(DockerImageDeleteException.class))
.build();

ComponentIdentifier componentId = new ComponentIdentifier("test.container.component", new Semver("1.0.0"));
ComponentArtifact artifact = ComponentArtifact.builder()
.artifactUri(new URI("450817829141.dkr.ecr.us-east-1.amazonaws.com/integrationdockerimage:latest"))
.build();

DockerImageDownloader downloader = new DockerImageDownloader(componentId, artifact, tempRootDir, kernel.getContext(), kernel.getContext().get(ComponentStore.class));

// Use reflection to set the private retry config
java.lang.reflect.Field field = DockerImageDownloader.class.getDeclaredField("finiteDeleteAttemptsRetryConfig");
field.setAccessible(true);
field.set(downloader, retryConfig);

return downloader;
}

private void preloadLocalStoreContent() throws URISyntaxException, IOException {
Path localStoreContentPath =
Paths.get(DockerImageArtifactDownloadTest.class.getResource("downloader").toURI());
Expand Down
25 changes: 19 additions & 6 deletions src/main/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.aws.greengrass.componentmanager.models.ComponentIdentifier;
import com.aws.greengrass.componentmanager.models.ComponentRecipe;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.ConnectionException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerImageDeleteException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerLoginException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerServiceUnavailableException;
import com.aws.greengrass.dependency.Context;
Expand Down Expand Up @@ -61,9 +62,16 @@ public class DockerImageDownloader extends ArtifactDownloader {
private RetryUtils.RetryConfig finiteAttemptsRetryConfig =
RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(10L))
.maxRetryInterval(Duration.ofMinutes(32L)).maxAttempt(30).retryableExceptions(
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
SdkClientException.class, ServerException.class)).build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extremely nit: i dont think this is formatted as expected. Can you take a look?


@Setter(AccessLevel.PACKAGE)
private RetryUtils.RetryConfig finiteDeleteAttemptsRetryConfig =
RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(10L))
.maxRetryInterval(Duration.ofMinutes(2L)).maxAttempt(5).retryableExceptions(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

These configs are flagged under GS-Tech Static config campaign and is considered to be a violation. Please migrate these configs

Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
DockerImageDeleteException.class)).build();

/**
* Constructor.
*
Expand Down Expand Up @@ -310,15 +318,20 @@ private <T> T runWithConnectionErrorCheck(CrashableSupplier<T, Exception> task)
*/
@Override
public void cleanup() throws IOException {
// this docker image not only used by itself
try {
if (!ifImageUsedByOther(componentStore)) {
Image image = DockerImageArtifactParser
.getImage(ComponentArtifact.builder().artifactUri(artifact.getArtifactUri()).build());
dockerClient.deleteImage(image);
RetryUtils.runWithRetry(finiteDeleteAttemptsRetryConfig, () -> {

Image image = DockerImageArtifactParser
.getImage(ComponentArtifact.builder().artifactUri(artifact.getArtifactUri()).build());
dockerClient.deleteImage(image);
return null;
}, "docker-image-cleanup", logger);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extremely nit: I have a similar complaint with the formatting here

} catch (PackageLoadingException | InvalidArtifactUriException e) {
} catch (PackageLoadingException | InvalidArtifactUriException | InterruptedException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

Problem: InterruptedException is ignored. This can delay thread shutdown and clear the thread’s interrupt status. Only code that implements a thread’s interruption policy can swallow an interruption request.

Fix: Rethrow the InterruptedException or reinterrupt the current thread using Thread.currentThread().interrupt() so that higher-level interrupt handlers can function correctly.
If you are wrapping the InterruptedException inside a RuntimeException, call Thread.currentThread().interrupt() before throwing the RuntimeException.

Remember: Do this only for InterruptedException. Doing this for other types of exceptions may give rise to subtle bugs.

Learn more about interrupts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I think the behavior and the warning message thrown for an Interrupted Exception should be different. I also think current thread needs to be interrupted as recommended in this comment.

Also, in my head, the error handling should look a bit like this

try {
    ...
    ...
    ...
} catch (PackageLoadingException | InvalidArtifactUriException) {
    throw new IOException("Failed to cleanup docker image after retries", e);
} catch (InterruptedException e) {
    logger.atWarn().setCause(e).log("Failed to cleanup docker image");
    Thread.currentThread.interrupt();
} catch (Exception e) {
    throw new IOException("Failed to cleanup docker image after retries", e);
}

Lastly, please log the component artifact which we were unable to delete.

throw new IOException(e);
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that your code handles a broad swath of exceptions in the catch block, potentially trapping dissimilar issues or problems that should not be dealt with at this point in the program.

Learn more

throw new IOException("Failed to cleanup docker image after retries", e);
}
}

Expand Down
55 changes: 55 additions & 0 deletions ...st/java/com/aws/greengrass/componentmanager/plugins/docker/DockerImageDownloaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.aws.greengrass.componentmanager.models.ComponentIdentifier;
import com.aws.greengrass.componentmanager.models.ComponentRecipe;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.ConnectionException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerImageDeleteException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerLoginException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerPullException;
import com.aws.greengrass.componentmanager.plugins.docker.exceptions.DockerServiceUnavailableException;
Expand All @@ -33,6 +34,7 @@
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.services.ecr.model.ServerException;

import java.io.IOException;
import java.net.URI;
import java.nio.file.Path;
import java.time.Duration;
Expand Down Expand Up @@ -84,6 +86,12 @@ public class DockerImageDownloaderTest {
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
SdkClientException.class, ServerException.class)).build();

private final RetryUtils.RetryConfig finiteDeleteAttemptsRetryConfig =
RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofMillis(50L))
.maxRetryInterval(Duration.ofMillis(50L)).maxAttempt(3).retryableExceptions(
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class,
DockerImageDeleteException.class)).build();

@Mock
private DefaultDockerClient dockerClient;
@Mock
Expand Down Expand Up @@ -564,6 +572,52 @@ void GIVEN_a_artifact_with_private_ecr_image_WHEN_image_not_used_by_others_THEN_
verify(dockerClient, times(1)).deleteImage(any());
}

@Test
void GIVEN_cleanup_WHEN_delete_image_succeeds_THEN_no_retries_needed() throws Exception {
URI artifactUri = new URI("450817829141.dkr.ecr.us-east-1.amazonaws.com/integrationdockerimage:latest");
DockerImageDownloader downloader = spy(getDownloader(artifactUri));

doReturn(false).when(downloader).ifImageUsedByOther(any());
doNothing().when(dockerClient).deleteImage(any());

downloader.cleanup();

verify(dockerClient, times(1)).deleteImage(any());
}

@Test
void GIVEN_cleanup_WHEN_delete_image_fails_then_succeeds_THEN_retry_and_succeed(ExtensionContext extensionContext) throws Exception {
ignoreExceptionOfType(extensionContext, DockerImageDeleteException.class);

URI artifactUri = new URI("450817829141.dkr.ecr.us-east-1.amazonaws.com/integrationdockerimage:latest");
DockerImageDownloader downloader = spy(getDownloader(artifactUri));

doReturn(false).when(downloader).ifImageUsedByOther(any());
doThrow(new DockerImageDeleteException("Delete failed"))
.doThrow(new DockerImageDeleteException("Delete failed"))
.doNothing().when(dockerClient).deleteImage(any());

downloader.cleanup();

verify(dockerClient, times(3)).deleteImage(any());
}

@Test
void GIVEN_cleanup_WHEN_delete_image_exhausts_retries_THEN_throw_IOException(ExtensionContext extensionContext) throws Exception {
ignoreExceptionOfType(extensionContext, DockerImageDeleteException.class);

URI artifactUri = new URI("450817829141.dkr.ecr.us-east-1.amazonaws.com/integrationdockerimage:latest");
DockerImageDownloader downloader = spy(getDownloader(artifactUri));

doReturn(false).when(downloader).ifImageUsedByOther(any());
doThrow(new DockerImageDeleteException("Delete failed")).when(dockerClient).deleteImage(any());

IOException exception = assertThrows(IOException.class, () -> downloader.cleanup());
assertTrue(exception.getCause() instanceof DockerImageDeleteException);

verify(dockerClient, times(3)).deleteImage(any());
}

@Test
void GIVEN_network_error_WHEN_download_docker_image_THEN_retry_download_image_until_succeed(
ExtensionContext extensionContext) throws Exception {
Expand Down Expand Up @@ -618,6 +672,7 @@ private DockerImageDownloader getDownloader(URI artifactUri) {
mqttClient, componentStore);
downloader.setInfiniteAttemptsRetryConfig(infiniteAttemptsRetryConfig);
downloader.setFiniteAttemptsRetryConfig(finiteAttemptsRetryConfig);
downloader.setFiniteDeleteAttemptsRetryConfig(finiteDeleteAttemptsRetryConfig);
return downloader;
}
}
Loading