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

Autodetermine heap settings based on node roles and total system memory #65905

Merged
merged 39 commits into from
Dec 16, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
0098632
Autodetermine heap settings based on node roles and total system memory
mark-vieira Dec 4, 2020
045ddea
Fix spotless
mark-vieira Dec 4, 2020
c57b8a9
Fix existing packaging tests
mark-vieira Dec 5, 2020
372d7e8
More packaging test fixes
mark-vieira Dec 5, 2020
6c49f48
More fixes
mark-vieira Dec 5, 2020
afe45b7
Set heap size when starting windows service in packaging tests
mark-vieira Dec 5, 2020
bf25202
Add some better exception handling for poorly formatted config files
mark-vieira Dec 5, 2020
6ba5c66
restore heap defaults in jvm.options
rjernst Dec 9, 2020
8e6966c
Reuse findFinalOptions from ergonomics
rjernst Dec 10, 2020
e3726e7
cleanup role responses
rjernst Dec 10, 2020
2efc239
address tests
rjernst Dec 10, 2020
5daf41c
docker test
rjernst Dec 10, 2020
c6f432a
Fix up Docker test
pugnascotia Dec 10, 2020
d44c784
More fixes
pugnascotia Dec 10, 2020
1c7565c
Merge branch 'master' into HEAD
rjernst Dec 11, 2020
8ca2053
checkstyle
rjernst Dec 11, 2020
f3d025b
spotless
rjernst Dec 11, 2020
ec827f0
null guard
rjernst Dec 12, 2020
75cb18b
Add fix for earlier jdks which used InitialHeap instead of MinHeap
mark-vieira Dec 14, 2020
c36892b
Merge branch 'master' into machine-dependent-heap
elasticmachine Dec 14, 2020
88a81a1
Remove Xmx/Xms from jvm.options
rjernst Dec 14, 2020
ca7e564
Set 1g heap for packaging tests by default
rjernst Dec 15, 2020
755cfac
guard for no installation yet when setting heap to 1g
rjernst Dec 15, 2020
1c3552e
fix format oops
rjernst Dec 15, 2020
472df8f
tweak tests overriding heap
rjernst Dec 15, 2020
759e76f
Use temp config directory when appropriate
mark-vieira Dec 15, 2020
7c7e71e
Merge branch 'master' into machine-dependent-heap
elasticmachine Dec 15, 2020
f407562
More packaging test fixes
mark-vieira Dec 15, 2020
8c45424
Even more packaging test fixes for platform-specific line separators
mark-vieira Dec 15, 2020
557af6a
Fix forbidden apis
mark-vieira Dec 16, 2020
47baa29
Only create parent when required
mark-vieira Dec 16, 2020
b6032e8
No need to set default args since we do it in PackagingTestCase setup
mark-vieira Dec 16, 2020
bce1e4f
Fix docker packaging tests
mark-vieira Dec 16, 2020
f6fed4b
Ensure we add a newline so we can safely append to this file
mark-vieira Dec 16, 2020
41f7d4d
Merge branch 'master' into machine-dependent-heap
elasticmachine Dec 16, 2020
7068cd0
Fix packaging test when heap is overriden
mark-vieira Dec 16, 2020
df4ba2e
Fix checkstyle
mark-vieira Dec 16, 2020
2a53668
remove heap options before removing rpm
rjernst Dec 16, 2020
9b1cf77
Fix rpm tests
mark-vieira Dec 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions distribution/src/config/jvm.options
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@
# Xms represents the initial size of the JVM heap
# Xmx represents the maximum size of the JVM heap

-Xms${heap.min}
-Xmx${heap.max}
# -Xms${heap.min}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to think about how to change this. Should we remove this section entirely? Continue to include it also recommend folks stick with auto-determined heap size?

Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to leave it in with a note, since until now our users have always had to configure it, and our training courses will expect it to exist. Maybe we can remove it in master but retain it with a comment in 7.x?

Copy link
Member

Choose a reason for hiding this comment

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

It's tricky. Either way (removing, or commenting out), we are going to potentially break automation here. For example, if there's automation in place that downloads our tar archive, explodes it, and uses sed to replace the heap size.

That leads me to wonder out loud if we should not break users here. I'd propose the following: in 7.x, we leave this as-is in the jvm.options file. Perhaps we provide a warning when the heap size is set manually by a user, pointing them to documentation for machine-dependent heap sizes. In Cloud, we still want to activate the machine-dependent heap sizes, so we'd need deployments there to remove the -Xms/-Xmx flags from their jvm.options file.

In 8.0, we remove this from the default jvm.options file.

Copy link
Member

Choose a reason for hiding this comment

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

I understand the concern, though I'm not sure about a warning. I don't think we intend to disallow setting the heap manually right? Yet a warning for setting the heap would seem like just that. Such a sed replacement in practice would seem fragile. It could break on any change to the default.

I'm conflicted. I don't see a way to warn some users without annoying others. But I do think this is something we can followup on, so in this PR I will restore the original value.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion was:

Perhaps we provide a warning when the heap size is set manually by a user, pointing them to documentation for machine-dependent heap sizes.

Not a warning that we will disallow setting the heap size manually, but a permanent warning that the heap size is being set manually and we recommend not for the optimal experience.

But I do think this is something we can followup on, so in this PR I will restore the original value.

Are you saying then that the default experience will be to not get machine-dependent heap sizing? That seems contrary to our goals here.

Copy link
Member

Choose a reason for hiding this comment

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

If we leave the jvm.options as is, the default will remain not getting machine-dependent heap sizing. If we emit a warning, any user who chooses to not want to use machine-dependent heap sizing will get an annoying warning they cannot use. Do we agree on those statements?

My suggestion was to leave the jvm.options as is (so as not to break the theoretical users that could be using sed to replace the heap size), and then follow up with whatever we decide to do regarding helping users convert within 7.x. This suggestion is because cloud will not rely on the default.

Copy link
Member

Choose a reason for hiding this comment

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

If we leave the jvm.options as is, the default will remain not getting machine-dependent heap sizing. If we emit a warning, any user who chooses to not want to use machine-dependent heap sizing will get an annoying warning they cannot use. Do we agree on those statements?

Why can't they use it? The point of the warning is to tell them to stop setting the heap size manually, let Elasticsearch do it for them. The warning is useful to them.

Copy link
Member

@rjernst rjernst Dec 10, 2020

Choose a reason for hiding this comment

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

Sorry, not sure how "cannot use" got in my response, it was meant to be "cannot remove". That is it is a persistent warning that the only way to remove is to go with ES's memory choices. The warning is useful in that they find out they could be using our automatic heap sizing, but are we saying users will never tune heap anymore? I personally get annoyed if there are warnings I cannot remove with normal usage.

Copy link
Member

Choose a reason for hiding this comment

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

Jason and I spoke and came to an agreement on the above discussion. @jasontedor Let me know if I missed or misunderstood anything.

We should add a warning when heap size is manually specified, since our goal is to have users use the auto heap. There are however some issues that prevent us from doing that in this PR. First is a technical issue: how do we pass back the message when the only output of the jvm options parser is the final jvm options?. The second issue is allowing cloud time to adapt to using auto sizing so that we do not spew errors across all cloud deployments. I created #66308 to ensure we followup with adding this warning.

Regarding whether to leave the heap options or remove them in a minor, we think the auto heap sizing is important enough that we don't want to have any more new users specifying heap. So, we should remove the Xmx/Xms from jvm.options. Users should no longer be relying on modifying jvm.options, but instead specifying these in jvm.options.d files.

Copy link
Member

Choose a reason for hiding this comment

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

@mark-vieira @jasontedor I've removed Xmx/Xms in 88a81a1, but left in a comment explaining where the heap size should be set if it needs to be.

# -Xmx${heap.max}



Expand Down
3 changes: 2 additions & 1 deletion distribution/tools/launchers/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ apply plugin: 'elasticsearch.build'

dependencies {
compileOnly project(':distribution:tools:java-version-checker')
compileOnly "org.yaml:snakeyaml:${versions.snakeyaml}"
testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
testImplementation "junit:junit:${versions.junit}"
testImplementation "org.hamcrest:hamcrest:${versions.hamcrest}"
Expand All @@ -44,4 +45,4 @@ tasks.named("testingConventions").configure {

["javadoc", "loggerUsageCheck", "jarHell"].each { tsk ->
tasks.named(tsk).configure { enabled = false }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.tools.launchers;

import com.sun.management.OperatingSystemMXBean;
import org.elasticsearch.tools.java_version_checker.JavaVersion;
import org.elasticsearch.tools.java_version_checker.SuppressForbidden;

import java.lang.management.ManagementFactory;

/**
* A {@link SystemMemoryInfo} which delegates to {@link OperatingSystemMXBean}.
*
* <p>Prior to JDK 14 {@link OperatingSystemMXBean} did not take into consideration container memory limits when reporting total system
* memory. Therefore attempts to use this implementation on earlier JDKs will result in an {@link SystemMemoryInfoException}.
*/
@SuppressForbidden(reason = "Using com.sun internals is the only way to query total system memory")
public final class DefaultSystemMemoryInfo implements SystemMemoryInfo {
private final OperatingSystemMXBean operatingSystemMXBean;

public DefaultSystemMemoryInfo() {
this.operatingSystemMXBean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();
}

@Override
@SuppressWarnings("deprecation")
public long availableSystemMemory() throws SystemMemoryInfoException {
if (JavaVersion.majorVersion(JavaVersion.CURRENT) < 14) {
throw new SystemMemoryInfoException("The minimum required Java version is 14 to use " + this.getClass().getName());
}

return operatingSystemMXBean.getTotalPhysicalMemorySize();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ private List<String> jvmOptions(final Path config, Path plugins, final String es
throws InterruptedException, IOException, JvmOptionsFileParserException {

final List<String> jvmOptions = readJvmOptionsFiles(config);
final MachineDependentHeap machineDependentHeap = new MachineDependentHeap(new DefaultSystemMemoryInfo());

if (esJavaOpts != null) {
jvmOptions.addAll(
Expand All @@ -142,6 +143,7 @@ private List<String> jvmOptions(final Path config, Path plugins, final String es
}

final List<String> substitutedJvmOptions = substitutePlaceholders(jvmOptions, Collections.unmodifiableMap(substitutions));
substitutedJvmOptions.addAll(machineDependentHeap.determineHeapSettings(config, substitutedJvmOptions));
final List<String> ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions);
final List<String> systemJvmOptions = SystemJvmOptions.systemJvmOptions();
final List<String> bootstrapOptions = BootstrapJvmOptions.bootstrapJvmOptions(plugins);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.tools.launchers;

import org.yaml.snakeyaml.Yaml;
import org.yaml.snakeyaml.error.YAMLException;

import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;

import static java.lang.Math.max;
import static java.lang.Math.min;

/**
* Determines optimal default heap settings based on available system memory and assigned node roles.
*/
public final class MachineDependentHeap {
private static final long GB = 1024L * 1024L * 1024L; // 1GB
private static final long MAX_HEAP_SIZE = GB * 31; // 31GB
private static final long MAX_ML_HEAP_SIZE = GB * 2; // 2GB
private static final long MIN_HEAP_SIZE = 1024 * 1024 * 128; // 128MB
private static final int DEFAULT_HEAP_SIZE_MB = 1024;
private static final String ELASTICSEARCH_YML = "elasticsearch.yml";

private final SystemMemoryInfo systemMemoryInfo;

public MachineDependentHeap(SystemMemoryInfo systemMemoryInfo) {
this.systemMemoryInfo = systemMemoryInfo;
}

/**
* Calculate heap options.
*
* @param configDir path to config directory
* @param userDefinedJvmOptions JVM arguments provided by the user
* @return final heap options, or an empty collection if user provided heap options are to be used
* @throws IOException if unable to load elasticsearch.yml
*/
public List<String> determineHeapSettings(Path configDir, List<String> userDefinedJvmOptions) throws IOException {
if (userDefinedJvmOptions.stream().anyMatch(s -> s.startsWith("-Xms") || s.startsWith("-Xmx"))) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think check is sufficient. A user can also set -XX:MaxHeapSize and -XX:MinHeapSize for which -Xmx and -Xms are synonyms.

Note that in JvmErgonomics#choose we start up a second JVM and extract whether or not a setting was passed in on the command line. We might want to reuse that here and then check if MinHeapSize or MaxHeapSize are passed on the command line. This might be more reliable?

Copy link
Member

Choose a reason for hiding this comment

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

I've moved the method to find the final options so it is reused here.

// User has explicitly set memory settings so we use those
return Collections.emptyList();
}

Path config = configDir.resolve(ELASTICSEARCH_YML);
try (InputStream in = Files.newInputStream(config)) {
return determineHeapSettings(in);
}
}

List<String> determineHeapSettings(InputStream config) {
MachineNodeRole nodeRole = NodeRoleParser.parse(config);

try {
long availableSystemMemory = systemMemoryInfo.availableSystemMemory();
return options(nodeRole.heap(availableSystemMemory));
} catch (SystemMemoryInfo.SystemMemoryInfoException e) {
// If unable to determine system memory (ex: incompatible jdk version) fallback to defaults
return options(DEFAULT_HEAP_SIZE_MB);
}
}

private static List<String> options(int heapSize) {
return List.of("-Xms" + heapSize + "m", "-Xmx" + heapSize + "m");
}

/**
* Parses role information from elasticsearch.yml and determines machine node role.
*/
static class NodeRoleParser {
private static final Set<String> LEGACY_ROLE_SETTINGS = Set.of(
"node.master",
"node.ingest",
"node.data",
"node.voting_only",
"node.ml",
"node.transform",
"node.remote_cluster_client"
);

@SuppressWarnings("unchecked")
public static MachineNodeRole parse(InputStream config) {
Yaml yaml = new Yaml();
Map<String, Object> root;
try {
root = yaml.load(config);
} catch (ClassCastException ex) {
// Strangely formatted config, so just return defaults and let startup settings validation catch the problem
return MachineNodeRole.UNKNOWN;
} catch (YAMLException ex) {
throw new IllegalStateException("Unable to parse elasticsearch.yml:", ex);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should contain the full path? Or if we should return UNKNOWN and let startup fail us here too?

Copy link
Member

Choose a reason for hiding this comment

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

I like returning UNKOWN and letting Elasticsearch handle the misconfiguration.

}

if (root != null) {
Map<String, Object> map = flatten(root, null);

if (hasLegacySettings(map.keySet())) {
// We don't attempt to auto-determine heap if legacy role settings are used
return MachineNodeRole.UNKNOWN;
} else {
List<String> roles = null;
try {
if (map.containsKey("node.roles")) {
roles = (List<String>) map.get("node.roles");
}
} catch (ClassCastException ex) {
throw new IllegalStateException("Unable to parse elasticsearch.yml. Expected 'node.roles' to be a list.");
Copy link
Member

Choose a reason for hiding this comment

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

Return UNKNOWN and let Elasticsearch handle it?

}

if (roles == null || roles.isEmpty()) {
// If roles are missing or empty (coordinating node) assume defaults and consider this a data node
return MachineNodeRole.DATA;
} else if (containsOnly(roles, "master", "voting_only")) {
return MachineNodeRole.MASTER_ONLY;
} else if (containsOnly(roles, "ml", "voting_only")) {
return MachineNodeRole.ML_ONLY;
mark-vieira marked this conversation as resolved.
Show resolved Hide resolved
} else {
return MachineNodeRole.DATA;
}
}
} else { // if the config is completely empty, then assume defaults and consider this a data node
return MachineNodeRole.DATA;
}
}

/**
* Flattens a nested configuration structure. This creates a consistent way of referencing settings from a config file that uses
* a mix of object and flat setting notation. The returned map is a single-level deep structure of dot-notation property names
* to values.
*
* <p>No attempt is made to deterministically deal with duplicate settings, nor are they explicitly disallowed.
*
* @param config nested configuration map
* @param parentPath parent node path or {@code null} if parsing the root node
* @return flattened configuration map
*/
@SuppressWarnings("unchecked")
private static Map<String, Object> flatten(Map<String, Object> config, String parentPath) {
Map<String, Object> flatMap = new HashMap<>();
String prefix = parentPath != null ? parentPath + "." : "";

for (Map.Entry<String, Object> entry : config.entrySet()) {
if (entry.getValue() instanceof Map) {
flatMap.putAll(flatten((Map<String, Object>) entry.getValue(), prefix + entry.getKey()));
} else {
flatMap.put(prefix + entry.getKey(), entry.getValue());
}
}

return flatMap;
}

@SuppressWarnings("unchecked")
private static <T> boolean containsOnly(Collection<T> collection, T... items) {
return Arrays.asList(items).containsAll(collection);
}

private static boolean hasLegacySettings(Set<String> keys) {
return LEGACY_ROLE_SETTINGS.stream().anyMatch(keys::contains);
}
}

enum MachineNodeRole {
/**
* Master-only node.
*
* <p>Heap is computed as 60% of total system memory up to a maximum of 31 gigabytes.
*/
MASTER_ONLY(m -> mb(min((long) (m * .6), MAX_HEAP_SIZE))),

/**
* Machine learning only node.
*
* <p>Heap is computed as:
* <ul>
* <li>40% of total system memory when less than 2 gigabytes.</li>
* <li>25% of total system memory when greater than 2 gigabytes up to a maximum of 2 gigabytes.</li>
* </ul>
*/
ML_ONLY(m -> mb(m < (GB * 2) ? (long) (m * .4) : (long) min(m * .25, MAX_ML_HEAP_SIZE))),

/**
* Data node. Essentially any node that isn't a master or ML only node.
*
* <p>Heap is computed as:
* <ul>
* <li>40% of total system memory when less than 1 gigabyte with a minimum of 128 megabytes.</li>
* <li>50% of total system memory when greater than 1 gigabyte up to a maximum of 31 gigabytes.</li>
* </ul>
*/
DATA(m -> mb(m < GB ? max((long) (m * .4), MIN_HEAP_SIZE) : min((long) (m * .5), MAX_HEAP_SIZE))),

/**
* Unknown role node.
*
* <p>Hard-code heap to a default of 1 gigabyte.
*/
UNKNOWN(m -> DEFAULT_HEAP_SIZE_MB);

private final Function<Long, Integer> formula;

MachineNodeRole(Function<Long, Integer> formula) {
this.formula = formula;
}

/**
* Determine the appropriate heap size for the given role and available system memory.
*
* @param systemMemory total available system memory in bytes
* @return recommended heap size in megabytes
*/
public int heap(long systemMemory) {
return formula.apply(systemMemory);
}

private static int mb(long bytes) {
return (int) (bytes / (1024 * 1024));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.tools.launchers;

/**
* Determines available system memory that could be allocated for Elasticsearch, to include JVM heap and other native processes.
* The "available system memory" is defined as the total system memory which is visible to the Elasticsearch process. For instances
* in which Elasticsearch is running in a containerized environment (i.e. Docker) this is expected to be the limits set for the container,
* not the host system.
*/
public interface SystemMemoryInfo {

/**
*
* @return total system memory available to heap or native process allocation in bytes
* @throws SystemMemoryInfoException if unable to determine available system memory
*/
long availableSystemMemory() throws SystemMemoryInfoException;

class SystemMemoryInfoException extends Exception {
public SystemMemoryInfoException(String message) {
super(message);
}
}
}
Loading