-
Notifications
You must be signed in to change notification settings - Fork 50
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.
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
base: main
Are you sure you want to change the base?
Conversation
@Setter(AccessLevel.PACKAGE) | ||
private RetryUtils.RetryConfig finiteDeleteAttemptsRetryConfig = | ||
RetryUtils.RetryConfig.builder().initialRetryInterval(Duration.ofSeconds(10L)) | ||
.maxRetryInterval(Duration.ofMinutes(2L)).maxAttempt(5).retryableExceptions( |
There was a problem hiding this comment.
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
} catch (PackageLoadingException | InvalidArtifactUriException e) { | ||
} catch (PackageLoadingException | InvalidArtifactUriException | InterruptedException e) { | ||
throw new IOException(e); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
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.
}, "docker-image-cleanup", logger); | ||
} | ||
} catch (PackageLoadingException | InvalidArtifactUriException e) { | ||
} catch (PackageLoadingException | InvalidArtifactUriException | InterruptedException e) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7011b66 |
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7011b66 |
.maxRetryInterval(Duration.ofMinutes(32L)).maxAttempt(30).retryableExceptions( | ||
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class, | ||
Arrays.asList(DockerServiceUnavailableException.class, DockerLoginException.class, | ||
SdkClientException.class, ServerException.class)).build(); |
There was a problem hiding this comment.
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?
dockerClient.deleteImage(image); | ||
return null; | ||
}, "docker-image-cleanup", logger); | ||
} |
There was a problem hiding this comment.
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
Issue #, if available:
Description of changes:
Currently we only try to remove the image once without retry. this could happen when the container is still spinning down which would cause the action to fail.
Why is this change necessary:
adding retries allows for consistency when deleting customer images.
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.