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

Make docker tests more reliable #68512

Merged

Conversation

pugnascotia
Copy link
Contributor

Closes #66656.

Current, Docker#verifyOssInstallation(...) checks the permissions of
elasticsearch.keystore, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check elasticsearch.keystore and
remove the check from verifyOssInstallation().

Closes elastic#66656.

Current, `Docker#verifyOssInstallation(...)` checks the permissions of
`elasticsearch.keystore`, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check `elasticsearch.keystore` and
remove the check from `verifyOssInstallation()`.
@pugnascotia pugnascotia added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.11.0 v7.12.0 labels Feb 4, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Feb 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

* 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.

@pugnascotia pugnascotia merged commit f35f636 into elastic:master Feb 5, 2021
@pugnascotia pugnascotia deleted the 66656-docker-test-improvements branch February 5, 2021 20:48
pugnascotia added a commit that referenced this pull request Feb 5, 2021
Closes #66656.

Current, `Docker#verifyOssInstallation(...)` checks the permissions of
`elasticsearch.keystore`, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check `elasticsearch.keystore` and
remove the check from `verifyOssInstallation()`.
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in abc6454

pugnascotia added a commit that referenced this pull request Feb 8, 2021
Closes #66656.

Current, `Docker#verifyOssInstallation(...)` checks the permissions of
`elasticsearch.keystore`, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check `elasticsearch.keystore` and
remove the check from `verifyOssInstallation()`.
easyice pushed a commit to easyice/elasticsearch that referenced this pull request Mar 25, 2021
Closes elastic#66656.

Current, `Docker#verifyOssInstallation(...)` checks the permissions of
`elasticsearch.keystore`, however this often causes problems when a test
forgets to wait for ES and the keystore doesn't exist yet.

Instead, add a test specifically to check `elasticsearch.keystore` and
remove the check from `verifyOssInstallation()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v7.11.0 v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] DockerTests test085EnvironmentVariablesAreRespectedUnderDockerExec failing on Fedora 32
4 participants