Remove NoopGatewayAllocator in favor of a more realistic mock #20637

bleskes · 2016-09-23T10:30:35Z

Many of our unit tests instantiate an AllocationService, which requires having a GatewayAllocator. Today almost all of our test use a class called NoopGatewayAllocator which does nothing, effectively leaving all shard assignments to the balanced allocator. This is sad as it means we test a system that behaves differently than our production logic in very basic things. For example, a started primary that is lost will be assigned to a node that didn't use to have it.

This PR removes NoopGatewayAllocator in favor of a new TestGatewayAllocator that inherits the standard GatewayAllocator and overrides shard information fetching to return information based on historical assignments the allocator has done. The only exception is BalanceConfigurationTests which does test only the balancer and I opted to not have it work around the GatewayAllocator being in it's way.

The PR also fixes all resulting failures plus one production failure for which I will open a dedicated PR (note - that fix is included here as well to make sure it works).

…RecoverySource When initializing a new index routing table, we make a decision where the primary shards should be recovered from. This can be an empty folder for new indices, a set of specific allocation ids for old indices or a snapshot. We currently allow callers of IndexRoutingTable.initializeEmpty to supply the source but also set it automatically if null is given. Sadly the current logic is reusing the supplied parameter to store the result of the automatic decision. This is flawed if some of the decision should be *different* between the different index shard (as the first decision that is maid sticks). This commit fixes this but also simplifies the API to always make an automatic decision

the recovery source bug prevented propper coverage

…cator

… RecoverySource (#20638) When initializing a new index routing table, we make a decision where the primary shards should be recovered from. This can be an empty folder for new indices, a set of specific allocation ids for old indices or a snapshot. We currently allow callers of `IndexRoutingTable.initializeEmpty` to supply the source but also set it automatically if null is given. Sadly the current logic is reusing the supplied parameter to store the result of the automatic decision. This is flawed if some of the decision should be *different* between the different index shard (as the first decision that is maid sticks). This commit fixes this but also simplifies the API to always make an automatic decision. This was discovered while working on #20637 which strengthens the testing infra and caused this to bubble up. I put it as a separate commit to make sure it is not lost as part of a bigger test only PR.

bleskes · 2016-09-23T11:29:07Z

@ywelsch I've merged master into this so #20638 will be remove from it.

ywelsch

Left minor comments / questions. It will be interesting t o combine this with IndicesClusterStateServiceRandomUpdatesTests by introducing random failures into the TestGatewayAllocator to see how the system reacts to random failures during the shard fetching ;-)

ywelsch · 2016-09-23T11:05:15Z

core/src/main/java/org/elasticsearch/cluster/ClusterState.java

@@ -290,7 +290,7 @@ public String prettyPrint() {
            for (int shard = 0; shard < indexMetaData.getNumberOfShards(); shard++) {
                sb.append(TAB).append(TAB).append(shard).append(": ");
                sb.append("p_term [").append(indexMetaData.primaryTerm(shard)).append("], ");
-                sb.append("a_ids ").append(indexMetaData.inSyncAllocationIds(shard)).append("\n");
+                sb.append("sa_ids ").append(indexMetaData.inSyncAllocationIds(shard)).append("\n");


what is "sa" supposed to mean? Maybe insync_ids or isa_ids (inSyncAllocationIds).

synced allocations, but I agree that isa_ids are better.

ywelsch · 2016-09-23T11:06:16Z

core/src/main/java/org/elasticsearch/gateway/PrimaryShardAllocator.java

@@ -138,7 +138,8 @@ public UnassignedShardDecision makeAllocationDecision(final ShardRouting unassig

        if (inSyncAllocationIds.isEmpty()) {
            assert Version.indexCreated(indexMetaData.getSettings()).before(Version.V_5_0_0_alpha1) :
-                "trying to allocated a primary with an empty allocation id set, but index is new";
+                "trying to allocated a primary with an empty in sync allocation id set, but index is new. index: "


trying to allocate a primary ...

will change

ywelsch · 2016-09-23T11:16:51Z

core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java

+                if (primaryShard.initializing() &&
+                    RecoverySource.isInitialRecovery(primaryShard.recoverySource().getType()) == false ) {
+                    // simulate a primary was initialized based on aid
+                    insyncAids.add(primaryShard.allocationId().getId());


it would be interesting to add this as assertion to IndexRoutingTable#validate after the line

if (shardRouting.active() && indexMetaData.inSyncAllocationIds(shardRouting.id()).contains(shardRouting.allocationId().getId()) == false) {

(essentially to validate whether this reverse engineering is correct)

good idea. added.

ywelsch · 2016-09-23T11:24:36Z

...src/test/java/org/elasticsearch/cluster/routing/allocation/FilterAllocationDeciderTests.java

    }

    private ClusterState createInitialClusterState(AllocationService service, Settings settings) {
-        boolean shrinkIndex = randomBoolean();
+        RecoverySource.Type recoveryType = randomFrom(RecoverySource.Type.EMPTY_STORE,
+            RecoverySource.Type.LOCAL_SHARDS, RecoverySource.Type.SNAPSHOT);


if you make RecoverySource.INITIAL_RECOVERY_TYPES public, you can use that directly here instead.

I opted to do it like it is now for two reasons - it's more explicit so people reading can know what's going on and it offers an external validation of what's in INITIAL_RECOVERY_TYPES.

ywelsch · 2016-09-23T11:37:15Z

core/src/test/java/org/elasticsearch/gateway/PrimaryShardAllocatorTests.java

+        // with old version, we can't know if a shard was allocated before or not
+        allocation = routingAllocationWithOnePrimaryNoReplicas(yesAllocationDeciders(),
+            INDEX_REOPENED, Version.CURRENT);
+        //randomFrom(INDEX_CREATED, CLUSTER_RECOVERED, INDEX_REOPENED)


good catch. left over from debugging. I put it back into it's place.

ywelsch · 2016-09-23T11:43:51Z

test/framework/src/main/java/org/elasticsearch/test/gateway/TestGatewayAllocator.java

+ */
+public class TestGatewayAllocator extends GatewayAllocator {
+
+    Map<String, Map<ShardId,ShardRouting>> knownAllocations = new HashMap<>();


ShardId, space ShardRouting ;-)

The way this map is queried, it seems more efficient to make it a
Map<ShardId, Map<String /* node id */, ShardRouting>>

you have been spending too much time with the TLA model :) . Of course, you are right but I opted to go for clarity and simplicity here. I think it's the right thing to optimize for here.

ywelsch · 2016-09-23T11:49:43Z

test/framework/src/main/java/org/elasticsearch/test/gateway/TestGatewayAllocator.java

+
+        primaryShardAllocator.allocateUnassigned(allocation);
+        replicaShardAllocator.processExistingRecoveries(allocation);
+        replicaShardAllocator.allocateUnassigned(allocation);


Replace this method by

@Override public void allocateUnassigned(RoutingAllocation allocation) { updateNodes(allocation); super.allocateUnassigned(allocation); }

I now remember why I did it this way - I couldn't find a good way to get all the inner classes to work as they can not be constructed and passed around as a parameter. I took another approach with 13b5c5c

I think the cleaner way would be to add a constructor

public GatewayAllocator(Settings settings, final TransportNodesListGatewayStartedShards startedAction, final TransportNodesListShardStoreMetaData storeAction, PrimaryShardAllocator primaryShardAllocator, ReplicaShardAllocator replicaShardAllocator) { ... }

to GatewayAllocator.

yes,the problem is that all implementations of ReplicaShardAllocator & PrimaryShardAllocator are non-static inner classes.

ah, that's unfortunate :-/

ywelsch · 2016-09-23T11:54:04Z

test/framework/src/main/java/org/elasticsearch/test/gateway/TestGatewayAllocator.java

+public class TestGatewayAllocator extends GatewayAllocator {
+
+    Map<String, Map<ShardId,ShardRouting>> knownAllocations = new HashMap<>();
+    Map<String, DiscoveryNode> knownNodes = new HashMap<>();


It's simpler just to store DiscoveryNodes directly.

that's a great idea.

ywelsch · 2016-09-23T11:56:26Z

test/framework/src/main/java/org/elasticsearch/test/gateway/TestGatewayAllocator.java

+        }
+    };
+
+    public TestGatewayAllocator() {


move the constructor up before the first method

it is the first method. the rest are fields. I personally don't care. let me know what you prefer.

right, I missed that. Ignore my comment.

ywelsch · 2016-09-23T12:04:12Z

test/framework/src/main/java/org/elasticsearch/test/gateway/TestGatewayAllocator.java

+
+/**
+ * A gateway allocator implementation that keeps an in memory list of started shard allocation
+ * that are used as replies to the, normally async, fetch data requests.


can you add a few sentences how it behaves under nodes that disappear / reappear.

GatewayAllocator#applyFailedShards to take both a RoutingAllocation and a list of shards to apply. This allows better mock allocators to be created as being done in elastic#20637.

Changes the API of GatewayAllocator#applyStartedShards and GatewayAllocator#applyFailedShards to take both a RoutingAllocation and a list of shards to apply. This allows better mock allocators to be created as being done in #20637. Closes #20642

abeyad · 2016-09-23T13:33:29Z

@bleskes in #20626 and #20642, StartedRerouteAllocation and FailedRerouteAllocation were removed, the consequence to your PR being that GatewayAllocator#applyStartedShards now takes (RoutingAllocation, List<ShardRouting> startedShards) and GatewayAllocator#applyFailedShards now takes (RoutingAllocation, List<FailedShard> failedShards)

bleskes · 2016-09-23T13:34:47Z

@abeyad yep saw it. Thanks for the extra ping.

Changes the API of GatewayAllocator#applyStartedShards and GatewayAllocator#applyFailedShards to take both a RoutingAllocation and a list of shards to apply. This allows better mock allocators to be created as being done in #20637. Closes #20642

…cator (code doesn't compile yet)

bleskes · 2016-09-23T14:10:34Z

@ywelsch thanks for the review. I merged from master (to get all the changes in) and pushed some more commits as a fall out of the changes you suggested. Can you take another look?

ywelsch

LGTM, thanks @bleskes!

bleskes · 2016-09-23T19:00:57Z

@ywelsch I'm holding on pushing this. The extra assertion in IndexRoutingTable#validate has surfaced another issue with shadow replicas that I want to talk about (around not reusing aid with reinitlizing a replica as a primary - see [1]). I'm leaning towards removing this assertion for now so we can get this in and follow up after discussion.

[1] look for the only caller of RoutingChangesObserver#startedPrimaryReinitialized

ywelsch · 2016-09-23T21:07:01Z

I'm good if you add && IndexMetaData.isIndexUsingShadowReplicas(indexMetaData.getSettings()) == false to the assertion, get this in and open an issue for the shadow replica case.

…le#validate

Many of our unit tests instantiate an `AllocationService`, which requires having a `GatewayAllocator`. Today almost all of our test use a class called `NoopGatewayAllocator` which does nothing, effectively leaving all shard assignments to the balanced allocator. This is sad as it means we test a system that behaves differently than our production logic in very basic things. For example, a started primary that is lost will be assigned to a node that didn't use to have it. This PR removes `NoopGatewayAllocator` in favor of a new `TestGatewayAllocator` that inherits the standard `GatewayAllocator` and overrides shard information fetching to return information based on historical assignments the allocator has done. The only exception is `BalanceConfigurationTests` which does test only the balancer and I opted to not have it work around the `GatewayAllocator` being in it's way.

Many of our unit tests instantiate an `AllocationService`, which requires having a `GatewayAllocator`. Today almost all of our test use a class called `NoopGatewayAllocator` which does nothing, effectively leaving all shard assignments to the balanced allocator. This is sad as it means we test a system that behaves differently than our production logic in very basic things. For example, a started primary that is lost will be assigned to a node that didn't use to have it. This commit removes `NoopGatewayAllocator` in favor of a new `TestGatewayAllocator` that inherits the standard `GatewayAllocator` and overrides shard information fetching to return information based on historical assignments the allocator has done. The only exception is `BalanceConfigurationTests` which does test only the balancer and I opted to not have it work around the `GatewayAllocator` being in it's way.

* master: (1199 commits) [DOCS] Remove non-valid link to mapping migration document Revert "Default `include_in_all` for numeric-like types to false" test: add a test with ipv6 address docs: clearify that both ip4 and ip6 addresses are supported Include complex settings in settings requests Add production warning for pre-release builds Clean up confusing error message on unhandled endpoint [TEST] Increase logging level in testDelayShards() change health from string to enum (#20661) Provide error message when plugin id is missing Document that sliced scroll works for reindex Make reindex-from-remote ignore unknown fields Remove NoopGatewayAllocator in favor of a more realistic mock (#20637) Remove Marvel character reference from guide Fix documentation for setting Java I/O temp dir Update client benchmarks to log4j2 Changes the API of GatewayAllocator#applyStartedShards and (#20642) Removes FailedRerouteAllocation and StartedRerouteAllocation IndexRoutingTable.initializeEmpty shouldn't override supplied primary RecoverySource (#20638) Smoke tester: Adjust to latest changes (#20611) ...

bleskes added 10 commits September 21, 2016 21:23

introduce TestGatewayAllocator

aa08104

re-introduce NoopGatewayAllocator to test the BalanceShardsAllocator

af843b7

fix PrimaryTermsTests

f9cc864

java docs

e57d411

improvements to PrimaryShardAllocatorTests

cc72dab

fix RandomAllocationDeciderTests

5377eb4

fix ClusterStateHealthTests

7845c79

the recovery source bug prevented propper coverage

fix testElectionBasedOnConflictingNodes

66a68b0

Merge remote-tracking branch 'upstream/master' into test_gateway_allo…

0f2f737

…cator

bleskes added >enhancement >test Issues or PRs that are addressing/adding tests v6.0.0-alpha1 v5.1.1 v5.0.0 labels Sep 23, 2016

bleskes assigned ywelsch Sep 23, 2016

bleskes mentioned this pull request Sep 23, 2016

IndexRoutingTable.initializeEmpty shouldn't override supplied primary RecoverySource #20638

Merged

merge master

9171756

clintongormley added :Allocation and removed >enhancement labels Sep 23, 2016

ywelsch reviewed Sep 23, 2016

View reviewed changes

abeyad mentioned this pull request Sep 23, 2016

GatewayAllocator apply[Started|Failed]Shards method change #20642

Merged

bleskes added 6 commits September 23, 2016 15:41

feedback

3b23135

Merge remote-tracking branch 'upstream/master' into test_gateway_allo…

f9d2100

…cator (code doesn't compile yet)

adapt to latest chagnes in master

15ffb3a

allow using custom shard allocators with no copy paste code

13b5c5c

spaces are important

8b4cf8f

fix new assertions and remove unneeded flag from applyStartedShards

cd104f0

bleskes added 2 commits September 23, 2016 16:19

allow for old indices in IndexRoutingTable.validate

127448e

delivering happiness

a5df31b

ywelsch approved these changes Sep 23, 2016

View reviewed changes

bleskes mentioned this pull request Sep 24, 2016

Should shadow indices reuse allocation ids when promoting a replica to primary #20650

Closed

exclude shadow indices from primary aid validation in IndexRoutingTab…

f741e17

…le#validate

bleskes merged commit ee76c1a into elastic:master Sep 25, 2016

bleskes deleted the test_gateway_allocator branch September 25, 2016 18:15

clintongormley added v5.0.0-rc1 and removed v5.0.0 labels Oct 7, 2016

clintongormley removed the v5.1.1 label Dec 8, 2016

lcawl added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Allocation labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove NoopGatewayAllocator in favor of a more realistic mock #20637

Remove NoopGatewayAllocator in favor of a more realistic mock #20637

bleskes commented Sep 23, 2016

bleskes commented Sep 23, 2016

ywelsch left a comment

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016 •

edited

Loading

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016 •

edited

Loading

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

ywelsch Sep 23, 2016

ywelsch Sep 23, 2016

bleskes Sep 23, 2016

abeyad commented Sep 23, 2016

bleskes commented Sep 23, 2016

bleskes commented Sep 23, 2016

ywelsch left a comment

bleskes commented Sep 23, 2016

ywelsch commented Sep 23, 2016

Remove NoopGatewayAllocator in favor of a more realistic mock #20637

Remove NoopGatewayAllocator in favor of a more realistic mock #20637

Conversation

bleskes commented Sep 23, 2016

bleskes commented Sep 23, 2016

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abeyad commented Sep 23, 2016

bleskes commented Sep 23, 2016

bleskes commented Sep 23, 2016

ywelsch left a comment

Choose a reason for hiding this comment

bleskes commented Sep 23, 2016

ywelsch commented Sep 23, 2016

bleskes Sep 23, 2016 •

edited

Loading

ywelsch Sep 23, 2016 •

edited

Loading