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

Include Homebrew bin when searching for Docker binaries #3481

Closed
wants to merge 2 commits into from
Closed

Include Homebrew bin when searching for Docker binaries #3481

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2385dd3
Include bin location used by Homebrew for MacOS users
lukas-vlcek May 31, 2022
dfb83f5
Lower the logging level
lukas-vlcek Jul 1, 2022
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
106 changes: 64 additions & 42 deletions buildSrc/src/main/java/org/opensearch/gradle/docker/DockerSupportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Build service for detecting available Docker installation and checking for compatibility with OpenSearch Docker image build
Expand All @@ -66,13 +67,17 @@ public abstract class DockerSupportService implements BuildService<DockerSupport

private static Logger LOGGER = Logging.getLogger(DockerSupportService.class);
// Defines the possible locations of the Docker CLI. These will be searched in order.
private static String[] DOCKER_BINARIES_UNIX = { "/usr/bin/docker", "/usr/local/bin/docker" };
private static String[] LINUX_BIN_LOCATIONS = { "/usr/local/bin", "/usr/bin", "/opt/homebrew/bin" };

private static String[] DOCKER_BINARIES_UNIX = Stream.of(LINUX_BIN_LOCATIONS).map(e -> e.concat("/docker")).toArray(String[]::new);

private static String[] DOCKER_BINARIES_WINDOWS = { System.getenv("PROGRAMFILES") + "\\Docker\\Docker\\resources\\bin\\docker.exe" };

private static String[] DOCKER_BINARIES = Os.isFamily(Os.FAMILY_WINDOWS) ? DOCKER_BINARIES_WINDOWS : DOCKER_BINARIES_UNIX;

private static String[] DOCKER_COMPOSE_BINARIES_UNIX = { "/usr/local/bin/docker-compose", "/usr/bin/docker-compose" };
private static String[] DOCKER_COMPOSE_BINARIES_UNIX = Stream.of(LINUX_BIN_LOCATIONS)
.map(e -> e.concat("/docker-compose"))
.toArray(String[]::new);

private static String[] DOCKER_COMPOSE_BINARIES_WINDOWS = {
System.getenv("PROGRAMFILES") + "\\Docker\\Docker\\resources\\bin\\docker-compose.exe" };
Expand All @@ -84,7 +89,7 @@ public abstract class DockerSupportService implements BuildService<DockerSupport
private static final Version MINIMUM_DOCKER_VERSION = Version.fromString("17.05.0");

private final ExecOperations execOperations;
private DockerAvailability dockerAvailability;
private volatile DockerAvailability dockerAvailability;

@Inject
public DockerSupportService(ExecOperations execOperations) {
Expand All @@ -98,48 +103,61 @@ public DockerSupportService(ExecOperations execOperations) {
*/
public DockerAvailability getDockerAvailability() {
if (this.dockerAvailability == null) {
String dockerPath = null;
Result lastResult = null;
Version version = null;
boolean isVersionHighEnough = false;
boolean isComposeAvailable = false;

// Check if the Docker binary exists
final Optional<String> dockerBinary = getDockerPath();
if (isExcludedOs() == false && dockerBinary.isPresent()) {
dockerPath = dockerBinary.get();

// Since we use a multi-stage Docker build, check the Docker version meets minimum requirement
lastResult = runCommand(dockerPath, "version", "--format", "{{.Server.Version}}");

if (lastResult.isSuccess()) {
version = Version.fromString(lastResult.stdout.trim(), Version.Mode.RELAXED);

isVersionHighEnough = version.onOrAfter(MINIMUM_DOCKER_VERSION);

if (isVersionHighEnough) {
// Check that we can execute a privileged command
lastResult = runCommand(dockerPath, "images");
// Gradle BuildService implementations must be thread-safe
synchronized (this) {
Copy link
Collaborator

@reta reta Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is interesting, I think we discussed that @lukas-vlcek offline, the volatile + synchronized definitely make sense to keep implementation thread safe, I am not sure it is worth doing that here: at worst, the getDockerAvailability() may be called a few times, but this is no data race and the outcome would be the same (since the environment is the same).

@dblock wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

...this is no data race and the outcome would be the same

I'm not as familiar with the distro tests here. Does this logic path need to be atomic? Could not synchronizing be trappy such that gradle parallel execution cause a concurrency issue where one build thread changes state and impacts another? Not just with distro tests today but with future contributions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nknize the logic is atomic (the state is a single pointer), the Docker detection logic could run a few times (potentially) which is the only issue I see here, marking getDockerAvailability() as synchronized would address that (no need for volatile or double check locking) but I have never seen an issue with this service in any builds.

String dockerPath = null;
Result lastResult = null;
Version version = null;
boolean isVersionHighEnough = false;
boolean isComposeAvailable = false;

// Check if the Docker binary exists
final Optional<String> dockerBinary = getDockerPath();
if (isExcludedOs() == false && dockerBinary.isPresent()) {
dockerPath = dockerBinary.get();

// Since we use a multi-stage Docker build, check the Docker version meets minimum requirement
lastResult = runCommand(dockerPath, "version", "--format", "{{.Server.Version}}");
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Docker binary found at: {}", dockerPath);
LOGGER.debug("Docker version check: {}", lastResult.toString());
}

// If docker all checks out, see if docker-compose is available and working
Optional<String> composePath = getDockerComposePath();
if (lastResult.isSuccess() && composePath.isPresent()) {
isComposeAvailable = runCommand(composePath.get(), "version").isSuccess();
if (lastResult.isSuccess()) {
version = Version.fromString(lastResult.stdout.trim(), Version.Mode.RELAXED);

isVersionHighEnough = version.onOrAfter(MINIMUM_DOCKER_VERSION);

if (isVersionHighEnough) {
// Check that we can execute a privileged command
lastResult = runCommand(dockerPath, "images");

// If docker all checks out, see if docker-compose is available and working
Optional<String> composePath = getDockerComposePath();
if (lastResult.isSuccess() && composePath.isPresent()) {
Result composeCmdCheck = runCommand(composePath.get(), "version");
isComposeAvailable = composeCmdCheck.isSuccess();
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Docker-compose binary found at: {}", composePath);
LOGGER.debug("Docker-compose version check: {}", composeCmdCheck.toString());
}
}
}
}
}
}

boolean isAvailable = isVersionHighEnough && lastResult != null && lastResult.isSuccess();

this.dockerAvailability = new DockerAvailability(
isAvailable,
isComposeAvailable,
isVersionHighEnough,
dockerPath,
version,
lastResult
);
boolean isAvailable = isVersionHighEnough && lastResult != null && lastResult.isSuccess();

// "Double checked locking"
if (this.dockerAvailability == null) this.dockerAvailability = new DockerAvailability(
isAvailable,
isComposeAvailable,
isVersionHighEnough,
dockerPath,
version,
lastResult
);
}
}

return this.dockerAvailability;
Expand Down Expand Up @@ -272,6 +290,10 @@ static Map<String, String> parseOsRelease(final List<String> osReleaseLines) {
return values;
}

private Optional<String> findFirstPath(String[] paths) {
return Arrays.asList(paths).stream().filter(path -> new File(path).exists()).findFirst();
}

/**
* Searches the entries in {@link #DOCKER_BINARIES} for the Docker CLI. This method does
* not check whether the Docker installation appears usable, see {@link #getDockerAvailability()}
Expand All @@ -281,7 +303,7 @@ static Map<String, String> parseOsRelease(final List<String> osReleaseLines) {
*/
private Optional<String> getDockerPath() {
// Check if the Docker binary exists
return Arrays.asList(DOCKER_BINARIES).stream().filter(path -> new File(path).exists()).findFirst();
return findFirstPath(DOCKER_BINARIES);
}

/**
Expand All @@ -292,7 +314,7 @@ private Optional<String> getDockerPath() {
*/
private Optional<String> getDockerComposePath() {
// Check if the Docker binary exists
return Arrays.asList(DOCKER_COMPOSE_BINARIES).stream().filter(path -> new File(path).exists()).findFirst();
return findFirstPath(DOCKER_COMPOSE_BINARIES);
}

private void throwDockerRequiredException(final String message) {
Expand Down