From c29fec48766ce43d0060829a3e98afdf729cc5d8 Mon Sep 17 00:00:00 2001 From: Ben Kelley Date: Tue, 20 Feb 2018 12:08:13 +1100 Subject: [PATCH] Allow the "retries" parameter to apply to pull as well as push. Signed-off-by: Ben Kelley --- .../asciidoc/inc/build/_configuration.adoc | 3 + .../asciidoc/inc/start/_configuration.adoc | 5 +- .../asciidoc/inc/watch/_configuration.adoc | 4 ++ .../io/fabric8/maven/docker/BuildMojo.java | 5 +- .../io/fabric8/maven/docker/StartMojo.java | 5 +- .../io/fabric8/maven/docker/WatchMojo.java | 5 +- .../maven/docker/access/DockerAccess.java | 2 +- .../access/hc/DockerAccessWithHcClient.java | 15 +++-- .../maven/docker/service/BuildService.java | 9 +-- .../maven/docker/service/RegistryService.java | 4 +- .../maven/docker/service/WatchService.java | 8 +-- src/test/java/integration/DockerAccessIT.java | 2 +- .../java/integration/DockerAccessWinIT.java | 2 +- .../hc/DockerAccessWithHcClientTest.java | 55 ++++++++++++++++--- .../docker/service/RegistryServiceTest.java | 6 +- 15 files changed, 95 insertions(+), 35 deletions(-) diff --git a/src/main/asciidoc/inc/build/_configuration.adoc b/src/main/asciidoc/inc/build/_configuration.adoc index 4b2e20f53..398c16992 100644 --- a/src/main/asciidoc/inc/build/_configuration.adoc +++ b/src/main/asciidoc/inc/build/_configuration.adoc @@ -90,6 +90,9 @@ A provided `` takes precedence over the name given here. This tag is usefu | *ports* | The exposed ports which is a list of `` elements, one for each port to expose. Whitespace is trimmed from each element and empty elements are ignored. The format can be either pure numerical ("8080") or with the protocol attached ("8080/tcp"). +| *retries* +| If pulling an image is required, how often should a pull be retried before giving up. This useful for flaky registries which tend to return 500 error codes from time to time. The default is 0 which means no retry at all. + | *runCmds* | Commands to be run during the build process. It contains *run* elements which are passed to the shell. Whitespace is trimmed from each element and empty elements are ignored. The run commands are inserted right after the assembly and after *workdir* into the Dockerfile. This tag is not to be confused with the `` section for this image which specifies the runtime behaviour when starting containers. diff --git a/src/main/asciidoc/inc/start/_configuration.adoc b/src/main/asciidoc/inc/start/_configuration.adoc index 7cc37086a..8c8d952b9 100644 --- a/src/main/asciidoc/inc/start/_configuration.adoc +++ b/src/main/asciidoc/inc/start/_configuration.adoc @@ -5,6 +5,10 @@ In addition to the <>, this goal supports the following gl |=== | Element | Description | Property +| *retries* +| If pulling an image is required, how often should a pull be retried before giving up. This useful for flaky registries which tend to return 500 error codes from time to time. The default is 0 which means no retry at all. +| `docker.pull.retries` + | *showLogs* | In order to switch on globally the logs *showLogs* can be used as global configuration (i.e. outside of ``). If set it will print out all standard output and standard error messages for all containers started. As value the images for which logs should be shown can be given as a comma separated list. @@ -14,7 +18,6 @@ In addition to the <>, this goal supports the following gl | *startParallel* | Starts docker images in parallel while dependencies expressed as <> or <> are respected. This option can significantly reduce the startup time because independent containers do not need to wait for each other. | `docker.startParallel` - |=== The `` configuration element knows the following sub elements: diff --git a/src/main/asciidoc/inc/watch/_configuration.adoc b/src/main/asciidoc/inc/watch/_configuration.adoc index b0ad8edd0..2d0fa0242 100644 --- a/src/main/asciidoc/inc/watch/_configuration.adoc +++ b/src/main/asciidoc/inc/watch/_configuration.adoc @@ -45,6 +45,10 @@ below how this can be specified. | *removeVolumes* | if set to `true` will remove any volumes associated to the container as well. This option will be ignored if either `keepContainer` or `keepRunning` are `true`. | `docker.removeVolumes` + +| *retries* +| If pulling an image is required, how often should a pull be retried before giving up. This useful for flaky registries which tend to return 500 error codes from time to time. The default is 0 which means no retry at all. +| `docker.pull.retries` |=== Image specific watch configuration goes into an extra image-level `` section (i.e. `+...+`). The following parameters are recognized: diff --git a/src/main/java/io/fabric8/maven/docker/BuildMojo.java b/src/main/java/io/fabric8/maven/docker/BuildMojo.java index 6f8d167ab..71fdaa4b4 100644 --- a/src/main/java/io/fabric8/maven/docker/BuildMojo.java +++ b/src/main/java/io/fabric8/maven/docker/BuildMojo.java @@ -29,6 +29,9 @@ public class BuildMojo extends AbstractBuildSupportMojo { @Parameter(property = "docker.skip.build", defaultValue = "false") protected boolean skipBuild; + @Parameter(property = "docker.pull.retries", defaultValue = "0") + private int retries; + @Override protected void executeInternal(ServiceHub hub) throws DockerAccessException, MojoExecutionException { if (skipBuild) { @@ -56,7 +59,7 @@ protected void buildAndTag(ServiceHub hub, ImageConfiguration imageConfig) ImagePullManager pullManager = getImagePullManager(determinePullPolicy(imageConfig.getBuildConfiguration()), autoPull); BuildService buildService = hub.getBuildService(); - buildService.buildImage(imageConfig, pullManager, buildContext); + buildService.buildImage(imageConfig, pullManager, buildContext, retries); if (!skipTag) { buildService.tagImage(imageConfig.getName(), imageConfig); } diff --git a/src/main/java/io/fabric8/maven/docker/StartMojo.java b/src/main/java/io/fabric8/maven/docker/StartMojo.java index 2588ca71a..f7287943f 100644 --- a/src/main/java/io/fabric8/maven/docker/StartMojo.java +++ b/src/main/java/io/fabric8/maven/docker/StartMojo.java @@ -48,6 +48,9 @@ public class StartMojo extends AbstractDockerMojo { @Parameter(property = "docker.startParallel", defaultValue = "false") private boolean startParallel; + @Parameter(property = "docker.pull.retries", defaultValue = "0") + private int retries; + // whether to block during to start. Set it via System property docker.follow private boolean follow; @@ -307,7 +310,7 @@ private Queue prepareStart(ServiceHub hub, QueryService quer ImagePullManager pullManager = getImagePullManager(determinePullPolicy(runConfig), autoPull); hub.getRegistryService().pullImageWithPolicy(imageConfig.getName(), pullManager, registryConfig, - queryService.hasImage(imageConfig.getName())); + queryService.hasImage(imageConfig.getName()), retries); NetworkConfig config = runConfig.getNetworkingConfig(); if (autoCreateCustomNetworks && config.isCustomNetwork()) { diff --git a/src/main/java/io/fabric8/maven/docker/WatchMojo.java b/src/main/java/io/fabric8/maven/docker/WatchMojo.java index aff249d6b..cbf3c6299 100644 --- a/src/main/java/io/fabric8/maven/docker/WatchMojo.java +++ b/src/main/java/io/fabric8/maven/docker/WatchMojo.java @@ -66,6 +66,9 @@ public class WatchMojo extends AbstractBuildSupportMojo { @Parameter(property = "docker.autoCreateCustomNetworks", defaultValue = "false") protected boolean autoCreateCustomNetworks; + @Parameter(property = "docker.pull.retries", defaultValue = "0") + private int retries; + @Override protected synchronized void executeInternal(ServiceHub hub) throws DockerAccessException, MojoExecutionException { @@ -73,7 +76,7 @@ protected synchronized void executeInternal(ServiceHub hub) throws DockerAccessE BuildService.BuildContext buildContext = getBuildContext(); WatchService.WatchContext watchContext = getWatchContext(); - hub.getWatchService().watch(watchContext, buildContext, getResolvedImages()); + hub.getWatchService().watch(watchContext, buildContext, getResolvedImages(), retries); } protected WatchService.WatchContext getWatchContext() throws MojoExecutionException { diff --git a/src/main/java/io/fabric8/maven/docker/access/DockerAccess.java b/src/main/java/io/fabric8/maven/docker/access/DockerAccess.java index 9fce39430..1317f1ada 100644 --- a/src/main/java/io/fabric8/maven/docker/access/DockerAccess.java +++ b/src/main/java/io/fabric8/maven/docker/access/DockerAccess.java @@ -178,7 +178,7 @@ void copyArchive(String containerId, File archive, String targetPath) * @param registry an optional registry from where to pull the image. Can be null. * @throws DockerAccessException if the image couldn't be pulled. */ - void pullImage(String image, AuthConfig authConfig, String registry) throws DockerAccessException; + void pullImage(String image, AuthConfig authConfig, String registry, int retries) throws DockerAccessException; /** * Push an image to a registry. An registry can be specified which is used as target diff --git a/src/main/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClient.java b/src/main/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClient.java index f54b61b8e..1030927a7 100644 --- a/src/main/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClient.java +++ b/src/main/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClient.java @@ -365,14 +365,13 @@ public void loadImage(String image, File tarArchive) throws DockerAccessExceptio } @Override - public void pullImage(String image, AuthConfig authConfig, String registry) + public void pullImage(String image, AuthConfig authConfig, String registry, int retries) throws DockerAccessException { ImageName name = new ImageName(image); String pullUrl = urlBuilder.pullImage(name, registry); try { - delegate.post(pullUrl, null, createAuthHeader(authConfig), - createPullOrPushResponseHandler(), HTTP_OK); + doPushPullImage(pullUrl, createAuthHeader(authConfig), createBuildResponseHandler(), HTTP_OK, retries); } catch (IOException e) { throw new DockerAccessException(e, "Unable to pull '%s'%s", image, (registry != null) ? " from registry '" + registry + "'" : ""); } @@ -386,7 +385,7 @@ public void pushImage(String image, AuthConfig authConfig, String registry, int String temporaryImage = tagTemporaryImage(name, registry); DockerAccessException dae = null; try { - doPushImage(pushUrl, createAuthHeader(authConfig), createPullOrPushResponseHandler(), HTTP_OK, retries); + doPushPullImage(pushUrl, createAuthHeader(authConfig), createPullOrPushResponseHandler(), HTTP_OK, retries); } catch (IOException e) { dae = new DockerAccessException(e, "Unable to push '%s'%s", image, (registry != null) ? " from registry '" + registry + "'" : ""); throw dae; @@ -595,16 +594,16 @@ private boolean isRetryableErrorCode(int errorCode) { return errorCode == HTTP_INTERNAL_ERROR; } - private void doPushImage(String url, Map header, HcChunkedResponseHandlerWrapper handler, int status, + private void doPushPullImage(String url, Map header, HcChunkedResponseHandlerWrapper handler, int status, int retries) throws IOException { - // 0: The original attemp, 1..retry: possible retries. + // 0: The original attempt, 1..retry: possible retries. for (int i = 0; i <= retries; i++) { try { - delegate.post(url, null, header, handler, HTTP_OK); + delegate.post(url, null, header, handler, status); return; } catch (HttpResponseException e) { if (isRetryableErrorCode(e.getStatusCode()) && i != retries) { - log.warn("failed to push image to [{}], retrying...", url); + log.warn("failed to push/pull image to/from [%s], retrying...", url); } else { throw e; } diff --git a/src/main/java/io/fabric8/maven/docker/service/BuildService.java b/src/main/java/io/fabric8/maven/docker/service/BuildService.java index 8b43388ce..1bda01165 100644 --- a/src/main/java/io/fabric8/maven/docker/service/BuildService.java +++ b/src/main/java/io/fabric8/maven/docker/service/BuildService.java @@ -48,14 +48,15 @@ public class BuildService { * * @param imageConfig the image configuration * @param buildContext the build context + * @param pullRetries the number of times to retry if pulling an image fails * @throws DockerAccessException * @throws MojoExecutionException */ - public void buildImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, BuildContext buildContext) + public void buildImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, BuildContext buildContext, int pullRetries) throws DockerAccessException, MojoExecutionException { if (imagePullManager != null) { - autoPullBaseImage(imageConfig, imagePullManager, buildContext); + autoPullBaseImage(imageConfig, imagePullManager, buildContext, pullRetries); } buildImage(imageConfig, buildContext.getMojoParameters(), checkForNocache(imageConfig), addBuildArgs(buildContext)); @@ -190,7 +191,7 @@ private Map addBuildArgsFromProperties(Properties properties) { return buildArgs; } - private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, BuildContext buildContext) + private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, BuildContext buildContext, int retries) throws DockerAccessException, MojoExecutionException { BuildImageConfiguration buildConfig = imageConfig.getBuildConfiguration(); @@ -206,7 +207,7 @@ private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager fromImage = extractBaseFromConfiguration(buildConfig); } if (fromImage != null && !DockerAssemblyManager.SCRATCH_IMAGE.equals(fromImage)) { - registryService.pullImageWithPolicy(fromImage, imagePullManager, buildContext.getRegistryConfig(), queryService.hasImage(fromImage)); + registryService.pullImageWithPolicy(fromImage, imagePullManager, buildContext.getRegistryConfig(), queryService.hasImage(fromImage), retries); } } diff --git a/src/main/java/io/fabric8/maven/docker/service/RegistryService.java b/src/main/java/io/fabric8/maven/docker/service/RegistryService.java index e52b66580..f3783c7fd 100644 --- a/src/main/java/io/fabric8/maven/docker/service/RegistryService.java +++ b/src/main/java/io/fabric8/maven/docker/service/RegistryService.java @@ -76,7 +76,7 @@ public void pushImages(Collection imageConfigs, * @throws DockerAccessException * @throws MojoExecutionException */ - public void pullImageWithPolicy(String image, ImagePullManager pullManager, RegistryConfig registryConfig, boolean hasImage) + public void pullImageWithPolicy(String image, ImagePullManager pullManager, RegistryConfig registryConfig, boolean hasImage, int retries) throws DockerAccessException, MojoExecutionException { // Already pulled, so we don't need to take care @@ -95,7 +95,7 @@ public void pullImageWithPolicy(String image, ImagePullManager pullManager, Regi imageName.getRegistry(), registryConfig.getRegistry()); docker.pullImage(imageName.getFullName(), - createAuthConfig(false, null, actualRegistry, registryConfig), actualRegistry); + createAuthConfig(false, null, actualRegistry, registryConfig), actualRegistry, retries); log.info("Pulled %s in %s", imageName.getFullName(), EnvUtil.formatDurationTill(time)); pullManager.pulled(image); diff --git a/src/main/java/io/fabric8/maven/docker/service/WatchService.java b/src/main/java/io/fabric8/maven/docker/service/WatchService.java index 40c1de25e..6289a6c4a 100644 --- a/src/main/java/io/fabric8/maven/docker/service/WatchService.java +++ b/src/main/java/io/fabric8/maven/docker/service/WatchService.java @@ -52,7 +52,7 @@ public WatchService(ArchiveService archiveService, BuildService buildService, Do this.log = log; } - public synchronized void watch(WatchContext context, BuildService.BuildContext buildContext, List images) throws DockerAccessException, + public synchronized void watch(WatchContext context, BuildService.BuildContext buildContext, List images, int pullRetries) throws DockerAccessException, MojoExecutionException { // Important to be be a single threaded scheduler since watch jobs must run serialized @@ -83,7 +83,7 @@ public synchronized void watch(WatchContext context, BuildService.BuildContext b } if (watcher.isBuild()) { - schedule(executor, createBuildWatchTask(watcher, context.getMojoParameters(), watchMode == WatchMode.both, buildContext), interval); + schedule(executor, createBuildWatchTask(watcher, context.getMojoParameters(), watchMode == WatchMode.both, buildContext, pullRetries), interval); tasks.add("rebuilding"); } } @@ -149,7 +149,7 @@ private void callPostExec(ImageWatcher watcher) throws DockerAccessException, Ex } private Runnable createBuildWatchTask(final ImageWatcher watcher, - final MojoParameters mojoParameters, final boolean doRestart, final BuildService.BuildContext buildContext) + final MojoParameters mojoParameters, final boolean doRestart, final BuildService.BuildContext buildContext, final int pullRetries) throws MojoExecutionException { final ImageConfiguration imageConfig = watcher.getImageConfiguration(); final AssemblyFiles files = archiveService.getAssemblyFiles(imageConfig, mojoParameters); @@ -171,7 +171,7 @@ public void run() { watcher.getWatchContext().getImageCustomizer().execute(imageConfig); } - buildService.buildImage(imageConfig, null, buildContext); + buildService.buildImage(imageConfig, null, buildContext, pullRetries); String name = imageConfig.getName(); watcher.setImageId(queryService.getImageId(name)); diff --git a/src/test/java/integration/DockerAccessIT.java b/src/test/java/integration/DockerAccessIT.java index bf0ce9f6b..b9a0047c1 100644 --- a/src/test/java/integration/DockerAccessIT.java +++ b/src/test/java/integration/DockerAccessIT.java @@ -135,7 +135,7 @@ private void testDoesNotHave() throws DockerAccessException { } private void testPullImage() throws DockerAccessException { - dockerClient.pullImage(IMAGE, null, null); + dockerClient.pullImage(IMAGE, null, null, 0); assertTrue(hasImage(IMAGE)); } diff --git a/src/test/java/integration/DockerAccessWinIT.java b/src/test/java/integration/DockerAccessWinIT.java index c59aee92d..a4af4a5e8 100644 --- a/src/test/java/integration/DockerAccessWinIT.java +++ b/src/test/java/integration/DockerAccessWinIT.java @@ -123,7 +123,7 @@ private void testDoesNotHave() throws DockerAccessException { } private void testPullImage() throws DockerAccessException { - dockerClient.pullImage(IMAGE, null, null); + dockerClient.pullImage(IMAGE, null, null, 0); assertTrue(hasImage(IMAGE)); } diff --git a/src/test/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClientTest.java b/src/test/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClientTest.java index baf19161d..196be2da0 100644 --- a/src/test/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClientTest.java +++ b/src/test/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClientTest.java @@ -34,7 +34,7 @@ public class DockerAccessWithHcClientTest { @Mocked private Logger mockLogger; - private int pushRetries; + private int pushPullRetries; private String registry; @@ -57,7 +57,7 @@ ApacheHttpClientDelegate createHttpClient(ClientBuilder builder) throws IOExcept @Test public void testPushFailes_noRetry() throws Exception { givenAnImageName("test"); - givenThePushWillFail(0,false); + givenThePushOrPullWillFail(0,false); whenPushImage(); thenImageWasNotPushed(); } @@ -66,7 +66,7 @@ public void testPushFailes_noRetry() throws Exception { public void testRetryPush() throws Exception { givenAnImageName("test"); givenANumberOfRetries(1); - givenThePushWillFail(1, true); + givenThePushOrPullWillFail(1, true); whenPushImage(); thenImageWasPushed(); } @@ -75,11 +75,37 @@ public void testRetryPush() throws Exception { public void testRetriesExceeded() throws Exception { givenAnImageName("test"); givenANumberOfRetries(1); - givenThePushWillFail(1, false); + givenThePushOrPullWillFail(1, false); whenPushImage(); thenImageWasNotPushed(); } + @Test + public void testPullFailes_noRetry() throws Exception { + givenAnImageName("test"); + givenThePushOrPullWillFail(0,false); + whenPullImage(); + thenImageWasNotPulled(); + } + + @Test + public void testRetryPull() throws Exception { + givenAnImageName("test"); + givenANumberOfRetries(1); + givenThePushOrPullWillFail(1, true); + whenPullImage(); + thenImageWasPulled(); + } + + @Test + public void testPullRetriesExceeded() throws Exception { + givenAnImageName("test"); + givenANumberOfRetries(1); + givenThePushOrPullWillFail(1, false); + whenPullImage(); + thenImageWasNotPulled(); + } + @Test public void testLoadImage() { givenAnImageName("test"); @@ -120,7 +146,7 @@ private void givenAnImageName(String imageName) { } private void givenANumberOfRetries(int retries) { - this.pushRetries = retries; + this.pushPullRetries = retries; } private void givenArchiveFile(String archiveFile) { @@ -136,7 +162,7 @@ private void givenCompression(ArchiveCompression compression) { } @SuppressWarnings({"rawtypes", "unchecked"}) - private void givenThePushWillFail(final int retries, final boolean suceedAtEnd) throws IOException { + private void givenThePushOrPullWillFail(final int retries, final boolean suceedAtEnd) throws IOException { new StrictExpectations() {{ int fail = retries + (suceedAtEnd ? 0 : 1); mockDelegate.post(anyString, null, (Map) any, (ResponseHandler) any, 200); @@ -169,9 +195,24 @@ private void thenImageWasPushed() { assertNull(thrownException); } + private void thenImageWasNotPulled() { + assertNotNull(thrownException); + } + + private void thenImageWasPulled() { + assertNull(thrownException); + } + private void whenPushImage() { try { - client.pushImage(imageName, authConfig, registry, pushRetries); + client.pushImage(imageName, authConfig, registry, pushPullRetries); + } catch (Exception e) { + thrownException = e; + } + } + private void whenPullImage() { + try { + client.pullImage(imageName, authConfig, registry, pushPullRetries); } catch (Exception e) { thrownException = e; } diff --git a/src/test/java/io/fabric8/maven/docker/service/RegistryServiceTest.java b/src/test/java/io/fabric8/maven/docker/service/RegistryServiceTest.java index e97218004..7d5f975c1 100644 --- a/src/test/java/io/fabric8/maven/docker/service/RegistryServiceTest.java +++ b/src/test/java/io/fabric8/maven/docker/service/RegistryServiceTest.java @@ -199,7 +199,7 @@ private void thenNoExceptionThrown() { } private void thenImageHasNotBeenPulled() throws DockerAccessException { new Verifications() {{ - docker.pullImage(anyString, (AuthConfig) withNotNull(), anyString); times = 0; + docker.pullImage(anyString, (AuthConfig) withNotNull(), anyString, anyInt); times = 0; }}; } @@ -222,7 +222,7 @@ private void thenImageHasBeenPulled() throws DockerAccessException { private void thenImageHasBeenPulledWithRegistry(final String registry) throws DockerAccessException { new Verifications() {{ - docker.pullImage(imageName, (AuthConfig) withNotNull(), registry); + docker.pullImage(imageName, (AuthConfig) withNotNull(), registry, anyInt); }}; assertTrue(cacheStore.get(imageName) != null); } @@ -240,7 +240,7 @@ private void whenAutoPullImage() { if (registry != null) { registryConfigBuilder.registry(registry); } - registryService.pullImageWithPolicy(imageName, pullManager, registryConfigBuilder.build(), hasImage); + registryService.pullImageWithPolicy(imageName, pullManager, registryConfigBuilder.build(), hasImage, 0); } catch (Exception e) { //e.printStackTrace();