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.

Sign up for GitHub

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

Jump to bottom

Make docker tests more reliable #68512

Merged
merged 3 commits into from Feb 5, 2021
Merged

Make docker tests more reliable #68512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4eb4cca
Make docker tests more reliable
pugnascotia Feb 4, 2021
8c7c022
Bunch of tiny tweaks and Javadoc
pugnascotia Feb 5, 2021
8957332
Merge remote-tracking branch 'upstream/master' into 66656-docker-test…
pugnascotia Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion buildSrc/src/main/java/org/elasticsearch/gradle/docker/DockerSupportService.java
View file
Open in desktop
Expand Up @@ -311,7 +311,7 @@ public static class DockerAvailability {
* <ul>
* <li>Installed</li>
* <li>Executable</li>
* <li>Is at least version compatibile with minimum version</li>
pugnascotia marked this conversation as resolved.
Show resolved Hide resolved
* <li>Is at least version compatible with minimum version</li>
* <li>Can execute a command that requires privileges</li>
* </ul>
*/
Expand Down
14 changes: 11 additions & 3 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@
import java.util.stream.Collectors;

import static java.nio.file.attribute.PosixFilePermissions.fromString;
import static org.elasticsearch.packaging.util.Docker.assertPermissionsAndOwnership;
import static org.elasticsearch.packaging.util.Docker.chownWithPrivilegeEscalation;
import static org.elasticsearch.packaging.util.Docker.copyFromContainer;
import static org.elasticsearch.packaging.util.Docker.existsInContainer;
Expand Down Expand Up @@ -97,9 +98,7 @@ public void teardownTest() {
/**
* Checks that the Docker image can be run, and that it passes various checks.
*/
public void test010Install() throws Exception {
// Wait for the container to come up, because we assert the state of some files that Elasticsearch creates on startup.
waitForElasticsearch(installation);
pugnascotia marked this conversation as resolved.
Show resolved Hide resolved
public void test010Install() {
verifyContainerInstallation(installation, distribution());
}

Expand Down Expand Up @@ -146,6 +145,15 @@ public void test041AmazonCaCertsAreInTheKeystore() {
assertTrue("Expected Amazon trusted cert in cacerts", matches);
}

/**
* Check that when the keystore is created on startup, it is created with the correct permissions.
*/
public void test042KeystorePermissionsAreCorrect() throws Exception {
waitForElasticsearch(installation);
Copy link
Contributor

Choose a reason for hiding this comment

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

The failures seem to indicate that waitForElasticseach() doesn't seem to be robust enough. Do we have theories on why that method would return, but the keystore file would not be there? Wouldn't we expect this test to occasionally fail still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that waitForElasticseach() is the problem. The original problem in #66656 was due to to the test running a CLI tool without waiting for ES to start up and create the keystore, which in this case the tool needed.

I removed the keystore from the verifyOssInstallation() because most tests don't care about it, and have a test fail because it didn't wait for a file to be created that the test doesn't care about is just...silly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I guess my point being that the new test you added is still susceptible to the same underlying cause of those other failures, unless I'm missing something. Indeed I agree we don't need superfluous assertions in those other tests, so if nothing else this should fail less often. We'll keep an eye on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically this - if the new test is failing often, at least I have somewhere more specific to start investigating.


assertPermissionsAndOwnership(installation.config("elasticsearch.keystore"), p660);
}

/**
* Send some basic index, count and delete requests, in order to check that the installation
* is minimally functional.
Expand Down
2 changes: 0 additions & 2 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java
View file
Open in desktop
Expand Up @@ -58,7 +58,6 @@
import static org.elasticsearch.packaging.util.Cleanup.cleanEverything;
import static org.elasticsearch.packaging.util.Docker.ensureImageIsLoaded;
import static org.elasticsearch.packaging.util.Docker.removeContainer;
import static org.elasticsearch.packaging.util.Docker.waitForElasticsearch;
import static org.elasticsearch.packaging.util.FileExistenceMatchers.fileExists;
import static org.elasticsearch.packaging.util.FileUtils.append;
import static org.hamcrest.CoreMatchers.anyOf;
Expand Down Expand Up @@ -214,7 +213,6 @@ protected static void install() throws Exception {
case DOCKER:
case DOCKER_UBI:
installation = Docker.runContainer(distribution);
waitForElasticsearch(installation);
Docker.verifyContainerInstallation(installation, distribution);
break;
default:
Expand Down
3 changes: 0 additions & 3 deletions qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java
View file
Open in desktop
Expand Up @@ -28,7 +28,6 @@

import static java.nio.file.attribute.PosixFilePermissions.fromString;
import static org.elasticsearch.packaging.util.FileMatcher.p644;
import static org.elasticsearch.packaging.util.FileMatcher.p660;
import static org.elasticsearch.packaging.util.FileMatcher.p664;
import static org.elasticsearch.packaging.util.FileMatcher.p755;
import static org.elasticsearch.packaging.util.FileMatcher.p770;
Expand Down Expand Up @@ -452,8 +451,6 @@ private static void verifyOssInstallation(Installation es) {

Stream.of(es.modules).forEach(dir -> assertPermissionsAndOwnership(dir, p755));

assertPermissionsAndOwnership(es.config("elasticsearch.keystore"), p660);

Stream.of("elasticsearch.yml", "jvm.options", "log4j2.properties")
.forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p664));

Expand Down