Skip to content

Workaround for JDK bug with total mem on Debian8#68542

Merged
danhermann merged 2 commits intoelastic:masterfrom
danhermann:66629_total_mem_on_debian8
Feb 8, 2021
Merged

Workaround for JDK bug with total mem on Debian8#68542
danhermann merged 2 commits intoelastic:masterfrom
danhermann:66629_total_mem_on_debian8

Conversation

@danhermann
Copy link
Copy Markdown
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.

@danhermann danhermann added >bug :Core/Infra/Stats Statistics tracking and retrieval APIs v8.0.0 v7.12.0 v7.11.1 labels Feb 4, 2021
@elasticmachine elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Feb 4, 2021
@elasticmachine
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

@elasticmachine run elasticsearch-ci/1

@danhermann
Copy link
Copy Markdown
Contributor Author

@elasticmachine run elasticsearch-ci/default-distro

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

assertNull(cgroup);
}

public void testGetTotalMemFromProcMeminfo() throws Exception {
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@danhermann
Copy link
Copy Markdown
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Core/Infra/Stats Statistics tracking and retrieval APIs Team:Data Management (obsolete) DO NOT USE. This team no longer exists. 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