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

All Elasticsearch module tests fail #640

Closed
vlaskal opened this issue Oct 28, 2022 · 15 comments · Fixed by #652
Closed

All Elasticsearch module tests fail #640

vlaskal opened this issue Oct 28, 2022 · 15 comments · Fixed by #652
Assignees
Labels
bug Something isn't working
Milestone

Comments

@vlaskal
Copy link
Contributor

vlaskal commented Oct 28, 2022

Describe the bug
All Elasticsearch module tests fail on clean operating system.

To Reproduce
Steps to reproduce the behavior:

  1. Install Docker desktop (Rancher desktop, Moby engine)
  2. Run Elasticsearch module tests
  3. All tests fail with following error message: Docker.DotNet.DockerApiException : Docker API responded with status code=Conflict, response={"message":"Container 776f26413636a94482827057706c065e5d697951e882c6a84a0827979d6aea6a is not running"}

Expected behavior
All tests should pass.

Screenshots
image

Desktop (please complete the following information):

  • version: 2.1.0
  • os: Windows 11 (WSL), Ubuntu Desktop 20.04 LTS
  • docker: Docker dekstop, Rancher Desktop, Moby engine (Microsoft build)

Additional context
Elasticsearch container does not want to start and fail. Container logs provide the following evidences:

@kiview
Copy link
Member

kiview commented Oct 28, 2022

In Java, we implemented this change regarding default heapsize for ES, so it would probably help here as well?
testcontainers/testcontainers-java#5684

@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 28, 2022

Thank you @kiview for quick reply.
I created an PR #641 where I disabled bootstrap check and based on documentation disallow use mmap.
@kiview and @HofmeisterAn Could you look at it and suggest if this is enough?
Or should we solve it even for developers who uses this module for testing purpose or is it their responsibility?

@kiview
Copy link
Member

kiview commented Oct 28, 2022

I am not that experienced with the current module structure in the .NET implementation, but in general, if we can identify good default values for a container in the context of integration testing, it generally makes sense to make them part of the default module implementation.

@HofmeisterAn
Copy link
Collaborator

I would favor to increase the default heap size too. Modules should contain a set of configurations that devs can immediately start. I think it is fine to set or override the env variable, for now. I would not prefer to mount the option files. @kiview what do you think?

@HofmeisterAn HofmeisterAn added the bug Something isn't working label Oct 28, 2022
@kiview
Copy link
Member

kiview commented Oct 28, 2022

In Java, we copy (don't mount) a default config file into the container before startup:
https://github.com/testcontainers/testcontainers-java/pull/5684/files#diff-8af1f477d2bcf3c11f31ecc8b703ce0873f869dbefd5714fa3c78810f02a7cc7R104

One of the reasons was also about the ENV approach being not a super stable integration point:

Elasticsearch 8 seems to have renamed ES_JAVA_OPTS to CLI_JAVA_OPTS on their docs https://www.elastic.co/guide/en/elasticsearch/reference/current/advanced-configuration.html. After testing, it seems like they have backwards compatibility for ES_JAVA_OPTS - but usage of both at the same time is unpredictable.

@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 28, 2022

I will update the PR to have default value set up to minimal required value. I will try all possible ways to do it.

@HofmeisterAn
Copy link
Collaborator

I am not sure if we can "copy" files like Java does at the moment. If not it would be a good opportunity to add it, otherwise we can simply use the Java approach right away.

@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 29, 2022

Will investigate .NET possibilities

@HofmeisterAn
Copy link
Collaborator

It looks like it is not supported yet, but we can add it without much effort I think. We simply copy read-only files (from IMountconfigurations) before the container starts. Here is a simple test:

diff --git a/src/Testcontainers/Containers/TestcontainersContainer.cs b/src/Testcontainers/Containers/TestcontainersContainer.cs
index 2b2a199..a3ca7bd 100644
--- a/src/Testcontainers/Containers/TestcontainersContainer.cs
+++ b/src/Testcontainers/Containers/TestcontainersContainer.cs
@@ -303,6 +303,9 @@ namespace DotNet.Testcontainers.Containers
       var id = await this.client.RunAsync(this.configuration, ct)
         .ConfigureAwait(false);
 
+      await this.client.CopyFileAsync(id, "/tmp/foo", Array.Empty<byte>(), 384, 0, 0, ct)
+        .ConfigureAwait(false);
+
       return await this.client.InspectContainer(id, ct)
         .ConfigureAwait(false);
     }

OC, we need some more logic to detect which files we copy / directories we mount.

@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 30, 2022

I was thinking same way but with full implementation. I will push it soon.

@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Oct 30, 2022

I thought about it too. I think we should move it to DockerContainerOperations or TestcontainersClient and then just filter the Mounts property.

_ = Task.WhenAll(configuration.Mounts
  .Where(mount => AccessMode.ReadOnly.Equals(mount.AccessMode))
  .Where(mount => File.Exists(mount.Source))
  .Select(mount => this.CopyFileAsync(id, mount.Target, Array.Empty<byte>(), 420, 0, 0)));

Probably we need another property in ITestcontainersConfiguration, and distinguish here between configurations, we copy and others we mount.

@HofmeisterAn
Copy link
Collaborator

Running the tests on Windows (WSL) I noticed that this issue is not related to the Elasticsearch JVM options. It is a host limitation:

@kiview Do I miss anything? How does tc-java make sure the host is set up correct?

vlaskal added a commit to vlaskal/dotnet-testcontainers that referenced this issue Nov 5, 2022
HofmeisterAn pushed a commit that referenced this issue Nov 6, 2022
…annot access same file stream in the same process)
@kiview
Copy link
Member

kiview commented Nov 7, 2022

@HofmeisterAn You are completely right, I mixed this up with the other issue we observed in Testcontainers for Java regarding non-optimal default JVM heap settings for Elasticsearch.

However, I just tested Docker Desktop with WSL2 backend on my Windows, and the 7.9.2 and 8.3.0 ES images work completely fine without changing the kernel settings.

The JVM settings we set are:

-Xms2147483648
-Xmx2147483648

So how do you think I should be able to reproduce the error in Java?

@HofmeisterAn
Copy link
Collaborator

@kiview can you check following:

wsl -d docker-desktop
cat /proc/sys/vm/max_map_count

@kiview
Copy link
Member

kiview commented Nov 7, 2022

cat /proc/sys/vm/max_map_count
65530

which is the default low value. So quite interesting that I don't see this issue occur when running the ElasticsearchContainerTests in Testcontainers for Java.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants