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

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Oct 7, 2019

Closes #47639, and unmutes tests that were muted in b958467.

The packaging tests are defined dynamically in DistroTestPlugin, which includes Docker image tests. The plugin didn't allow for the possibility that Docker isn't installed or is otherwise unavailable when defining the packaging tests, which caused failures when executing on e.g. CentOS 6 or Oracle Linux.

This PR changes the plugin to check for Docker before defining the Docker packaging tests. As part of this, the Docker-checking code in BuildPlugin was extracted into a Docker utility class, so that it can be called from more than one place.

I'm not particularly proud of the Docker.java implementation, it feels a little clumsy, so I'd welcome any thoughts here.

The Docker packaging tests were being defined irrespective of whether
Docker was actually available in the current environment. Change this to
check that Docker is available and vaguely functional before defining
the test tasks.

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 pugnascotia force-pushed the fix-docker-tests-on-gcp branch from 6dfab1c to d2dccce Compare October 7, 2019 15:20
@pugnascotia pugnascotia changed the title WIP - fix Docker packaging tests on GCP Only define Docker packaging tests where Docker is available Oct 8, 2019
@pugnascotia pugnascotia changed the title Only define Docker packaging tests where Docker is available Only define Docker pkg tests if Docker is available Oct 8, 2019
@pugnascotia pugnascotia requested a review from alpar-t October 8, 2019 09:24
@pugnascotia pugnascotia added :Delivery/Build Build or test infrastructure :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Oct 8, 2019
@elasticmachine
Copy link
Collaborator

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

@alpar-t
Copy link
Contributor

alpar-t commented Oct 9, 2019

This approach is a bit fragile. On some platforms we always want to have docker and we always want to test for it. Unfortunately Jenkins doesn't make this easy for matrix jobs, but since we are in more control of the environment, I'm wondering if we should make this specific, like parsing /etc/os-release or /etc/issue and only allow docker to be missing on specific platforms where we know it's not in the images ?

There's a similar situation with the test fixtures plugin that skips tests if docker compose is not available but that's different in that those types of tests should be able to run anywhere ( part of ./gradlew check ) and we didn't want to make docker compose mandatory. I'we been meaning to add a flag that would force docker-compose to be available and use it in CI.

My concern is that docker install might be skipped in the image and we'll never notice that we are not running these test.s

@pugnascotia
Copy link
Contributor Author

@atorok Right now, the precense or absence of Docker within Vagrant depends on whether we installed Docker via the Vagrantfile. However, that doesn't actually answer question: is Docker supposed to be installed in this environment? We could maintain a list / matrix somewhere that specifies where Docker is supposed to be installed, and use that to drive both Vagrantfile and DistroTestPlugin. What do you think?

@pugnascotia
Copy link
Contributor Author

Actually, looking at how the last CI run failed, I think we do need a list of where Docker is and isn't supposed to be installed, because it looks like the CI worker has Docker and so Gradle happily declared the Docker tests tasks, which it then tried to run on centos-6, which failed. I'll need to think again here.

@alpar-t
Copy link
Contributor

alpar-t commented Oct 9, 2019

centos-6 doesn't AFAIK have docker. The failure in the PR check is infra related

14:40:30 ERROR: Error cloning remote repo 'origin'

I think we should assume docker by default, and have a list of the few platforms where it's ok to be missing.

While at it, could you add a check that runs something like docker ps and also integrate this with TestFixturesPlugin.dockerComposeSupported ?
I think the docker compose stuff should go into the same utility class and have docker supported as a pre condition.
The reason for docker ps has to do with test fixtures. We ran into situations where docker is installed but the user doesn't have permission to it.

Introduce a VM exclude list, so that the docker packaging tests are run
on all VMs other than those listed. Previously the code checked that
whether Docker was installed, but that wasn't sufficient becasue the
check would happen on the CI host, instead of in the VM.
The Version class is very strict about what suffices it allows after the
patch number, which meant it rejected Docker versions such as
`19.03.2-ce`. Introduce a "relaxed" parsing mode that allow any style of
suffix.
@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

elasticmachine and others added 5 commits October 10, 2019 03:11
Separately to running packaging tests in Vagrant, we also run packaging
tests through CI on a variety of operating systems. Change how the test
tasks are generated so that the local Docker packaging tests are only
defined if the host OS is supported.
@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging

@mark-vieira
Copy link
Contributor

I feel like we're talking in circles now about some of the finer points of the implementation. Can I suggest merging now as-is, subject to a clear CI run of course, since we'll certinaly iterate more on this over time?

👍

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2 since it experience a peculiar transient error.

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix as, yet again, the wrong commit was tested.

@alpar-t
Copy link
Contributor

alpar-t commented Nov 1, 2019

I would really like us to remove project.ext.shouldTestDocker before merging, as it's really confusing and unnecessary i.m.h.o

@pugnascotia
Copy link
Contributor Author

I would really like us to remove project.ext.shouldTestDocker before merging, as it's really confusing and unnecessary i.m.h.o

@atorok I tried removing this and relying on detecting whether Gradle is running in a VM via IN_VM_SYSPROP, but the problem is that the test tasks are defined before we even have a VM running, so detection cannot work here. I'm going to leave shouldTestDocker, and maybe we can think of something better in the future.

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix to pick up the right commit

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix to pick up the right commit

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix to pick up the right commit

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix to pick up the right commit

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@pugnascotia
Copy link
Contributor Author

It's green! Green I tell you!

@pugnascotia pugnascotia merged commit 1f3d101 into elastic:master Nov 4, 2019
@pugnascotia pugnascotia deleted the fix-docker-tests-on-gcp branch November 4, 2019 17:38
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Nov 5, 2019
Closes elastic#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
@alpar-t
Copy link
Contributor

alpar-t commented Nov 5, 2019

I would really like us to remove project.ext.shouldTestDocker before merging, as it's really confusing and unnecessary i.m.h.o

@atorok I tried removing this and relying on detecting whether Gradle is running in a VM via IN_VM_SYSPROP, but the problem is that the test tasks are defined before we even have a VM running, so detection cannot work here. I'm going to leave shouldTestDocker, and maybe we can think of something better in the future.

@pugnascotia I don't quite see why this is necessary. AFAIK, When running tests in a VM, we run the tasks in the sub-projects, and when running in the GCP the tasks are attached to the qa/os projects, so these can have different dependencies. I was under the impression that when running in vagrant we do not need any detection at the Gradle level, the tests will just skip if docket is not available on the VM. With GCP, we 1. make sure docker is available on platforms we expect it to be and 2. make sure we don't pull in docker dependencies on platforms where it's not available.

pugnascotia added a commit that referenced this pull request Nov 5, 2019
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
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker packaging tests break the matrix.
5 participants