Introduce packaging tests for Docker #46599

pugnascotia · 2019-09-11T14:44:26Z

Closes #37617. Add packaging tests for our Docker images, similar to what we have for RPMs or Debian packages. This works by running a container and probing it e.g. via docker exec. Test can also be run in Vagrant, by exporting the Docker images to disk and loading them again in VMs.

Notes:

Docker is installed via Vagrantfile in a selection of boxes. We could change the boxes built by Infra if we felt it was worthwhile, I wasn't sure.
The Docker images are only tested with the bundled JDK. It seemed counter to the spirit of using containers to run one using a bundled JDK, though there's nothing stopped a user doing that.

Add packaging tests for our Docker images, similar to what we have for RPMs or Debian packages. This works by running a container and probing it e.g. via `docker exec`. Test can also be run in Vagrant, by exporting the Docker images to disk and loading them again in VMs. Docker is installed via `Vagrantfile` in a selection of boxes.

elasticmachine · 2019-09-11T14:44:28Z

Pinging @elastic/es-core-infra

dliappis

Great work this is really needed after integrating the Docker parts from the old separate repo!

I took an initial pass and left some comments.

qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java

dliappis · 2019-09-12T10:34:50Z

Vagrantfile

@@ -183,6 +183,35 @@ def deb_common(config, name, extra: '')
    install_command: 'apt-get install -y',
    extra: extra_with_lintian
  )
+  deb_docker(config)


As discussed privately, there's not much value running this on all distros (and not all support Docker like centos-6) so better be specific about where we call this.

I think it would be better to bake this into the image rather than install on-demand.
The CI packaging images already have docker ( at least the ones that support it ),
so we would need to add it to vagrant images only.

I'll sync with Infra about this, and remove it from Vagrantfile once the boxes include Docker.

Vagrantfile

alpar-t

Thanks for taking this on!
Unfortunately we don't have Pr checks for running these directly on jenkins workers, so it would be nice to test this before merging to make sure the CI jobs don't break.

alpar-t · 2019-09-12T11:34:51Z

Vagrantfile

@@ -183,6 +183,35 @@ def deb_common(config, name, extra: '')
    install_command: 'apt-get install -y',
    extra: extra_with_lintian
  )
+  deb_docker(config)


I think it would be better to bake this into the image rather than install on-demand.
The CI packaging images already have docker ( at least the ones that support it ),
so we would need to add it to vagrant images only.

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

distribution/docker/build.gradle

alpar-t · 2019-09-12T11:40:41Z

distribution/docker/docker-export/build.gradle

@@ -0,0 +1,2 @@
+// This file is intentionally blank. All configuration of the
+// export is done in the parent project.


Why do we need a project for the extraction ? Could it just be a task on parent ?

It's to do with DistributionDownloadPlugin - as far as I can tell, it's written to create depdencies between ES distributions and the default config on a project. I'm new to Gradle, but it looks like it doesn't depend on a task because the plugin needs to be able to locate the built archives. I'd be very happy to be pointed at better ways of doing this. I had to spend a while deciphering what was going on between DistroTestPlugin and DistributionDownloadPlugin.

Also, the same pattern is used in :distribution:packages.

I see, it's a bit strange that this we have projects for export only here, but I'll defer to @rjernst

qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java

pugnascotia · 2019-09-18T15:41:02Z

@elasticmachine run elasticsearch-ci/1

pugnascotia · 2019-09-18T15:42:51Z

@elasticmachine run elasticsearch-ci/packaging

…tests

pugnascotia · 2019-09-19T13:36:25Z

Mysterious - for some reason, the centos-6 Vagrant box is not only trying to install Docker when it shouldn't be, it's trying to do with apt-get! 🤷‍♂

dliappis

Left a few more contents related to the Vagrantfile.

dliappis · 2019-09-20T08:15:32Z

Vagrantfile

@@ -185,6 +191,34 @@ def deb_common(config, name, extra: '')
  )
 end

+def deb_docker(config)
+  config.vm.provision 'install Docker using apt', run: 'always', type: 'shell', inline: <<-SHELL


Is run: 'always' here on purpose? Do we intend to attempt to install docker on every vagrant up/reload operation? Details here.

Vagrantfile

…tests

pugnascotia · 2019-09-20T11:29:52Z

Thanks @dliappis - facepalms aplenty. 🤦‍♂

pugnascotia · 2019-09-25T10:06:36Z

@elasticmachine update branch

pugnascotia · 2019-09-25T11:48:23Z

@elasticmachine run elasticsearch-ci/packaging

pugnascotia · 2019-09-26T15:16:10Z

@elasticmachine run elasticsearch-ci/packaging as there was a transient error.

pugnascotia · 2019-09-27T11:16:41Z

@elasticmachine run elasticsearch-ci/packaging as Jenkins restarted.

It turns out that it's not the same as Debian.

pugnascotia · 2019-09-29T11:22:55Z

CI is green if someone would like to give me a LGTM?

…tests

alpar-t

LGTM

We still have to make sure this works with the new workers.
If #46924 is merged, PR checks for it are coming soon, if this happens to be merged before, I'll re test to make sure before merging mine.

pugnascotia · 2019-10-01T13:02:24Z

@atorok is your PR close to merging? I don't mind waiting if it is.

alpar-t · 2019-10-01T14:53:36Z

@atorok is your PR close to merging? I don't mind waiting if it is.

Yes it is.

pugnascotia · 2019-10-01T15:13:49Z

@elasticmachine update branch

rjernst

Thanks for the work here! Better testing of our docker images is greatly needed.

After reading through this, it appears we are running docker within each VM. At first I was confused by this, but I guess this matches up with docker being another distribution, and we want to test it on each platform (similar to how we test java on each platform).

One general thing I noticed though is we seem to have duplicated a lot of the assertions that exist within the archive tests. This would mean making any change to the archive structure would require changing multiple tests. Could we reuse these assertions?

rjernst · 2019-10-04T06:39:43Z

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

@@ -370,7 +370,8 @@ private static void addDistro(NamedDomainObjectContainer<ElasticsearchDistributi
            if (type == Type.ARCHIVE) {
                d.setPlatform(platform);
            }
-            d.setBundledJdk(bundledJdk);
+            // We don't test Docker images with a non-bundled JDK
+            d.setBundledJdk(type == Type.DOCKER || bundledJdk);


This should be handled outside of addDistro, otherwise we are adding the same distribution twice? ie around line 325 where this method is called

rjernst · 2019-10-04T06:40:33Z

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

+            distro.getPlatform(),
+            distro.getFlavor(),
+            // We don't test Docker images with a non-bundled JDK
+            type == Type.DOCKER || distro.getBundledJdk());


This shouldn't be necessary, see my previous comment about where we add the distros. We should never add a docker distro with bundledJdk=false

rjernst · 2019-10-04T06:41:35Z

distribution/docker/build.gradle

+  if (subProject.name.contains('docker-export')) {
+    apply plugin: 'distribution'
+
+    def oss = subProject.name.startsWith('oss')


Please use static types. It makes the code easier to reason about, eg String here.

rjernst · 2019-10-04T06:45:13Z

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

+    /**
+     * Check that a keystore can be manually created using the provided CLI tool.
+     */
+    public void test40CreateKeystoreManually() throws InterruptedException {


This doesn't really match with the pattern we've been working towards. We want test classes for categories of checks, like keystore related (which @williamrandolph has been splitting out in a separate PR). Which distribution is being used should not matter to the test, only that we know how to do primitive operations for the given distribution (eg start, stop, run cli tool, etc)

rjernst · 2019-10-04T06:46:34Z

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

+     * Check whether the elasticsearch-certutil tool has been shipped correctly,
+     * and if present then it can execute.
+     */
+    public void test90SecurityCliPackaging() {


We definitely don't want to have copies of every test in archive tests. Most of that class needs to be broken up. I wonder if in the interum ArchiveTests could be adapted to run when docker type is used?

rjernst · 2019-10-04T06:47:50Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Distribution.java

@@ -57,12 +64,17 @@ public boolean isPackage() {
        return packaging == Packaging.RPM || packaging == Packaging.DEB;
    }

+    public boolean isDocker() {


Why is this needed? It doesn't seem any clearer than packaging == Packaging.DOCKER

To be honest I was copying the pattern of the existing code. e.g. isArchive() is only called in one place 🤷‍♂. I can remove it.

rjernst · 2019-10-04T06:50:27Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java

+        ).forEach(dir -> assertPermissionsAndOwnership(dir, p755));
+
+        Stream.of(
+            "elasticsearch",


I don't think we should copy everything that is in Archives. This can (and will) get out of sync. I think we need to think more about how to decouple the checks so they can run agnostically.

How about I do that in a follow-up PR? This one has been waiting to merge for a while. I can then take the time to refactor the packaging tests more generally.

…tests

…ker-packaging-tests

pugnascotia · 2019-10-05T14:38:44Z

I'm going to merge this PR as-is, and once #44775 is merged, I'll refactor the packaging tests to reduce duplication. The infrastructure here will unblock @dliappis and another PR of mine.

alpar-t · 2019-10-07T06:14:14Z

@pugnascotia this broke the new packaging tests: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob+packaging-tests-unix-next/os=ubuntu-16.04-packaging/13/console

alpar-t · 2019-10-07T08:29:25Z

@pugnascotia sorry I missed this during review, we really want to back-port testing and build related changes like this, especially anything that changes the build code needs to be back-ported so the code doesn't diverge making it easy to port improvement to older branches that are still built in CI.

pugnascotia · 2019-10-07T08:37:21Z

No problem, happy to backport.

Re: the CI failure, it looks like the docker build step failed, which I'm pretty sure is transient and not related to my changes. yum is complaining about its mirrors.

alpar-t · 2019-10-07T09:56:23Z

I added more details to #47639 there's more to it, and some of the tests fail too

Closes elastic#37617. Add packaging tests for our Docker images, similar to what we have for RPMs or Debian packages. This works by running a container and probing it e.g. via `docker exec`. Test can also be run in Vagrant, by exporting the Docker images to disk and loading them again in VMs. Docker is installed via `Vagrantfile` in a selection of boxes.

Backport of #46599 and #47640. Add packaging tests for Docker. * Introduce packaging tests for Docker (#46599) Closes #37617. Add packaging tests for our Docker images, similar to what we have for RPMs or Debian packages. This works by running a container and probing it e.g. via `docker exec`. Test can also be run in Vagrant, by exporting the Docker images to disk and loading them again in VMs. Docker is installed via `Vagrantfile` in a selection of boxes. * Only define Docker pkg tests if Docker is available (#47640) Closes #47639, and unmutes tests that were muted in b958467. The Docker packaging tests were being defined irrespective of whether Docker was actually available in the current environment. Instead, implement exclude lists so that in environments where Docker is not available, no Docker packaging tests are defined. For CI hosts, the build checks `.ci/dockerOnLinuxExclusions`. The Vagrant VMs can defined the extension property `shouldTestDocker` property to opt-in to packaging tests. As part of this, define a seperate utility class for checking Docker, and call that instead of defining checks in-line in BuildPlugin.groovy

pugnascotia added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 labels Sep 11, 2019

pugnascotia requested review from rjernst and dliappis September 11, 2019 14:44

dliappis requested changes Sep 12, 2019

View reviewed changes

alpar-t reviewed Sep 12, 2019

View reviewed changes

pugnascotia added 2 commits September 12, 2019 13:55

Install Docker in fewer OSs via Vagrantfile

519342f

Address review feedback

1980219

pugnascotia added 2 commits September 19, 2019 12:24

Merge remote-tracking branch 'upstream/master' into docker-packaging-…

4862f7b

…tests

Vagrantfile tweaks

dcf27b3

dliappis requested changes Sep 20, 2019

View reviewed changes

pugnascotia added 2 commits September 20, 2019 12:29

Vagrantfile fixes

3cb1bfe

Merge remote-tracking branch 'upstream/master' into docker-packaging-…

4423f72

…tests

Fix gaffe with mapping file extension to Packaging value

563882e

Merge branch 'master' into docker-packaging-tests

fa01608

Attempt to make Docker packaging tests more robust

3797a3e

Fix docker install on Ubuntu

62d78ac

It turns out that it's not the same as Debian.

Merge remote-tracking branch 'upstream/master' into docker-packaging-…

3ccccfe

…tests

alpar-t approved these changes Oct 1, 2019

View reviewed changes

pugnascotia closed this Oct 1, 2019

pugnascotia reopened this Oct 1, 2019

Tweak exception handing in waitForElasticsearch(...)

f527808

Merge branch 'master' into docker-packaging-tests

f2b83d3

rjernst requested changes Oct 4, 2019

View reviewed changes

pugnascotia added 3 commits October 4, 2019 09:30

Merge remote-tracking branch 'upstream/master' into docker-packaging-…

3b6deb8

…tests

Address review feedback

e7aca88

Merge remote-tracking branch 'origin/docker-packaging-tests' into doc…

2b79a88

…ker-packaging-tests

pugnascotia mentioned this pull request Oct 4, 2019

Support _FILE suffixed env vars in Docker entrypoint #47573

Merged

pugnascotia merged commit da59dfe into elastic:master Oct 5, 2019

pugnascotia deleted the docker-packaging-tests branch October 5, 2019 14:40

alpar-t mentioned this pull request Oct 7, 2019

Docker packaging tests break the matrix. #47639

Closed

alpar-t added backport pending v7.5.0 labels Oct 7, 2019

pugnascotia mentioned this pull request Nov 5, 2019

Add Docker packaging tests on 7.x #48857

Merged

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

		@@ -0,0 +1,2 @@
		// This file is intentionally blank. All configuration of the
		// export is done in the parent project.

Introduce packaging tests for Docker #46599

Introduce packaging tests for Docker #46599

Conversation

pugnascotia commented Sep 11, 2019 • edited Loading

elasticmachine commented Sep 11, 2019

dliappis 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

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

pugnascotia commented Sep 18, 2019

pugnascotia commented Sep 18, 2019

pugnascotia commented Sep 19, 2019

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented Sep 20, 2019

pugnascotia commented Sep 25, 2019

pugnascotia commented Sep 25, 2019

pugnascotia commented Sep 26, 2019

pugnascotia commented Sep 27, 2019

pugnascotia commented Sep 29, 2019

alpar-t left a comment

Choose a reason for hiding this comment

pugnascotia commented Oct 1, 2019

alpar-t commented Oct 1, 2019

pugnascotia commented Oct 1, 2019

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

pugnascotia commented Oct 5, 2019

alpar-t commented Oct 7, 2019

alpar-t commented Oct 7, 2019

pugnascotia commented Oct 7, 2019

alpar-t commented Oct 7, 2019

pugnascotia commented Sep 11, 2019 •

edited

Loading