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

Workaround for JDK bug with total mem on Debian8 #68542

Merged
merged 2 commits into from Feb 8, 2021

Conversation

danhermann
Copy link
Contributor

Reads total system memory from /proc/meminfo on Debian8 systems if the JDK reports 0 total memory. See #66885 (comment) for more background.

Fixes #66629.

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Feb 4, 2021
@elasticmachine
Copy link
Collaborator

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

@danhermann
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@danhermann
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/default-distro

Copy link
Contributor

@henningandersen henningandersen left a 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.

Comment on lines 672 to 674
final Optional<String> maybeMemTotalLine = memTotalLines.size() == 1 ? Optional.of(memTotalLines.get(0)) : Optional.empty();
if (maybeMemTotalLine.isPresent()) {
final String memTotalLine = maybeMemTotalLine.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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 {
Copy link
Contributor

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.

@danhermann
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@danhermann
Copy link
Contributor Author

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.

@danhermann danhermann merged commit 2096050 into elastic:master Feb 8, 2021
@danhermann danhermann deleted the 66629_total_mem_on_debian8 branch February 8, 2021 13:56
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Feb 8, 2021
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Feb 8, 2021
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Feb 9, 2021
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Feb 19, 2021
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Feb 19, 2021
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Feb 22, 2021
Unmute the YAML tests that were muted due to the problem
of elastic#66885.

The underlying problem was fixed by elastic#68542.
droberts195 added a commit that referenced this pull request Feb 22, 2021
Unmute the YAML tests that were muted due to the problem
of #66885.

The underlying problem was fixed by #68542.
droberts195 added a commit that referenced this pull request Feb 22, 2021
Unmute the YAML tests that were muted due to the problem
of #66885.

The underlying problem was fixed by #68542.
droberts195 added a commit that referenced this pull request Feb 22, 2021
Unmute the YAML tests that were muted due to the problem
of #66885.

The underlying problem was fixed by #68542.
easyice pushed a commit to easyice/elasticsearch that referenced this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Stats Statistics tracking and retrieval APIs Team:Data Management Meta label for data/management team v7.11.1 v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OsProbeTests.testOsStats and memory-related YAML test failures on Debian 8
4 participants