[CCR] Added history uuid validation #33546

martijnvg · 2018-09-09T04:47:05Z

For correctness we need to verify whether the history uuid of the leader
index shards never changes while that index is being followed.

The history UUIDs are recorded as custom index metadata in the follow index.
The follow api validates whether the current history UUIDs of the leader
index shards are the same as the recorded history UUIDs.
If not the follow api fails.
While a follow index is following a leader index; shard follow tasks
on each shard changes api call verify whether their current history uuid
is the same as the recorded history uuid.

Relates to #30086

For correctness we need to verify whether the history uuid of the leader index shards never changes while that index is being followed. * The history UUIDs are recorded as custom index metadata in the follow index. * The follow api validates whether the current history UUIDs of the leader index shards are the same as the recorded history UUIDs. If not the follow api fails. * While a follow index is following a leader index; shard follow tasks on each shard changes api call verify whether their current history uuid is the same as the recorded history uuid. Relates to elastic#30086

elasticmachine · 2018-09-09T04:47:07Z

Pinging @elastic/es-distributed

…hecker to avoid duplication of logic

jasontedor · 2018-09-09T11:39:11Z

@martijnvg It seems my latest push from #33496 introduced some merge conflicts here. Would you resolve them?

bleskes · 2018-09-09T14:07:25Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java

+        for (Map.Entry<Integer, String> entry : leaderIndexHistoryUUID.entrySet()) {
+            String recordedLeaderIndexHistoryUUID = recordedHistoryUUIDs.get(entry.getKey());
+            if (entry.getValue().equals(recordedLeaderIndexHistoryUUID) == false) {
+                throw new IllegalArgumentException("follow index [" + request.followerIndex + "] should reference [" +


maybe add some info as to how this can happen? (restore from snapshot is a likely cause, I think)

bleskes · 2018-09-09T14:14:10Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+
+        CheckedConsumer<IndicesStatsResponse, Exception> indicesStatsHandler = indicesStatsResponse -> {
+            IndexStats indexStats = indicesStatsResponse.getIndices().get(leaderIndex);
+            Map<Integer, String> historyUUIDs = new HashMap<>();


random drive by comment - we need to make sure we got history uuids for all the shards and a map data structure (compared to say, an array) hides it. Also - can we assert that the history uuids of all the shard copies that we got are identical?

I agree this would be clearer with a String[].

So then the index of the array indicates to which shard a historyUUID belongs?

So then the index of the array indicates to which shard a historyUUID belongs?

Correct. +1.

jasontedor · 2018-09-09T14:19:23Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java

@@ -82,6 +82,8 @@
 public class Ccr extends Plugin implements ActionPlugin, PersistentTaskPlugin, EnginePlugin {

    public static final String CCR_THREAD_POOL_NAME = "ccr";
+    public static final String CCR_CUSTOM_METADATA_KEY = "ccr";
+    public static final String CCR_CUSTOM_METADATA_LEADER_INDEX_HISTORY_UUID_KEY = "leader_index_history_uuid";


The history UUID is per shard (as is clear you understand based on your implementation), so I think the name is misleading.

jasontedor · 2018-09-09T14:20:39Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CreateAndFollowIndexAction.java

-            createFollowerIndex(leaderIndexMetadata, request, listener);
+            final String leaderIndex = request.getFollowRequest().getLeaderIndex();
+            final IndexMetaData leaderIndexMetadata = state.getMetaData().index(leaderIndex);
+            Consumer<Map<Integer, String>> handler = historyUUID -> {


I think historyUUIDs would be clearer to indicate there is one per shard.

dnhatn

@martijnvg This looks good. I left some comments.

dnhatn · 2018-09-10T16:56:13Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTask.java

@@ -278,6 +278,11 @@ protected void onOperationsFetched(Translog.Operation[] operations) {

    synchronized void innerHandleReadResponse(long from, long maxRequiredSeqNo, ShardChangesAction.Response response) {
        onOperationsFetched(response.getOperations());
+        if (params.getRecordedLeaderIndexHistoryUUID().equals(response.getHistoryUUID()) == false) {
+            markAsFailed(new IllegalStateException("unexpected history uuid, expected [" +


How about passing historyUUID to the Request then let IndexShard#newChangesSnapshot validate the historyUUID before creating a changes snapshot? I think it's less error-prone than examining the Response.

dnhatn · 2018-09-10T16:57:25Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+
+        CheckedConsumer<IndicesStatsResponse, Exception> indicesStatsHandler = indicesStatsResponse -> {
+            IndexStats indexStats = indicesStatsResponse.getIndices().get(leaderIndex);
+            Map<Integer, String> historyUUIDs = new HashMap<>();


So then the index of the array indicates to which shard a historyUUID belongs?

Correct. +1.

dnhatn · 2018-09-10T17:00:24Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java

@@ -568,4 +598,16 @@ private static Settings filter(Settings originalSettings) {
        return settings.build();
    }

+    private static Map<Integer, String> convert(Map<String, String> ccrMetadata) {


Should we name this method a bit less abstract? extractIndexShardHistoryUUIDs?

dnhatn · 2018-09-10T17:02:48Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+     * @param leaderIndex                               the name of the leader index
+     * @param onFailure                                 the failure consumer
+     * @param historyUUIDConsumer                       the leader index history uuid and consumer
+     * @param <T>                                       the type of response the listener is waiting for


Not sure if we need to make this a generic method?

dnhatn · 2018-09-10T17:05:53Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java

@@ -82,6 +82,8 @@
 public class Ccr extends Plugin implements ActionPlugin, PersistentTaskPlugin, EnginePlugin {

    public static final String CCR_THREAD_POOL_NAME = "ccr";
+    public static final String CCR_CUSTOM_METADATA_KEY = "ccr";
+    public static final String CCR_CUSTOM_METADATA_LEADER_INDEX_SHARDS_HISTORY_UUIDS = "leader_index_shard_history_uuids";


Should we make SHARDS singular? CCR_CUSTOM_METADATA_LEADER_INDEX_SHARDS_HISTORY_UUIDS?

martijnvg · 2018-09-10T20:28:36Z

@dnhatn Thanks for reviewing. I've updated this PR.

dnhatn

Thanks @martijnvg. I left some comments around how we handle the corner cases for commit stats.

dnhatn · 2018-09-11T02:11:29Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+            String[] historyUUIDs = new String[leaderIndexMetaData.getNumberOfShards()];
+            for (IndexShardStats indexShardStats : indexStats) {
+                for (ShardStats shardStats : indexShardStats) {
+                    CommitStats commitStats = shardStats.getCommitStats();


commitStats might be null if a shard is (being) closed.

dnhatn · 2018-09-11T02:13:42Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+                    CommitStats commitStats = shardStats.getCommitStats();
+                    String historyUUID = commitStats.getUserData().get(Engine.HISTORY_UUID_KEY);
+                    ShardId shardId = shardStats.getShardRouting().shardId();
+                    historyUUIDs[shardId.id()] = historyUUID;


You might have missed Boaz's comment:

Also - can we assert that the history uuids of all the shard copies that we got are identical

Moreover, not every shard is allocated or associated with a historyUUID. Should we fail if there is no historyUUID for a shardId?

oops, I will add the checks.

Moreover, not every shard is allocated or associated with a historyUUID

In what cases does a shard does not have a historyUUID?

can we assert that the history uuids of all the shard copies that we got are identical

In what cases are history uuids not unique between shards?

In what cases does a shard does not have a historyUUID?

I see, not yet started shards have no history uuid, which is more likely for replica shards.

dnhatn · 2018-09-11T02:33:17Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+            historyUUIDConsumer.accept(historyUUIDs);
+        };
+        IndicesStatsRequest request = new IndicesStatsRequest();
+        request.indices(leaderIndex);


We can "clear" all flags to reduce this stat request.

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java

dnhatn · 2018-09-11T02:48:50Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

     *
     * @param client                      the client
     * @param clusterAlias                the remote cluster alias
     * @param leaderIndex                 the name of the leader index
     * @param onFailure                   the failure consumer
-     * @param leaderIndexMetadataConsumer the leader index metadata consumer
+     * @param consumer                    the consumer for supplying the leader index metadata and historyUUIDs of all leader shards


We can re-indent the javadocs.

dnhatn · 2018-09-11T02:55:12Z

...in/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowTaskReplicationTests.java

-        ShardFollowTask params = new ShardFollowTask(null, new ShardId("follow_index", "", 0),
-            new ShardId("leader_index", "", 0), between(1, 64), between(1, 8), Long.MAX_VALUE, between(1, 4), 10240,
-            TimeValue.timeValueMillis(10), TimeValue.timeValueMillis(10), Collections.emptyMap());
+        ShardFollowTask params = new ShardFollowTask(


Can we have a simple unit test showing that if the request's historyUUID is mismatched, no operation is received and we abort the follow-task?

I have added a test here: 9620d7b

I added a method to ESIndexLevelReplicationTestCase so that I can reinit the primary shard. Which then allowed me to change the history uuid and restart the shard in the test. I think this an okay test approach, what do you think?

I am ok with this approach. How about just changing a historyUUID in the requests of the follow-task without really bootstrapping new history on the leader?

How about just changing a historyUUID in the requests of the follow-task without really bootstrapping new history on the leader?

That would change the recorded history uuid, which is immutable in non test scenarios. It would trigger the same error to be thrown, but it feels reverse to me. In this test now the current history uuid changes, which I think is more realistic.

in case of history uuid changed.

martijnvg · 2018-09-11T09:52:58Z

@dnhatn I've updated the PR.

dnhatn

@martijnvg I left some more comments around the commit stats.

dnhatn · 2018-09-11T15:05:23Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java

+                        onFailure.accept(new IllegalArgumentException("leader index's commit stats are missing"));
+                        return;
+                    }
+                    String historyUUID = commitStats.getUserData().get(Engine.HISTORY_UUID_KEY);


Martijn, I am sorry. I should have been clearer here:

If a commit stats is not null, it should have a valid history UUID. We can remove the null check historyUUID == null.

If a primary is unassigned, the index_shard_stats of that primary is not returned in the response; thus we won't have a history UUID for that shardId in the array. I think we should check that every entry in the historyUUIDs array is not null; otherwise, we should fail the request. WDYT?

Please note the assertion assert new HashSet<>(Arrays.asList(historyUUIDs)).size() == leaderIndexMetaData.getNumberOfShards(); does not guarantee that every entry is non-null.

I think we should check that every entry in the historyUUIDs array is not null; otherwise, we should fail the request. WDYT?

Agreed

Please note the assertion assert new HashSet<>(Arrays.asList(historyUUIDs)).size() == leaderIndexMetaData.getNumberOfShards(); does not guarantee that every entry is non-null.

Good point. I will check each entry individually.

martijnvg · 2018-09-11T15:42:30Z

@dnhatn I think I resolved the comment around commit stats.

dnhatn

LGTM. Thanks @martijnvg for extra iterations.

dnhatn · 2018-09-11T16:02:41Z

...in/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowTaskReplicationTests.java

+
+            assertBusy(() -> {
+                assertThat(shardFollowTask.isStopped(), is(true));
+                assertThat(shardFollowTask.getFailure().getMessage(), equalTo("unexpected history uuid, expected [" + oldHistoryUUID +


Not sure why this test fails on my machine.

java.lang.AssertionError: Expected: "unexpected history uuid, expected [_x3AmMhzQsa91ee-ah9Aug], actual [ZWUMypAWSEq9P3BEFIAGfA]" but: was "retrying failed [11] times, aborting..."

I think that is because it took to long to re-initialise the leader primary shard. Currently we retry up to 10 times and then fail. This will change when I merge #33371, which I will do before merging this PR and then this failure will not occur.

martijnvg · 2018-09-12T14:34:39Z

@elasticmachine run gradle build tests

For correctness we need to verify whether the history uuid of the leader index shards never changes while that index is being followed. * The history UUIDs are recorded as custom index metadata in the follow index. * The follow api validates whether the current history UUIDs of the leader index shards are the same as the recorded history UUIDs. If not the follow api fails. * While a follow index is following a leader index; shard follow tasks on each shard changes api call verify whether their current history uuid is the same as the recorded history uuid. Relates to #30086 Co-authored-by: Nhat Nguyen <[email protected]>

martijnvg added >enhancement review v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.5.0 labels Sep 9, 2018

martijnvg requested a review from dnhatn September 9, 2018 04:47

martijnvg added 3 commits September 9, 2018 08:25

renamed method

624b416

added unit test

0ed85c5

isolated logic that fetches history UUIDs and moved it to CcrLicenseC…

0685ee4

…hecker to avoid duplication of logic

Merge remote-tracking branch 'es/master' into ccr_validate_history_uuid

8293265

bleskes reviewed Sep 9, 2018

View reviewed changes

jasontedor reviewed Sep 9, 2018

View reviewed changes

iter

96edbde

jasontedor removed v6.5.0 v7.0.0 labels Sep 10, 2018

dnhatn reviewed Sep 10, 2018

View reviewed changes

martijnvg added 5 commits September 10, 2018 20:50

moved history uuid validation to internal shard changes api

b9ae5ce

keep recorded index shard uuids as string[] around

ee475a0

iter

9df2ac6

Merge remote-tracking branch 'es/master' into ccr_validate_history_uuid

eb5f7cd

fixed test after merge

e7e0f14

dnhatn reviewed Sep 11, 2018

View reviewed changes

martijnvg added 2 commits September 11, 2018 11:47

iter

43fbbee

added test that verifies that we stop and fail the shard follow task

9620d7b

in case of history uuid changed.

martijnvg added 2 commits September 11, 2018 12:06

fixed checkstyle violations and fixed error message assertions

8fee5ba

ignore replica shard stats

c2952b6

dnhatn reviewed Sep 11, 2018

View reviewed changes

iter

620505e

dnhatn approved these changes Sep 11, 2018

View reviewed changes

martijnvg added 4 commits September 12, 2018 08:42

Merge remote-tracking branch 'es/master' into ccr_validate_history_uuid

600626e

Merge remote-tracking branch 'es/master' into ccr_validate_history_uuid

8beaa0a

reduce timeout

89829fa

fixed checkstyle violation

7fa9070

martijnvg merged commit 5fa8131 into elastic:master Sep 12, 2018

jaymode mentioned this pull request Jun 15, 2020

Replace private index settings with index metadata #50758

Closed

[CCR] Added history uuid validation #33546

[CCR] Added history uuid validation #33546

Conversation

martijnvg commented Sep 9, 2018

elasticmachine commented Sep 9, 2018

jasontedor commented Sep 9, 2018

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

dnhatn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Sep 10, 2018

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn Sep 11, 2018 • 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

martijnvg commented Sep 11, 2018

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Sep 11, 2018

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Sep 12, 2018

dnhatn Sep 11, 2018 •

edited

Loading