Include Homebrew bin when searching for Docker binaries #3481

lukas-vlcek · 2022-05-31T12:33:58Z

Signed-off-by: Lukáš Vlček lukas.vlcek@aiven.io

Description

When docker and docker-compose are installed using brew then the /bin location is different (compared to manual binaries installation). This commit extends possible locations that are searched when checking if docker is available on Mac family Os machines.

Issues Resolved

Closes #3476

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2022-05-31T13:07:48Z

❌ Gradle Check failure 94b4fe0160c5fbd2c287517bb49b69473571ebc6
Log 5680

Reports 5680

opensearch-ci-bot · 2022-05-31T13:22:31Z

✅ Gradle Check success 84d81208ccb7854f60cb38ef3e123bb0796c309f
Log 5681

Reports 5681

nknize

Thanks for doing this. Just left a quick question if we should make this more homebrew specific? Or do we think there will be other macOS specific urls?

nknize · 2022-06-01T14:45:40Z

buildSrc/src/main/java/org/opensearch/gradle/docker/DockerSupportService.java

@@ -68,17 +69,30 @@ public abstract class DockerSupportService implements BuildService<DockerSupport
    // 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" };

+    // On Mac you can install docker and docker-compose via brew adding more options where the binaries can be located.
+    private static String[] DOCKER_BINARIES_MACOS = Stream.concat(


/opt isn't mac specific. Not sure we need the DOCKER_BINARIES_MACOS. Maybe make this DOCKER_BINARIES_HOMEBREW and add /opt/homebrew/bin/docker, and /home/linuxbrew/.linuxbrew per the homebrew documentation?

@nknize I think this is fair point. Let me update the PR.

@nknize Thinking more about this, depending on how robust we want to make it but why don't we simply use which docker and which docker-compose on Linux and MacOS based systems instead? My gut tells me there can be always some edge case where docker (or other binary in general) can be installed in custom location...

What is the chance that which docker fails (i.e. gives no useful answer) if the docker is installed on the system? (Edit: maybe if the system runs in very light container that does not include which?)

If I understand the code correctly then the only change/implication would be that the paths to search in would not be static class members but they will have to be evaluated only after the ExecOperations execOperations is injected into the SupportService ctor. That shouldn't be a big deal... WDYT?

I like that idea. How about using the static variables as a fall back if which docker does fail? And since OS X is unix based anyway, you should just be able to add the new homebrew paths to the existing DOCKER_BINARIES_UNIX variable instead of a new DOCKER_BINARIES_MACOS?

Hi @nknize,

I updated my PR and I think it is ready for review.

I decided to not include the which functionality into this PR for now because it turns out to be more complicated. Basically the which output can differ based on host systems quite a lot (these are more or less corner cases but we need to deal with them as well, things like "aliases" or "shell functions"). This would lead to more complex parsing of the host output than I like for this PR. I will open a new ticket to enable discussion about it and provide some of the findings I made. In the end I think we will need to add functionality where users will define their own path to container engine anyway (think about people who have both the docker and podman installed at the same time [yes, I do believe we will have podman build compatibility in the future 😄 ]).

For now, let this PR address only the Homebrew locations.

reta · 2022-06-06T17:31:24Z

buildSrc/src/main/java/org/opensearch/gradle/docker/DockerSupportService.java

@@ -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", "/home/linuxbrew/.linuxbrew/bin" };


"/home/linuxbrew/.linuxbrew/bin" this one is unclear: basically we are pointing to the home folder of the linuxbrew user which basically should fail the lookup unless run under linuxbrew user or root. Or I am missing something?

I'm not sure anything will fail here unless the Docker CLI can't be found at all? I think like the PATH env variable it will search folders even if they don't exist and only throw an error if the binaries are not present? Might verify?

Aside from that technical nuance, I think I agree that we should leave the ~linuxbrew user out of the path here. Users can always create a /usr/local/bin softlink to the linuxbrew home directory if this is their configuration.

Thanks @nknize , I think "fail" is indeed too strong, I would rephrase that as "unsuccessful lookup" since we attaching to particular user, thank you.

Perhaps we should add one or two debug messages here as well? If I am not mistaken right now user does not know which location is used.

Perhaps we should add one or two debug messages here as well?

Debug verbosity here would be good! Anything to help users understand what's going on is a great thing. I'll keep an eye on the PR and review when ready.

opensearch-ci-bot · 2022-06-06T17:47:27Z

❌ Gradle Check failure 26fb2e62ee73496b8c7a60f4c2a559d518223b04
Log 5805

Reports 5805

opensearch-ci-bot · 2022-06-06T17:53:40Z

✅ Gradle Check success 1d903357b6a7bb1358b9295a0a80a6a9696474c5
Log 5806

Reports 5806

nknize · 2022-06-29T16:21:41Z

Just checking in on this @lukas-vlcek. I think there was one outstanding item?

Perhaps we should add one or two debug messages here as well? If I am not mistaken right now user does not know which location is used.

lukas-vlcek · 2022-06-29T16:42:27Z

@nknize Yes, thanks for pinging me. I will be finishing this by the end of the week for sure.

github-actions · 2022-07-01T12:55:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/116/
CommitID: cb26cfbaf9598392dc73ed93cf616af408fbaf0b

- This commit adds location of docker binaries when installed via Homebrew on MacOS - Making initialization of the DockerAvailability thread-safe Notice that Homebrew can also be used on Linux, but we are not listing this default bin location for now. This means Linux users using brew to install docker will need to create a suitable alias as a workaround. Closes opensearch-project#3476 Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>

github-actions · 2022-07-01T16:51:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/121/
CommitID: cb26cfbaf9598392dc73ed93cf616af408fbaf0b

lukas-vlcek · 2022-07-01T17:02:09Z

Interesting, why is github-actions bot trying to use the old commitID?
I did force push into my branch and the new commit should be 2385dd3 and not cb26cfb.

prudhvigodithi · 2022-07-01T19:01:33Z

I see .pull_request.head.sha does not give the force commit sha
Found some related issue.
https://gh.neting.ccmunity/t/github-actions-how-to-to-get-pr-merge-commit-sha-in-push/209044
@lukas-vlcek can you do a dummy commit and do a normal push again please?
Also just curious did u use --force-with-lease or --force while pushing ?

lukas-vlcek · 2022-07-01T19:41:27Z

@prudhvigodithi I used just -f.

I added one more commit. Lowered the logging level to DEBUG (so not really a "dummy" commit).

See opensearch-project#3476 Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>

bbarani · 2022-07-01T20:18:47Z

Thanks. The gradle check job has picked up the new commit now.

github-actions · 2022-07-01T20:33:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/124/
CommitID: dfb83f5

prudhvigodithi · 2022-07-01T20:39:15Z

@prudhvigodithi I used just -f

Thanks @lukas-vlcek I have raised this as a bug to investigate on force commits.
Thank you

lukas-vlcek · 2022-07-01T20:44:24Z

Thank you @prudhvigodithi !

dblock · 2022-07-05T16:36:56Z

@nknize you're good with this?

reta · 2022-07-05T16:59:29Z

buildSrc/src/main/java/org/opensearch/gradle/docker/DockerSupportService.java

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


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?

...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?

@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.

opensearch-trigger-bot · 2023-07-13T15:22:22Z

This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days.

opensearch-trigger-bot · 2023-07-21T15:21:29Z

This PR was closed because it has been stalled for 7 days with no activity.

lukas-vlcek requested review from a team and reta as code owners May 31, 2022 12:33

lukas-vlcek force-pushed the 3476_docker_brew branch from 94b4fe0 to 84d8120 Compare May 31, 2022 12:50

nknize requested changes Jun 1, 2022

View reviewed changes

lukas-vlcek force-pushed the 3476_docker_brew branch 2 times, most recently from 26fb2e6 to 1d90335 Compare June 6, 2022 17:21

reta reviewed Jun 6, 2022

View reviewed changes

lukas-vlcek force-pushed the 3476_docker_brew branch from 1d90335 to cb26cfb Compare July 1, 2022 12:29

lukas-vlcek force-pushed the 3476_docker_brew branch from cb26cfb to 2385dd3 Compare July 1, 2022 16:46

lukas-vlcek requested review from nknize and reta July 1, 2022 16:47

Lower the logging level

dfb83f5

See opensearch-project#3476 Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>

prudhvigodithi mentioned this pull request Jul 1, 2022

[Bug]: github.event.pull_request.head.sha used in Gradle check does not identify force commit sha's. opensearch-project/opensearch-build#2292

Open

reta reviewed Jul 5, 2022

View reviewed changes

lukas-vlcek mentioned this pull request Mar 29, 2023

[BUG] Docker & Docker compose installed from Homebrew not recognized #3476

Open

dblock self-assigned this May 11, 2023

opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 13, 2023

opensearch-trigger-bot bot closed this Jul 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include Homebrew bin when searching for Docker binaries #3481

Include Homebrew bin when searching for Docker binaries #3481

lukas-vlcek commented May 31, 2022 •

edited

Loading

opensearch-ci-bot commented May 31, 2022

opensearch-ci-bot commented May 31, 2022

nknize left a comment

nknize Jun 1, 2022

lukas-vlcek Jun 1, 2022

lukas-vlcek Jun 2, 2022 •

edited

Loading

nknize Jun 2, 2022

lukas-vlcek Jun 6, 2022

reta Jun 6, 2022

nknize Jun 6, 2022

reta Jun 6, 2022

lukas-vlcek Jun 6, 2022

nknize Jun 6, 2022

opensearch-ci-bot commented Jun 6, 2022

opensearch-ci-bot commented Jun 6, 2022

nknize commented Jun 29, 2022

lukas-vlcek commented Jun 29, 2022

github-actions bot commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

lukas-vlcek commented Jul 1, 2022

prudhvigodithi commented Jul 1, 2022

lukas-vlcek commented Jul 1, 2022 •

edited

Loading

bbarani commented Jul 1, 2022 •

edited

Loading

github-actions bot commented Jul 1, 2022

prudhvigodithi commented Jul 1, 2022 •

edited

Loading

lukas-vlcek commented Jul 1, 2022

dblock commented Jul 5, 2022

reta Jul 5, 2022 •

edited

Loading

nknize Jul 5, 2022

reta Jul 5, 2022

opensearch-trigger-bot bot commented Jul 13, 2023

opensearch-trigger-bot bot commented Jul 21, 2023

Include Homebrew bin when searching for Docker binaries #3481

Include Homebrew bin when searching for Docker binaries #3481

Conversation

lukas-vlcek commented May 31, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented May 31, 2022

opensearch-ci-bot commented May 31, 2022

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-vlcek Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 6, 2022

opensearch-ci-bot commented Jun 6, 2022

nknize commented Jun 29, 2022

lukas-vlcek commented Jun 29, 2022

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

lukas-vlcek commented Jul 1, 2022

prudhvigodithi commented Jul 1, 2022

lukas-vlcek commented Jul 1, 2022 • edited Loading

bbarani commented Jul 1, 2022 • edited Loading

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

prudhvigodithi commented Jul 1, 2022 • edited Loading

lukas-vlcek commented Jul 1, 2022

dblock commented Jul 5, 2022

reta Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 13, 2023

opensearch-trigger-bot bot commented Jul 21, 2023

lukas-vlcek commented May 31, 2022 •

edited

Loading

lukas-vlcek Jun 2, 2022 •

edited

Loading

lukas-vlcek commented Jul 1, 2022 •

edited

Loading

bbarani commented Jul 1, 2022 •

edited

Loading

prudhvigodithi commented Jul 1, 2022 •

edited

Loading

reta Jul 5, 2022 •

edited

Loading