Add support for index pattern exclusion in CCR AutoFollow #72935

fcofdez · 2021-05-11T14:30:59Z

This pull request adds the ability to specify exclusion patterns for Auto-Follow patterns. This allows excluding indices that match any of the inclusion patterns and also match some of the exclusion patterns giving more fine grained control in scenarios where this is important.

Related #67686

elasticmachine · 2021-05-12T08:21:39Z

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

henningandersen

Thanks, this is looking good. I have a number of smaller comments only. I will request review from Tanguy too, since he authored auto-following initially.

.../rest-high-level/src/main/java/org/elasticsearch/client/ccr/PutAutoFollowPatternRequest.java

client/rest-high-level/src/test/java/org/elasticsearch/client/CCRIT.java

docs/reference/ccr/apis/auto-follow/get-auto-follow-pattern.asciidoc

docs/reference/ccr/apis/auto-follow/put-auto-follow-pattern.asciidoc

...n/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/PutAutoFollowPatternRequestTests.java

...c/test/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternActionTests.java

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

tlrx

I left some minor comments and I agree with Henning's comments that should be addressed. Looks good o.w. 👍

tlrx · 2021-05-17T10:17:33Z

x-pack/plugin/ccr/qa/multi-cluster/src/test/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java

+            assertBusy(() -> {
+                assertThat(getNumberOfSuccessfulFollowedIndices(), equalTo(initialNumberOfSuccessfulFollowedIndices + 1));
+            });


+1

I would also prefer to use a test specific prefix for the autofollow pattern name and inclusion/exclusion patterns (like testexcluions-*) because all tests share the same cluster which is not cleaned up after a test case is executed. Using a test specific prefix helps to debug those when they fail.

x-pack/plugin/ccr/qa/multi-cluster/src/test/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java

x-pack/plugin/ccr/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/ccr/auto_follow.yml

docs/reference/ccr/apis/auto-follow/put-auto-follow-pattern.asciidoc

…rn-auto-follow

followed index

…rn-auto-follow

fcofdez · 2021-05-25T11:24:00Z

Thanks both for the review, I've addressed your comments.

tlrx · 2021-05-26T08:25:40Z

...high-level/src/test/java/org/elasticsearch/client/ccr/GetAutoFollowPatternResponseTests.java

@@ -39,6 +39,7 @@
        for (int i = 0; i < numPatterns; i++) {
            String remoteCluster = randomAlphaOfLength(4);
            List<String> leaderIndexPatterns = Collections.singletonList(randomAlphaOfLength(4));
+            List<String> leaderIndexExclusionsPatterns = Collections.singletonList(randomAlphaOfLength(4));


Maybe randomize with empty, single and multiple exclusions patterns?

Addressed in c531a60

tlrx · 2021-05-26T08:46:46Z

x-pack/plugin/ccr/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/ccr/auto_follow.yml

@@ -84,6 +204,7 @@
        body:
          remote_cluster: local
          leader_index_patterns: ['logs-*']
+          leader_index_exclusion_patterns: ['logs-excluded']


I would have expected bwc tests to fail with this 🤔

That's right, maybe for a PR we don't execute all the BWC test matrix? I'll remove the exclusion pattern in that test.

tlrx · 2021-05-26T08:51:43Z

x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java

+        });
+        assertTrue(ESIntegTestCase.indexExists("copy-logs-201701", followerClient()));
+
+        createLeaderIndex("logs-201801", leaderIndexSettings);


@fcofdez I don't think it is resolved, doesn't it?

tlrx · 2021-05-26T09:28:08Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+                final IndexMetadata leaderIndexMetadata = remoteMetadata.index(leaderIndexName);
+
+                if (AutoFollowPattern.match(newLeaderPatterns, newLeaderIndexExclusionPatterns, indexAbstraction) == false) {
+                    logger.warn("The follower index {} for leader index {} does not match against the updated auto follow " +


I'm a bit torn about this - I wonder if we should make it more explicit to the user by returning an error. The same question applies when inclusion patterns are updated - a follower index might previously match a pattern but not anymore. Maybe we should prevent such update and require either a new pattern to be created or the follower indices to be unfollowed first?

I wasn't sure about what we wanted to do in these cases. I'm wondering if returning an error when a follower index was matching a previous pattern is a breaking change or not. In any case I think it's sensible to return an error in this scenario, I'll include it 👍.

In any case I think it's sensible to return an error in this scenario, I'll include it +1.

Maybe wait for @henningandersen's opinion first

We discussed this offline and we agreed that instead of emitting a warning or failing the request when an Auto-Follow patter is updated and previously configured follower indices don't match anymore we'll keep the current behaviour. Instead we'll improve the current docs.

…rn-auto-follow

tlrx

LGTM

tlrx · 2021-06-03T07:52:44Z

x-pack/plugin/ccr/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/ccr/auto_follow.yml

+---
+"Test put and delete auto follow pattern with exclusion patterns":
+  - skip:
+      version: " - 7.13.99"


I'm surprised this test passed when executed against the 7.x branch 🤔

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

…rn-auto-follow

fcofdez · 2021-06-04T11:54:09Z

Thanks Tanguy and Henning!

This commit adds the ability to specify exclusion patterns in Auto-Follow patterns. This allows excluding indices that match any of the inclusion patterns and also match some of the exclusion patterns giving more fine grained control in scenarios where this is important. Related elastic#67686 Backport of elastic#72935

This commit adds the ability to specify exclusion patterns in Auto-Follow patterns. This allows excluding indices that match any of the inclusion patterns and also match some of the exclusion patterns giving more fine grained control in scenarios where this is important. Related #67686 Backport of #72935

Relates #72935

…llowed Use an empty ClusterState as a base cluster state for that test Closes elastic#73797 Relates elastic#72935

…llowed (#73811) Use an empty ClusterState as a base cluster state to take into account the case where no leader candidate indices are created Closes #73797 Relates #72935

…llowed Use an empty ClusterState as a base cluster state to take into account the case where no leader candidate indices are created Closes elastic#73797 Relates elastic#72935 Backport of elastic#73811

…llowed (#73826) Use an empty ClusterState as a base cluster state to take into account the case where no leader candidate indices are created Closes #73797 Relates #72935 Backport of #73811

fcofdez force-pushed the add-exclusion-pattern-auto-follow branch from af122b7 to 29119ef Compare May 11, 2021 14:37

Add support for index pattern exclusion in CCR AutoFollow

9ec9685

fcofdez force-pushed the add-exclusion-pattern-auto-follow branch from 1c76a5b to 9ec9685 Compare May 11, 2021 16:42

fcofdez added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v8.0.0 >enhancement v7.14.0 labels May 12, 2021

fcofdez requested a review from henningandersen May 12, 2021 08:21

fcofdez marked this pull request as ready for review May 12, 2021 08:21

henningandersen reviewed May 14, 2021

View reviewed changes

henningandersen requested a review from tlrx May 14, 2021 08:48

tlrx reviewed May 17, 2021

View reviewed changes

fcofdez added 4 commits May 17, 2021 14:04

Merge remote-tracking branch 'origin/master' into add-exclusion-patte…

ce5b08c

…rn-auto-follow

Tackle review comments

c758c22

Add warning when an updated autofollow pattern does not match against a

44f7a85

followed index

Merge remote-tracking branch 'origin/master' into add-exclusion-patte…

ecffb4f

…rn-auto-follow

fcofdez requested review from tlrx and henningandersen May 25, 2021 11:23

tlrx reviewed May 26, 2021

View reviewed changes

fcofdez added 4 commits May 26, 2021 12:25

Review comments

c531a60

Merge remote-tracking branch 'origin/master' into add-exclusion-patte…

4ba557a

…rn-auto-follow

Remove unnecessary warn check and update docs

7394989

Merge remote-tracking branch 'origin/master' into add-exclusion-patte…

f96cfb4

…rn-auto-follow

fcofdez requested a review from tlrx June 2, 2021 09:49

tlrx approved these changes Jun 3, 2021

View reviewed changes

fcofdez added 2 commits June 4, 2021 11:31

Merge remote-tracking branch 'origin/master' into add-exclusion-patte…

8453931

…rn-auto-follow

Remove unused logger

02ffb5a

fcofdez merged commit a8101f3 into elastic:master Jun 4, 2021

fcofdez mentioned this pull request Jun 4, 2021

[7.x] Add support for index pattern exclusion in CCR AutoFollow #73765

Merged

fcofdez mentioned this pull request Jun 4, 2021

Fix AutoFollow version checks #73776

Merged

fcofdez added a commit that referenced this pull request Jun 4, 2021

Fix AutoFollow version checks (#73776)

0628213

Relates #72935

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Jun 7, 2021

Fix AutoFollowCoordinatorTests#testExcludedPatternIndicesAreNotAutoFo…

2b3518a

…llowed Use an empty ClusterState as a base cluster state for that test Closes elastic#73797 Relates elastic#72935

fcofdez mentioned this pull request Jun 7, 2021

Fix AutoFollowCoordinatorTests#testExcludedPatternIndicesAreNotAutoFollowed #73811

Merged

fcofdez mentioned this pull request Jun 7, 2021

[7.x] Fix AutoFollowCoordinatorTests#testExcludedPatternIndicesAreNotAutoFollowed #73826

Merged

stevejgordon mentioned this pull request Jul 1, 2021

7.14.0 Meta Ticket elastic/elasticsearch-net#5776

Closed

14 tasks

fcofdez mentioned this pull request Jul 9, 2021

Auto-follow all indices with CCR except for system indices #67686

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for index pattern exclusion in CCR AutoFollow #72935

Add support for index pattern exclusion in CCR AutoFollow #72935

fcofdez commented May 11, 2021 •

edited

Loading

elasticmachine commented May 12, 2021

henningandersen left a comment

tlrx left a comment

tlrx May 17, 2021

fcofdez commented May 25, 2021

tlrx May 26, 2021

fcofdez May 26, 2021

tlrx May 26, 2021

fcofdez May 26, 2021

tlrx May 26, 2021

tlrx May 26, 2021

fcofdez May 26, 2021

tlrx May 26, 2021

fcofdez Jun 2, 2021

tlrx left a comment

tlrx Jun 3, 2021

fcofdez commented Jun 4, 2021

Add support for index pattern exclusion in CCR AutoFollow #72935

Add support for index pattern exclusion in CCR AutoFollow #72935

Conversation

fcofdez commented May 11, 2021 • edited Loading

elasticmachine commented May 12, 2021

henningandersen left a comment

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented May 25, 2021

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

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented Jun 4, 2021

fcofdez commented May 11, 2021 •

edited

Loading