Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Lakshya Taragi <[email protected]>
  • Loading branch information
@ltaragi
ltaragi committed Apr 1, 2024
1 parent 33c3cd2 commit 5e3b82a
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 264 deletions.
66 changes: 18 additions & 48 deletions ...rnalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public class RemoteStoreMigrationAllocationIT extends MigrationBaseTestCase {

// tests for primary shard copy allocation with MIXED mode and REMOTE_STORE direction

public void testDontAllocateNewPrimaryShardOnNonRemoteNodeForMixedModeAndRemoteStoreDirection() throws Exception {
public void testAllocateNewPrimaryShardForMixedModeAndRemoteStoreDirection() throws Exception {
logger.info(" --> initialize cluster");
initializeCluster(false);

Expand All @@ -71,59 +71,29 @@ public void testDontAllocateNewPrimaryShardOnNonRemoteNodeForMixedModeAndRemoteS
setDirection(REMOTE_STORE.direction);

Decision decision = getDecisionForTargetNode(nonRemoteNode, true, true, false);
Decision.Type type = Decision.Type.NO;
assertEquals(type, decision.type());
assertEquals(Decision.Type.NO, decision.type());
assertEquals(
"[remote_store migration_direction]: primary shard copy can not be allocated to a non-remote node",
decision.getExplanation().toLowerCase(Locale.ROOT)
);

logger.info(" --> attempt allocation");
logger.info(" --> attempt allocation on non-remote node");
attemptAllocation(nonRemoteNodeName);

logger.info(" --> verify non-allocation of primary shard");
logger.info(" --> verify non-allocation of primary shard on non-remote node");
assertNonAllocation(true);
}

public void testAllocateNewPrimaryShardOnRemoteNodeForMixedModeAndRemoteStoreDirection() throws Exception {
logger.info(" --> initialize cluster");
initializeCluster(false);

logger.info(" --> add remote and non-remote nodes");
setClusterMode(MIXED.mode);
addRemote = true;
String remoteNodeName = internalCluster().startNode();
addRemote = false;
String nonRemoteNodeName = internalCluster().startNode();
internalCluster().validateClusterFormed();
DiscoveryNode remoteNode = assertNodeInCluster(remoteNodeName);
DiscoveryNode nonRemoteNode = assertNodeInCluster(nonRemoteNodeName);

logger.info(" --> verify expected decision for allocating a new primary shard on a remote node");
prepareIndexWithoutReplica(Optional.empty());

logger.info(" --> set remote_store direction");
setDirection(REMOTE_STORE.direction);

Decision decision = getDecisionForTargetNode(remoteNode, true, true, false);
prepareDecisions();
decision = getDecisionForTargetNode(remoteNode, true, true, false);
assertEquals(Decision.Type.YES, decision.type());
assertEquals(
"[remote_store migration_direction]: primary shard copy can be allocated to a remote node",
decision.getExplanation().toLowerCase(Locale.ROOT)
);

logger.info(" --> attempt allocation");
client.admin()
.indices()
.prepareUpdateSettings(TEST_INDEX)
.setSettings(
Settings.builder()
.put("index.routing.allocation.include._name", allNodesExcept(null))
.put("index.routing.allocation.exclude._name", "")
)
.execute()
.actionGet();

logger.info(" --> attempt allocation on remote node");
attemptAllocation(remoteNodeName);
ensureGreen(TEST_INDEX);

logger.info(" --> verify allocation of primary shard");
Expand Down Expand Up @@ -236,11 +206,11 @@ public void testAllocateNewReplicaShardOnNonRemoteNodeIfPrimaryShardOnNonRemoteN
logger.info(" --> verify expected decision for replica shard");
prepareDecisions();
Decision decision = getDecisionForTargetNode(nonRemoteNode2, false, true, false);
Decision.Type type = Decision.Type.YES;
String reason = "[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node";
Decision.Type expectedType = Decision.Type.YES;
String expectedReason = "[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node";

assertEquals(type, decision.type());
assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
assertEquals(expectedType, decision.type());
assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));

logger.info(" --> allocate replica shard on the other non-remote node");
attemptAllocation(nonRemoteNodeName2);
Expand Down Expand Up @@ -276,8 +246,8 @@ public void testAllocateNewReplicaShardOnNonRemoteNodeIfPrimaryShardOnRemoteNode
prepareDecisions();
Decision decision = getDecisionForTargetNode(nonRemoteNode, false, true, false);

Decision.Type type = Decision.Type.YES;
assertEquals(type, decision.type());
Decision.Type expectedType = Decision.Type.YES;
assertEquals(expectedType, decision.type());
assertEquals(
"[remote_store migration_direction]: replica shard copy can be allocated to a non-remote node",
decision.getExplanation().toLowerCase(Locale.ROOT)
Expand Down Expand Up @@ -352,13 +322,13 @@ public void testAlwaysAllocateNewShardForStrictMode() throws Exception {
prepareDecisions();
Decision decision = getDecisionForTargetNode(targetNode, !isReplicaAllocation, true, false);
assertEquals(Decision.Type.YES, decision.type());
String reason = String.format(
String expectedReason = String.format(
Locale.ROOT,
"[remote_store migration_direction]: %s shard copy can be allocated to a %s node for strict compatibility mode",
(isReplicaAllocation ? "replica" : "primary"),
(isRemoteCluster ? "remote" : "non-remote")
);
assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));

logger.info(" --> attempt allocation");
attemptAllocation(targetNode.getName());
Expand Down Expand Up @@ -410,12 +380,12 @@ public void testDontAllocateToNonRemoteNodeForRemoteStoreBackedIndex() throws Ex
prepareDecisions();
Decision decision = getDecisionForTargetNode(nonRemoteNode, !isReplicaAllocation, false, false);
assertEquals(Decision.Type.NO, decision.type());
String reason = String.format(
String expectedReason = String.format(
Locale.ROOT,
"[remote_store migration_direction]: %s shard copy can not be allocated to a non-remote node because a remote store backed index's shard copy can only be allocated to a remote node",
(isReplicaAllocation ? "replica" : "primary")
);
assertEquals(reason, decision.getExplanation().toLowerCase(Locale.ROOT));
assertEquals(expectedReason, decision.getExplanation().toLowerCase(Locale.ROOT));

logger.info(" --> attempt allocation of shard on non-remote node");
attemptAllocation(nonRemoteNodeName);
Expand Down
79 changes: 4 additions & 75 deletions ...ClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,14 @@

import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.settings.SettingsException;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.index.IndexSettings;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.snapshots.SnapshotInfo;
import org.opensearch.snapshots.SnapshotState;
import org.opensearch.test.InternalTestCluster;
import org.opensearch.test.OpenSearchIntegTestCase;

import java.nio.file.Path;
import java.util.Optional;

import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY;
Expand All @@ -38,7 +34,6 @@
import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.prepareIndexWithoutReplica;
import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.setClusterMode;
import static org.opensearch.remotemigration.RemoteStoreMigrationAllocationIT.setDirection;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, autoManageMasterNodes = false)
public class RemoteStoreMigrationSettingsUpdateIT extends MigrationBaseTestCase {
Expand All @@ -56,9 +51,9 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()

logger.info(" --> add non-remote node");
addRemote = false;
String remoteNodeName = internalCluster().startNode();
String nonRemoteNodeName = internalCluster().startNode();
internalCluster().validateClusterFormed();
assertNodeInCluster(remoteNodeName);
assertNodeInCluster(nonRemoteNodeName);

logger.info(" --> create an index");
prepareIndexWithoutReplica(Optional.of(indexName1));
Expand All @@ -72,9 +67,9 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()

logger.info(" --> add remote node");
addRemote = true;
String nonRemoteNodeName = internalCluster().startNode();
String remoteNodeName = internalCluster().startNode();
internalCluster().validateClusterFormed();
assertNodeInCluster(nonRemoteNodeName);
assertNodeInCluster(remoteNodeName);

logger.info(" --> create another index");
prepareIndexWithoutReplica(Optional.of(indexName2));
Expand All @@ -83,72 +78,6 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode()
assertRemoteStoreBackedIndex(indexName2);
}

public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() throws Exception {
logger.info(" --> initialize cluster: gives non remote cluster manager");
initializeCluster(false);

logger.info(" --> add remote and non-remote nodes");
setClusterMode(MIXED.mode);
addRemote = false;
String nonRemoteNodeName = internalCluster().startNode();
addRemote = true;
String remoteNodeName = internalCluster().startNode();
internalCluster().validateClusterFormed();
assertNodeInCluster(nonRemoteNodeName);
assertNodeInCluster(remoteNodeName);

logger.info(" --> create a non remote-backed index");
client.admin()
.indices()
.prepareCreate(TEST_INDEX)
.setSettings(
Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0).build()
)
.get();

logger.info(" --> verify that non remote stored backed index is created");
assertNonRemoteStoreBackedIndex(TEST_INDEX);

logger.info(" --> create repository");
String snapshotName = "test-snapshot";
String snapshotRepoName = "test-restore-snapshot-repo";
Path snapshotRepoNameAbsolutePath = randomRepoPath().toAbsolutePath();
assertAcked(
clusterAdmin().preparePutRepository(snapshotRepoName)
.setType("fs")
.setSettings(Settings.builder().put("location", snapshotRepoNameAbsolutePath))
);

logger.info(" --> create snapshot of non remote stored backed index");

SnapshotInfo snapshotInfo = client().admin()
.cluster()
.prepareCreateSnapshot(snapshotRepoName, snapshotName)
.setIndices(TEST_INDEX)
.setWaitForCompletion(true)
.get()
.getSnapshotInfo();

assertEquals(SnapshotState.SUCCESS, snapshotInfo.state());
assertTrue(snapshotInfo.successfulShards() > 0);
assertEquals(0, snapshotInfo.failedShards());

logger.info(" --> restore index from snapshot under NONE direction");
String restoredIndexName1 = TEST_INDEX + "-restored1";
restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName1);

logger.info(" --> verify that restored index is non remote-backed");
assertNonRemoteStoreBackedIndex(restoredIndexName1);

logger.info(" --> restore index from snapshot under REMOTE_STORE direction");
setDirection(REMOTE_STORE.direction);
String restoredIndexName2 = TEST_INDEX + "-restored2";
restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName2);

logger.info(" --> verify that restored index is non remote-backed");
assertRemoteStoreBackedIndex(restoredIndexName2);
}

// compatibility mode setting test

public void testSwitchToStrictMode() throws Exception {
Expand Down
29 changes: 16 additions & 13 deletions ...va/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@
import org.opensearch.transport.TransportService;

import java.io.IOException;
import java.util.Map;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

/**
* Transport action for updating cluster settings
Expand Down Expand Up @@ -268,21 +270,22 @@ public ClusterState execute(final ClusterState currentState) {
);
}

private void validateCompatibilityModeSettingRequest(ClusterUpdateSettingsRequest request, ClusterState state) {
/**
* Verifies that while trying to switch to STRICT compatibility mode, all nodes must be of the
* same type (all remote or all non-remote). If not, it throws SettingsException error
* @param request cluster settings update request, for settings to be updated and new values
* @param clusterState current state of cluster, for information on nodes
*/
private void validateCompatibilityModeSettingRequest(ClusterUpdateSettingsRequest request, ClusterState clusterState) {
if (RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.exists(request.persistentSettings())) {
String value = request.persistentSettings().get(RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey());
if (value.equals(RemoteStoreNodeService.CompatibilityMode.STRICT.mode)) {
boolean hasRemoteNode = false, hasNonRemoteNode = false;
Map<String, DiscoveryNode> nodes = state.nodes().getNodes();
for (Map.Entry<String, DiscoveryNode> entry : nodes.entrySet()) {
DiscoveryNode node = entry.getValue();
if (node.isRemoteStoreNode()) {
hasRemoteNode = true;
continue;
}
hasNonRemoteNode = true;
}
if (hasRemoteNode && hasNonRemoteNode) {
List<DiscoveryNode> discoveryNodeList = new ArrayList<>(clusterState.nodes().getNodes().values());
Optional<DiscoveryNode> remoteNode = discoveryNodeList.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst();
Optional<DiscoveryNode> nonRemoteNode = discoveryNodeList.stream()
.filter(dn -> dn.isRemoteStoreNode() == false)
.findFirst();
if (remoteNode.isPresent() && nonRemoteNode.isPresent()) {
throw new SettingsException(
"can not switch to STRICT compatibility mode when the cluster contains both remote and non-remote nodes"
);
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,8 @@ public Iterator<Setting<?>> settings() {
Property.Final
);

public static final String SETTING_REMOTE_STORE_PREFIX = "index.remote_store.";

public static final String SETTING_REMOTE_STORE_ENABLED = "index.remote_store.enabled";

public static final String SETTING_REMOTE_SEGMENT_STORE_REPOSITORY = "index.remote_store.segment.repository";
Expand Down
Loading

0 comments on commit 5e3b82a

Please sign in to comment.