Skip to content

Make docker tests more reliable #68512

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

Merged
merged 3 commits into from
Feb 5, 2021
Merged

Make docker tests more reliable #68512

Show file tree
Hide file tree
Changes from all commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
Original file line number Diff line number Diff line change
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>
* <li>Is at least version compatible with minimum version</li>
* <li>Can execute a command that requires privileges</li>
* </ul>
*/
Expand Down
20 changes: 14 additions & 6 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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);
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 Expand Up @@ -183,7 +191,7 @@ public void test070BindMountCustomPathConfAndJvmOptions() throws Exception {

waitForElasticsearch(installation);

final JsonNode nodes = getJson("_nodes").get("nodes");
final JsonNode nodes = getJson("/_nodes").get("nodes");
final String nodeId = nodes.fieldNames().next();

final int heapSize = nodes.at("/" + nodeId + "/jvm/mem/heap_init_in_bytes").intValue();
Expand Down Expand Up @@ -211,7 +219,7 @@ public void test071BindMountCustomPathWithDifferentUID() throws Exception {

waitForElasticsearch(installation);

final JsonNode nodes = getJson("_nodes");
final JsonNode nodes = getJson("/_nodes");

assertThat(nodes.at("/_nodes/total").intValue(), equalTo(1));
assertThat(nodes.at("/_nodes/successful").intValue(), equalTo(1));
Expand Down Expand Up @@ -718,7 +726,7 @@ public void test131InitProcessHasCorrectPID() {
public void test140CgroupOsStatsAreAvailable() throws Exception {
waitForElasticsearch(installation);

final JsonNode nodes = getJson("_nodes/stats/os").get("nodes");
final JsonNode nodes = getJson("/_nodes/stats/os").get("nodes");

final String nodeId = nodes.fieldNames().next();

Expand Down
2 changes: 0 additions & 2 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/PackagingTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
34 changes: 30 additions & 4 deletions qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Stream;

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 @@ -81,6 +81,7 @@ public static void ensureImageIsLoaded(Distribution distribution) {
* successfully.
*
* @param distribution details about the docker image being tested
* @return an installation that models the running container
*/
public static Installation runContainer(Distribution distribution) {
return runContainer(distribution, DockerRun.builder());
Expand All @@ -92,6 +93,7 @@ public static Installation runContainer(Distribution distribution) {
*
* @param distribution details about the docker image being tested
* @param builder the command to run
* @return an installation that models the running container
*/
public static Installation runContainer(Distribution distribution, DockerRun builder) {
executeDockerRun(distribution, builder);
Expand Down Expand Up @@ -289,13 +291,17 @@ public Result run(String script) {

/**
* Checks whether a path exists in the Docker container.
* @param path the path that ought to exist
* @return whether the path exists
*/
public static boolean existsInContainer(Path path) {
return existsInContainer(path.toString());
}

/**
* Checks whether a path exists in the Docker container.
* @param path the path that ought to exist
* @return whether the path exists
*/
public static boolean existsInContainer(String path) {
logger.debug("Checking whether file " + path + " exists in container");
Expand Down Expand Up @@ -394,6 +400,8 @@ public static void chownWithPrivilegeEscalation(Path localPath, String ownership

/**
* Checks that the specified path's permissions and ownership match those specified.
* @param path the path to check
* @param expectedPermissions the unix permissions that the path ought to have
*/
public static void assertPermissionsAndOwnership(Path path, Set<PosixFilePermission> expectedPermissions) {
logger.debug("Checking permissions and ownership of [" + path + "]");
Expand All @@ -415,6 +423,7 @@ public static void assertPermissionsAndOwnership(Path path, Set<PosixFilePermiss

/**
* Waits for up to 20 seconds for a path to exist in the container.
* @param path the path to await
*/
public static void waitForPathToExist(Path path) throws InterruptedException {
int attempt = 0;
Expand All @@ -432,6 +441,8 @@ public static void waitForPathToExist(Path path) throws InterruptedException {

/**
* Perform a variety of checks on an installation. If the current distribution is not OSS, additional checks are carried out.
* @param installation the installation to verify
* @param distribution the distribution to verify
*/
public static void verifyContainerInstallation(Installation installation, Distribution distribution) {
verifyOssInstallation(installation);
Expand All @@ -452,8 +463,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 Expand Up @@ -538,14 +547,31 @@ public static String getContainerId() {
return containerId;
}

/**
* Performs an HTTP GET to <code>http://localhost:9200/</code> with the supplied path.
* @param path the path to fetch, which must start with <code>/</code>
* @return the parsed response
*/
public static JsonNode getJson(String path) throws Exception {
final String pluginsResponse = makeRequest(Request.Get("http://localhost:9200/" + path));
path = Objects.requireNonNull(path).trim();
if (path.isEmpty()) {
throw new IllegalArgumentException("path must be supplied");
}
if (path.startsWith("/") == false) {
throw new IllegalArgumentException("path must start with /");
}
final String pluginsResponse = makeRequest(Request.Get("http://localhost:9200" + path));

ObjectMapper mapper = new ObjectMapper();

return mapper.readTree(pluginsResponse);
}

/**
* Fetches all the labels for a Docker image
* @param distribution required to derive the image name
* @return a mapping from label name to value
*/
public static Map<String, String> getImageLabels(Distribution distribution) throws Exception {
// The format below extracts the .Config.Labels value, and prints it as json. Without the json
// modifier, a stringified Go map is printed instead, which isn't helpful.
Expand Down