Introduce Docker images build #36246

jasontedor · 2018-12-05T04:07:06Z

This commit introduces the building of the Docker images as bonafide packaging formats alongside our existing archive and packaging distributions. This build is migrated from a dedicated repository, and converted to Gradle in the process.

elasticmachine · 2018-12-05T04:07:07Z

Pinging @elastic/es-core-infra

jasontedor · 2018-12-05T04:07:26Z

Testing will come in a follow-up, after we have discussion and agree on an approach.

alpar-t

LGTM, just a few points that I think are worth discussing.

alpar-t · 2018-12-05T07:22:51Z

distribution/docker/build.gradle

+        "docker.elastic.co/elasticsearch/elasticsearch-full:${VersionProperties.elasticsearch}"
+      ]
+    }
+    executable 'docker'


This means that we'll need docker installed even for ./gradlew assemble.
Are we ok with any version of docker for that?
It might make sense to add up-front checks like we have for the JDK to make sure it's there and it works.
Permissions are also something we are likely to run into as many tutorials online advertise the use of sudo docker people might not have permissions set up for the user running the build.

@atorok I pushed 684fcb5.

@atorok I pushed one more change, to check if we can even run Docker: 26fe606

This gives output like:

FAILURE: Build failed with an exception. * What went wrong: a problem occurred running Docker yet it is required to run the following tasks: :distribution:docker:buildDockerImage :distribution:docker:buildOssDockerImage the problem is that [docker images] exited with exit code [1] with error output [Got permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Get http://%2Fvar%2Frun%2Fdocker.sock/v1.39/images/json: dial unix /var/run/docker.sock: connect: permission denied]

Your input @nik9000 @rjernst would be appreciated here too!

That seems right. We talked about making a parameter that let you explicitly skip the tasks which would be a nice thing to have. Something like -Dbuild.docker=false.

@nik9000 I pushed b20be22. It's okay?

I think this would be an ideal time to consider using img or Buildah as the build executor instead of the Docker daemon. I personally consider this to be the "next step" in improving the build, and it just seems like a great time to redefine the dependencies.

Getting away from the Docker daemon not only improves the local build experience for developers, especially if they don't run Linux on their workstations, it also makes a much better story for running builds in container environments, like the work that @mgreau is actively pursuing.

Thank you for the suggestion. Alas the Docker daemon will stay around since we are migrating some of our testing away from Vagrant fixtures to Docker Compose fixtures. That’s not to say we won’t take your suggestion up.

alpar-t · 2018-12-05T07:37:40Z

distribution/docker/build.gradle

+  return "${prefix}${oss ? 'Oss' : ''}${suffix}"
+}
+
+void addProcessDockerfileTask(final boolean oss) {


Why are we preferring multiple Copy tasks over a single one that does this multiple times:

from ('...') { into '..' }

I think that would be easier to read and understand what's being copied where.
We could additionally make it a Sync task to make sure no stale files end up in the build.
This won't be a problem so much in CI where we clean but could happen locally.
Having more tasks could save us a bit when only parts change but I don't think that's worth sacrificing visibility over it.

Okay, I simplified into a single copy task for the static files, although I still left the generation of the Dockerfile separate. See 8e78667.

distribution/docker/build.gradle

rjernst

Looks good, a couple minor comments

rjernst · 2018-12-05T19:11:24Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -232,6 +232,60 @@ class BuildPlugin implements Plugin<Project> {
        project.ext.java9Home = project.rootProject.ext.java9Home
    }

+    static void requireDocker(final Task task) {
+        final Project rootProject = task.project.rootProject
+        if (rootProject.hasProperty('requiresDocker') == false) {


Instead of using an essentially global property like this, can we have a property on the tasks using docker themselves? Then we can use a .all closure on the tasks container looking for the property, and check for docker in the middle of configuration instead of waiting until all projects are configured. There would still be a rootProject property to "remember" docker was already checked, but the tasks won't (directly) modify the root project.

+1 on marking the task, but I would make it a custom task and use tasks.withType.
The checks could live in the same class as static methods.
We could at some time introduce an interface and have a generic "this task requires something that we need to check up-front" mechanism, we already have others: docker-compose, jdk versions tasks (tests) that require specific inputs to run.

I think these are very good suggestions but I’m going to ask one of you to pick this up on a follow-up please?

rjernst · 2018-12-05T19:11:57Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                    } else {
+                        assert exitCode > 0 && dockerErrorOutput != null
+                        throw new GradleException(
+                                String.format(


I think it would be useful to clearly distinguish docker not being found in one of the two expected locations, vs an error when trying to run the binary.

@rjernst So I think I did distinguished the two cases, or am I misunderstanding what you mean?

Sorry, you are right. My eyes glossed over the first if block.

If you use a task property and a tasks.all as I suggested, I think this can all be moved together so that the if/elseif on the docker file check can end with an else that throws the error, so you don't need to fake out the exit code.

rjernst · 2018-12-05T19:18:19Z

distribution/docker/build.gradle

+      from 'src/docker/bin'
+    }
+
+    from 'src/docker/config'


Can we structure src/docker/config so it is flattened, like bin, so that there is a config dir in the output? This isn't critical, but the lack of parallelism annoys my OCD. :)

I pushed 0c2e6ff.

alpar-t

Thanks @jasontedor. I left some comments on the new commits.

alpar-t · 2018-12-06T06:03:06Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+        } else {
+            // do not overwrite an existing onlyIf closure
+            final Closure onlyIf = task.onlyIf
+            task.onlyIf { onlyIf && false }


A task can have multiple onlyIfs but if any one of them return false the others don't really matter. This can be expressed more straight forward as task.enabled = false

I pushed ed4bf10.

alpar-t · 2018-12-06T06:07:03Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                    dockerErrorOutput = null
+                } else {
+                    // the Docker binary executes, check that we can execute a privileged command
+                    final Process process = new ProcessBuilder(dockerBinary, "images").start()


nit: project.exec or LoggedExec.exec(project would be more idiomatic here.

I pushed some changes related to this.

alpar-t · 2018-12-06T06:17:23Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -232,6 +232,60 @@ class BuildPlugin implements Plugin<Project> {
        project.ext.java9Home = project.rootProject.ext.java9Home
    }

+    static void requireDocker(final Task task) {
+        final Project rootProject = task.project.rootProject
+        if (rootProject.hasProperty('requiresDocker') == false) {


+1 on marking the task, but I would make it a custom task and use tasks.withType.
The checks could live in the same class as static methods.
We could at some time introduce an interface and have a generic "this task requires something that we need to check up-front" mechanism, we already have others: docker-compose, jdk versions tasks (tests) that require specific inputs to run.

* master: (133 commits) SNAPSHOT: Increase Timeout to Stabilize Test (elastic#36294) Fix get certificates HLRC API (elastic#36198) Avoid shutting down the only master (elastic#36272) Fix typo in comment Fix total hits serialization of the search response (elastic#36290) Fix FullClusterRestartIT#testRollupIDSchemeAfterRestart Mute FullClusterRestartIT#testRollupIDSchemeAfterRestart as we await a fix. [Docs] Add Profile API limitations (elastic#36252) Make sure test don't use Math.random for reproducability (elastic#36241) Fix compilation ingest: support default pipeline through an alias (elastic#36231) Zen2: Rename tombstones to exclusions (elastic#36226) [Zen2] Hide not recovered state (elastic#36224) Test: mute testDataNodeRestartWithBusyMasterDuringSnapshot Test: mute testSnapshotAndRestoreWithNested Revert "Test: mute failing mtermvector rest test" Test: mute failing mtermvector rest test add version 6.5.3 (elastic#36268) Make hits.total an object in the search response (elastic#35849) [DOCS] Fixes broken link in execute watch ...

jasontedor · 2018-12-06T16:38:23Z

@rjernst @nik9000 @atorok Please take another look. I would like the mechanics of how we address requiring Docker in a follow-up with either @rjernst or @atorok taking the lead here as it seems the two of you have some thoughts to iterate on and it's not germane to our main story which is getting the building of the Docker images into this repository.

rjernst

LGTM

This commit introduces the building of the Docker images as bonafide packaging formats alongside our existing archive and packaging distributions. This build is migrated from a dedicated repository, and converted to Gradle in the process.

ingest-geoip and ingest-user-agent are now packaged as modules in Elasticsearch. Relates to elastic/elasticsearch#36956 and elastic/elasticsearch#36246

jasontedor added 4 commits December 4, 2018 19:03

Introduce Docker images build

39c2372

This commit introduces the building of the Docker images as bonafide packaging formats alongside our existing archive and packaging distributions. This build is migrated from a dedicated repository, and converted to Gradle in the process.

Cleanup

a12dc3c

More cleaup

b7d2f08

Rename

efad7cb

jasontedor added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v7.0.0 v6.6.0 labels Dec 5, 2018

jasontedor requested a review from rjernst December 5, 2018 04:07

jasontedor requested a review from alpar-t December 5, 2018 04:09

Remove unnecessary files

a9c9166

jasontedor removed the v6.6.0 label Dec 5, 2018

jasontedor added 2 commits December 4, 2018 23:14

Fix SCM URL

f8364d3

Add missing newline

d50a5ce

alpar-t reviewed Dec 5, 2018

View reviewed changes

Simplify copy tasks

8e78667

alpar-t reviewed Dec 5, 2018

View reviewed changes

distribution/docker/build.gradle Outdated Show resolved Hide resolved

jasontedor added 4 commits December 5, 2018 12:51

Be explicit about requiring Docker

684fcb5

More checks

26fe606

Remove newline

e065cd8

Add comments

6419019

jasontedor requested review from nik9000 and alpar-t December 5, 2018 19:13

rjernst reviewed Dec 5, 2018

View reviewed changes

jasontedor added 2 commits December 5, 2018 15:55

Move config to config

0c2e6ff

Add build.docker property

b20be22

alpar-t reviewed Dec 6, 2018

View reviewed changes

jasontedor added 3 commits December 6, 2018 10:17

Disable instead of only if

ed4bf10

Simplify process execution

507eaec

Simplify

cc31ddd

jasontedor requested review from rjernst and alpar-t December 6, 2018 16:37

jasontedor added the v6.6.0 label Dec 6, 2018

rjernst approved these changes Dec 6, 2018

View reviewed changes

jasontedor merged commit 11dd412 into elastic:master Dec 6, 2018

jasontedor deleted the build-docker-images branch December 6, 2018 22:21

ywelsch mentioned this pull request Dec 10, 2018

Docker images failing build #36414

Closed

jasontedor added the release highlight label Dec 18, 2018

ebadyano mentioned this pull request Jan 18, 2019

Remove ingest-geoip and ingest-user-agent from core plugins list elastic/rally-teams#13

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Docker images build #36246

Introduce Docker images build #36246

jasontedor commented Dec 5, 2018

elasticmachine commented Dec 5, 2018

jasontedor commented Dec 5, 2018

alpar-t left a comment

alpar-t Dec 5, 2018

jasontedor Dec 5, 2018

jasontedor Dec 5, 2018

nik9000 Dec 5, 2018

jasontedor Dec 5, 2018

jarpy Dec 5, 2018

jasontedor Dec 6, 2018

alpar-t Dec 5, 2018

jasontedor Dec 5, 2018

rjernst left a comment

rjernst Dec 5, 2018

alpar-t Dec 6, 2018

jasontedor Dec 6, 2018

rjernst Dec 5, 2018

jasontedor Dec 5, 2018

rjernst Dec 5, 2018

rjernst Dec 5, 2018

rjernst Dec 5, 2018

jasontedor Dec 5, 2018

alpar-t left a comment

alpar-t Dec 6, 2018

jasontedor Dec 6, 2018

alpar-t Dec 6, 2018

jasontedor Dec 6, 2018

alpar-t Dec 6, 2018

jasontedor commented Dec 6, 2018

rjernst left a comment

Introduce Docker images build #36246

Introduce Docker images build #36246

Conversation

jasontedor commented Dec 5, 2018

elasticmachine commented Dec 5, 2018

jasontedor commented Dec 5, 2018

alpar-t left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t left a 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

jasontedor commented Dec 6, 2018

rjernst left a comment

Choose a reason for hiding this comment