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.

Sign up for GitHub

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

Breaking change for single data node setting #73737

Merged
merged 9 commits into from
Jun 7, 2021
Merged

Breaking change for single data node setting #73737

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f368bb8
Breaking change for single data node setting
henningandersen Jun 3, 2021
8e8e62b
cs
henningandersen Jun 4, 2021
50a3df6
close block
henningandersen Jun 4, 2021
108ae11
matcher contains is not collection contains
henningandersen Jun 4, 2021
91eb0b1
Remove obsolete test for non-single data node
henningandersen Jun 4, 2021
4348b92
Randomly set the setting.
henningandersen Jun 4, 2021
50c489e
Update docs/reference/modules/cluster/disk_allocator.asciidoc
henningandersen Jun 4, 2021
c5b5adb
Remove the check, since always true.
henningandersen Jun 4, 2021
12e3ca6
Breaking breaking change changes...
henningandersen Jun 4, 2021
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
15 changes: 15 additions & 0 deletions docs/reference/migration/migrate_8_0/settings.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,3 +280,18 @@ per index data path settings.

*Impact* +
Discontinue use of the deprecated settings.

[[single-data-node-watermark-setting]]
.Single data node watermark setting deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also consider the change in allocation behaviour on a single node to be a breaking change, and indicate that folks should set cluster.routing.allocation.disk.threshold_enabled: false if they want to keep the old behaviour?

[%collapsible]
====
*Details* +
In 7.14, setting `cluster.routing.allocation.disk.watermark.enable_for_single_data_node`
to false was deprecated. Starting in 8.0, the only legal value will be
true. In a future release, the setting will be removed completely, with same
behavior as if the setting was `true`.

*Impact* +
Discontinue use of the deprecated setting.
====

8 changes: 4 additions & 4 deletions docs/reference/modules/cluster/disk_allocator.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ Controls the high watermark. It defaults to `90%`, meaning that {es} will attemp

`cluster.routing.allocation.disk.watermark.enable_for_single_data_node`::
(<<static-cluster-setting,Static>>)
For a single data node, the default is to disregard disk watermarks when
making an allocation decision. This is deprecated behavior and will be
changed in 8.0. This setting can be set to `true` to enable the
disk watermarks for a single data node cluster (will become default in 8.0).
In earlier releases, the default were to disregard disk watermarks for a single
henningandersen marked this conversation as resolved.
Show resolved Hide resolved
data node cluster when making an allocation decision. This is deprecated behavior
since 7.14 and has been removed in 8.0. The only valid value for this setting
is now `true`. The setting will be removed in a future release.

[[cluster-routing-flood-stage]]
// tag::cluster-routing-flood-stage-tag[]
Expand Down
15 changes: 14 additions & 1 deletion .../main/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@
import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.shard.ShardId;
Expand Down Expand Up @@ -66,11 +68,22 @@
public class DiskThresholdDecider extends AllocationDecider {

private static final Logger logger = LogManager.getLogger(DiskThresholdDecider.class);
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(DiskThresholdDecider.class);

public static final String NAME = "disk_threshold";

public static final Setting<Boolean> ENABLE_FOR_SINGLE_DATA_NODE =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this means the enableForSingleDataNode field is now always true and so can be inlined.

Setting.boolSetting("cluster.routing.allocation.disk.watermark.enable_for_single_data_node", false, Setting.Property.NodeScope);
Setting.boolSetting("cluster.routing.allocation.disk.watermark.enable_for_single_data_node", true,
new Setting.Validator<>() {
@Override
public void validate(Boolean value) {
if (value == Boolean.FALSE) {
throw new SettingsException("setting [{}=false] is not allowed, only true is valid",
ENABLE_FOR_SINGLE_DATA_NODE.getKey());
}
}
},
Setting.Property.NodeScope, Setting.Property.Deprecated);

public static final Setting<Boolean> SETTING_IGNORE_DISK_WATERMARKS =
Setting.boolSetting("index.routing.allocation.disk.watermark.ignore", false,
Expand Down
158 changes: 26 additions & 132 deletions .../java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDeciderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.elasticsearch.common.UUIDs;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.shard.ShardId;
Expand Down Expand Up @@ -925,141 +926,15 @@ Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLU
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().relocatingNodeId(), equalTo("node2"));
}

public void testForSingleDataNode() {
// remove test in 9.0
Settings diskSettings = Settings.builder()
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_THRESHOLD_ENABLED_SETTING.getKey(), true)
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey(), "60%")
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "70%").build();

ImmutableOpenMap.Builder<String, DiskUsage> usagesBuilder = ImmutableOpenMap.builder();
usagesBuilder.put("node1", new DiskUsage("node1", "n1", "/dev/null", 100, 100)); // 0% used
usagesBuilder.put("node2", new DiskUsage("node2", "n2", "/dev/null", 100, 20)); // 80% used
usagesBuilder.put("node3", new DiskUsage("node3", "n3", "/dev/null", 100, 100)); // 0% used
ImmutableOpenMap<String, DiskUsage> usages = usagesBuilder.build();

// We have an index with 1 primary shards each taking 40 bytes. Each node has 100 bytes available
ImmutableOpenMap.Builder<String, Long> shardSizes = ImmutableOpenMap.builder();
shardSizes.put("[test][0][p]", 40L);
shardSizes.put("[test][1][p]", 40L);
final ClusterInfo clusterInfo = new DevNullClusterInfo(usages, usages, shardSizes.build());

DiskThresholdDecider diskThresholdDecider = makeDecider(diskSettings);
Metadata metadata = Metadata.builder()
.put(IndexMetadata.builder("test").settings(settings(Version.CURRENT)).numberOfShards(2).numberOfReplicas(0))
.build();

RoutingTable initialRoutingTable = RoutingTable.builder()
.addAsNew(metadata.index("test"))
.build();

logger.info("--> adding one master node, one data node");
DiscoveryNode discoveryNode1 = new DiscoveryNode("", "node1", buildNewFakeTransportAddress(), emptyMap(),
singleton(DiscoveryNodeRole.MASTER_ROLE), Version.CURRENT);
DiscoveryNode discoveryNode2 = new DiscoveryNode("", "node2", buildNewFakeTransportAddress(), emptyMap(),
singleton(DiscoveryNodeRole.DATA_ROLE), Version.CURRENT);

DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(discoveryNode1).add(discoveryNode2).build();
ClusterState baseClusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY))
.metadata(metadata)
.routingTable(initialRoutingTable)
.nodes(discoveryNodes)
.build();

// Two shards consumes 80% of disk space in data node, but we have only one data node, shards should remain.
ShardRouting firstRouting = TestShardRouting.newShardRouting("test", 0, "node2", null, true, ShardRoutingState.STARTED);
ShardRouting secondRouting = TestShardRouting.newShardRouting("test", 1, "node2", null, true, ShardRoutingState.STARTED);
RoutingNode firstRoutingNode = new RoutingNode("node2", discoveryNode2, firstRouting, secondRouting);

RoutingTable.Builder builder = RoutingTable.builder().add(
IndexRoutingTable.builder(firstRouting.index())
.addIndexShard(new IndexShardRoutingTable.Builder(firstRouting.shardId())
.addShard(firstRouting)
.build()
)
.addIndexShard(new IndexShardRoutingTable.Builder(secondRouting.shardId())
.addShard(secondRouting)
.build()
)
);
ClusterState clusterState = ClusterState.builder(baseClusterState).routingTable(builder.build()).build();
RoutingAllocation routingAllocation = new RoutingAllocation(null, new RoutingNodes(clusterState), clusterState, clusterInfo,
null, System.nanoTime());
routingAllocation.debugDecision(true);
Decision decision = diskThresholdDecider.canRemain(firstRouting, firstRoutingNode, routingAllocation);

// Two shards should start happily
assertThat(decision.type(), equalTo(Decision.Type.YES));
assertThat(decision.getExplanation(), containsString("there is only a single data node present"));
ClusterInfoService cis = () -> {
logger.info("--> calling fake getClusterInfo");
return clusterInfo;
};

AllocationDeciders deciders = new AllocationDeciders(new HashSet<>(Arrays.asList(
new SameShardAllocationDecider(
Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)
),
diskThresholdDecider
)));

AllocationService strategy = new AllocationService(deciders, new TestGatewayAllocator(),
new BalancedShardsAllocator(Settings.EMPTY), cis, EmptySnapshotsInfoService.INSTANCE);
ClusterState result = strategy.reroute(clusterState, "reroute");

assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().state(), equalTo(STARTED));
assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().currentNodeId(), equalTo("node2"));
assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().relocatingNodeId(), nullValue());
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().state(), equalTo(STARTED));
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().currentNodeId(), equalTo("node2"));
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().relocatingNodeId(), nullValue());

// Add another datanode, it should relocate.
logger.info("--> adding node3");
DiscoveryNode discoveryNode3 = new DiscoveryNode("", "node3", buildNewFakeTransportAddress(), emptyMap(),
singleton(DiscoveryNodeRole.DATA_ROLE), Version.CURRENT);
ClusterState updateClusterState = ClusterState.builder(clusterState).nodes(DiscoveryNodes.builder(clusterState.nodes())
.add(discoveryNode3)).build();

firstRouting = TestShardRouting.newShardRouting("test", 0, "node2", null, true, ShardRoutingState.STARTED);
secondRouting = TestShardRouting.newShardRouting("test", 1, "node2", "node3", true, ShardRoutingState.RELOCATING);
firstRoutingNode = new RoutingNode("node2", discoveryNode2, firstRouting, secondRouting);
builder = RoutingTable.builder().add(
IndexRoutingTable.builder(firstRouting.index())
.addIndexShard(new IndexShardRoutingTable.Builder(firstRouting.shardId())
.addShard(firstRouting)
.build()
)
.addIndexShard(new IndexShardRoutingTable.Builder(secondRouting.shardId())
.addShard(secondRouting)
.build()
)
);

clusterState = ClusterState.builder(updateClusterState).routingTable(builder.build()).build();
routingAllocation = new RoutingAllocation(null, new RoutingNodes(clusterState), clusterState, clusterInfo, null,
System.nanoTime());
routingAllocation.debugDecision(true);
decision = diskThresholdDecider.canRemain(firstRouting, firstRoutingNode, routingAllocation);
assertThat(decision.type(), equalTo(Decision.Type.YES));
assertThat(((Decision.Single) decision).getExplanation(), containsString(
"there is enough disk on this node for the shard to remain, free: [60b]"));

result = strategy.reroute(clusterState, "reroute");
assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().state(), equalTo(STARTED));
assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().currentNodeId(), equalTo("node2"));
assertThat(result.routingTable().index("test").getShards().get(0).primaryShard().relocatingNodeId(), nullValue());
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().state(), equalTo(RELOCATING));
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().currentNodeId(), equalTo("node2"));
assertThat(result.routingTable().index("test").getShards().get(1).primaryShard().relocatingNodeId(), equalTo("node3"));
}

public void testWatermarksEnabledForSingleDataNode() {
Settings diskSettings = Settings.builder()
.put(DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.getKey(), true)
Settings.Builder builder = Settings.builder()
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_THRESHOLD_ENABLED_SETTING.getKey(), true)
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey(), "60%")
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "70%").build();
.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "70%");
if (randomBoolean()) {
builder.put(DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.getKey(), true);
}
Settings diskSettings = builder.build();

ImmutableOpenMap.Builder<String, DiskUsage> usagesBuilder = ImmutableOpenMap.builder();
usagesBuilder.put("data", new DiskUsage("data", "data", "/dev/null", 100, 20)); // 80% used
Expand Down Expand Up @@ -1131,6 +1006,25 @@ Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLU
"the shard cannot remain on this node because it is above the high watermark cluster setting" +
" [cluster.routing.allocation.disk.watermark.high=70%] and there is less than the required [30.0%] free disk on node," +
" actual free: [20.0%]"));

if (DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.exists(diskSettings)) {
assertSettingDeprecationsAndWarnings(new Setting<?>[] { DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE });
}
}

public void testSingleDataNodeDeprecationWarning() {
Settings settings = Settings.builder()
.put(DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.getKey(), false)
.build();

IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new DiskThresholdDecider(settings, new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)));

assertThat(e.getCause().getMessage(),
equalTo("setting [cluster.routing.allocation.disk.watermark.enable_for_single_data_node=false] is not allowed," +
" only true is valid"));

assertSettingDeprecationsAndWarnings(new Setting<?>[]{ DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE });
}

public void testDiskThresholdWithSnapshotShardSizes() {
Expand Down
3 changes: 2 additions & 1 deletion ...ugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ private DeprecationChecks() {
Collections.emptyList();

static List<BiFunction<Settings, PluginsAndModules, DeprecationIssue>> NODE_SETTINGS_CHECKS = List.of(
NodeDeprecationChecks::checkSharedDataPathSetting
NodeDeprecationChecks::checkSharedDataPathSetting,
NodeDeprecationChecks::checkSingleDataNodeWatermarkSetting
);

static List<Function<IndexMetadata, DeprecationIssue>> INDEX_SETTINGS_CHECKS = List.of(
Expand Down
15 changes: 15 additions & 0 deletions .../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.xpack.deprecation;

import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules;
import org.elasticsearch.cluster.routing.allocation.decider.DiskThresholdDecider;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
Expand Down Expand Up @@ -125,4 +126,18 @@ static DeprecationIssue checkSharedDataPathSetting(final Settings settings, fina
}
return null;
}

static DeprecationIssue checkSingleDataNodeWatermarkSetting(final Settings settings, final PluginsAndModules pluginsAndModules) {
if (DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.exists(settings)) {
String key = DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.getKey();
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
String.format(Locale.ROOT, "setting [%s] is deprecated and will not be available in a future version", key),
"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/" +
"breaking-changes-7.14.html#deprecate-single-data-node-watermark",
String.format(Locale.ROOT, "found [%s] configured. Discontinue use of this setting.", key)
);
}

return null;
}
}
38 changes: 30 additions & 8 deletions ...ecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,22 @@

package org.elasticsearch.xpack.deprecation;

import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;

import java.util.List;

import static org.elasticsearch.xpack.deprecation.DeprecationChecks.NODE_SETTINGS_CHECKS;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;

import java.util.List;

import org.elasticsearch.cluster.routing.allocation.decider.DiskThresholdDecider;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;

public class NodeDeprecationChecksTests extends ESTestCase {

public void testRemovedSettingNotSet() {
Expand Down Expand Up @@ -62,4 +64,24 @@ public void testSharedDataPathSetting() {
"Found shared data path configured. Discontinue use of this setting."
)));
}

public void testSingleDataNodeWatermarkSetting() {
Settings settings = Settings.builder()
.put(DiskThresholdDecider.ENABLE_FOR_SINGLE_DATA_NODE.getKey(), true)
.build();

List<DeprecationIssue> issues = DeprecationChecks.filterChecks(NODE_SETTINGS_CHECKS, c -> c.apply(settings, null));

final String expectedUrl =
"https://www.elastic.co/guide/en/elasticsearch/reference/7.14/" +
"breaking-changes-7.14.html#deprecate-single-data-node-watermark";
assertThat(issues, hasItem(
new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
"setting [cluster.routing.allocation.disk.watermark.enable_for_single_data_node] is deprecated and" +
" will not be available in a future version",
expectedUrl,
"found [cluster.routing.allocation.disk.watermark.enable_for_single_data_node] configured." +
" Discontinue use of this setting."
)));
}
}