Recalculate balance using up-to date weights when THROTTLE by idegtiarenko · Pull Request #88385 · elastic/elasticsearch

idegtiarenko · 2022-07-08T14:11:20Z

Fix a bug when a shard could be re-balanced using outdated node weights in case 2 or more shards movements in a row for a single index were simulated due to THROTTLE.

Closes: #88384

Add a test that checks a corner situation when currently after moving some shards around we end up with exactly the same balance as before.

elasticsearchmachine · 2022-07-12T09:55:40Z

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

elasticmachine · 2022-07-12T11:44:37Z

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

idegtiarenko · 2022-07-12T11:45:02Z

...ava/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorTests.java

+        addIndex(metadataBuilder, routingTableBuilder, "index-3", 10, Map.of("node-0", 3, "node-1", 3, "node-2", 4));
+        addIndex(metadataBuilder, routingTableBuilder, "index-4", 10, Map.of("node-0", 4, "node-1", 3, "node-2", 3));
+        addIndex(metadataBuilder, routingTableBuilder, "index-5", 10, Map.of("node-0", 3, "node-1", 4, "node-2", 3));
+        addIndex(metadataBuilder, routingTableBuilder, "index-6", 10, Map.of("node-0", 3, "node-1", 3, "node-2", 4));


I am checking if it is possible to reproduce it with a simpler setup

idegtiarenko · 2022-07-12T11:48:00Z

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

                        assert decision.type() == Type.THROTTLE;
                        minNode.addShard(shard.relocate(minNode.getNodeId(), shardSize));
-                        return false;
+                        return true;


To add a bit of context:

return false is used when exiting this method without moving a shard and as a result not updating the node weights.
This means that previously the weights here were not updated if a move was simulated. This could lead to rebalancing using outdated weights if two or more moves in a row for a single index are simulated.

👍

The two branches of the if (decision.type() == Type.YES) test are now almost the same. Does it work to consolidate them?

I think there's more to consolidate. How about this?

diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java index 97bdbd7367a..66c1c7f2001 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java @@ -1073,18 +1073,21 @@ public class BalancedShardsAllocator implements ShardsAllocator { maxNode.removeShard(shard); long shardSize = allocation.clusterInfo().getShardSize(shard, ShardRouting.UNAVAILABLE_EXPECTED_SHARD_SIZE); - if (decision.type() == Type.YES) { - /* only allocate on the cluster if we are not throttled */ - logger.debug("Relocate [{}] from [{}] to [{}]", shard, maxNode.getNodeId(), minNode.getNodeId()); - minNode.addShard(routingNodes.relocateShard(shard, minNode.getNodeId(), shardSize, allocation.changes()).v1()); - return true; - } else { - /* allocate on the model even if throttled */ - logger.debug("Simulate relocation of [{}] from [{}] to [{}]", shard, maxNode.getNodeId(), minNode.getNodeId()); - assert decision.type() == Type.THROTTLE; - minNode.addShard(shard.relocate(minNode.getNodeId(), shardSize)); - return false; - } + assert decision.type() == Type.YES || decision.type() == Type.THROTTLE : decision.type(); + logger.debug( + "decision [{}]: relocate [{}] from [{}] to [{}]", + decision.type(), + shard, + maxNode.getNodeId(), + minNode.getNodeId() + ); + minNode.addShard( + decision.type() == Type.YES + /* only allocate on the cluster if we are not throttled */ + ? routingNodes.relocateShard(shard, minNode.getNodeId(), shardSize, allocation.changes()).v1() + : shard.relocate(minNode.getNodeId(), shardSize) + ); + return true; } } logger.trace("No shards of [{}] can relocate from [{}] to [{}]", idx, maxNode.getNodeId(), minNode.getNodeId());

DaveCTurner · 2022-07-12T14:08:35Z

...ava/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocatorTests.java

+        addIndex(metadataBuilder, routingTableBuilder, "index-0", Map.of("node-0", 4, "node-1", 4, "node-2", 2));
+        addIndex(metadataBuilder, routingTableBuilder, "index-1", Map.of("node-0", 4, "node-1", 4, "node-2", 2));
+        addIndex(metadataBuilder, routingTableBuilder, "index-2", Map.of("node-0", 3, "node-1", 3, "node-2", 4));
+        addIndex(metadataBuilder, routingTableBuilder, "index-3", Map.of("node-0", 3, "node-1", 3, "node-2", 4));
+        addIndex(metadataBuilder, routingTableBuilder, "index-4", Map.of("node-0", 4, "node-1", 3, "node-2", 3));
+        addIndex(metadataBuilder, routingTableBuilder, "index-5", Map.of("node-0", 3, "node-1", 4, "node-2", 3));
+        addIndex(metadataBuilder, routingTableBuilder, "index-6", Map.of("node-0", 3, "node-1", 3, "node-2", 4));


I think this might be quite dependent on the ordering of these indices in a map. Should we randomise their names and the order in which they're added?

DaveCTurner · 2022-07-12T14:10:09Z

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

                        assert decision.type() == Type.THROTTLE;
                        minNode.addShard(shard.relocate(minNode.getNodeId(), shardSize));
-                        return false;
+                        return true;


👍

The two branches of the if (decision.type() == Type.YES) test are now almost the same. Does it work to consolidate them?

DaveCTurner

LGTM

pugnascotia · 2022-07-22T08:11:52Z

docs/changelog/88385.yaml

@@ -0,0 +1,5 @@
+pr: 88385
+summary: Test balance improves after rebalancing


Can we improve the summary here? It doesn't tell me much about the bug or the fix.

Sure, opened a pr for this: #88709

Test balance improves after rebalancing

c9a782a

Add a test that checks a corner situation when currently after moving some shards around we end up with exactly the same balance as before.

idegtiarenko added :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Meta label for distributed team. labels Jul 8, 2022

elasticsearchmachine added the v8.4.0 label Jul 8, 2022

idegtiarenko added 2 commits July 12, 2022 10:46

add test logging

d4537c8

attempt to fix the test

c2b3b9d

idegtiarenko added the >bug label Jul 12, 2022

Update docs/changelog/88385.yaml

7da2053

idegtiarenko marked this pull request as ready for review July 12, 2022 11:44

idegtiarenko commented Jul 12, 2022

View reviewed changes

idegtiarenko changed the title ~~Test balance improves after rebalancing~~ Recalculate balance using up-to date weights when THROTTLE Jul 12, 2022

idegtiarenko requested a review from DaveCTurner July 12, 2022 12:49

idegtiarenko added 2 commits July 12, 2022 15:52

rename

3dd3f5c

Merge branch 'master' into test_balance_improves

95efa35

DaveCTurner reviewed Jul 12, 2022

View reviewed changes

fix comments

875a507

idegtiarenko requested a review from DaveCTurner July 12, 2022 14:32

merge branches

198ecaf

DaveCTurner approved these changes Jul 12, 2022

View reviewed changes

idegtiarenko merged commit a23e416 into elastic:master Jul 13, 2022

idegtiarenko deleted the test_balance_improves branch July 13, 2022 06:59

pugnascotia reviewed Jul 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recalculate balance using up-to date weights when THROTTLE#88385

Recalculate balance using up-to date weights when THROTTLE#88385
idegtiarenko merged 8 commits intoelastic:masterfrom
idegtiarenko:test_balance_improves

idegtiarenko commented Jul 8, 2022 •

edited

Loading

Uh oh!

elasticsearchmachine commented Jul 12, 2022

Uh oh!

elasticmachine commented Jul 12, 2022

Uh oh!

idegtiarenko Jul 12, 2022 •

edited

Loading

Uh oh!

idegtiarenko Jul 12, 2022

Uh oh!

DaveCTurner Jul 12, 2022

Uh oh!

DaveCTurner Jul 12, 2022

Uh oh!

DaveCTurner Jul 12, 2022

Uh oh!

DaveCTurner Jul 12, 2022

Uh oh!

DaveCTurner left a comment

Uh oh!

pugnascotia Jul 22, 2022

Uh oh!

idegtiarenko Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -0,0 +1,5 @@
		pr: 88385
		summary: Test balance improves after rebalancing

Conversation

idegtiarenko commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 12, 2022

Uh oh!

elasticmachine commented Jul 12, 2022

Uh oh!

idegtiarenko Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

pugnascotia Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

idegtiarenko Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

idegtiarenko commented Jul 8, 2022 •

edited

Loading

idegtiarenko Jul 12, 2022 •

edited

Loading