-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support prioritization of DockerClientProviderStrategies #362
Changes from 1 commit
49345d1
411c2da
851c784
42e38b6
cb27292
7a72d3b
4ad4607
757ac77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,16 +5,20 @@ | |
import com.github.dockerjava.core.DockerClientConfig; | ||
import com.github.dockerjava.netty.NettyDockerCmdExecFactory; | ||
import com.google.common.base.Throwables; | ||
import org.apache.commons.io.IOUtils; | ||
import org.jetbrains.annotations.Nullable; | ||
import org.rnorth.ducttape.TimeoutException; | ||
import org.rnorth.ducttape.ratelimits.RateLimiter; | ||
import org.rnorth.ducttape.ratelimits.RateLimiterBuilder; | ||
import org.rnorth.ducttape.unreliables.Unreliables; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Comparator; | ||
import java.util.List; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.stream.Stream; | ||
|
||
/** | ||
* Mechanism to find a viable Docker client configuration according to the host system environment. | ||
|
@@ -39,6 +43,14 @@ public abstract class DockerClientProviderStrategy { | |
*/ | ||
public abstract String getDescription(); | ||
|
||
protected boolean isApplicable() { | ||
return true; | ||
} | ||
|
||
protected int getPriority() { | ||
return 0; | ||
} | ||
|
||
protected static final Logger LOGGER = LoggerFactory.getLogger(DockerClientProviderStrategy.class); | ||
|
||
/** | ||
|
@@ -49,45 +61,51 @@ public abstract class DockerClientProviderStrategy { | |
public static DockerClientProviderStrategy getFirstValidStrategy(List<DockerClientProviderStrategy> strategies) { | ||
List<String> configurationFailures = new ArrayList<>(); | ||
|
||
for (DockerClientProviderStrategy strategy : strategies) { | ||
try { | ||
strategy.test(); | ||
LOGGER.info("Looking for Docker environment. Tried {}", strategy.getDescription()); | ||
return strategy; | ||
} catch (Exception | ExceptionInInitializerError | NoClassDefFoundError e) { | ||
@Nullable String throwableMessage = e.getMessage(); | ||
@SuppressWarnings("ThrowableResultOfMethodCallIgnored") | ||
Throwable rootCause = Throwables.getRootCause(e); | ||
@Nullable String rootCauseMessage = rootCause.getMessage(); | ||
|
||
String failureDescription; | ||
if (throwableMessage != null && throwableMessage.equals(rootCauseMessage)) { | ||
failureDescription = String.format("%s: failed with exception %s (%s)", | ||
strategy.getClass().getSimpleName(), | ||
e.getClass().getSimpleName(), | ||
throwableMessage); | ||
} else { | ||
failureDescription = String.format("%s: failed with exception %s (%s). Root cause %s (%s)", | ||
strategy.getClass().getSimpleName(), | ||
e.getClass().getSimpleName(), | ||
throwableMessage, | ||
rootCause.getClass().getSimpleName(), | ||
rootCauseMessage | ||
); | ||
} | ||
configurationFailures.add(failureDescription); | ||
|
||
LOGGER.debug(failureDescription); | ||
} | ||
} | ||
|
||
LOGGER.error("Could not find a valid Docker environment. Please check configuration. Attempted configurations were:"); | ||
for (String failureMessage : configurationFailures) { | ||
LOGGER.error(" " + failureMessage); | ||
} | ||
LOGGER.error("As no valid configuration was found, execution cannot continue"); | ||
|
||
throw new IllegalStateException("Could not find a valid Docker environment. Please see logs and check configuration"); | ||
return strategies.stream() | ||
.filter(DockerClientProviderStrategy::isApplicable) | ||
.sorted(Comparator.comparing(DockerClientProviderStrategy::getPriority).reversed()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only slight concern with determining the order at run time is that it means there's no one place you can look in the code to see the sequence. Now people will have to look at the implementations of all the strategies to figure out what order they'll run in. Having said that, this does allow us to do dynamic prioritisation. So, e.g. as I mentioned on Slack, caching last known good config somewhere. Is that the kind of thing you had in mind too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore that - the PR title gives it away 🤦♂️ 👍 ! |
||
.flatMap(strategy -> { | ||
try { | ||
strategy.test(); | ||
LOGGER.info("Found Docker environment with {}", strategy.getDescription()); | ||
return Stream.of(strategy); | ||
} catch (Exception | ExceptionInInitializerError | NoClassDefFoundError e) { | ||
@Nullable String throwableMessage = e.getMessage(); | ||
@SuppressWarnings("ThrowableResultOfMethodCallIgnored") | ||
Throwable rootCause = Throwables.getRootCause(e); | ||
@Nullable String rootCauseMessage = rootCause.getMessage(); | ||
|
||
String failureDescription; | ||
if (throwableMessage != null && throwableMessage.equals(rootCauseMessage)) { | ||
failureDescription = String.format("%s: failed with exception %s (%s)", | ||
strategy.getClass().getSimpleName(), | ||
e.getClass().getSimpleName(), | ||
throwableMessage); | ||
} else { | ||
failureDescription = String.format("%s: failed with exception %s (%s). Root cause %s (%s)", | ||
strategy.getClass().getSimpleName(), | ||
e.getClass().getSimpleName(), | ||
throwableMessage, | ||
rootCause.getClass().getSimpleName(), | ||
rootCauseMessage | ||
); | ||
} | ||
configurationFailures.add(failureDescription); | ||
|
||
LOGGER.debug(failureDescription); | ||
return Stream.empty(); | ||
} | ||
}) | ||
.findAny() | ||
.orElseThrow(() -> { | ||
LOGGER.error("Could not find a valid Docker environment. Please check configuration. Attempted configurations were:"); | ||
for (String failureMessage : configurationFailures) { | ||
LOGGER.error(" " + failureMessage); | ||
} | ||
LOGGER.error("As no valid configuration was found, execution cannot continue"); | ||
|
||
return new IllegalStateException("Could not find a valid Docker environment. Please see logs and check configuration"); | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -105,13 +123,18 @@ protected DockerClient getClientForConfig(DockerClientConfig config) { | |
} | ||
|
||
protected void ping(DockerClient client, int timeoutInSeconds) { | ||
Unreliables.retryUntilSuccess(timeoutInSeconds, TimeUnit.SECONDS, () -> { | ||
return PING_RATE_LIMITER.getWhenReady(() -> { | ||
LOGGER.debug("Pinging docker daemon..."); | ||
client.pingCmd().exec(); | ||
return true; | ||
try { | ||
Unreliables.retryUntilSuccess(timeoutInSeconds, TimeUnit.SECONDS, () -> { | ||
return PING_RATE_LIMITER.getWhenReady(() -> { | ||
LOGGER.debug("Pinging docker daemon..."); | ||
client.pingCmd().exec(); | ||
return true; | ||
}); | ||
}); | ||
}); | ||
} catch (TimeoutException e) { | ||
IOUtils.closeQuietly(client); | ||
throw e; | ||
} | ||
} | ||
|
||
public String getDockerHostIpAddress() { | ||
|
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.
Maybe worth mentioning in a comment how the priority is used (i.e. highest to lowest) for avoidance of doubt.