-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ElasticsearchContainer is not waiting for container being ready #2982
Comments
It is worth pointing out that this line was originally copy-pasted from Couchbase and allowing for the UNAUTHRIZED response here was not anything deliberate or stemming from ElasticSearch implementation details. See the conversation which accompanied the change: #826 (comment) |
@lvorona i think this will not work, if elasticsearch security is enabled. Best way would be to call the cluster health api with credentials. |
The user who enables elasticsearch security can still override the defaults similar to the way I have posted in the initial comment. This is only to make the defaults to work. Currently a user who has a simple single container test has to add extra code. It would make sense to change the defaults to work out of the box and document the changes needed when security is enabled. Perhaps I could change the health check to take security into account as well. Let me take a look. |
Thanks for the PR, @slovdahl. I'm not sure if the original line just a straight copy and paste from the Couchbase module (at least that's not my interpretation of #826 (comment)) @dadoonet, could you perhaps lend your opinion on the suggested approach in this ticket and the PR (#2983)? |
Just for the record, I did not submit the PR 😄 I just added a proper cross-reference. |
Thank you for fixing the PR. I have pushed one more commit that points the HttpWaitStrategy at the dedicated ElasticSearch health endpoint 1: It took me a while to figure out the container's behaviour, but I have tested with various settings and waiting for the 200 on the dedicated healthcheck endpoint behaved the best. Some important notes:
As a reminder, this issue came out of us looking for the root of a unit test flakiness. It does not fail all the time, but I have a small project (a pom.xml and a single unit test) that allowed me to reproduce the issue. Let me know if you want it, I'll share the code. |
Actually, I was not enabling the authentication properly. The Cluster health check requires authentication, and I had to add the following clause for the test to pass:
I feel like this is still an improvement over the test being flacky. I could add this snippet to the documentation (https://www.testcontainers.org/modules/elasticsearch/), but I need help to know which repository to update for that. |
Hey. I think this is what I'm bringing with #2320 hopefully. |
Great, then the user will not have to override the wait strategy, since your container already knows if the credentials are needed or not. Feel free to incorporate this change into your PR if that would make it easier for you to manage the change. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
This has been resolved (see comments above). |
In /modules/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java#L55 the ElasticsearchContainer is waiting for either of two status codes:
But ES returning UNAUTHORIZED means that it is not 100% ready yet. Due to this, a simple test relying on the default
ElasticsearchContainer
without any changes and trying to use its API fails in about 10%-20% of invocations with errors likeor if one adds default ES user credentials with
The current workaround is (Scala code):
This fixes the flaky test and I believe this should be the default in the
ElasticsearchContainer
.The text was updated successfully, but these errors were encountered: