Prevent snapshot backed indices to be followed using CCR #70580
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tlrx
added
>enhancement
:Distributed Indexing/CCR
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Build failure is #70621, I'll merge master again to pick up the test muting. |
henningandersen
approved these changes
Mar 23, 2021
| "index to follow [%s] is a searchable snapshot index and cannot be used for cross-cluster replication purpose", | ||
| indexToFollow.getName() | ||
| ); | ||
| LOGGER.warn(message); |
There was a problem hiding this comment.
I wonder if this needs to be a warning? It seems like a documented behavior and therefore this could be just DEBUG?
I wonder if we should update docs for auto-follow to state that searchable snapshots cannot be followed and will be ignored by auto-follow patterns?
There was a problem hiding this comment.
I wonder if this needs to be a warning? It seems like a documented behavior and therefore this could be just DEBUG?
I did not find any documentation relative to this - hence the initial WARN level - but maybe you found something in the CCR doc? I pushed e8ab12a to move to debug.
I wonder if we should update docs for auto-follow to state that searchable snapshots cannot be followed and will be ignored by auto-follow patterns?
I think we should and I'd like to do a follow up PR for this.
There was a problem hiding this comment.
I did not see any such info, I just thought it was more logical to simply ignore these than to warn, in particular because of the issues to use * patterns when auto-following like #67686
| final String mountedIndex = "mounted-" + testPrefix; | ||
| if ("leader".equals(targetCluster)) { | ||
| final String repositoryPath = System.getProperty("tests.leader_cluster_repository_path") + '/' + testPrefix; | ||
| assertThat("Missing system property [tests.leader_cluster_repository_path]", repositoryPath, not(emptyOrNullString())); |
There was a problem hiding this comment.
I think this can never be true? Did you mean to check that the system property is set?
There was a problem hiding this comment.
Thanks for catching this!
Indeed I wanted to check the existence of the sysprop but I later added a suffix which made this assertion useless. Fixing the assertion allowed to catch a bug where the sysprop was not correctly passed everywhere so I pushed a2a5fdf to fix all of that.
| { | ||
| try (var leaderClient = buildLeaderClient()) { | ||
| final String repositoryPath = System.getProperty("tests.leader_cluster_repository_path") + '/' + testPrefix; | ||
| assertThat("Missing system property [tests.leader_cluster_repository_path]", repositoryPath, not(emptyOrNullString())); |
There was a problem hiding this comment.
I think you meant to check the value of the system property and not the value of the concatenated string (which cannot be empty).
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Mar 24, 2021
Today nothing prevents CCR's auto-follow patterns to pick up snapshot backed indices on a remote cluster. This can lead to various errors on the follower cluster that are not obvious to troubleshoot for a user (ex: multiple engine factories provided). This commit adds verifications to CCR to make it fail faster when a user tries to follow an index that is backed by a snapshot, providing a more obvious error message. Backport of elastic#70580
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Mar 24, 2021
Today nothing prevents CCR's auto-follow patterns to pick up snapshot backed indices on a remote cluster. This can lead to various errors on the follower cluster that are not obvious to troubleshoot for a user (ex: multiple engine factories provided). This commit adds verifications to CCR to make it fail faster when a user tries to follow an index that is backed by a snapshot, providing a more obvious error message. Backport of elastic#70580
|
Thanks Henning! |
tlrx
added a commit
that referenced
this pull request
Mar 24, 2021
Today nothing prevents CCR's auto-follow patterns to pick up snapshot backed indices on a remote cluster. This can lead to various errors on the follower cluster that are not obvious to troubleshoot for a user (ex: multiple engine factories provided). This commit adds verifications to CCR to make it fail faster when a user tries to follow an index that is backed by a snapshot, providing a more obvious error message. Backport of #70580
tlrx
added a commit
that referenced
this pull request
Mar 24, 2021
Today nothing prevents CCR's auto-follow patterns to pick up snapshot backed indices on a remote cluster. This can lead to various errors on the follower cluster that are not obvious to troubleshoot for a user (ex: multiple engine factories provided). This commit adds verifications to CCR to make it fail faster when a user tries to follow an index that is backed by a snapshot, providing a more obvious error message. Backport of #70580
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Apr 6, 2021
…lastic#70863) This commit adds a note in CCR document about auto-follow patterns that should not match searchable snapshots indices. Relates elastic#70580 (comment)
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Apr 6, 2021
…lastic#70863) This commit adds a note in CCR document about auto-follow patterns that should not match searchable snapshots indices. Relates elastic#70580 (comment)
tlrx
added a commit
that referenced
this pull request
Apr 6, 2021
tlrx
added a commit
that referenced
this pull request
Apr 6, 2021
tlrx
added a commit
that referenced
this pull request
Jun 28, 2021
The test AutoFollowIT.testAutoFollowSearchableSnapshotsFails was added in #70580 in order to test that mounted indices of a leader cluster are not auto-followed in a follower cluster using CCR. This test sometimes fails because it expects 2 indices to be followed (the -regular and the -index indices) but not the mounted one. This looks wrong as the -index index is deleted soon after it is snapshotted, and this index only exist to create a snapshot that can be later mounted as an index in the leader cluster. This commit changes the test so that the -index index, the repository and the snapshot are created at the beginning of the test. Then the test creates the mounted index and the regular one and can now asserts that only the regular one was auto-followed. Closes #74486
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Jun 28, 2021
…4498) The test AutoFollowIT.testAutoFollowSearchableSnapshotsFails was added in elastic#70580 in order to test that mounted indices of a leader cluster are not auto-followed in a follower cluster using CCR. This test sometimes fails because it expects 2 indices to be followed (the -regular and the -index indices) but not the mounted one. This looks wrong as the -index index is deleted soon after it is snapshotted, and this index only exist to create a snapshot that can be later mounted as an index in the leader cluster. This commit changes the test so that the -index index, the repository and the snapshot are created at the beginning of the test. Then the test creates the mounted index and the regular one and can now asserts that only the regular one was auto-followed. Closes elastic#74486
tlrx
added a commit
that referenced
this pull request
Jun 28, 2021
The test AutoFollowIT.testAutoFollowSearchableSnapshotsFails was added in #70580 in order to test that mounted indices of a leader cluster are not auto-followed in a follower cluster using CCR. This test sometimes fails because it expects 2 indices to be followed (the -regular and the -index indices) but not the mounted one. This looks wrong as the -index index is deleted soon after it is snapshotted, and this index only exist to create a snapshot that can be later mounted as an index in the leader cluster. This commit changes the test so that the -index index, the repository and the snapshot are created at the beginning of the test. Then the test creates the mounted index and the regular one and can now asserts that only the regular one was auto-followed. Backport of #74498 Closes #74486
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today nothing prevents CCR's auto-follow patterns to pick up snapshot backed indices on a remote cluster. This can lead to various errors on the follower cluster that are not obvious to troubleshoot for a user (ex:
multiple engine factories provided).This pull request adds verifications to CCR to make it fail faster when a user tries to follow an index that is backed by a snapshot, providing a more obvious error message.