Add validation of the number_of_shards parameter in Shrink Action of ILM by gaobinlong · Pull Request #74219 · elastic/elasticsearch

gaobinlong · 2021-06-17T07:25:29Z

Relates to #72724.

The main changes of the PR are:

Add validation of the number_of_shards parameter in the SetSingleNodeAllocateStep, fail the step if number_of_shards is not a factor of the source index's shards count.
Add some test code for the change.

elasticmachine · 2021-06-25T04:02:25Z

Pinging @elastic/es-core-features (Team:Core/Features)

joegallo · 2021-07-01T16:27:41Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SetSingleNodeAllocateStep.java

+        if (numberOfShards != null) {
+            int sourceNumberOfShards = indexMetadata.getNumberOfShards();
+            int factor = sourceNumberOfShards / numberOfShards;
+            if (factor * numberOfShards < sourceNumberOfShards) {


Just curious, is there any particular reason you didn't go with if (sourceNumberOfShards % numberOfShards != 0) {?

Just a habit, % is better, I think.

joegallo · 2021-07-01T21:42:31Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SetSingleNodeAllocateStep.java

    @Override
    public void performAction(IndexMetadata indexMetadata, ClusterState clusterState,
                              ClusterStateObserver observer, ActionListener<Boolean> listener) {
+        String indexName = indexMetadata.getIndex().getName();


What do you think about refactoring all of this into some new Step? I like the logic overall, and I agree that we should do the validation before the single allocation starts, I'm just not sure it belongs within this step.

Yeah, it's a good idea, I will refactor the code soon.

hi @joegallo , I've refactor the code yet, can you take a look at the new commit?

andreidan · 2021-07-08T13:41:22Z

@elasticmachine ok to test

andreidan

Thanks for iterating on this @gaobinlong

I've left a few suggestions (and I suspect there'll be a few ITs failures) but this is looking good

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CheckTargetShardsCountStep.java

gaobinlong · 2021-07-13T11:46:40Z

@andreidan, the issues you mentioned above have been resolved now, I noticed the test failures and I'm still working on fixing them.

gaobinlong · 2021-07-22T07:35:53Z

@andreidan , I have no idea about how to fix the test failure in ClusterStateWaitThresholdBreachTests#testWaitInShrunkShardsAllocatedExceedsThreshold:

elasticsearch/x-pack/plugin/ilm/src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ClusterStateWaitThresholdBreachTests.java

Line 109 in 2c5e406

    
           // ILM will retry the shrink step because the number of shards to shrink to is gt the current number of shards

, as we add a CheckTargetShardsCountStep before the CheckShrinkReadyStep and ShrunkShardsAllocatedStep, the Shrink Action will stuck in the CheckTargetShardsCountStep so the ClusterStateWaitUntilThresholdStep defined in ShrinkAction#toSteps will not execute:

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

Line 193 in 2c5e406

    
           ClusterStateWaitUntilThresholdStep allocated = new ClusterStateWaitUntilThresholdStep(

, maybe we can fail the ShrinkStep by triggering the shard's max doc count limit when executing the ResizeAction:

elasticsearch/server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java

Line 177 in 2c5e406

    
           throw new IllegalStateException("Can't merge index with more than [" + IndexWriter.MAX_DOCS

, but I don't know how to mock the DocsStats.
Can you give me some cue to fix the failure?

andreidan · 2021-07-26T15:34:13Z

@gaobinlong Great point. I've opened #75695 to make this particular test rely on the shrunk index not being able to allocate (as opposed to the managed index not being able to shrink successfully as it was before).

If you merge the main branch into your branch after #75695 is merged things should start working for this particular test (there might be others that use the same strategy for failing the shrink action - ie. higher configured number of shards - so they'll need a similar change to ClusterStateWaitThresholdBreachTests.

…hrink

gaobinlong · 2021-07-31T01:52:48Z

@andreidan thanks for your help, I've fixed other thest failures except the ShrinkActionIT#testShrinkStepMovesForwardIfShrunkIndexIsCreatedBetweenRetries:

elasticsearch/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/ShrinkActionIT.java

Line 306 in 2dcc337

    
           public void testShrinkStepMovesForwardIfShrunkIndexIsCreatedBetweenRetries() throws Exception {

, as it's hard to make the ShrinkStep failed and ShrinkAction will stuck in the new added CheckTargetShardsCountStep if the target shards count is higher than the source index's shards count, so I delete the test method directly. Can you help to take a look at the new commits?

andreidan

Thanks for iterating on this @gaobinlong

I think this is nearly complete.

I agree testShrinkStepMovesForwardIfShrunkIndexIsCreatedBetweenRetries is not feasible anymore, however can you please create a unit test in ShrinkStepTests instead? (basically have a clusterState that already contains the shruken index and assert the success listener method is called once by performAction)

gaobinlong · 2021-08-06T02:26:33Z

@andreidan，I have done that, can you help to take a look again?

dakrone · 2021-08-09T22:37:24Z

@joegallo as @andreidan is on vacation this week, could you take another peek at this PR?

andreidan

LGTM, thanks for iterating on this @gaobinlong

…n of ILM (elastic#74219) Add validation of the number_of_shards parameter in Shrink Action of ILM (cherry picked from commit 58feb4e) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

…n of ILM (#74219) (#76565) Add validation of the number_of_shards parameter in Shrink Action of ILM (cherry picked from commit 58feb4e) Signed-off-by: Andrei Dan <andrei.dan@elastic.co> Co-authored-by: bellengao <gbl_long@163.com>

* master: (868 commits) Query API key - Rest spec and yaml tests (elastic#76238) Delay shard reassignment from nodes which are known to be restarting (elastic#75606) Reenable bwc tests for elastic#76475 (elastic#76576) Set version to 7.15 in BWC code (elastic#76577) Don't remove warning headers on all failure (elastic#76434) Disable bwc tests for elastic#76475 (elastic#76541) Re-enable bwc tests (elastic#76567) Keep track of data recovered from snapshots in RecoveryState (elastic#76499) [Transform] Align transform checkpoint range with date_histogram interval for better performance (elastic#74004) EQL: Remove "wildcard" function (elastic#76099) Fix 'accept' and 'content_type' fields for search_mvt API Add persistent licensed feature tracking (elastic#76476) Add system data streams to feature state snapshots (elastic#75902) fix the error message for instance methods that don't exist (elastic#76512) ILM: Add validation of the number_of_shards parameter in Shrink Action of ILM (elastic#74219) remove dashboard only reserved role (elastic#76507) Fix Stack Overflow in UnassignedInfo in Corner Case (elastic#76480) Add (Extended)KeyUsage KeyUsage, CipherSuite & Protocol to SSL diagnostics (elastic#65634) Add recovery from snapshot to tests (elastic#76535) Reenable BwC Tests after elastic#76532 (elastic#76534) ...

Add validation of the number_of_shards parameter in Shrink Action of ILM

977ab67

elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Jun 17, 2021

mark-vieira added the :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. label Jun 25, 2021

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Jun 25, 2021

andreidan self-requested a review July 1, 2021 15:35

joegallo reviewed Jul 1, 2021

View reviewed changes

bellengao added 3 commits July 6, 2021 15:28

Merge remote-tracking branch 'origin/master' into enhance_shrink

609d402

Use a single step

1f7c94c

Merge remote-tracking branch 'origin/master' into enhance_shrink

e7bc201

andreidan reviewed Jul 8, 2021

View reviewed changes

bellengao added 2 commits July 11, 2021 23:48

add more checks and fix test failure

4644f45

Merge remote-tracking branch 'origin/master' into enhance_shrink

e41b71d

andreidan mentioned this pull request Jul 26, 2021

ILM ClusterStateWaitThresholdBreachTests cycles due to shrunk-shards-allocated #75695

Merged

bellengao added 6 commits July 28, 2021 10:34

Merge remote-tracking branch 'origin/master' into enhance_shrink

6e810f4

Merge remote-tracking branch 'upstream/enhance_shrink' into enhance_s…

cfa6827

…hrink

fix test failure

ee9b060

Merge remote-tracking branch 'origin/master' into enhance_shrink

3f50349

fix test failure

4d3400e

Merge remote-tracking branch 'origin/master' into enhance_shrink

c67d7e0

andreidan requested changes Aug 3, 2021

View reviewed changes

bellengao added 2 commits August 5, 2021 19:44

Merge remote-tracking branch 'origin/master' into enhance_shrink

68bec26

add new test

a4a5064

dakrone requested a review from joegallo August 9, 2021 22:37

andreidan added v7.15.0 v8.0.0 labels Aug 16, 2021

andreidan approved these changes Aug 16, 2021

View reviewed changes

andreidan added the auto-backport Automatically create backport pull requests when merged label Aug 16, 2021

andreidan merged commit 58feb4e into elastic:master Aug 16, 2021

andreidan mentioned this pull request Aug 16, 2021

[7.x] ILM: Add validation of the number_of_shards parameter in Shrink Action (#74219) #76565

Merged

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

joegallo added the >enhancement label Sep 22, 2021

Conversation

gaobinlong commented Jun 17, 2021

Uh oh!

elasticmachine commented Jun 25, 2021

Uh oh!

joegallo Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

gaobinlong Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

joegallo Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

gaobinlong Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

gaobinlong Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

andreidan commented Jul 8, 2021

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaobinlong commented Jul 13, 2021

Uh oh!

gaobinlong commented Jul 22, 2021

Uh oh!

andreidan commented Jul 26, 2021

Uh oh!

gaobinlong commented Jul 31, 2021

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

gaobinlong commented Aug 6, 2021

Uh oh!

dakrone commented Aug 9, 2021

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants