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.

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

Only define Docker pkg tests if Docker is available #47640

Merged
merged 50 commits into from
Nov 4, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
521d41d
Revert "Mute docker packaging tests (DockerTests)"
pugnascotia Oct 7, 2019
c01dd7b
Only define Docker pkg tests where appropriate
pugnascotia Oct 7, 2019
d2dccce
Fix lint error
pugnascotia Oct 7, 2019
5eb2a28
Docs and refactoring
pugnascotia Oct 8, 2019
ec8b085
Enable IPv4 forwarding on ubuntu in 16.04
pugnascotia Oct 8, 2019
1f48f44
Use host networking when building docker images
pugnascotia Oct 8, 2019
560adb6
Address review feedback
pugnascotia Oct 9, 2019
7212a9c
Exclude Docker distro tests per VM
pugnascotia Oct 9, 2019
de9bb33
Handle Docker version numbers
pugnascotia Oct 10, 2019
2020084
Merge branch 'master' into fix-docker-tests-on-gcp
elasticmachine Oct 10, 2019
e8a920a
Only run Docker tests on some host OSs
pugnascotia Oct 10, 2019
2d8ca75
Make /etc/os-release parsing better
pugnascotia Oct 10, 2019
1684857
Try to fix Docker task generation
pugnascotia Oct 10, 2019
1641b38
Add a debug stmt, figure out debian-8 issue
pugnascotia Oct 11, 2019
54d322c
Merge branch 'master' into fix-docker-tests-on-gcp
elasticmachine Oct 22, 2019
2d96534
Fix how we use data from /etc/os-release
pugnascotia Oct 23, 2019
64051ed
Fix checkstyle
pugnascotia Oct 23, 2019
6a64065
Trying to diagnose CI failures
pugnascotia Oct 23, 2019
8cb1389
Still trying to diagnose failures on CI
pugnascotia Oct 23, 2019
b0e3a9e
Please, just print my debug info
pugnascotia Oct 23, 2019
a20b3d4
Please, just print my debug info
pugnascotia Oct 24, 2019
daec2ad
Still trying to debug
pugnascotia Oct 24, 2019
be25db4
Only define docker distros when Docker is available
pugnascotia Oct 24, 2019
12bb939
Merge branch 'master' into fix-docker-tests-on-gcp
elasticmachine Oct 26, 2019
31fec0b
Use host networking when building docker images
pugnascotia Oct 26, 2019
5c785c7
Merge branch 'master' into fix-docker-tests-on-gcp
elasticmachine Oct 28, 2019
2293017
Revert docker network change
pugnascotia Oct 28, 2019
8c819fb
Workaround infra issue 15654
pugnascotia Oct 29, 2019
55d4cfc
Merge remote-tracking branch 'upstream/master' into fix-docker-tests-…
pugnascotia Oct 29, 2019
b8d44c7
Increase docker pkg test timeouts
pugnascotia Oct 29, 2019
dc19fed
Merge remote-tracking branch 'upstream/master' into fix-docker-tests-…
pugnascotia Oct 29, 2019
e98e4bc
Tiny change to try and get CI to work
pugnascotia Oct 29, 2019
0831884
Docker test fixes following JVM options work
pugnascotia Oct 29, 2019
0a29d8d
Simplify how Docker test tasks are defined
pugnascotia Oct 30, 2019
048832a
Reimplement Docker tests exclude list
pugnascotia Oct 30, 2019
99ded1c
Remove sles-15 from .ci/dockerOnLinuxExclusions
pugnascotia Oct 30, 2019
12bd9fb
Fix access modifiers in DockerUtils
pugnascotia Oct 31, 2019
97fca15
Use project.exec to run commands in DockerUtils
pugnascotia Oct 31, 2019
617f494
Fix
pugnascotia Oct 31, 2019
ffcf939
Fix JavaDoc ref
pugnascotia Oct 31, 2019
0b49a7a
Don't always create a docker distribution
pugnascotia Oct 31, 2019
611b914
Fix logic
pugnascotia Oct 31, 2019
aac11a4
Update .ci/dockerOnLinuxExclusions
pugnascotia Oct 31, 2019
e0010a6
Expand comment re: configuring Docker tasks for Vagrant
pugnascotia Nov 4, 2019
f189ca1
Merge remote-tracking branch 'upstream/master' into fix-docker-tests-…
pugnascotia Nov 4, 2019
63a758d
Tweak logging
pugnascotia Nov 4, 2019
3e8d10d
Add debugging statements
pugnascotia Nov 4, 2019
cedacee
Pointless change to get CI to run the right code
pugnascotia Nov 4, 2019
ef924fc
Various fixes to docker packaging test logic
pugnascotia Nov 4, 2019
9d50aaa
Pointless change to try and get CI to run the right code
pugnascotia Nov 4, 2019
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
4 changes: 4 additions & 0 deletions Vagrantfile
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@ def ubuntu_docker(config)

# Add vagrant to the Docker group, so that it can run commands
usermod -aG docker vagrant

# Enable IPv4 forwarding
sed -i '/net.ipv4.ip_forward/s/^#//' /etc/sysctl.conf
systemctl restart networking
SHELL
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,17 @@ import org.gradle.external.javadoc.CoreJavadocOptions
import org.gradle.internal.jvm.Jvm
import org.gradle.language.base.plugins.LifecycleBasePlugin
import org.gradle.process.CommandLineArgumentProvider
import org.gradle.process.ExecResult
import org.gradle.process.ExecSpec
import org.gradle.util.GradleVersion

import java.nio.charset.StandardCharsets
import java.nio.file.Files
import java.util.regex.Matcher

import static DockerUtils.DOCKER_BINARIES
import static DockerUtils.getDockerAvailability
import static DockerUtils.getDockerPath
import static org.elasticsearch.gradle.tool.Boilerplate.findByName
import static org.elasticsearch.gradle.tool.Boilerplate.maybeConfigure

/**
* Encapsulates build configuration for elasticsearch projects.
*/
Expand Down Expand Up @@ -189,8 +190,7 @@ class BuildPlugin implements Plugin<Project> {
*/

// check if the Docker binary exists and record its path
final List<String> maybeDockerBinaries = ['/usr/bin/docker', '/usr/local/bin/docker']
final String dockerBinary = maybeDockerBinaries.find { it -> new File(it).exists() }
final String dockerBinary = getDockerPath().orElse(null)

final boolean buildDocker
final String buildDockerProperty = System.getProperty("build.docker")
Copy link
Contributor

Choose a reason for hiding this comment

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

This property is a left over and it's my bad for not removing it yet. See #45034.
Please remove this property here together with the conditionals and the ext.set('buildDocker' from bellow it as it's not used, and make sure we don't mention it in any new code we add.

The way this is meant to work is that docker is required if you need it to build something, trying to build that something without docker should fail. It's optional for anything involving testing.
We usually get around this by only calling build ( which includes assemble ) on CI workers that have docker, and calling check when they might not , which doesn't include a full assemble and only builds what it needs for the tests, if no tests can run nothing gets assembled. The logic is implemented in the test fixtures plugin.
The docker rest tests are a special case that the test fixtures plugin doesn't fully handle, see distribution/docker/build.gradle that has this :

 if (TestFixturesPlugin.dockerComposeSupported()) {
    dependsOn assemble
  }

We got into this situation because we have platforms for which we can't have docker in CI, but still want to run the rest of the tests. Ideally we would just require docker for development as we rely on it in some many ways these days. (Of course we would have to support test fixtures on mac and windows which we don't yet, but that's a different story).

The situation is a bit different here because we are able to pick when to run and when not to because we know what platform we are running against, but that's only when we are running against vagrant and the subprojects are actually in use. In this case the VMs will have docker as we install it in the Vagrant file, but the host also has to have it to build the image, which our bare metal workers do have, so we are ok to fail, since there's no way to test docker without having it installed.

On the GCP images, we may not have it, so we can't build nor test, anything docker related, so I think we need to keep the same semantics here, avoid pulling in the dependency to build the images and skip the docker related tests ( which we already do ), since this has to do with GCP it's similar to the rest of CI, as in we don't have a way of configuring the platforms that are ok not to have docker, so the only way to implement my earlier suggestion to have a list of platforms where it's ok not to have docker is in CI and it would be similar for every CI job and worker we have, not specific to packaging tests.
I think the easiest way to accomplish this is to skip configuring the distribution altogether in this case, similar to how we deal with docker rest tests.

We don't really care about not having docker on the host running the vagrant tests,
so I don't think we should do anything about it.
As sych the part of the PR that configures the shouldTestDocker ext property should be discarded.

Expand All @@ -209,77 +209,56 @@ class BuildPlugin implements Plugin<Project> {
ext.set('requiresDocker', [])
rootProject.gradle.taskGraph.whenReady { TaskExecutionGraph taskGraph ->
final List<String> tasks = taskGraph.allTasks.intersect(ext.get('requiresDocker') as List<Task>).collect { " ${it.path}".toString()}

if (tasks.isEmpty() == false) {
final DockerUtils.DockerAvailability dockerAvailability = getDockerAvailability()
pugnascotia marked this conversation as resolved.
Show resolved Hide resolved

if (dockerAvailability.isAvailable() == true) {
return
}

/*
* There are tasks in the task graph that require Docker. Now we are failing because either the Docker binary does not
* exist or because execution of a privileged Docker command failed.
* There are tasks in the task graph that require Docker.
* Now we are failing because either the Docker binary does
* not exist or because execution of a privileged Docker
* command failed.
*/
if (dockerBinary == null) {
if (dockerAvailability.getPath() == null) {
final String message = String.format(
Locale.ROOT,
"Docker (checked [%s]) is required to run the following task%s: \n%s",
maybeDockerBinaries.join(","),
DOCKER_BINARIES.join(","),
tasks.size() > 1 ? "s" : "",
tasks.join('\n'))
throwDockerRequiredException(message)
}

// we use a multi-stage Docker build, check the Docker version since 17.05
final ByteArrayOutputStream dockerVersionOutput = new ByteArrayOutputStream()
LoggedExec.exec(
rootProject,
{ ExecSpec it ->
it.commandLine = [dockerBinary, '--version']
it.standardOutput = dockerVersionOutput
})
final String dockerVersion = dockerVersionOutput.toString().trim()
checkDockerVersionRecent(dockerVersion)

final ByteArrayOutputStream dockerImagesErrorOutput = new ByteArrayOutputStream()
// the Docker binary executes, check that we can execute a privileged command
final ExecResult dockerImagesResult = LoggedExec.exec(
rootProject,
{ ExecSpec it ->
it.commandLine = [dockerBinary, "images"]
it.errorOutput = dockerImagesErrorOutput
it.ignoreExitValue = true
})

if (dockerImagesResult.exitValue != 0) {
if (dockerAvailability.isVersionHighEnough() == false) {
final String message = String.format(
Locale.ROOT,
"a problem occurred running Docker from [%s] yet it is required to run the following task%s: \n%s\n" +
"the problem is that Docker exited with exit code [%d] with standard error output [%s]",
dockerBinary,
tasks.size() > 1 ? "s" : "",
tasks.join('\n'),
dockerImagesResult.exitValue,
dockerImagesErrorOutput.toString().trim())
"building Docker images requires Docker version 17.05+ due to use of multi-stage builds yet was [%s]",
dockerAvailability.getVersion())
throwDockerRequiredException(message)
}

// Some other problem, print the error
final String message = String.format(
Locale.ROOT,
"a problem occurred running Docker from [%s] yet it is required to run the following task%s: \n%s\n" +
"the problem is that Docker exited with exit code [%d] with standard error output [%s]",
dockerBinary,
tasks.size() > 1 ? "s" : "",
tasks.join('\n'),
dockerAvailability.getLastCommand().exitCode,
dockerAvailability.getLastCommand().stderr.trim())
throwDockerRequiredException(message)
}
}
}

(ext.get('requiresDocker') as List<Task>).add(task)
}

protected static void checkDockerVersionRecent(String dockerVersion) {
final Matcher matcher = dockerVersion =~ /Docker version (\d+\.\d+)\.\d+(?:-[a-zA-Z0-9]+)?, build [0-9a-f]{7,40}/
assert matcher.matches(): dockerVersion
final dockerMajorMinorVersion = matcher.group(1)
final String[] majorMinor = dockerMajorMinorVersion.split("\\.")
if (Integer.parseInt(majorMinor[0]) < 17
|| (Integer.parseInt(majorMinor[0]) == 17 && Integer.parseInt(majorMinor[1]) < 5)) {
final String message = String.format(
Locale.ROOT,
"building Docker images requires Docker version 17.05+ due to use of multi-stage builds yet was [%s]",
dockerVersion)
throwDockerRequiredException(message)
}
}

private static void throwDockerRequiredException(final String message) {
throw new GradleException(
message + "\nyou can address this by attending to the reported issue, "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import java.util.Random;
import java.util.stream.Collectors;

import static org.elasticsearch.gradle.DockerUtils.getDockerAvailability;
import static org.elasticsearch.gradle.vagrant.VagrantMachine.convertLinuxPath;
import static org.elasticsearch.gradle.vagrant.VagrantMachine.convertWindowsPath;

Expand Down Expand Up @@ -319,8 +320,15 @@ private List<ElasticsearchDistribution> configureDistributions(Project project,
List<ElasticsearchDistribution> currentDistros = new ArrayList<>();
List<ElasticsearchDistribution> upgradeDistros = new ArrayList<>();

// Docker disabled for https://github.com/elastic/elasticsearch/issues/47639
for (Type type : Arrays.asList(Type.DEB, Type.RPM /*,Type.DOCKER*/)) {
final String buildDockerProperty = System.getProperty("build.docker");
final boolean shouldAddDocker = (buildDockerProperty == null || "true".equals(buildDockerProperty))
&& getDockerAvailability().isAvailable();

final List<Type> applicablePackageTypes = shouldAddDocker
? List.of(Type.DEB, Type.RPM, Type.DOCKER)
: List.of(Type.DEB, Type.RPM);

for (Type type : applicablePackageTypes) {
for (Flavor flavor : Flavor.values()) {
for (boolean bundledJdk : Arrays.asList(true, false)) {
// We should never add a Docker distro with bundledJdk == false
Expand All @@ -338,6 +346,7 @@ private List<ElasticsearchDistribution> configureDistributions(Project project,
addDistro(distributions, type, null, Flavor.OSS, true, upgradeVersion.toString(), upgradeDistros);
}
}

for (Platform platform : Arrays.asList(Platform.LINUX, Platform.WINDOWS)) {
for (Flavor flavor : Flavor.values()) {
for (boolean bundledJdk : Arrays.asList(true, false)) {
Expand Down
Loading