Do not open indices with broken settings #26995
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
Today we are lenient and we open an index if it has broken settings. This can happen if a user installs a plugin that registers an index setting, creates an index with that setting, stop their node, removes the plugin, and then restarts the node. In this case, the index will have a setting that we do not recognize yet we open the index anyway. This leniency is dangerous so this commit removes it. Note that we still are lenient on upgrades and we should really reconsider this in a follow-up.
jasontedor
commented
Oct 12, 2017
| @@ -82,7 +82,6 @@ public MetaDataIndexUpgradeService(Settings settings, NamedXContentRegistry xCon | |||
| public IndexMetaData upgradeIndexMetaData(IndexMetaData indexMetaData, Version minimumIndexCompatibilityVersion) { | |||
| // Throws an exception if there are too-old segments: | |||
| if (isUpgraded(indexMetaData)) { | |||
| assert indexMetaData == archiveBrokenIndexSettings(indexMetaData) : "all settings must have been upgraded before"; | |||
There was a problem hiding this comment.
I really do not like removing this but it is in the way of testing this change and I do see hacks to get around it but I would rather not. Let's discuss this?
There was a problem hiding this comment.
well I think it's not necessarily correct to have this assertion since if you have a plugin that you remove with a setting it will fail. so I think it's ok to remove it no?
|
@s1monw I am opening this PR for discussion. |
s1monw
approved these changes
Oct 24, 2017
| @@ -82,7 +82,6 @@ public MetaDataIndexUpgradeService(Settings settings, NamedXContentRegistry xCon | |||
| public IndexMetaData upgradeIndexMetaData(IndexMetaData indexMetaData, Version minimumIndexCompatibilityVersion) { | |||
| // Throws an exception if there are too-old segments: | |||
| if (isUpgraded(indexMetaData)) { | |||
| assert indexMetaData == archiveBrokenIndexSettings(indexMetaData) : "all settings must have been upgraded before"; | |||
There was a problem hiding this comment.
well I think it's not necessarily correct to have this assertion since if you have a plugin that you remove with a setting it will fail. so I think it's ok to remove it no?
| @@ -430,7 +430,8 @@ private synchronized IndexService createIndexService(final String reason, | |||
| IndicesFieldDataCache indicesFieldDataCache, | |||
| List<IndexEventListener> builtInListeners, | |||
| IndexingOperationListener... indexingOperationListeners) throws IOException { | |||
| final IndexSettings idxSettings = new IndexSettings(indexMetaData, this.settings, indexScopeSetting); | |||
| final IndexSettings idxSettings = new IndexSettings(indexMetaData, this.settings, indexScopedSettings); | |||
| indexScopedSettings.validate(indexMetaData.getSettings(), true, true); | |||
There was a problem hiding this comment.
can you leave a comment why we pass true here
| .settings(Settings.builder().put(metaData.getSettings()).put("index.foo", "true")) | ||
| .build(); | ||
| // so evil | ||
| IndexMetaData.FORMAT.write(brokenMetaData, services.indexPaths(brokenMetaData.getIndex())); |
|
@jasontedor why didn't you merge this? |
|
I forgot about this one, sorry @s1monw. I will bring up to date with master and integrate tomorrow. |
* master: (414 commits) Set ACK timeout on indices service test Implement byte array reusage in `NioTransport` (elastic#27696) [TEST] Remove leftover ES temp directories before Vagrant tests (elastic#27722) Cleanup split strings by comma method Remove unused import from AliasResolveRoutingIT Add read timeouts to http module (elastic#27713) Fix routing with leading or trailing whitespace remove await fix from FullClusterRestartIT.testRecovery Add missing 's' to tmpdir name (elastic#27721) [Issue-27716]: CONTRIBUTING.md IntelliJ configurations settings are confusing. (elastic#27717) [TEST] Now actually wait for merges Test out of order delivery of append only index and retry with an intermediate delete [TEST] remove code duplications in RequestTests [Tests] Add test for GeoShapeFieldType#setStrategyName (elastic#27703) Remove unused *Commit* classes (elastic#27714) Add test for writer operation buffer accounting (elastic#27707) [TEST] Wait for merging to complete before testing breaker Add Open Index API to the high level REST client (elastic#27574) Correcting some minor typos in comments Add unreleased v5.6.6 version ...
jasontedor
added a commit
that referenced
this pull request
Dec 8, 2017
Today we are lenient and we open an index if it has broken settings. This can happen if a user installs a plugin that registers an index setting, creates an index with that setting, stop their node, removes the plugin, and then restarts the node. In this case, the index will have a setting that we do not recognize yet we open the index anyway. This leniency is dangerous so this commit removes it. Note that we still are lenient on upgrades and we should really reconsider this in a follow-up. Relates #26995
jasontedor
added a commit
that referenced
this pull request
Dec 9, 2017
This commit fixes the test of an index with an unknown setting. The problem here is that we were manipulating the index state on disk, but a cluster state update could arrive between us manipulating the index state on disk and us restarting the node, leading to the index state that we just intentionally broke being fixed. As such, after restart, the index state would not be in the state that we expected it to be in and the test would fail. To address this, we hook into the restart and break the index state immediately before the node is started again. Relates #26995
jasontedor
added a commit
that referenced
this pull request
Dec 9, 2017
This commit fixes the test of an index with an unknown setting. The problem here is that we were manipulating the index state on disk, but a cluster state update could arrive between us manipulating the index state on disk and us restarting the node, leading to the index state that we just intentionally broke being fixed. As such, after restart, the index state would not be in the state that we expected it to be in and the test would fail. To address this, we hook into the restart and break the index state immediately before the node is started again. Relates #26995
mayya-sharipova
added a commit
to mayya-sharipova/elasticsearch
that referenced
this pull request
Feb 8, 2018
- remove the leniency on opening indices with unrecognized settings and instead such an index will be closed with the unrecognized settings archived - do not open any index with archived settings - users can remove archived settings via the wildcard archived - return "index_open_exception" 400 status code when trying to open an index with broken settings Relates to elastic#26995 Closes elastic#26998
lcawl
added
:Search/Search
clintongormley
added
:Distributed Indexing/Distributed
mayya-sharipova
added a commit
to mayya-sharipova/elasticsearch
that referenced
this pull request
Mar 15, 2018
- remove the leniency on opening indices with unrecognized settings and instead such an index will be closed with the unrecognized settings archived - do not open any index with archived settings - users can remove archived settings via the wildcard archived - return "index_open_exception" 400 status code when trying to open an index with broken settings Relates to elastic#26995 Closes elastic#26998
mayya-sharipova
added a commit
to mayya-sharipova/elasticsearch
that referenced
this pull request
Mar 16, 2018
- remove the leniency on opening indices with unrecognized settings and instead such an index will be closed with the unrecognized settings archived - do not open any index with archived settings - users can remove archived settings via the wildcard archived - return illegal_argument_exception 400 status code when trying to open an index with broken/archived settings Relates to elastic#26995 Closes elastic#26998
mayya-sharipova
added a commit
to mayya-sharipova/elasticsearch
that referenced
this pull request
Mar 16, 2018
- remove the leniency on opening indices with unrecognized settings and instead such an index will be closed with the unrecognized settings archived - do not open any index with archived settings - users can remove archived settings via the wildcard archived - return illegal_argument_exception 400 status code when trying to open an index with broken/archived settings Relates to elastic#26995 Closes elastic#26998
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 we are lenient and we open an index if it has broken settings. This can happen if a user installs a plugin that registers an index setting, creates an index with that setting, stop their node, removes the plugin, and then restarts the node. In this case, the index will have a setting that we do not recognize yet we open the index anyway. This leniency is dangerous so this commit removes it. Note that we still are lenient on upgrades and we should really reconsider this in a follow-up.