Add validation of the number_of_shards parameter in Shrink Action of ILM

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SetSingleNodeAllocateStep.java

@@ -45,18 +45,37 @@ public class SetSingleNodeAllocateStep extends AsyncActionStep {
     private static final Logger logger = LogManager.getLogger(SetSingleNodeAllocateStep.class);
     public static final String NAME = "set-single-node-allocation";
 
-    public SetSingleNodeAllocateStep(StepKey key, StepKey nextStepKey, Client client) {
+    private Integer numberOfShards;
+
+    public SetSingleNodeAllocateStep(StepKey key, StepKey nextStepKey, Client client, Integer numberOfShards) {
         super(key, nextStepKey, client);
+        this.numberOfShards = numberOfShards;
     }
 
     @Override
     public boolean isRetryable() {
         return true;
     }
 
+    public Integer getNumberOfShards() {
+        return numberOfShards;
+    }
+
     @Override
     public void performAction(IndexMetadata indexMetadata, ClusterState clusterState,
                               ClusterStateObserver observer, ActionListener<Boolean> listener) {
+        String indexName = indexMetadata.getIndex().getName();
+        if (numberOfShards != null) {
+            int sourceNumberOfShards = indexMetadata.getNumberOfShards();
+            int factor = sourceNumberOfShards / numberOfShards;
+            if (factor * numberOfShards < sourceNumberOfShards) {
+                logger.debug("the number of shards [" + sourceNumberOfShards + "] of the source index [" + indexName +
+                    "] must be a multiple of [" + numberOfShards + "]");
+                listener.onFailure(new IllegalArgumentException("the number of shards [" + sourceNumberOfShards +
+                    "] of the source index [" + indexName + "] must be a multiple of [" + numberOfShards + "]"));
+                return;
+            }
+        }
         // These allocation deciders were chosen because these are the conditions that can prevent
         // allocation long-term, and that we can inspect in advance. Most other allocation deciders
         // will either only delay relocation (e.g. ThrottlingAllocationDecider), or don't work very
@@ -72,7 +91,6 @@ public void performAction(IndexMetadata indexMetadata, ClusterState clusterState
         RoutingAllocation allocation = new RoutingAllocation(allocationDeciders, routingNodes, clusterState, null,
                 null, System.nanoTime());
         List<String> validNodeIds = new ArrayList<>();
-        String indexName = indexMetadata.getIndex().getName();
         final Map<ShardId, List<ShardRouting>> routingsByShardId = clusterState.getRoutingTable()
             .allShards(indexName)
             .stream()

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ShrinkAction.java

@@ -178,7 +178,8 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
             new GenerateUniqueIndexNameStep(generateShrinkIndexNameKey, setSingleNodeKey, SHRUNKEN_INDEX_PREFIX,
                 (generatedIndexName, lifecycleStateBuilder) -> lifecycleStateBuilder.setShrinkIndexName(generatedIndexName));
         // choose a node to collocate the source index in preparation for shrink
-        SetSingleNodeAllocateStep setSingleNodeStep = new SetSingleNodeAllocateStep(setSingleNodeKey, allocationRoutedKey, client);
+        SetSingleNodeAllocateStep setSingleNodeStep = new SetSingleNodeAllocateStep(setSingleNodeKey, allocationRoutedKey, client,
+            numberOfShards);
 
         // wait for the source shards to be collocated before attempting to shrink the index. we're waiting until a configured threshold is
         // breached (controlled by LifecycleSettings.LIFECYCLE_STEP_WAIT_TIME_THRESHOLD) at which point we rewind to the

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetSingleNodeAllocateStepTests.java

@@ -51,7 +51,7 @@ public class SetSingleNodeAllocateStepTests extends AbstractStepTestCase<SetSing
 
     @Override
     protected SetSingleNodeAllocateStep createRandomInstance() {
-        return new SetSingleNodeAllocateStep(randomStepKey(), randomStepKey(), client);
+        return new SetSingleNodeAllocateStep(randomStepKey(), randomStepKey(), client, null);
     }
 
     @Override
@@ -70,12 +70,12 @@ protected SetSingleNodeAllocateStep mutateInstance(SetSingleNodeAllocateStep ins
             throw new AssertionError("Illegal randomisation branch");
         }
 
-        return new SetSingleNodeAllocateStep(key, nextKey, instance.getClient());
+        return new SetSingleNodeAllocateStep(key, nextKey, instance.getClient(), null);
     }
 
     @Override
     protected SetSingleNodeAllocateStep copyInstance(SetSingleNodeAllocateStep instance) {
-        return new SetSingleNodeAllocateStep(instance.getKey(), instance.getNextStepKey(), client);
+        return new SetSingleNodeAllocateStep(instance.getKey(), instance.getNextStepKey(), client, instance.getNumberOfShards());
     }
 
     public static void assertSettingsRequestContainsValueFrom(UpdateSettingsRequest request, String settingsKey,
@@ -502,6 +502,39 @@ public void testPerformActionNewShardsExistButWithInvalidAttributes() {
         assertNodeSelected(indexMetadata, indexMetadata.getIndex(), oldNodeIds, discoveryNodes, indexRoutingTable.build());
     }
 
+    public void testPerformActionWithInvalidTargetShards() {
+        final int numNodes = randomIntBetween(1, 20);
+        IndexMetadata indexMetadata = IndexMetadata.builder(randomAlphaOfLength(10)).settings(settings(Version.CURRENT))
+            .numberOfShards(10).numberOfReplicas(randomIntBetween(0, numNodes - 1)).build();
+        Index index = indexMetadata.getIndex();
+
+        Settings validNodeSettings = Settings.EMPTY;
+        DiscoveryNodes.Builder nodes = DiscoveryNodes.builder();
+        for (int i = 0; i < numNodes; i++) {
+            String nodeId = "node_id_" + i;
+            String nodeName = "node_" + i;
+            int nodePort = 9300 + i;
+            Settings nodeSettings = Settings.builder().put(validNodeSettings).put(Node.NODE_NAME_SETTING.getKey(), nodeName).build();
+            nodes.add(
+                DiscoveryNode.createLocal(nodeSettings, new TransportAddress(TransportAddress.META_ADDRESS, nodePort), nodeId));
+        }
+
+        DiscoveryNodes discoveryNodes = nodes.build();
+        IndexRoutingTable.Builder indexRoutingTable = createRoutingTable(indexMetadata, index, discoveryNodes);
+
+        ImmutableOpenMap.Builder<String, IndexMetadata> indices = ImmutableOpenMap.<String, IndexMetadata>builder().fPut(index.getName(),
+            indexMetadata);
+        ClusterState clusterState = ClusterState.builder(ClusterState.EMPTY_STATE).metadata(Metadata.builder().indices(indices.build()))
+            .nodes(discoveryNodes).routingTable(RoutingTable.builder().add(indexRoutingTable).build()).build();
+
+        SetSingleNodeAllocateStep step = new SetSingleNodeAllocateStep(randomStepKey(), randomStepKey(), client, 3);
+
+        expectThrows(IllegalArgumentException.class,
+            () -> PlainActionFuture.<Boolean, Exception>get(f -> step.performAction(indexMetadata, clusterState, null, f)));
+
+        Mockito.verifyZeroInteractions(client);
+    }
+
     private void assertNodeSelected(IndexMetadata indexMetadata, Index index,
                                     Set<String> validNodeIds, DiscoveryNodes.Builder nodes) throws IOException {
         DiscoveryNodes discoveryNodes = nodes.build();