Detect remnants of path.data/default.path.data bug #24099
Merged
Conversation
This file contains hidden or 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
In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended. This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node.
jasontedor
added
:Core/Infra/Core
s1monw
suggested changes
Apr 14, 2017
Contributor
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
I am not sure this change is OK and I would like to discuss different solutions for this down the road. I'd like to not add the permission and find the possible issue earlier in the game. I think we can only do that if we ignore the nodeLockID which I think we should. It is an edgecase we are trying to solve. if we'd not look at that and only look if there is data in default.path.data we would do as good as this solution IMO but with much less trouble. I think this is over engineering we are trying to do here but I am ok with compromising on dropping the sophisticated method here in 6.0 since our recommendation is to upgrade to a latest version first before you upgrade even if it's a full cluster restart. We can still have a simpler check in 6.0 then. @rjernst WDYT can you review this as well?
| return nodePaths; | ||
| } | ||
|
|
||
| public int nodeLockId() { |
Contributor
There was a problem hiding this comment.
can we call this getNodeLockId() pls
| * @param logger a logger where messages regarding the detection will be logged | ||
| * @throws IOException if an I/O exception occurs reading the directory structure | ||
| */ | ||
| static void checkForIndexDataInDefaultPathData( |
Contributor
There was a problem hiding this comment.
you have 140 chars use them :)
| final Settings settings, | ||
| final NodeEnvironment nodeEnv, | ||
| final Logger logger) throws IOException { | ||
| if (!Environment.PATH_DATA_SETTING.exists(settings) || !Environment.DEFAULT_PATH_DATA_SETTING.exists(settings)) return; |
| } | ||
| } | ||
|
|
||
| if (clean) return; |
| boolean clean = true; | ||
| for (final String defaultPathData : Environment.DEFAULT_PATH_DATA_SETTING.get(settings)) { | ||
| final Path nodeDirectory = NodeEnvironment.resolveNodePath(getPath(defaultPathData), nodeEnv.nodeLockId()); | ||
| if (!Files.exists(nodeDirectory)) continue; |
Contributor
There was a problem hiding this comment.
please wrap in {} and use == false
| if (!Files.exists(nodeDirectory)) continue; | ||
| final NodeEnvironment.NodePath nodePath = new NodeEnvironment.NodePath(nodeDirectory); | ||
| final Set<String> availableIndexFolders = nodeEnv.availableIndexFoldersForPath(nodePath); | ||
| if (availableIndexFolders.isEmpty()) continue; |
s1monw
approved these changes
Apr 17, 2017
Contributor
|
@jasontedor lets merge this and I will look into improving this in master. I think the solution is good enough and we should make progress over perfection |
jasontedor
added a commit
that referenced
this pull request
Apr 17, 2017
In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended. This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node. Relates #24099
jasontedor
added a commit
that referenced
this pull request
Apr 17, 2017
In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended. This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node. Relates #24099
jasontedor
added a commit
that referenced
this pull request
Apr 17, 2017
In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended. This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node. Relates #24099
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. When this bug concerns path.data and default.path.data, we ended up in a situation where the paths specified in both settings would be used to write index data. Since our packaging sets default.path.data, users that configure multiple data paths via an array and use the packaging are subject to having shards land in paths in default.path.data when that is very likely not what they intended.
This commit is an attempt to rectify this situation. If path.data and default.path.data are configured, we check for the presence of indices there. If we find any, we log messages explaining the situation and fail the node.
Closes #23981, supersedes #24052, relates #24074, relates #24093