diff --git a/CHANGELOG.md b/CHANGELOG.md index b3aa88360e2e8..af736f81cf654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Service does not start on Windows with OpenJDK ([#20615](https://github.com/opensearch-project/OpenSearch/pull/20615)) - Update RemoteClusterStateCleanupManager to performed batched deletions of stale ClusterMetadataManifests and address deletion timeout issues ([#20566](https://github.com/opensearch-project/OpenSearch/pull/20566)) - Fix the regression of terms agg optimization at high cardinality ([#20623](https://github.com/opensearch-project/OpenSearch/pull/20623)) +- Fix Shard routings for closed index are allocated again without opening the index ([#20648](https://github.com/opensearch-project/OpenSearch/pull/20648)) - Leveraging segment-global ordinal mapping for efficient terms aggregation ([#20624](https://github.com/opensearch-project/OpenSearch/pull/20624)) - Support Docker distribution builds for ppc64le, arm64 and s390x ([#20678](https://github.com/opensearch-project/OpenSearch/pull/20678)) - Harden detection of HTTP/3 support by ensuring Quic native libraries are available for the target platform ([#20680](https://github.com/opensearch-project/OpenSearch/pull/20680)) diff --git a/server/src/main/java/org/opensearch/cluster/routing/ShardRouting.java b/server/src/main/java/org/opensearch/cluster/routing/ShardRouting.java index 81558d00aa697..dd47453883f25 100644 --- a/server/src/main/java/org/opensearch/cluster/routing/ShardRouting.java +++ b/server/src/main/java/org/opensearch/cluster/routing/ShardRouting.java @@ -956,4 +956,11 @@ public boolean unassignedReasonIndexCreated() { } return false; } + + public boolean isClosedIndexShard() { + if (unassigned() && unassignedInfo != null) { + return unassignedInfo.getReason() == UnassignedInfo.Reason.INDEX_CLOSED; + } + return false; + } } diff --git a/server/src/main/java/org/opensearch/gateway/GatewayAllocator.java b/server/src/main/java/org/opensearch/gateway/GatewayAllocator.java index eaacb5dbfbd17..36ef4813f8018 100644 --- a/server/src/main/java/org/opensearch/gateway/GatewayAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/GatewayAllocator.java @@ -180,6 +180,10 @@ protected static void innerAllocatedUnassigned( ExistingShardsAllocator.UnassignedAllocationHandler unassignedAllocationHandler ) { assert shardRouting.unassigned(); + // Skip allocation for closed index shards + if (shardRouting.isClosedIndexShard()) { + return; + } if (shardRouting.primary()) { primaryShardAllocator.allocateUnassigned(shardRouting, allocation, unassignedAllocationHandler); } else { diff --git a/server/src/main/java/org/opensearch/gateway/ShardsBatchGatewayAllocator.java b/server/src/main/java/org/opensearch/gateway/ShardsBatchGatewayAllocator.java index d72716a461389..d2b9ca83cafe3 100644 --- a/server/src/main/java/org/opensearch/gateway/ShardsBatchGatewayAllocator.java +++ b/server/src/main/java/org/opensearch/gateway/ShardsBatchGatewayAllocator.java @@ -401,6 +401,10 @@ protected Set createAndUpdateBatches(RoutingAllocation allocation, boole Set batchedShardsToAssign = Sets.newHashSet(); // add all unassigned shards to the batch if they are not already in a batch unassigned.forEach(shardRouting -> { + // Skip allocation for closed index shards - for new shards and already-batched shards whose index subsequently closed + if (shardRouting.isClosedIndexShard()) { + return; + } if ((currentBatchedShards.containsKey(shardRouting.shardId()) == false) && (shardRouting.primary() == primary)) { assert shardRouting.unassigned(); newShardsToBatch.put(shardRouting.shardId(), shardRouting); diff --git a/server/src/test/java/org/opensearch/cluster/routing/ShardRoutingTests.java b/server/src/test/java/org/opensearch/cluster/routing/ShardRoutingTests.java index 63c7b5f70f85c..3620da54b62ed 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/ShardRoutingTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/ShardRoutingTests.java @@ -313,6 +313,44 @@ public void testSwapPrimaryWithReplica() { assertFalse(activeReplicaShard1.primary()); } + public void testIsClosedIndexShard() { + ShardRouting closedShard = TestShardRouting.newShardRouting( + "test", + 0, + null, + null, + false, + ShardRoutingState.UNASSIGNED, + new UnassignedInfo(UnassignedInfo.Reason.INDEX_CLOSED, "index closed") + ); + assertTrue(closedShard.isClosedIndexShard()); + + ShardRouting createdShard = TestShardRouting.newShardRouting( + "test", + 0, + null, + null, + false, + ShardRoutingState.UNASSIGNED, + new UnassignedInfo(UnassignedInfo.Reason.INDEX_CREATED, "index created") + ); + assertFalse(createdShard.isClosedIndexShard()); + + ShardRouting startedShard = TestShardRouting.newShardRouting("test", 0, "node1", true, ShardRoutingState.STARTED); + assertFalse(startedShard.isClosedIndexShard()); + + ShardRouting initializingShard = TestShardRouting.newShardRouting( + "test", + 0, + "node1", + null, + false, + ShardRoutingState.INITIALIZING, + new UnassignedInfo(UnassignedInfo.Reason.INDEX_CLOSED, "index closed") + ); + assertFalse(initializingShard.isClosedIndexShard()); + } + public void testExpectedSize() throws IOException { final int iters = randomIntBetween(10, 100); for (int i = 0; i < iters; i++) { diff --git a/server/src/test/java/org/opensearch/gateway/GatewayAllocatorTests.java b/server/src/test/java/org/opensearch/gateway/GatewayAllocatorTests.java index 7a3b5f576449c..7d29e5ed1c2b7 100644 --- a/server/src/test/java/org/opensearch/gateway/GatewayAllocatorTests.java +++ b/server/src/test/java/org/opensearch/gateway/GatewayAllocatorTests.java @@ -23,12 +23,14 @@ import org.opensearch.cluster.routing.IndexShardRoutingTable; import org.opensearch.cluster.routing.RecoverySource; import org.opensearch.cluster.routing.RerouteService; +import org.opensearch.cluster.routing.RoutingChangesObserver; import org.opensearch.cluster.routing.RoutingNodes; import org.opensearch.cluster.routing.RoutingTable; import org.opensearch.cluster.routing.ShardRouting; import org.opensearch.cluster.routing.ShardRoutingState; import org.opensearch.cluster.routing.TestShardRouting; import org.opensearch.cluster.routing.UnassignedInfo; +import org.opensearch.cluster.routing.allocation.ExistingShardsAllocator; import org.opensearch.cluster.routing.allocation.RoutingAllocation; import org.opensearch.cluster.routing.allocation.decider.AllocationDeciders; import org.opensearch.cluster.service.ClusterService; @@ -665,4 +667,407 @@ private Tuple, Set> createBatchesAndAssert(int expectedBatch assertEquals(testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch().keySet(), replicaBatches); return new Tuple<>(primaryBatches, replicaBatches); } + + public void testIndexClosedShardsNotBatchedInBatchMode() { + final ShardId shardId = new ShardId("test-closed-index", "_na_", 0); + final DiscoveryNode node = newNode("node1"); + + ShardRouting primaryShard = ShardRouting.newUnassigned( + shardId, + true, + RecoverySource.EmptyStoreRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.INDEX_CLOSED, + "index closed", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + ShardRouting replicaShard = ShardRouting.newUnassigned( + shardId, + false, + RecoverySource.PeerRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.INDEX_CLOSED, + "index closed", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + Metadata metadata = Metadata.builder() + .put( + IndexMetadata.builder(shardId.getIndexName()) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .state(IndexMetadata.State.CLOSE) + ) + .build(); + + IndexRoutingTable.Builder indexRoutingTable = IndexRoutingTable.builder(shardId.getIndex()) + .addIndexShard(new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard).addShard(replicaShard).build()); + + RoutingTable routingTable = RoutingTable.builder().add(indexRoutingTable).build(); + + clusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .routingTable(routingTable) + .build(); + + testAllocation = new RoutingAllocation( + new AllocationDeciders(Collections.emptyList()), + new RoutingNodes(clusterState, false), + clusterState, + ClusterInfo.EMPTY, + SnapshotShardSizeInfo.EMPTY, + System.nanoTime() + ); + + Set primaryBatches = testShardsBatchGatewayAllocator.createAndUpdateBatches(testAllocation, true); + Set replicaBatches = testShardsBatchGatewayAllocator.createAndUpdateBatches(testAllocation, false); + + assertEquals("Primary shards with INDEX_CLOSED should not be batched", 0, primaryBatches.size()); + assertEquals("Replica shards with INDEX_CLOSED should not be batched", 0, replicaBatches.size()); + assertEquals(0, testShardsBatchGatewayAllocator.getBatchIdToStartedShardBatch().size()); + assertEquals(0, testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch().size()); + } + + public void testIndexClosedShardsSkippedInNonBatchMode() { + final ShardId shardId = new ShardId("test-closed-index", "_na_", 0); + + ShardRouting primaryShard = ShardRouting.newUnassigned( + shardId, + true, + RecoverySource.EmptyStoreRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.INDEX_CLOSED, + "index closed", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + ShardRouting replicaShard = ShardRouting.newUnassigned( + shardId, + false, + RecoverySource.PeerRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.INDEX_CLOSED, + "index closed", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + Metadata metadata = Metadata.builder() + .put( + IndexMetadata.builder(shardId.getIndexName()) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .state(IndexMetadata.State.CLOSE) + ) + .build(); + + IndexRoutingTable.Builder indexRoutingTable = IndexRoutingTable.builder(shardId.getIndex()) + .addIndexShard(new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard).addShard(replicaShard).build()); + + RoutingTable routingTable = RoutingTable.builder().add(indexRoutingTable).build(); + + ClusterState state = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .routingTable(routingTable) + .build(); + + RoutingAllocation allocation = new RoutingAllocation( + new AllocationDeciders(Collections.emptyList()), + new RoutingNodes(state, false), + state, + ClusterInfo.EMPTY, + SnapshotShardSizeInfo.EMPTY, + System.nanoTime() + ); + + AtomicBoolean allocationAttempted = new AtomicBoolean(false); + + ExistingShardsAllocator.UnassignedAllocationHandler handler = new ExistingShardsAllocator.UnassignedAllocationHandler() { + @Override + public ShardRouting initialize( + String nodeId, + String allocationId, + long expectedShardSize, + RoutingChangesObserver routingChanges + ) { + allocationAttempted.set(true); + return null; + } + + @Override + public void removeAndIgnore(UnassignedInfo.AllocationStatus attemptedStatus, RoutingChangesObserver changes) { + allocationAttempted.set(true); + } + + @Override + public ShardRouting updateUnassigned( + UnassignedInfo unassignedInfo, + RecoverySource recoverySource, + RoutingChangesObserver changes + ) { + return null; + } + }; + + GatewayAllocator.innerAllocatedUnassigned(allocation, new PrimaryShardAllocator() { + @Override + protected AsyncShardFetch.FetchResult fetchData(ShardRouting shard, RoutingAllocation allocation) { + return null; + } + }, new ReplicaShardAllocator() { + @Override + protected AsyncShardFetch.FetchResult fetchData(ShardRouting shard, RoutingAllocation allocation) { + return null; + } + + @Override + protected boolean hasInitiatedFetching(ShardRouting shard) { + return false; + } + }, primaryShard, handler); + + assertFalse("Primary shard with INDEX_CLOSED should not trigger allocation", allocationAttempted.get()); + + allocationAttempted.set(false); + + GatewayAllocator.innerAllocatedUnassigned(allocation, new PrimaryShardAllocator() { + @Override + protected AsyncShardFetch.FetchResult fetchData(ShardRouting shard, RoutingAllocation allocation) { + return null; + } + }, new ReplicaShardAllocator() { + @Override + protected AsyncShardFetch.FetchResult fetchData(ShardRouting shard, RoutingAllocation allocation) { + return null; + } + + @Override + protected boolean hasInitiatedFetching(ShardRouting shard) { + return false; + } + }, replicaShard, handler); + + assertFalse("Replica shard with INDEX_CLOSED should not trigger allocation", allocationAttempted.get()); + } + + public void testNonClosedShardsStillBatchedNormally() { + final ShardId shardId = new ShardId("test-normal-index", "_na_", 0); + final DiscoveryNode node = newNode("node1"); + + ShardRouting primaryShard = TestShardRouting.newShardRouting(shardId, node.getId(), true, ShardRoutingState.STARTED); + + ShardRouting replicaShard = ShardRouting.newUnassigned( + shardId, + false, + RecoverySource.PeerRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.REPLICA_ADDED, + "replica added", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + Metadata metadata = Metadata.builder() + .put( + IndexMetadata.builder(shardId.getIndexName()) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .state(IndexMetadata.State.OPEN) + .putInSyncAllocationIds(0, Sets.newHashSet(primaryShard.allocationId().getId())) + ) + .build(); + + IndexRoutingTable.Builder indexRoutingTable = IndexRoutingTable.builder(shardId.getIndex()) + .addIndexShard(new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard).addShard(replicaShard).build()); + + RoutingTable routingTable = RoutingTable.builder().add(indexRoutingTable).build(); + + clusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .routingTable(routingTable) + .build(); + + testAllocation = new RoutingAllocation( + new AllocationDeciders(Collections.emptyList()), + new RoutingNodes(clusterState, false), + clusterState, + ClusterInfo.EMPTY, + SnapshotShardSizeInfo.EMPTY, + System.nanoTime() + ); + + Set replicaBatches = testShardsBatchGatewayAllocator.createAndUpdateBatches(testAllocation, false); + + assertEquals("Replica shard with REPLICA_ADDED should be batched", 1, replicaBatches.size()); + assertEquals(1, testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch().size()); + + ShardsBatchGatewayAllocator.ShardsBatch batch = testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch() + .values() + .iterator() + .next(); + assertTrue("Batch should contain the replica shard", batch.getBatchedShards().contains(shardId)); + } + + public void testAlreadyBatchedShardIndexClosedLater() { + final ShardId shardId = new ShardId("test-index", "_na_", 0); + final DiscoveryNode node = newNode("node1"); + + ShardRouting primaryShard = TestShardRouting.newShardRouting(shardId, node.getId(), true, ShardRoutingState.STARTED); + + ShardRouting replicaShard = ShardRouting.newUnassigned( + shardId, + false, + RecoverySource.PeerRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.REPLICA_ADDED, + "replica added", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + Metadata metadata = Metadata.builder() + .put( + IndexMetadata.builder(shardId.getIndexName()) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .state(IndexMetadata.State.OPEN) + .putInSyncAllocationIds(0, Sets.newHashSet(primaryShard.allocationId().getId())) + ) + .build(); + + IndexRoutingTable.Builder indexRoutingTable = IndexRoutingTable.builder(shardId.getIndex()) + .addIndexShard(new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard).addShard(replicaShard).build()); + + RoutingTable routingTable = RoutingTable.builder().add(indexRoutingTable).build(); + + clusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .routingTable(routingTable) + .build(); + + testAllocation = new RoutingAllocation( + new AllocationDeciders(Collections.emptyList()), + new RoutingNodes(clusterState, false), + clusterState, + ClusterInfo.EMPTY, + SnapshotShardSizeInfo.EMPTY, + System.nanoTime() + ); + + Set replicaBatches = testShardsBatchGatewayAllocator.createAndUpdateBatches(testAllocation, false); + assertEquals("Replica shard should be batched", 1, replicaBatches.size()); + assertEquals(1, testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch().size()); + + ShardsBatchGatewayAllocator.ShardsBatch batch = testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch() + .values() + .iterator() + .next(); + assertTrue("Batch should contain the replica shard", batch.getBatchedShards().contains(shardId)); + + ShardRouting closedReplicaShard = ShardRouting.newUnassigned( + shardId, + false, + RecoverySource.PeerRecoverySource.INSTANCE, + new UnassignedInfo( + UnassignedInfo.Reason.INDEX_CLOSED, + "index closed", + null, + 0, + System.nanoTime(), + System.currentTimeMillis(), + false, + UnassignedInfo.AllocationStatus.NO_ATTEMPT, + Collections.emptySet() + ) + ); + + Metadata closedMetadata = Metadata.builder() + .put( + IndexMetadata.builder(shardId.getIndexName()) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .state(IndexMetadata.State.CLOSE) + .putInSyncAllocationIds(0, Sets.newHashSet(primaryShard.allocationId().getId())) + ) + .build(); + + IndexRoutingTable.Builder closedIndexRoutingTable = IndexRoutingTable.builder(shardId.getIndex()) + .addIndexShard(new IndexShardRoutingTable.Builder(shardId).addShard(primaryShard).addShard(closedReplicaShard).build()); + + RoutingTable closedRoutingTable = RoutingTable.builder().add(closedIndexRoutingTable).build(); + + ClusterState closedClusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(closedMetadata) + .routingTable(closedRoutingTable) + .build(); + + RoutingAllocation closedAllocation = new RoutingAllocation( + new AllocationDeciders(Collections.emptyList()), + new RoutingNodes(closedClusterState, false), + closedClusterState, + ClusterInfo.EMPTY, + SnapshotShardSizeInfo.EMPTY, + System.nanoTime() + ); + + Set batchesAfterClose = testShardsBatchGatewayAllocator.createAndUpdateBatches(closedAllocation, false); + + if (batchesAfterClose.isEmpty()) { + assertEquals("Batch should be cleaned up when empty", 0, testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch().size()); + } else { + ShardsBatchGatewayAllocator.ShardsBatch batchAfterClose = testShardsBatchGatewayAllocator.getBatchIdToStoreShardBatch() + .values() + .iterator() + .next(); + assertFalse( + "Batch should not contain the closed index shard after reroute", + batchAfterClose.getBatchedShards().contains(shardId) + ); + } + } }