Skip to content
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

Jump to bottom

Give EnvironmentAndSystemPropertyClientProviderStrategy the highest priority #4472

Merged
merged 3 commits into from
Oct 5, 2021
Merged

Give EnvironmentAndSystemPropertyClientProviderStrategy the highest priority #4472

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d516bf6
Give EnvironmentAndSystemPropertyClientProviderStrategy the highest p…
kiview Sep 20, 2021
fddaa0d
Use filter and findFirst
kiview Sep 23, 2021
7ad6a77
Refactor to using lists
kiview Sep 30, 2021
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
221 changes: 120 additions & 101 deletions core/src/main/java/org/testcontainers/dockerclient/DockerClientProviderStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.rnorth.ducttape.TimeoutException;
import org.rnorth.ducttape.ratelimits.RateLimiter;
Expand Down Expand Up @@ -114,108 +115,33 @@ public static DockerClientProviderStrategy getFirstValidStrategy(List<DockerClie

List<String> configurationFailures = new ArrayList<>();

String dockerClientStrategyClassName = TestcontainersConfiguration.getInstance().getDockerClientStrategyClassName();
return Stream
.concat(
Stream
.of(dockerClientStrategyClassName)
.filter(Objects::nonNull)
.flatMap(it -> {
try {
Class<? extends DockerClientProviderStrategy> strategyClass = (Class) Thread.currentThread().getContextClassLoader().loadClass(it);
return Stream.of(strategyClass.newInstance());
} catch (ClassNotFoundException e) {
log.warn("Can't instantiate a strategy from {} (ClassNotFoundException). " +
"This probably means that cached configuration refers to a client provider " +
"class that is not available in this version of Testcontainers. Other " +
"strategies will be tried instead.", it);
return Stream.empty();
} catch (InstantiationException | IllegalAccessException e) {
log.warn("Can't instantiate a strategy from {}", it, e);
return Stream.empty();
}
})
// Ignore persisted strategy if it's not persistable anymore
.filter(DockerClientProviderStrategy::isPersistable)
.peek(strategy -> log.info("Loaded {} from ~/.testcontainers.properties, will try it first", strategy.getClass().getName())),
strategies
.stream()
.sorted(Comparator.comparing(DockerClientProviderStrategy::getPriority).reversed())
)
.filter(new Predicate<DockerClientProviderStrategy>() {

final Set<Class<? extends DockerClientProviderStrategy>> classes = new HashSet<>();

@Override
public boolean test(DockerClientProviderStrategy dockerClientProviderStrategy) {
return classes.add(dockerClientProviderStrategy.getClass());
}
})
Stream<? extends DockerClientProviderStrategy> configuredDockerClientStrategy = loadConfiguredStrategy();

Stream<DockerClientProviderStrategy> orderedProvidedStrategies = strategies
.stream()
.sorted(Comparator.comparing(DockerClientProviderStrategy::getPriority).reversed());
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we really need these as variables? 😅 They make it hard to follow what is used where, while if they were arguments of Stream.concat, it would be clear that the only stream we process is orderedProvidedStrategies

Copy link
Member

Choose a reason for hiding this comment

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

Also, we don't have to use streams here, may as well go for a List of strategies, something like:

var allStrategies = new ArrayList<DockerClientProviderStrategy>();
allStrategies.add(new EnvironmentAndSystemPropertyClientProviderStrategy());

loadConfiguredStrategy().ifPresent(allStrategies::add);

strategies
              .stream()
              .sorted(Comparator.comparing(DockerClientProviderStrategy::getPriority).reversed())
              .collect(Collectors.toCollection(() -> allStrategies));

Copy link
Member Author

@kiview kiview Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, for me it made it much easier to follow like this, that's why I did it.
Introducing those variables gives natural breaking points while reading the code that summarizes what happened so far.

That being said, this is obviously down to personal preference and style as well, so we can proceed either way. It helped me a lot to understand the code and make the changes.

Edit:
Yes, the lists approach looks good, I just did not want to do bigger changes in the PR itself, so I was just restructuring what we had already.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for the variables for me - it helps convey what these are for

Copy link
Member Author

Choose a reason for hiding this comment

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

@bsideup
So, should we do this additional refactoring in this PR into using lists instead of Streams? Note that we don't have any explicit tests for this class, so we have to be careful to no accidentally introduce a regression.

Copy link
Member

Choose a reason for hiding this comment

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

I'd 👍 using lists as well here as well - it's more easily debuggable when looking at the intermediate states.

Copy link
Member

Choose a reason for hiding this comment

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

@kiview the refactoring to lists is simple and unlikely to introduce any regression, while being a great improvement to the readability / debuggability, so I suggest we do it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, will do this.


Stream<DockerClientProviderStrategy> orderedStrategiesToTryOut = Stream
.concat(
Stream.of(new EnvironmentAndSystemPropertyClientProviderStrategy()),
Stream.concat(
configuredDockerClientStrategy,
orderedProvidedStrategies
));

Predicate<DockerClientProviderStrategy> distinctStrategyClassPredicate = new Predicate<DockerClientProviderStrategy>() {
final Set<Class<? extends DockerClientProviderStrategy>> classes = new HashSet<>();

@Override
public boolean test(DockerClientProviderStrategy dockerClientProviderStrategy) {
return classes.add(dockerClientProviderStrategy.getClass());
}
};

return orderedStrategiesToTryOut
.filter(distinctStrategyClassPredicate)
.filter(DockerClientProviderStrategy::isApplicable)
.flatMap(strategy -> {
try {
DockerClient dockerClient = strategy.getDockerClient();

Info info;
try {
info = Unreliables.retryUntilSuccess(TestcontainersConfiguration.getInstance().getClientPingTimeout(), TimeUnit.SECONDS, () -> {
return strategy.PING_RATE_LIMITER.getWhenReady(() -> {
log.debug("Pinging docker daemon...");
return dockerClient.infoCmd().exec();
});
});
} catch (TimeoutException e) {
IOUtils.closeQuietly(dockerClient);
throw e;
}
log.info("Found Docker environment with {}", strategy.getDescription());
log.debug(
"Transport type: '{}', Docker host: '{}'",
TestcontainersConfiguration.getInstance().getTransportType(),
strategy.getTransportConfig().getDockerHost()
);

log.debug("Checking Docker OS type for {}", strategy.getDescription());
String osType = info.getOsType();
if (StringUtils.isBlank(osType)) {
log.warn("Could not determine Docker OS type");
} else if (!osType.equals("linux")) {
log.warn("{} is currently not supported", osType);
throw new InvalidConfigurationException(osType + " containers are currently not supported");
}

if (strategy.isPersistable()) {
TestcontainersConfiguration.getInstance().updateUserConfig("docker.client.strategy", strategy.getClass().getName());
}

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);

log.debug(failureDescription);
return Stream.empty();
}
})
.flatMap(strategy -> tryOutStrategy(configurationFailures, strategy))
Copy link
Member

Choose a reason for hiding this comment

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

This could also be filter, with tryOutStrategy returning a boolean, right?

Copy link
Member Author

@kiview kiview Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It could. I kept it like this, because I just made structural changes to the code and extracted methods.

Are you thinking about the boolean to make it more testable or to make the intent cleaner? I would agree that the intent of filter is much clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Making the intent clearer was my initial thinking, but it would make the function slightly easier to test in isolation as well.

.findAny()
Copy link
Member

Choose a reason for hiding this comment

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

Sorry: pedantic comment, and I think this comes from the original. Surely this should be findFirst()?

I don't think it's terribly risky, but again it's slightly clearer about the intent.

There's a separate can of wormsopportunities that also opens up when considering that we could try strategies in parallel (but just select the highest priority one that succeeds). I'd not push you to explore that in this PR though :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not pedantic at all, but again, that is how the original code does it and I consciously did not touch it.
And I think you are right, this is semantically wrong, since it will not comply with our priorities (although, since we run the stream non-parallel, it is most likely equivalent to findFirst(), but strictly speaking not guaranteed 😨 ).

I am happy to change at least the method in this PR.

.orElseThrow(() -> {
log.error("Could not find a valid Docker environment. Please check configuration. Attempted configurations were:");
Expand All @@ -229,6 +155,99 @@ public boolean test(DockerClientProviderStrategy dockerClientProviderStrategy) {
});
}

@NotNull
private static Stream<DockerClientProviderStrategy> tryOutStrategy(List<String> configurationFailures, DockerClientProviderStrategy strategy) {
try {
log.debug("Trying out strategy: {}", strategy.getClass().getSimpleName());
DockerClient dockerClient = strategy.getDockerClient();

Info info;
try {
info = Unreliables.retryUntilSuccess(TestcontainersConfiguration.getInstance().getClientPingTimeout(), TimeUnit.SECONDS, () -> {
return strategy.PING_RATE_LIMITER.getWhenReady(() -> {
log.debug("Pinging docker daemon...");
return dockerClient.infoCmd().exec();
});
});
} catch (TimeoutException e) {
IOUtils.closeQuietly(dockerClient);
throw e;
}
log.info("Found Docker environment with {}", strategy.getDescription());
log.debug(
"Transport type: '{}', Docker host: '{}'",
TestcontainersConfiguration.getInstance().getTransportType(),
strategy.getTransportConfig().getDockerHost()
);

log.debug("Checking Docker OS type for {}", strategy.getDescription());
String osType = info.getOsType();
if (StringUtils.isBlank(osType)) {
log.warn("Could not determine Docker OS type");
} else if (!osType.equals("linux")) {
log.warn("{} is currently not supported", osType);
throw new InvalidConfigurationException(osType + " containers are currently not supported");
}

if (strategy.isPersistable()) {
TestcontainersConfiguration.getInstance().updateUserConfig("docker.client.strategy", strategy.getClass().getName());
}

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);

log.debug(failureDescription);
return Stream.empty();
}
}

private static Stream<? extends DockerClientProviderStrategy> loadConfiguredStrategy() {
String configuredDockerClientStrategyClassName = TestcontainersConfiguration.getInstance().getDockerClientStrategyClassName();

return Stream
.of(configuredDockerClientStrategyClassName)
.filter(Objects::nonNull)
.flatMap(it -> {
try {
Class<? extends DockerClientProviderStrategy> strategyClass = (Class) Thread.currentThread().getContextClassLoader().loadClass(it);
return Stream.of(strategyClass.newInstance());
} catch (ClassNotFoundException e) {
log.warn("Can't instantiate a strategy from {} (ClassNotFoundException). " +
"This probably means that cached configuration refers to a client provider " +
"class that is not available in this version of Testcontainers. Other " +
"strategies will be tried instead.", it);
return Stream.empty();
} catch (InstantiationException | IllegalAccessException e) {
log.warn("Can't instantiate a strategy from {}", it, e);
return Stream.empty();
}
})
// Ignore persisted strategy if it's not persistable anymore
.filter(DockerClientProviderStrategy::isPersistable)
.peek(strategy -> log.info("Loaded {} from ~/.testcontainers.properties, will try it first", strategy.getClass().getName()));
}

public static DockerClient getClientForConfig(TransportConfig transportConfig) {
final DockerHttpClient dockerHttpClient;

Expand Down