From 2f2cb0a49824879ea7643771bdf702045d49ee21 Mon Sep 17 00:00:00 2001 From: Ewen Cluley Date: Thu, 25 Mar 2021 14:17:42 +0000 Subject: [PATCH 1/2] Avoid race condition when creating port bindings On container startup, sometimes the port information retrieved from the docker API is incomplete with port details being empty arrays, e.g. Ports:{"2525/tcp":[]}. This causes an IndexOutOfBoundsException to be thrown as the createPortBindings method assumes there will be at least one entry in the mapping. In these cases we should retry the API call to get the more complete port information. Signed-off-by: Ewen Cluley --- doc/changelog.md | 1 + pom.xml | 6 +++ .../maven/docker/model/ContainerDetails.java | 8 +++- .../docker/model/PortBindingException.java | 9 ++++ .../maven/docker/service/RunService.java | 27 +++++++++--- .../docker/model/ContainerDetailsTest.java | 16 +++++++ .../maven/docker/service/RunServiceTest.java | 43 ++++++++++++++++++- 7 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 src/main/java/io/fabric8/maven/docker/model/PortBindingException.java diff --git a/doc/changelog.md b/doc/changelog.md index 09d0df35d..3b9c7c5eb 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -13,6 +13,7 @@ - Add a skipPom parameter, skipping a project if packaging is pom ([1388](https://github.com/fabric8io/docker-maven-plugin/pull/1388)) - Add support for config to specify isolation technology for container ([1376](https://github.com/fabric8io/docker-maven-plugin/pull/1376)) - Add Docker build cache friendly example utilizing Spring Boot Layered JAR and Maven Assembly Plugin ([1412](https://github.com/fabric8io/docker-maven-plugin/pull/1412)) + - Retry port mapping to avoid race condition where complete port information may not be initially available in Docker Engine 20.10.5 ([1447](https://github.com/fabric8io/docker-maven-plugin/pull/1447)) * **0.34.1** (2020-09-27) - Fix NPE with "skipPush" and no build configuration given ([#1381](https://github.com/fabric8io/docker-maven-plugin/issues/1381)) diff --git a/pom.xml b/pom.xml index ffd638a98..0a62704ca 100644 --- a/pom.xml +++ b/pom.xml @@ -183,6 +183,12 @@ plexus-interpolation 1.24 + + + net.jodah + failsafe + 2.4.0 + diff --git a/src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java b/src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java index 3e851568c..9b556b267 100644 --- a/src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java +++ b/src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java @@ -8,6 +8,8 @@ import java.util.Set; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; +import com.google.gson.JsonArray; import com.google.gson.JsonObject; @@ -196,8 +198,12 @@ private Map createPortBindings(JsonObject ports) { if (ports.get(port).isJsonNull()) { addPortMapping(port, (PortBinding) null, portBindings); } else { + JsonArray hostMappings = ports.getAsJsonArray(port); + if (hostMappings.isJsonNull() || hostMappings.size() == 0) { + throw new PortBindingException(port, ports); + } // use the first entry in the array - JsonObject hostConfig = ports.getAsJsonArray(port).get(0).getAsJsonObject(); + JsonObject hostConfig = hostMappings.get(0).getAsJsonObject(); addPortMapping(port, hostConfig, portBindings); } } diff --git a/src/main/java/io/fabric8/maven/docker/model/PortBindingException.java b/src/main/java/io/fabric8/maven/docker/model/PortBindingException.java new file mode 100644 index 000000000..e911a7d34 --- /dev/null +++ b/src/main/java/io/fabric8/maven/docker/model/PortBindingException.java @@ -0,0 +1,9 @@ +package io.fabric8.maven.docker.model; + +import com.google.gson.JsonObject; + +public class PortBindingException extends RuntimeException { + public PortBindingException(String port, JsonObject portDetails) { + super(String.format("Failed to create binding for port '%s'. Container ports: %s", port, portDetails)); + } +} diff --git a/src/main/java/io/fabric8/maven/docker/service/RunService.java b/src/main/java/io/fabric8/maven/docker/service/RunService.java index 6a74cfee1..61faaecc2 100644 --- a/src/main/java/io/fabric8/maven/docker/service/RunService.java +++ b/src/main/java/io/fabric8/maven/docker/service/RunService.java @@ -20,6 +20,7 @@ */ import java.io.File; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -55,6 +56,7 @@ import io.fabric8.maven.docker.model.ContainerDetails; import io.fabric8.maven.docker.model.ExecDetails; import io.fabric8.maven.docker.model.Network; +import io.fabric8.maven.docker.model.PortBindingException; import io.fabric8.maven.docker.util.ContainerNamingUtil; import io.fabric8.maven.docker.util.EnvUtil; import io.fabric8.maven.docker.util.GavLabel; @@ -62,6 +64,8 @@ import io.fabric8.maven.docker.util.StartOrderResolver; import io.fabric8.maven.docker.wait.WaitTimeoutException; import io.fabric8.maven.docker.wait.WaitUtil; +import net.jodah.failsafe.Failsafe; +import net.jodah.failsafe.RetryPolicy; /** @@ -464,12 +468,23 @@ private void startContainer(ImageConfiguration imageConfig, String id, GavLabel } private void updateMappedPortsAndAddresses(String containerId, PortMapping mappedPorts) throws DockerAccessException { - Container container = queryService.getMandatoryContainer(containerId); - if (container.isRunning()) { - mappedPorts.updateProperties(container.getPortBindings()); - } else { - log.warn("Container %s is not running anymore, can not extract dynamic ports",containerId); - } + RetryPolicy retryPolicy = new RetryPolicy() + .withMaxAttempts(10) + .withDelay(Duration.ofMillis(250)) + .handle(PortBindingException.class) + .onFailedAttempt(f -> log.debug("Failed to update mapped ports for container %s (attempt %d), retrying", + containerId, f.getAttemptCount())) + .onRetriesExceeded(f -> log.warn("Failed to update mapped ports for container %s after %d retries", + containerId, f.getAttemptCount())); + + Failsafe.with(retryPolicy).run(() -> { + Container container = queryService.getMandatoryContainer(containerId); + if (container.isRunning()) { + mappedPorts.updateProperties(container.getPortBindings()); + } else { + log.warn("Container %s is not running anymore, can not extract dynamic ports", containerId); + } + }); } private void shutdown(ContainerTracker.ContainerShutdownDescriptor descriptor, boolean keepContainer, boolean removeVolumes) diff --git a/src/test/java/io/fabric8/maven/docker/model/ContainerDetailsTest.java b/src/test/java/io/fabric8/maven/docker/model/ContainerDetailsTest.java index dc42e0bc2..8ff3c986f 100644 --- a/src/test/java/io/fabric8/maven/docker/model/ContainerDetailsTest.java +++ b/src/test/java/io/fabric8/maven/docker/model/ContainerDetailsTest.java @@ -133,6 +133,13 @@ public void testCreateContainer() throws Exception { thenValidateContainer(); } + @Test(expected = PortBindingException.class) + public void testCreateContainerWithEmptyPortBindings() throws Exception { + givenAContainerWithUnboundPorts(); + whenCreateContainer(); + container.getPortBindings(); + } + private JsonArray createHostIpAndPort(int port, String ip) { JsonObject object = new JsonObject(); @@ -172,6 +179,15 @@ private void givenAContainerWithMappedPorts() { private void givenAContainerWithoutPorts() { json.add(ContainerDetails.NETWORK_SETTINGS, new JsonObject()); } + + private void givenAContainerWithUnboundPorts() { + JsonObject ports = new JsonObject(); + ports.add("80/tcp", new JsonArray()); + ports.add("52/udp", new JsonArray()); + JsonObject networkSettings = new JsonObject(); + networkSettings.add(ContainerDetails.PORTS, ports); + json.add(ContainerDetails.NETWORK_SETTINGS, networkSettings); + } private void givenContainerData() { json.addProperty(ContainerDetails.CREATED, "2015-01-06T15:47:31.485331387Z"); diff --git a/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java b/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java index c3377c675..6343ac7bd 100644 --- a/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java +++ b/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java @@ -1,10 +1,14 @@ package io.fabric8.maven.docker.service; +import com.google.common.collect.ImmutableMap; +import com.google.gson.Gson; import com.google.gson.JsonObject; import io.fabric8.maven.docker.access.ExecException; import io.fabric8.maven.docker.config.StopMode; import io.fabric8.maven.docker.config.VolumeConfiguration; +import io.fabric8.maven.docker.model.Container; +import io.fabric8.maven.docker.model.PortBindingException; import io.fabric8.maven.docker.util.GavLabel; import org.apache.commons.io.IOUtils; import org.apache.maven.execution.MavenSession; @@ -18,6 +22,7 @@ import java.lang.reflect.Field; import java.util.Arrays; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -42,11 +47,11 @@ import io.fabric8.maven.docker.util.Logger; import mockit.Expectations; import mockit.Mocked; +import mockit.Verifications; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; /** * This test need to be refactored. In fact, testing Mojos must be setup correctly at all. Blame on me that there are so @@ -336,6 +341,42 @@ public void testStopModeWithKill() throws DockerAccessException, ExecException { runService.stopContainer(container, createImageConfigWithStopMode(StopMode.kill), false, false); assertTrue("No wait", System.currentTimeMillis() - start < SHUTDOWN_WAIT); } + + @Test + public void retryIfInsufficientPortBindingInformation( + @Mocked Container container, + @Mocked ImageConfiguration imageConfiguration, + @Mocked PortMapping portMapping + ) throws DockerAccessException { + new Expectations() {{ + docker.createContainer(withAny(new ContainerCreateConfig("img")), anyString); result = "containerId"; + portMapping.needsPropertiesUpdate(); result = true; + queryService.getMandatoryContainer("containerId"); result = container; times = 2; + container.isRunning(); result = true; + container.getPortBindings(); result = new PortBindingException("5432/tcp", new Gson().fromJson("{\"5432/tcp\": []}", JsonObject.class)); + returns(ImmutableMap.of("5432/tcp", new Container.PortBinding(56741, "0.0.0.0")), ImmutableMap.of("5432/tcp", new Container.PortBinding(56741, "0.0.0.0"))); + }}; + String containerId = runService.createAndStartContainer(imageConfiguration, portMapping, new GavLabel("Im:A:Test"), properties, getBaseDirectory(), "blah", new Date()); + new Verifications() {{ + assertEquals("containerId", containerId); + }}; + } + + @Test(expected = PortBindingException.class) + public void failAfterRetryingIfInsufficientPortBindingInformation( + @Mocked Container container, + @Mocked ImageConfiguration imageConfiguration, + @Mocked PortMapping portMapping + ) throws DockerAccessException { + new Expectations() {{ + docker.createContainer(withAny(new ContainerCreateConfig("img")), anyString); result = "containerId"; + portMapping.needsPropertiesUpdate(); result = true; + queryService.getMandatoryContainer("containerId"); result = container; times = 10; + container.isRunning(); result = true; + container.getPortBindings(); result = new PortBindingException("5432/tcp", new Gson().fromJson("{\"5432/tcp\": []}", JsonObject.class)); + }}; + runService.createAndStartContainer(imageConfiguration, portMapping, new GavLabel("Im:A:Test"), properties, getBaseDirectory(), "blah", new Date()); + } private ImageConfiguration createImageConfig(int wait, int kill) { return new ImageConfiguration.Builder() From 4b7c1efb52e8232b5a689393334a5a122c506a00 Mon Sep 17 00:00:00 2001 From: Ewen Cluley Date: Tue, 30 Mar 2021 11:17:20 +0100 Subject: [PATCH 2/2] Use exponential backoff when retrying from 10ms to 100ms --- .../java/io/fabric8/maven/docker/service/RunService.java | 5 +++-- .../java/io/fabric8/maven/docker/service/RunServiceTest.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/fabric8/maven/docker/service/RunService.java b/src/main/java/io/fabric8/maven/docker/service/RunService.java index 61faaecc2..989be60d5 100644 --- a/src/main/java/io/fabric8/maven/docker/service/RunService.java +++ b/src/main/java/io/fabric8/maven/docker/service/RunService.java @@ -21,6 +21,7 @@ import java.io.File; import java.time.Duration; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -469,8 +470,8 @@ private void startContainer(ImageConfiguration imageConfig, String id, GavLabel private void updateMappedPortsAndAddresses(String containerId, PortMapping mappedPorts) throws DockerAccessException { RetryPolicy retryPolicy = new RetryPolicy() - .withMaxAttempts(10) - .withDelay(Duration.ofMillis(250)) + .withMaxAttempts(20) + .withBackoff(10, 100, ChronoUnit.MILLIS) .handle(PortBindingException.class) .onFailedAttempt(f -> log.debug("Failed to update mapped ports for container %s (attempt %d), retrying", containerId, f.getAttemptCount())) diff --git a/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java b/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java index 6343ac7bd..7d2fc405d 100644 --- a/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java +++ b/src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java @@ -371,7 +371,7 @@ public void failAfterRetryingIfInsufficientPortBindingInformation( new Expectations() {{ docker.createContainer(withAny(new ContainerCreateConfig("img")), anyString); result = "containerId"; portMapping.needsPropertiesUpdate(); result = true; - queryService.getMandatoryContainer("containerId"); result = container; times = 10; + queryService.getMandatoryContainer("containerId"); result = container; times = 20; container.isRunning(); result = true; container.getPortBindings(); result = new PortBindingException("5432/tcp", new Gson().fromJson("{\"5432/tcp\": []}", JsonObject.class)); }};