[7.x] Deprecate Auto-Follow system indices #73237

fcofdez · 2021-05-19T12:47:18Z

This commits deprecates Auto-Follow of system indices

fcofdez · 2021-05-25T11:40:11Z

.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java

+
+        return new DeprecationIssue(DeprecationIssue.Level.WARNING,
+            "Some follower indices follow remote system indices",
+            "https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html",


Where should I add the deprecation note?

I believe it should go to docs/reference/release-notes/7.14.asciidoc, you may need to add a reference from release-notes.asciidoc to it.

elasticmachine · 2021-05-31T08:04:53Z

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

henningandersen

I left a couple of comments.

Also, please fill in the PR description including links to the breaking change issue as well as any other relevant issues. Notice that the commit line length of the PR title is too long, should be max 50 characters.

henningandersen · 2021-05-31T19:24:58Z

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

+
+package org.elasticsearch.xpack.core.ccr;
+
+public class CCR {


I think we should stick to conventions for Ccr elsewhere and use Ccr as the base name of the class. Also, I would prefer to call it CcrConstants just to distinguish it from the other Ccr class (which is the plugin, kind of defining the module).

henningandersen · 2021-05-31T19:36:12Z

...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutFollowAction.java

@@ -135,6 +140,12 @@ private void createFollowerIndex(
            return;
        }

+        if (leaderIndexMetadata.isSystem()) {
+            deprecationLogger.deprecate(DeprecationCategory.INDICES,


This deprecation logging will fire on any explicit system index follow, whereas the breaking change in #72815 only skips auto-following system indices.

I would prefer to have the deprecation logging for auto-following only.

Yes, that's something we need to discuss. Currently there's no way to know if a followed index request come from AutoFollow or from an explicit user request. I was wondering if we should add some extra metadata in order to distinguish the following origin, in that case existing followed system indices won't show up in the deprecation API. wdyt @henningandersen?

henningandersen · 2021-05-31T19:37:17Z

.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java

+                continue;
+            }
+            if (indexMetadata.isSystem()) {
+                systemIndexFollowers.add(indexEntry.key);


Same comment on the scope as above, we should only log deprecation issues when the index was originally auto-followed I think?

…system-indices

henningandersen

I left a few comments.

henningandersen · 2021-06-03T10:03:55Z

docs/reference/migration/migrate_7_14.asciidoc

+
+*Impact* +
+In 8.0.0, remote system indices matching an <<ccr-auto-follow,auto-follow pattern>>
+won't be configured as a follower index automatically.


I think we should add a note on how to adapt to 8.0 behavior, by excluding patterns matching system indices like .tasks and .kibana-*.

henningandersen · 2021-06-07T10:32:38Z

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

@@ -196,6 +200,12 @@ private static void markExistingIndicesAsAutoFollowed(
        for (final IndexMetadata indexMetadata : leaderMetadata) {
            IndexAbstraction indexAbstraction = leaderMetadata.getIndicesLookup().get(indexMetadata.getIndex().getName());
            if (AutoFollowPattern.match(patterns, indexAbstraction)) {
+                if (indexAbstraction.isSystem()) {


I am torn on this. I think the logic here ensures that we do not follow these existing system indices anyway and therefore it hardly makes sense to have the deprecation warning here? I would opt to remove it.

My thinking here was that we'll deprecate this behaviour in 7.14 but keep auto following system indices until 8.0. is that a misunderstanding on my side?

henningandersen · 2021-06-07T10:44:45Z

.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java

+                continue;
+            }
+            final String leaderIndexUUID = ccrMetadata.get(CcrConstants.CCR_CUSTOM_METADATA_LEADER_INDEX_UUID_KEY);
+            if (indexMetadata.isSystem() && followedLeaderIndexUUIDs.contains(leaderIndexUUID)) {


I were a bit surprised that isSystem() is true here, but I did not find the place where we would filter that out. I suppose it is correct, since it is sort of a restore. Would be nice to validate though in a test?

We don't filter that metadata out. I'll add a test to validate it 👍

You were right about this, the flag is lost when the index is "restored". I had to write a new checker that reaches the remote cluster to tell if the leader index was a system index. See CcrAutoFollowedSystemIndicesChecker and CcrAutoFollowInfoFetcher

henningandersen · 2021-06-07T10:47:42Z

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

 import org.elasticsearch.xpack.ccr.CcrLicenseChecker;
 import org.elasticsearch.xpack.ccr.CcrSettings;
 import org.elasticsearch.xpack.core.ccr.AutoFollowMetadata;
 import org.elasticsearch.xpack.core.ccr.AutoFollowMetadata.AutoFollowPattern;
 import org.elasticsearch.xpack.core.ccr.AutoFollowStats;
+import org.elasticsearch.xpack.core.ccr.CcrConstants;


I think we should add a check to checkAutoFollowPattern to see if the leader index is a system index and output a deprecation warning if so?

(not related to the line the comment is on).

…system-indices

fcofdez · 2021-06-23T14:12:28Z

@henningandersen would you mind reviewing this PR when you have the chance? Thanks!

henningandersen

A few more comments, looking good otherwise.

henningandersen · 2021-06-28T14:37:58Z

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

@@ -132,11 +132,6 @@
 public class Ccr extends Plugin implements ActionPlugin, PersistentTaskPlugin, EnginePlugin, RepositoryPlugin, ClusterPlugin {

    public static final String CCR_THREAD_POOL_NAME = "ccr";
-    public static final String CCR_CUSTOM_METADATA_KEY = "ccr";


Perhaps leave a comment here, stating that these moved to CcrConstants in 7.x (since this targets 7.x only)?

Addressed in b80e0e9

henningandersen · 2021-06-28T14:41:38Z

.../plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinatorTests.java

@@ -2154,6 +2155,114 @@ void cleanFollowedRemoteIndices(ClusterState remoteClusterState, List<String> pa
        }
    }

+    public void testDeprecationWarningsAreEmittedWhenASystemIndexIsAutoFollowed() throws Exception {
+        // Set up a mock log appender to watch for the log message we expect


This comment seems to be out of date.

Addressed in b80e0e9 🤦

henningandersen · 2021-06-28T14:49:16Z

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

+    private static boolean isCurrentlyFollowed(PersistentTasksCustomMetadata persistentTasks, String indexName) {
+        return persistentTasks != null && persistentTasks.findTasks(ShardFollowTask.NAME, task -> true).stream()
+            .map(task -> (ShardFollowTask) task.getParams())
+            .anyMatch(shardFollowTask -> indexName.equals(shardFollowTask.getFollowShardId().getIndexName()));


Perhaps use Index rather than the index name, so that we compare both uuid and name.

Addressed in b80e0e9

henningandersen · 2021-06-28T14:58:20Z

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

+
+                        @Override
+                        public void onFailure(Exception e) {
+                            clusterResponsesListener.onFailure(e);


I think this bails out of the deprecation check and I wonder if a remote cluster might not be somewhat expected to be unavailable sometimes. I wonder if we should instead present something about remote cluster not available in deprecation info?

The deprecation API doesn't provide a way to return fine grained error information. Currently the only thing we can do is return a DeprecationIssue but I would find a bit confusing to present an error about a remote cluster unavailability as a DeprecationIssue.

…system-indices

henningandersen

LGTM.

…system-indices

Add deprecation warning when a system index is followed

9b8adc0

fcofdez force-pushed the deprecate-auto-follow-system-indices branch from 121c403 to 9b8adc0 Compare May 25, 2021 08:22

More precise naming

f38bc7a

fcofdez marked this pull request as ready for review May 25, 2021 11:38

fcofdez requested a review from henningandersen May 25, 2021 11:39

fcofdez commented May 25, 2021

View reviewed changes

fcofdez added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 31, 2021

fcofdez added :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v7.14.0 >breaking labels May 31, 2021

henningandersen reviewed May 31, 2021

View reviewed changes

fcofdez added 5 commits June 1, 2021 15:38

Warning only on auto-followed system indices

7e8d53e

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

8804d9d

…system-indices

Fix error

f4967e5

Add deprecation docs

a48b770

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

69910d7

…system-indices

fcofdez changed the title ~~[7.x] Add deprecation warning when a system index is followed~~ [7.x] Add deprecation warnings for auto-followed system indices Jun 1, 2021

fcofdez added 2 commits June 1, 2021 17:43

Fixes

d895be7

Cleanup

9066e41

fcofdez changed the title ~~[7.x] Add deprecation warnings for auto-followed system indices~~ [7.x] Deprecate Auto-Follow system indices Jun 2, 2021

fcofdez requested a review from henningandersen June 2, 2021 09:45

fcofdez added the >deprecation label Jun 2, 2021

henningandersen reviewed Jun 7, 2021

View reviewed changes

fcofdez added 6 commits June 8, 2021 13:50

Add proper auto follow system indices check

790a09f

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

88d5188

…system-indices

Remove unused code

19cae57

Unused imports

28918ce

Simplify test

08b3641

Include migrating behaviour

3a6cc0b

fcofdez added 3 commits June 9, 2021 10:01

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

3045ead

…system-indices

Simplify

962a747

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

36d81a6

…system-indices

fcofdez requested a review from henningandersen June 9, 2021 16:03

fcofdez added 2 commits June 23, 2021 12:04

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

a949a91

…system-indices

Fix compilation

74c289b

henningandersen reviewed Jun 28, 2021

View reviewed changes

fcofdez added 3 commits June 28, 2021 17:49

Review comments

b80e0e9

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

6a9edf8

…system-indices

Rearrange docs

8727ce5

fcofdez requested a review from henningandersen June 28, 2021 16:40

henningandersen approved these changes Jun 29, 2021

View reviewed changes

Merge remote-tracking branch 'origin/7.x' into deprecate-auto-follow-…

0573217

…system-indices

fcofdez merged commit bd02f91 into elastic:7.x Jun 29, 2021

henningandersen mentioned this pull request Jul 5, 2021

Avoid auto following leader system indices in CCR #72815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7.x] Deprecate Auto-Follow system indices #73237

[7.x] Deprecate Auto-Follow system indices #73237

fcofdez commented May 19, 2021 •

edited

Loading

fcofdez May 25, 2021

henningandersen May 31, 2021

elasticmachine commented May 31, 2021

henningandersen left a comment

henningandersen May 31, 2021

henningandersen May 31, 2021

fcofdez Jun 1, 2021

henningandersen May 31, 2021

henningandersen left a comment

henningandersen Jun 3, 2021

henningandersen Jun 7, 2021

fcofdez Jun 7, 2021

henningandersen Jun 7, 2021

fcofdez Jun 7, 2021

fcofdez Jun 9, 2021

henningandersen Jun 7, 2021

fcofdez commented Jun 23, 2021

henningandersen left a comment

henningandersen Jun 28, 2021

fcofdez Jun 28, 2021

henningandersen Jun 28, 2021

fcofdez Jun 28, 2021

henningandersen Jun 28, 2021

fcofdez Jun 28, 2021

henningandersen Jun 28, 2021

fcofdez Jun 28, 2021

henningandersen left a comment

[7.x] Deprecate Auto-Follow system indices #73237

[7.x] Deprecate Auto-Follow system indices #73237

Conversation

fcofdez commented May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 31, 2021

henningandersen 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

henningandersen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented Jun 23, 2021

henningandersen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

fcofdez commented May 19, 2021 •

edited

Loading