-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Workaround for JDK bug with total mem on Debian8 #68542
Workaround for JDK bug with total mem on Debian8 #68542
Conversation
Pinging @elastic/es-core-features (Team:Core/Features) |
* on some Linux variants such as Debian8. | ||
*/ | ||
@SuppressForbidden(reason = "access /proc/meminfo") | ||
List<String> readProcMeminfo() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not thrilled by everything that happens here, but by default, the value is cached so that this method shouldn't be called more than once per second and probably less frequently on systems with appropriate monitoring intervals.
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/default-distro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing this @danhermann
I wonder if we should reenable various tests on debian 8 that were previously disabled due to this, for instance TransportGetAutoscalingCapacityActionIT
and some of the ML rest tests? Can definitely also be handled in a follow-up.
final Optional<String> maybeMemTotalLine = memTotalLines.size() == 1 ? Optional.of(memTotalLines.get(0)) : Optional.empty(); | ||
if (maybeMemTotalLine.isPresent()) { | ||
final String memTotalLine = maybeMemTotalLine.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final Optional<String> maybeMemTotalLine = memTotalLines.size() == 1 ? Optional.of(memTotalLines.get(0)) : Optional.empty(); | |
if (maybeMemTotalLine.isPresent()) { | |
final String memTotalLine = maybeMemTotalLine.get(); | |
if (memTotalLines.size() == 1) { | |
final String memTotalLine = memTotalLines.get(0); |
OsProbe probe = buildStubOsProbe(true, "", List.of(), meminfoLines); | ||
assertThat(probe.getTotalMemFromProcMeminfo(), equalTo(0L)); | ||
|
||
// MemTotal line with invalid value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us for completeness also add a case of having wrong unit.
@@ -254,6 +254,50 @@ public void testCgroupProbeWithMissingMemory() { | |||
assertNull(cgroup); | |||
} | |||
|
|||
public void testGetTotalMemFromProcMeminfo() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test that does not stub out /proc/meminfo and verify that it gives us something > 0? It should run on debian 8 only.
@elasticmachine run elasticsearch-ci/bwc |
Thanks, @henningandersen! I've made the changes you requested and will open another PR to re-enable the tests that were failing due to this issue. Let me know if you see anything else related to this that needs to be addressed. |
Unmute the YAML tests that were muted due to the problem of elastic#66885. The underlying problem was fixed by elastic#68542.
Reads total system memory from
/proc/meminfo
on Debian8 systems if the JDK reports0
total memory. See #66885 (comment) for more background.Fixes #66629.