Consolidate docker availability build logic #52548

mark-vieira · 2020-02-20T01:04:40Z

This pull request consolidates and standardizes the mechanisms by which we determine the availability of a compatible Docker installation on the build host, as well as conditionally avoid certain tasks based on that availability. The latter has been refactored such that it is always deferred to task execution or graph calculation time so that in the majority case, when a build does not request any docker-related tasks, we don't make any such attempt to determine if Docker exists on the system.

Prior to this refactoring we have 3 main sources of docker availability logic:

The logic in DockerUtils which is quite robust and actually checks that Docker exists, can be executed with privileged commands and meets a minimum version requirement. This was used only for the purpose of determining whether we could build the Docker distributions.
Some logic in BuildPlugin for throwing an exception if any task declared it required a Docker installation meeting the requirements defined in (1). Again, only the :distribution:docker project leveraged this.
Logic in DistroTestPlugin to determine whether or not Docker distribution tests could be executed. This used a different, and more simple implementation than DockerUtils, only assuming Windows never works, Mac always works, and Linux works, unless it's a variant that has been explicitly blacklisted in CI.
The last bit existed in TestFixturesPlugins for the purposes of determining whether docker-compose exists on the system. This was another relatively simple implementation, simply looking for the executable from a known list of locations.

This pull request solves some of the inconsistencies listed above via:

The introduction of a single DockerSupportService that is used in all instances where a determination on whether a compatible Docker installation is needed. This includes building our Docker images, testing them and spinning up Docker-based test fixtures.
Some changes to ElasticsearchDistribution such that we can depend on a "lenient" distribution. If a distribution sets required = false and Docker is unavailable we simply skip building it. This allows testing that uses a locally build Docker distribution to be gracefully skipped in the absence of a Docker installation. This was previously done by checking for Docker availability at configuration time and omitting task dependencies (or entire tasks altogether) to avoid trying to build the Docker images.
Refactoring of any direct dependencies on Docker build tasks to use the DistributionDownloadPlugin and lenient distributions described in (2).

Signed-off-by: Mark Vieira <[email protected]>

elasticmachine · 2020-02-20T01:04:57Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

pugnascotia · 2020-02-20T09:36:37Z

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

+        throwDockerRequiredException(message);
+    }
+
+    private boolean isBlacklistedOs() {


We should stay away from terms like "blacklist" and "whitelist".

Suggested change

private boolean isBlacklistedOs() {

private boolean isExcludedOs() {

Agreed. I've renamed this.

pugnascotia · 2020-02-20T09:45:17Z

distribution/docker/build.gradle

+      flavor = distroFlavor
+      type = 'docker'
+      version = VersionProperties.getElasticsearch()
+      required = false // This ensures we skip this testing if Docker is unavailable


Does this only skip testing? Docker is required for a successful overall build on supported OSs, no?

This skips building the upstream docker images that are consumed by the tests. I realize now that comment isn't great, I'll improve it.

I updated the comment text to make it clear this simply makes building the images optional. The logic for actually skipping the tests themselves lives in TestFixturesPlugin.

pugnascotia

Just one thing I'd like to see changed and one question, but LGTM. This approach looks a lot tidier.

mark-vieira · 2020-02-20T17:32:21Z

@pugnascotia could you provide some context on the necessity of .ci/dockerOnLinuxExclusions? Are those machines with Docker installed but something is just borked with it? Would using your logic to actually run a command, test the version, etc be enough to rule out "incompatible" agents or do we still need to explicit exclusions?

Signed-off-by: Mark Vieira <[email protected]>

rjernst

This looks great. Just a few comments in addition to the terminology change of "required" to "failifUnavailable" we discussed offline.

rjernst · 2020-02-20T05:45:39Z

buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportPlugin.java

+import java.util.stream.Collectors;
+
+/**
+ * <p>Plugin providing {@link DockerSupportService} for detecting Docker installations and determining requirements for Docker-based


nit: This shouldn't start with <p>, as the first line of a javadoc is the short description.

rjernst · 2020-02-20T05:47:48Z

buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportPlugin.java

+ * <p>Plugin providing {@link DockerSupportService} for detecting Docker installations and determining requirements for Docker-based
+ * Elasticsearch build tasks.</p>
+ *
+ * <p>Additionally registers a task graph listener used to assert a compatible Docker installation exists when task requiring Docker are


I know it is whacky, but <p> should basically be used as a paragraph break, without a closing tag, in javadocs. From oracle docs (https://www.oracle.com/technetwork/articles/java/index-137868.html):

If you have more than one paragraph in the doc comment, separate the paragraphs with a
paragraph tag

Stupid javadocs. Done.

rjernst · 2020-02-20T05:51:05Z

buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportPlugin.java

+        project.getGradle().getTaskGraph().whenReady(graph -> {
+            List<String> dockerTasks = graph.getAllTasks().stream().filter(task -> {
+                ExtraPropertiesExtension ext = task.getExtensions().getExtraProperties();
+                return ext.has(REQUIRES_DOCKER_ATTRIBUTE) && ext.get(REQUIRES_DOCKER_ATTRIBUTE).equals("true");


Why are we setting the property to a boolean string instead of an actual boolean?

We were indeed setting to a boolean but were checking for a String. I've fixed this.

rjernst · 2020-02-20T05:55:52Z

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

+     *
+     * @throws GradleException if Docker is not available. The exception message gives the reason.
+     */
+    void assertDockerIsAvailable(List<String> tasks) {


nit: since we are using asserts, maybe call use check or ensure terminology?

rjernst · 2020-02-20T06:02:01Z

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

+
+    private void throwDockerRequiredException(final String message, Exception e) {
+        throw new GradleException(
+            message + "\nyou can address this by attending to the reported issue, " + "removing the offending tasks from being executed.",


removing -> or removing?

I've reworded this message.

rjernst · 2020-02-20T06:05:10Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/DistroTestPlugin.java

        return project.getTasks().register(destructiveDistroTestTaskName(distribution), Test.class, t -> {
+            // Disable Docker distribution tests unless a Docker installation is available
+            t.onlyIf(t2 -> distribution.getType() != Type.DOCKER || dockerSupport.get().getDockerAvailability().isAvailable);


Why onlyIf here instead of using the requiresDocker property?

As discussed, using requiresDocker is strict. It fails the build if docker is unavailable. We want to simply skip that task in that scenario.

Signed-off-by: Mark Vieira <[email protected]>

mark-vieira · 2020-02-20T19:41:05Z

@rjernst I believe I've addressed you feedback.

rjernst

LGTM

rjernst · 2020-02-20T20:51:31Z

buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchDistribution.java

@@ -270,6 +276,11 @@ void finalizeValues() {
                    "platform not allowed for elasticsearch distribution [" + name + "] of type [" + getType() + "]"
                );
            }
+            if (getType() == Type.DOCKER && bundledJdk.isPresent()) {
+                throw new IllegalArgumentException(
+                    "bundledJdk not allowed for elasticsearch distribution [" + name + "] of type [docker]"


bundledJdk setting?

or property or something like that? as it is written it sounds like docker can't use the bundledJdk, when in face it only uses it.

Signed-off-by: Mark Vieira <[email protected]>

pugnascotia · 2020-02-20T21:04:30Z

@pugnascotia could you provide some context on the necessity of .ci/dockerOnLinuxExclusions? Are those machines with Docker installed but something is just borked with it? Would using your logic to actually run a command, test the version, etc be enough to rule out "incompatible" agents or do we still need to explicit exclusions?

There were / are some CI machines that can't run Docker for better or worse reasons. I settled on assuming Docker is available unless we know otherwise. I didn't want to rely on detection in case, say, a Packer build went wrong and failed to include Docker or broke it.

mark-vieira · 2020-02-20T21:11:05Z

I didn't want to rely on detection in case, say, a Packer build went wrong and failed to include Docker or broke it.

Right, I guess my thought is that now that we look for Docker, and try and execute it, we could just rely on that logic for the most part. The risk there though is that should all our Packer builds suddenly stop installing Docker, we'd simply start silently skipping all these tests.

rjernst · 2020-02-20T21:32:45Z

I would rather keep the exclusion list until we can expect docker on all CI images at some future time. We should expect docker to work, and error if it doesn't, except in these edge cases.

mark-vieira · 2020-02-20T21:38:58Z

I would rather keep the exclusion list until we can expect docker on all CI images at some future time. We should expect docker to work, and error if it doesn't, except in these edge cases.

Works for me. It's not particularly costly to maintain the status quo.

Signed-off-by: Mark Vieira <[email protected]>

(cherry picked from commit c1a1047)

Consolidate docker availability build logic

f32d415

Signed-off-by: Mark Vieira <[email protected]>

mark-vieira requested review from pugnascotia and rjernst February 20, 2020 01:04

mark-vieira added the :Delivery/Build Build or test infrastructure label Feb 20, 2020

pugnascotia reviewed Feb 20, 2020

View reviewed changes

pugnascotia approved these changes Feb 20, 2020

View reviewed changes

Address feedback

08c44c7

Signed-off-by: Mark Vieira <[email protected]>

rjernst reviewed Feb 20, 2020

View reviewed changes

mark-vieira added 2 commits February 20, 2020 10:54

Address feedback

4a3cf2d

Signed-off-by: Mark Vieira <[email protected]>

Fix code style violations

f5fc6b7

Signed-off-by: Mark Vieira <[email protected]>

rjernst approved these changes Feb 20, 2020

View reviewed changes

Reword validation error messages

5f24188

Signed-off-by: Mark Vieira <[email protected]>

Fix unit test

d1b260d

Signed-off-by: Mark Vieira <[email protected]>

mark-vieira merged commit c1a1047 into elastic:master Feb 21, 2020

mark-vieira deleted the docker-support-refactor branch February 21, 2020 16:11

mark-vieira added v8.0.0 backport pending labels Feb 21, 2020

mark-vieira mentioned this pull request Feb 21, 2020

[Backport] Consolidate docker availability logic #52656

Merged

mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Feb 21, 2020

Consolidate docker availability build logic (elastic#52548)

eb90894

(cherry picked from commit c1a1047)

This was referenced Feb 21, 2020

Don't try to start hdfs fixtures when kerberos is unavailable #52661

Merged

[CI] Builds failing with "Execution failed for task ':x-pack:qa:oidc-op-tests:setupPorts'." #52678

Closed

mark-vieira mentioned this pull request Feb 22, 2020

Skip 'setupPorts' tasks when Docker is unavailable #52679

Merged

mark-vieira added v7.6.1 v7.7.0 and removed backport pending labels Feb 23, 2020

mark-vieira mentioned this pull request Feb 29, 2020

[CI] Release tests builds failing due to docker unavailability #52983

Closed

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	private boolean isBlacklistedOs() {
	private boolean isExcludedOs() {

Consolidate docker availability build logic #52548

Consolidate docker availability build logic #52548

Uh oh!

Conversation

mark-vieira commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia left a comment

Choose a reason for hiding this comment

Uh oh!

mark-vieira commented Feb 20, 2020

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-vieira commented Feb 20, 2020

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia commented Feb 20, 2020

Uh oh!

mark-vieira commented Feb 20, 2020

Uh oh!

rjernst commented Feb 20, 2020

Uh oh!

mark-vieira commented Feb 20, 2020

Uh oh!

Uh oh!

mark-vieira commented Feb 20, 2020 •

edited

Loading