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

Add support of default replica count change #5610

Merged
merged 23 commits into from
Jan 6, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
6 changes: 5 additions & 1 deletion .idea/runConfigurations/Debug_OpenSearch.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

## [Unreleased 3.0]
### Added
- Add support of default replica count change using total awareness attribute([#5610](https://github.com/opensearch-project/OpenSearch/pull/5610))
Copy link
Collaborator

Choose a reason for hiding this comment

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

correct this .

- Hardened token permissions in GitHub workflows ([#4587](https://github.com/opensearch-project/OpenSearch/pull/4587))
- Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847))
- Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.opensearch.ResourceAlreadyExistsException;
import org.opensearch.action.admin.indices.alias.Alias;
import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.opensearch.action.admin.indices.template.delete.DeleteIndexTemplateRequestBuilder;
import org.opensearch.action.admin.indices.template.put.PutIndexTemplateRequestBuilder;
import org.opensearch.cluster.ClusterState;
import org.opensearch.cluster.metadata.AutoExpandReplicas;
Expand Down Expand Up @@ -299,6 +300,31 @@ public void testRolloverWithIndexSettingsBalancedReplica() throws Exception {
manageReplicaBalanceSetting(false);
}

public void testRolloverWithIndexSettingsBalancedWithUseZoneForReplicaDefaultCount() throws Exception {
DeleteIndexTemplateRequestBuilder deleteTemplate = client().admin().indices().prepareDeleteTemplate("random_index_template");
assertAcked(deleteTemplate.execute().actionGet());

Alias testAlias = new Alias("test_alias");
boolean explicitWriteIndex = randomBoolean();
if (explicitWriteIndex) {
testAlias.writeIndex(true);
}
assertAcked(prepareCreate("test_index-2").addAlias(testAlias).get());
manageUseZoneForReplicaSetting(true);
index("test_index-2", "type1", "1", "field", "value");
flush("test_index-2");

final Settings settings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build();
client().admin().indices().prepareRolloverIndex("test_alias").settings(settings).alias(new Alias("extra_alias")).get();

final ClusterState state = client().admin().cluster().prepareState().get().getState();
final IndexMetadata newIndex = state.metadata().index("test_index-000003");
assertThat(newIndex.getNumberOfShards(), equalTo(3));
assertThat(newIndex.getNumberOfReplicas(), equalTo(2));
manageUseZoneForReplicaSetting(false);
randomIndexTemplate();
}

public void testRolloverWithIndexSettingsWithoutPrefix() throws Exception {
Alias testAlias = new Alias("test_alias");
boolean explicitWriteIndex = randomBoolean();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@
package org.opensearch.indices.settings;

import org.opensearch.action.admin.cluster.health.ClusterHealthResponse;
import org.opensearch.action.admin.indices.template.delete.DeleteIndexTemplateRequestBuilder;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.action.support.ActiveShardCount;
import org.opensearch.action.support.IndicesOptions;
import org.opensearch.cluster.ClusterState;
import org.opensearch.cluster.health.ClusterHealthStatus;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.Priority;
Expand Down Expand Up @@ -663,4 +665,78 @@ public void testAwarenessReplicaBalance() {
}
}

public void testAwarenessReplicaBalanceWithUseZoneForDefaultReplicaCount() {
createIndex("aware-replica", Settings.builder().put("index.number_of_replicas", 0).build());
createIndex(".system-index", Settings.builder().put("index.number_of_replicas", 0).build());
DeleteIndexTemplateRequestBuilder deleteTemplate = client().admin().indices().prepareDeleteTemplate("random_index_template");
assertAcked(deleteTemplate.execute().actionGet());
manageUseZoneForReplicaSetting(true);
int updated = 0;

try {

// replica count should not be changed and we should see the original replica count
client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(Settings.builder().put("refresh_interval", "1s"))
.execute()
.actionGet();
updated++;

final ClusterState state = client().admin().cluster().prepareState().get().getState();
final IndexMetadata newIndex = state.metadata().index("aware-replica");
assertThat(newIndex.getNumberOfReplicas(), equalTo(0));

// replica count of 2 is ideal
client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2))
.execute()
.actionGet();
updated++;

// Since auto expand replica setting take precedence, this should pass
client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(
Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)
.put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-2")
)
.execute()
.actionGet();
updated++;

// system index - should be able to update
client().admin()
.indices()
.prepareUpdateSettings(".system-index")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 3))
.execute()
.actionGet();
updated++;

client().admin()
.indices()
.prepareUpdateSettings("aware-replica")
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.execute()
.actionGet();
fail("should have thrown an exception about the replica count");

} catch (IllegalArgumentException e) {
assertEquals(
"Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [3];",
e.getMessage()
);
assertEquals(4, updated);
} finally {
manageUseZoneForReplicaSetting(false);
randomIndexTemplate();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -1031,22 +1031,30 @@ public void testPartitionedTemplate() throws Exception {
}

public void testAwarenessReplicaBalance() throws IOException {
manageReplicaBalanceSetting(true);
manageUseZoneForReplicaSetting(true);
int updated = 0;
try {
client().admin()
.indices()
.preparePutTemplate("template_1")
.setPatterns(Arrays.asList("a*", "b*"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2))
.get();
updated++;

client().admin()
.indices()
.preparePutTemplate("template_1")
.setPatterns(Arrays.asList("a*", "b*"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 4))
.get();
updated++;

client().admin()
.indices()
.preparePutTemplate("template_1")
.setPatterns(Arrays.asList("a*", "b*"))
.setSettings(Settings.builder().put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-2"))
.get();
updated++;

Expand All @@ -1060,12 +1068,12 @@ public void testAwarenessReplicaBalance() throws IOException {
fail("should have thrown an exception about the replica count");
} catch (InvalidIndexTemplateException e) {
assertEquals(
"index_template [template_1] invalid, cause [Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [2];]",
"index_template [template_1] invalid, cause [Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [3];]",
e.getMessage()
);
assertEquals(2, updated);
assertEquals(3, updated);
} finally {
manageReplicaBalanceSetting(false);
manageUseZoneForReplicaSetting(false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.opensearch.action.admin.indices.template.delete.DeleteIndexTemplateRequestBuilder;
import org.opensearch.action.admin.indices.template.get.GetIndexTemplatesResponse;
import org.opensearch.action.index.IndexRequestBuilder;
import org.opensearch.client.Client;
Expand Down Expand Up @@ -977,14 +978,17 @@ public void testForbidDisableSoftDeletesDuringRestore() throws Exception {
public void testRestoreBalancedReplica() {
try {
createRepository("test-repo", "fs");
DeleteIndexTemplateRequestBuilder deleteTemplate = client().admin().indices().prepareDeleteTemplate("random_index_template");
assertAcked(deleteTemplate.execute().actionGet());

createIndex("test-index", Settings.builder().put("index.number_of_replicas", 0).build());
createIndex(".system-index", Settings.builder().put("index.number_of_replicas", 0).build());
ensureGreen();
clusterAdmin().prepareCreateSnapshot("test-repo", "snapshot-0")
.setIndices("test-index", ".system-index")
.setWaitForCompletion(true)
.get();
manageReplicaBalanceSetting(true);
manageUseZoneForReplicaSetting(true);

final IllegalArgumentException restoreError = expectThrows(
IllegalArgumentException.class,
Expand All @@ -996,7 +1000,7 @@ public void testRestoreBalancedReplica() {
);
assertThat(
restoreError.getMessage(),
containsString("expected total copies needs to be a multiple of total awareness attributes [2]")
containsString("expected total copies needs to be a multiple of total awareness attributes [3]")
);

final IllegalArgumentException restoreError2 = expectThrows(
Expand All @@ -1005,14 +1009,14 @@ public void testRestoreBalancedReplica() {
.setRenamePattern("test-index")
.setRenameReplacement("new-index-2")
.setIndexSettings(
Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 1).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-2").build()
Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 1).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1").build()
)
.setIndices("test-index")
.get()
);
assertThat(
restoreError2.getMessage(),
containsString("expected max cap on auto expand to be a multiple of total awareness attributes [2]")
containsString("expected max cap on auto expand to be a multiple of total awareness attributes [3]")
);

RestoreSnapshotResponse restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0")
Expand All @@ -1028,7 +1032,7 @@ public void testRestoreBalancedReplica() {
restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "snapshot-0")
.setRenamePattern("test-index")
.setRenameReplacement("new-index")
.setIndexSettings(Settings.builder().put("index.number_of_replicas", 1).build())
.setIndexSettings(Settings.builder().put("index.number_of_replicas", 2).build())
.setWaitForCompletion(true)
.setIndices("test-index")
.execute()
Expand All @@ -1038,7 +1042,7 @@ public void testRestoreBalancedReplica() {
.setRenamePattern("test-index")
.setRenameReplacement("new-index-3")
.setIndexSettings(
Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 0).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1").build()
Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 0).put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-2").build()
)
.setWaitForCompletion(true)
.setIndices("test-index")
Expand All @@ -1047,7 +1051,9 @@ public void testRestoreBalancedReplica() {

assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));
} finally {
manageReplicaBalanceSetting(false);
manageUseZoneForReplicaSetting(false);
randomIndexTemplate();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,13 @@ public interface Custom extends NamedDiffable<Custom>, ToXContentFragment, Clust
EnumSet<XContentContext> context();
}

public static final Setting<Integer> DEFAULT_REPLICA_COUNT_SETTING = Setting.intSetting(
"cluster.default_replica_count",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use cluster.default_number_of_replicas as to keep parity with index.number_of_replicas ?

1,
Property.Dynamic,
Property.NodeScope
);

public static final Setting<Boolean> SETTING_READ_ONLY_SETTING = Setting.boolSetting(
"cluster.blocks.read_only",
false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
import static org.opensearch.cluster.metadata.Metadata.DEFAULT_REPLICA_COUNT_SETTING;

/**
* Service responsible for submitting create index requests
Expand Down Expand Up @@ -554,7 +555,6 @@ private ClusterState applyCreateIndexRequestWithV1Templates(
xContentRegistry
)
);

final Settings aggregatedIndexSettings = aggregateIndexSettings(
currentState,
request,
Expand Down Expand Up @@ -862,7 +862,7 @@ static Settings aggregateIndexSettings(
indexSettingsBuilder.put(SETTING_NUMBER_OF_SHARDS, INDEX_NUMBER_OF_SHARDS_SETTING.get(settings));
}
if (INDEX_NUMBER_OF_REPLICAS_SETTING.exists(indexSettingsBuilder) == false) {
indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings));
indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, DEFAULT_REPLICA_COUNT_SETTING.get(currentState.metadata().settings()));
}
if (settings.get(SETTING_AUTO_EXPAND_REPLICAS) != null && indexSettingsBuilder.get(SETTING_AUTO_EXPAND_REPLICAS) == null) {
indexSettingsBuilder.put(SETTING_AUTO_EXPAND_REPLICAS, settings.get(SETTING_AUTO_EXPAND_REPLICAS));
Expand Down Expand Up @@ -1189,10 +1189,10 @@ List<String> getIndexSettingsValidationErrors(
validationErrors.addAll(validatePrivateSettingsNotExplicitlySet(settings, indexScopedSettings));
}
if (indexName.isEmpty() || indexName.get().charAt(0) != '.') {
// Apply aware replica balance only to non system indices
// Apply aware replica balance validation only to non system indices
int replicaCount = settings.getAsInt(
IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY)
DEFAULT_REPLICA_COUNT_SETTING.get(this.clusterService.state().metadata().settings())
Comment on lines -1192 to +1195
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to ensure system indices do not break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This validation will not be applied on the system index

   if (indexName.isEmpty() || indexName.get().charAt(0) != '.') { <-- line 1191

);
AutoExpandReplicas autoExpandReplica = AutoExpandReplicas.SETTING.get(settings);
Optional<String> error = awarenessReplicaBalance.validate(replicaCount, autoExpandReplica);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
import org.opensearch.common.settings.Setting;
import org.opensearch.common.settings.Settings;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.HashMap;

import static java.lang.Math.max;
import static org.opensearch.cluster.routing.allocation.decider.AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING;
Expand All @@ -31,8 +31,9 @@
* Helps in balancing shards across all awareness attributes and ensuring high availability of data.
*/
public class AwarenessReplicaBalance {
public static final String SETTING_CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE = "cluster.routing.allocation.awareness.balance";
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: do we need this?

public static final Setting<Boolean> CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING = Setting.boolSetting(
"cluster.routing.allocation.awareness.balance",
SETTING_CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE,
false,
Setting.Property.Dynamic,
Setting.Property.NodeScope
Expand All @@ -54,14 +55,17 @@ public AwarenessReplicaBalance(Settings settings, ClusterSettings clusterSetting
);
setAwarenessBalance(CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.get(settings));
clusterSettings.addSettingsUpdateConsumer(CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING, this::setAwarenessBalance);

}

private void setAwarenessBalance(Boolean awarenessBalance) {
this.awarenessBalance = awarenessBalance;
}

private void setForcedAwarenessAttributes(Settings forceSettings) {
this.forcedAwarenessAttributes = getForcedAwarenessAttributes(forceSettings);
}

public static Map<String, List<String>> getForcedAwarenessAttributes(Settings forceSettings) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same do we need this change

Map<String, List<String>> forcedAwarenessAttributes = new HashMap<>();
Map<String, Settings> forceGroups = forceSettings.getAsGroups();
for (Map.Entry<String, Settings> entry : forceGroups.entrySet()) {
Expand All @@ -70,7 +74,7 @@ private void setForcedAwarenessAttributes(Settings forceSettings) {
forcedAwarenessAttributes.put(entry.getKey(), aValues);
}
}
this.forcedAwarenessAttributes = forcedAwarenessAttributes;
return forcedAwarenessAttributes;
}

private void setAwarenessAttributes(List<String> awarenessAttributes) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ public void apply(Settings value, Settings current, Settings previous) {
MappingUpdatedAction.INDICES_MAX_IN_FLIGHT_UPDATES_SETTING,
Metadata.SETTING_READ_ONLY_SETTING,
Metadata.SETTING_READ_ONLY_ALLOW_DELETE_SETTING,
Metadata.DEFAULT_REPLICA_COUNT_SETTING,
ShardLimitValidator.SETTING_CLUSTER_MAX_SHARDS_PER_NODE,
ShardLimitValidator.SETTING_CLUSTER_IGNORE_DOT_INDEXES,
RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING,
Expand Down
Loading