Fix autoexpand during node replace #96281

idegtiarenko · 2023-05-23T10:44:29Z

Prior to this change NodeReplacementAllocationDecider was unconditionally
skipping both replacement source and target nodes when calculation auto-expand
replicas. This is fixed by autoexpanding to the replacement node if source node
already had shards of the index.

Closes: #89527

Prior to this change NodeReplacementAllocationDecider was unconditionally skipping both replacement source and target nodes when calculation auto-expand replicas. This is fixed by autoexpanding to the replacement node if source node already had shards of the index

elasticsearchmachine · 2023-05-23T10:44:54Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2023-05-23T10:44:54Z

Hi @idegtiarenko, I've created a changelog YAML for you.

idegtiarenko · 2023-05-23T10:45:37Z

...a/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDecider.java

-                shardRouting.currentNodeId(),
-                node.nodeId()
+                node.nodeId(),
+                shardRouting.currentNodeId()


The order was flipped

idegtiarenko · 2023-05-23T10:46:23Z

.../elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java

-        EmptyClusterInfoService.INSTANCE,
-        EmptySnapshotsInfoService.INSTANCE,
-        TestShardRoutingRoleStrategies.DEFAULT_ROLE_ONLY
-    );


This was unused test setup, removed in a separate commit to make it easier to review

henningandersen

Thanks for working on this. I left some comments to address.

...a/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDecider.java

henningandersen · 2023-05-24T09:52:39Z

.../elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java

            )
            .build();
+        allocation = new RoutingAllocation(allocationDeciders, state, null, null, 0);
+        assertThatAutoExpandReplicasDidNotContract(indexMetadata, allocation);


Can we add verification of the decider also after the source node is shutdown, but with the shutdown record still in place?

henningandersen · 2023-05-24T10:01:42Z

.../elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java

+        );
+
+        // when replacing NODE_A with NODE_B
+        state = ClusterState.builder(state)
            .nodes(DiscoveryNodes.builder().add(NODE_A).add(NODE_B).add(NODE_C).build())


I think we need to test the scenario where we add the shutdown indication first and then add the node later (verifying that no contraction happens in between).

I specifically think that will fail due to NodeShutdownAllocationDecider saying NO to the source node - and there is no target node yet.

# Conflicts: # server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/NodeShutdownAllocationDecider.java # server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java # server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/NodeShutdownAllocationDeciderTests.java

# Conflicts: # server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDecider.java

idegtiarenko · 2023-06-02T09:16:38Z

@elasticsearchmachine please run elasticsearch-ci/bwc

elasticsearchmachine · 2023-06-04T18:08:57Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

henningandersen

Thanks for the additional work on this. I left a number of smaller comments and test comments.

...a/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDecider.java

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java

...org/elasticsearch/cluster/routing/allocation/decider/NodeShutdownAllocationDeciderTests.java

...down/src/internalClusterTest/java/org/elasticsearch/xpack/shutdown/NodeShutdownShardsIT.java

henningandersen

A couple of comments from last review seems unaddressed (if not, please point me to the verifications).

.../elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java

henningandersen

LGTM.

Prior to this change NodeReplacementAllocationDecider was unconditionally skipping both replacement source and target nodes when calculation auto-expand replicas. This is fixed by autoexpanding to the replacement node if source node already had shards of the index Backport of PR elastic#96281 amended for 7.17.x Closes elastic#89527

Prior to this change NodeReplacementAllocationDecider was unconditionally skipping both replacement source and target nodes when calculation auto-expand replicas. This is fixed by autoexpanding to the replacement node if source node already had shards of the index Backport of PR #96281 amended for 7.17.x Closes #89527 Co-authored-by: Ievgen Degtiarenko <[email protected]>

DaveCTurner · 2023-09-04T15:25:36Z

Added the v7.17.13 label because this has now been backported /cc @kingherc

idegtiarenko added 4 commits May 23, 2023 12:25

remove unneeded setup

3f9e6b9

simplify assertions

d435571

additional test case

caf6368

idegtiarenko requested review from dakrone and henningandersen May 23, 2023 10:44

Update docs/changelog/96281.yaml

30c7a11

idegtiarenko commented May 23, 2023

View reviewed changes

idegtiarenko added 2 commits May 23, 2023 18:03

fix checkstyle

1af43d5

fix checkstyle

5b9911d

henningandersen requested changes May 24, 2023

View reviewed changes

idegtiarenko added 4 commits May 24, 2023 15:57

add integration test

a7a5527

handle replacement that has not yet joined

f18cf7a

avoid unnecessary lookup

61479cb

idegtiarenko requested a review from henningandersen May 31, 2023 06:49

idegtiarenko added 2 commits June 2, 2023 09:25

Merge branch 'main' into fix_autoexpand_during_node_replace

f401e0c

# Conflicts: # server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDecider.java

Remove unnecessary parameter

d01e995

henningandersen added the :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown label Jun 4, 2023

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jun 4, 2023

henningandersen reviewed Jun 4, 2023

View reviewed changes

idegtiarenko added 4 commits June 5, 2023 09:49

Fix review comments

12df310

add javadoc

8eb32d9

update IT

14a6ea2

Merge branch 'main' into fix_autoexpand_during_node_replace

3b7109f

idegtiarenko requested a review from henningandersen June 5, 2023 08:45

henningandersen reviewed Jun 5, 2023

View reviewed changes

idegtiarenko requested a review from henningandersen June 6, 2023 07:11

henningandersen reviewed Jun 6, 2023

View reviewed changes

.../elasticsearch/cluster/routing/allocation/decider/NodeReplacementAllocationDeciderTests.java Outdated Show resolved Hide resolved

additional test case

84be141

idegtiarenko requested a review from henningandersen June 6, 2023 09:02

henningandersen approved these changes Jun 6, 2023

View reviewed changes

idegtiarenko added 2 commits June 6, 2023 11:15

Merge branch 'main' into fix_autoexpand_during_node_replace

cca349a

Merge branch 'main' into fix_autoexpand_during_node_replace

a501347

idegtiarenko merged commit 1bb0fa9 into elastic:main Jun 7, 2023

idegtiarenko deleted the fix_autoexpand_during_node_replace branch June 7, 2023 08:49

kingherc mentioned this pull request Aug 25, 2023

Fix autoexpand during node replace #98891

Merged

DaveCTurner added the v7.17.13 label Sep 4, 2023

Fix autoexpand during node replace #96281

Fix autoexpand during node replace #96281

Uh oh!

Conversation

idegtiarenko commented May 23, 2023

Uh oh!

elasticsearchmachine commented May 23, 2023

Uh oh!

elasticsearchmachine commented May 23, 2023

Uh oh!

idegtiarenko May 23, 2023

Choose a reason for hiding this comment

Uh oh!

idegtiarenko May 23, 2023

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henningandersen May 24, 2023

Choose a reason for hiding this comment

Uh oh!

henningandersen May 24, 2023

Choose a reason for hiding this comment

Uh oh!

idegtiarenko commented Jun 2, 2023

Uh oh!

elasticsearchmachine commented Jun 4, 2023

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Sep 4, 2023

Uh oh!

Uh oh!