Allow legacy index settings on legacy indices #90264

grcevski · 2022-09-22T14:08:05Z

Our backward compatibility on indices states that we support indices with one version less than the current version. Those legacy indices might have legacy index settings, which we typically archive on cluster restart. However, this archiving happens only on full cluster restart, not on rolling restarts, which effectively leaves the indices unusable in 8.x.

This PR adds code that ignores unknown settings on indices if the index created version is less than the current version.

Closes #84992

Add code to ignore unknown index settings on old indices, to allow rolling-upgrades to work.

elasticsearchmachine · 2022-09-22T14:08:29Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-09-22T14:08:30Z

Hi @grcevski, I've created a changelog YAML for you.

…icsearch into fix/legacy_index_settings

DaveCTurner

I think we shouldn't introduce a broad-brush lenience like this. We should instead reinstate the settings that we removed, along with a setting validator that prevents them from being set on 8.x indices.

By leaving these settings unregistered I think we prevent folks from adjusting or removing them on older indices, but these things should remain possible even in 8.x. I also think they'd still be archived in a full-cluster restart but we don't want that either.

(clarifying edit:) In the case of the slowlog settings I think it's ok if setting them has no effect in 8.x. But they still should remain registered.

grcevski · 2022-09-22T16:17:27Z

We should instead reinstate the settings that we removed, along with a setting validator that prevents them from being set on 8.x indices.

Good idea! I'll take that approach instead, it's similar to what we had done for camel case date field support.

grcevski · 2022-09-22T23:16:01Z

@DaveCTurner, I thought more about this and I think we should be archiving these settings before validation. If we silently accept them as valid settings for 7.x indices, we then run into a problem where the usage will be confusing to our end users.

For example, the slow logs setting did something in 7.x, even though we warned them of deprecation, but now in 8.x we will allow it to exist and be changed as a setting, but it will do nothing. So it's sort of valid setting you can set, but it does nothing anymore. It still breaks BWC, I'd rather we told them somehow their setting no longer works.

I think a better solution would be to archive the legacy settings when we validate them. I can bring back the list of these legacy settings and only archive those, but we archive all unknown settings already on restart, I think we might as well archive all of them.

DaveCTurner · 2022-09-23T07:49:52Z

I would very much prefer not to archive any index settings. We almost never do a full-cluster-restart upgrade any more, and during a rolling upgrade it is not at all obvious when to do the archiving, nor how we should behave in the mixed-version cluster before the archival happens.

It is unfortunate that in this case we removed the associated slowlog functionality prematurely, but ISTR we had good reasons for doing so. I think that won't cause too many problems however: the settings were properly deprecated in 7.x so the user should have already seen guidance about their use, and as long as the settings are still marked as deprecated in 8.x the user will continue to see warnings about not using them. We could even perhaps customise those warnings to indicate that they're already no-ops, although I wouldn't put much effort into that until we start to see evidence that it's needed.

Are there other settings to be reinstated or is it just about these slowlog ones? If there are others, we'll need to think about what to do with them on a case-by-case basis.

DaveCTurner

I left some thoughts mostly on code-habitability. I like this approach much more tho.

server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java

server/src/main/java/org/elasticsearch/common/settings/Setting.java

DaveCTurner · 2022-09-24T08:03:35Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+        /**
+         * Indicates that this index-level setting was deprecated and removed in the current major
+         */
+        DeprecatedAndRemovedInCurrentMajor


We should also reject non-index settings that use this property - see checkPropertyRequiresIndexScope. Also I think we can make this property mutually exclusive with Deprecated and DeprecatedWarning.

Also (maybe in a followup) can we forbid Deprecated on IndexScope settings and require all index settings to use this framework instead?

Good idea. About removing all Deprecated, I guess we'll need one more property, deprecated and removed in Next major, if we have deprecated any in 8.

By my reckoning there's currently exactly one such index setting that's deprecated in v8 and to be removed in v9: index.merge.policy.max_merge_at_once_explicit.

I'll leave it up to you whether to give this setting the same treatment now or later.

DaveCTurner · 2022-09-24T08:06:50Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

+        new Setting.Validator<>() {
+            @Override
+            public void validate(Integer value) {}
+
+            @Override
+            public void validate(Integer value, Map<Setting<?>, Object> settings) {
+                validateVersionDependentDeprecatedSetting(MAX_ADJACENCY_MATRIX_FILTERS_SETTING.getKey(), settings);
+            }
+
+            @Override
+            public Iterator<Setting<?>> settings() {
+                final List<Setting<?>> settings = List.of(IndexMetadata.SETTING_INDEX_VERSION_CREATED);
+                return settings.iterator();
+            }
+        },


Hmm I think we should do this validation automatically within the settings framework if a setting is marked as Property.DeprecatedAndRemovedInCurrentMajor. If we require implementors to remember to include this validator too then someone will forget and that'd be a bug.

Fair enough, I can add the validation at lower level.

pgomulka

I like the approach and left minor questions.

pgomulka · 2022-09-26T09:21:41Z

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/UpgradeWithOldIndexSettingsIT.java

+import static org.elasticsearch.rest.action.search.RestSearchAction.TOTAL_HITS_AS_INT_PARAM;
+import static org.hamcrest.Matchers.is;
+
+public class UpgradeWithOldIndexSettingsIT extends AbstractRollingTestCase {


out of curiosity.
Any particular reason not to use yml test for this?

It's just my comfort level with writing yml tests, but that definitely means I should add one :). I'll add a yml test too!

server/src/main/java/org/elasticsearch/common/settings/Setting.java

Co-authored-by: David Turner <[email protected]>

grcevski · 2022-09-27T23:16:35Z

@elasticsearchmachine run elasticsearch-ci/docs

DaveCTurner

Looks good to me; I left one optional suggestion

done

DaveCTurner · 2022-11-02T09:05:12Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

            if (propertiesAsSet.contains(Property.Deprecated) && propertiesAsSet.contains(Property.DeprecatedWarning)) {
                throw new IllegalArgumentException("setting [" + key + "] cannot be deprecated at both critical and warning levels");
            }
+            if ((propertiesAsSet.contains(Property.Deprecated) || propertiesAsSet.contains(Property.DeprecatedWarning))
+                && propertiesAsSet.contains(Property.IndexSettingDeprecatedInV7AndRemovedInV8)) {
+                throw new IllegalArgumentException("setting [" + key + "] cannot be just deprecated and deprecated and removed in V7");
+            }


Hmm suggestion got lost it seems. Here it is again:

Suggested change

if (propertiesAsSet.contains(Property.Deprecated) && propertiesAsSet.contains(Property.DeprecatedWarning)) {

throw new IllegalArgumentException("setting [" + key + "] cannot be deprecated at both critical and warning levels");

}

if ((propertiesAsSet.contains(Property.Deprecated) || propertiesAsSet.contains(Property.DeprecatedWarning))

&& propertiesAsSet.contains(Property.IndexSettingDeprecatedInV7AndRemovedInV8)) {

throw new IllegalArgumentException("setting [" + key + "] cannot be just deprecated and deprecated and removed in V7");

}

if (propertiesAsSet.stream().filter(DEPRECATED_PROPERTIES::contains).count() > 1) {

throw new IllegalArgumentException("setting [" + key + "] must be at most one of [" + DEPRECATED_PROPERTIES + "]");

}

That's a really nice code simplification. I'll add it in.

elasticsearchmachine · 2022-11-02T18:00:14Z

💚 Backport successful

Status	Branch	Result
✅	8.5

Add code to ignore unknown index settings on old indices, to allow rolling-upgrades to work. Co-authored-by: David Turner <[email protected]>

Add code to ignore unknown index settings on old indices, to allow rolling-upgrades to work. Co-authored-by: David Turner <[email protected]> Co-authored-by: David Turner <[email protected]>

* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java

…cation to a warning This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. This is an index setting and should have been marked as `IndexSettingDeprecatedInV8AndRemovedInV9`, but we can't change that in 8.x and will mark it as `IndexSettingDeprecatedInV9AndRemovedInV10` in 9.0 See elastic#90264 See ES-11224

…ion to a warning This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. This is an index setting and should have been marked as `IndexSettingDeprecatedInV8AndRemovedInV9`, but we can't change that in 8.x and will mark it as `IndexSettingDeprecatedInV9AndRemovedInV10` in 9.0 See elastic#90264 See ES-11224

This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. This is an index setting and should have been marked as `IndexSettingDeprecatedInV8AndRemovedInV9`, but we can't change that in 8.x and will mark it as `IndexSettingDeprecatedInV9AndRemovedInV10` in 9.0 See elastic#90264 See ES-11224

…ed in v10 Introduce `Property.IndexSettingDeprecatedInV9AndRemovedInV10` property setting to mark index settings that accepted on 9.x indices, but not on 10.0. See elastic#80574, elastic#90264 See ES-11224

…ion (#124792) This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. This is an index setting and should have been marked as `IndexSettingDeprecatedInV8AndRemovedInV9`, but we can't change that in 8.x and will mark it as `IndexSettingDeprecatedInV9AndRemovedInV10` in 9.0 See #90264 See ES-11224

…ed in v10 (#124807) Introduce `Property.IndexSettingDeprecatedInV9AndRemovedInV10` property setting to mark index settings that accepted on 9.x indices, but not on 10.0. See #80574, #90264 See ES-11224

Nikola Grcevski added 3 commits September 22, 2022 09:18

[BWC]Ignore unknown index settings

35d4a9b

Add code to ignore unknown index settings on old indices, to allow rolling-upgrades to work.

Add rolling upgrade test

8cf458d

Fix test bugs.

2e63bd0

grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team auto-backport-and-merge v8.5.1 v8.6.0 labels Sep 22, 2022

grcevski requested review from DaveCTurner and pgomulka September 22, 2022 14:08

Update docs/changelog/90264.yaml

5c14872

Nikola Grcevski added 2 commits September 22, 2022 10:30

Merge branch 'main' into fix/legacy_index_settings

97abce2

Merge branch 'fix/legacy_index_settings' of github.com:grcevski/elast…

82eaed7

…icsearch into fix/legacy_index_settings

DaveCTurner previously requested changes Sep 22, 2022

View reviewed changes

Nikola Grcevski added 2 commits September 22, 2022 15:25

Fix test for BWC > 8.0

b5fe879

WIP: rework to reuse old index settings

ad5669b

Nikola Grcevski added 3 commits September 23, 2022 12:06

Merge branch 'main' into fix/legacy_index_settings

837837a

Refactor for use with validators

1a99860

Remove test code.

9dfb706

DaveCTurner reviewed Sep 24, 2022

View reviewed changes

pgomulka approved these changes Sep 26, 2022

View reviewed changes

grcevski and others added 3 commits September 26, 2022 14:12

Use v7 explicitly in version checks.

fe74090

Co-authored-by: David Turner <[email protected]>

Use v7 explicitly in setting name.

99dc5a7

Co-authored-by: David Turner <[email protected]>

Merge branch 'main' into fix/legacy_index_settings

0d86dcd

Nikola Grcevski added 5 commits September 26, 2022 14:31

Fix compile issues

b03ad83

WIP: Apply feedback

d7d8e87

Finish work on new setting behaviour

ebd159b

Add more tests

1d25759

Fix tests

a6ff3b5

Merge branch 'main' into fix/legacy_index_settings

476de2d

grcevski requested a review from DaveCTurner September 28, 2022 16:57

DaveCTurner reviewed Nov 2, 2022

View reviewed changes

Nikola Grcevski added 2 commits November 2, 2022 09:28

Merge branch 'main' into fix/legacy_index_settings

a7948fc

Simplify compare expressions, add tests.

62a0f18

grcevski merged commit 0652a59 into elastic:main Nov 2, 2022

grcevski deleted the fix/legacy_index_settings branch November 2, 2022 17:58

grcevski mentioned this pull request Nov 2, 2022

[8.5] Allow legacy index settings on legacy indices (#90264) #91257

Merged

arteam mentioned this pull request Mar 13, 2025

[8.x] Remove index.merge.policy.max_merge_at_once_explicit deprecation #124792

Merged

arteam mentioned this pull request Mar 13, 2025

Mark index.merge.policy.max_merge_at_once_explicit not to be accepted in v10 #124807

Merged

Allow legacy index settings on legacy indices #90264

Allow legacy index settings on legacy indices #90264

Uh oh!

Conversation

grcevski commented Sep 22, 2022

Uh oh!

elasticsearchmachine commented Sep 22, 2022

Uh oh!

elasticsearchmachine commented Sep 22, 2022

Uh oh!

DaveCTurner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Sep 22, 2022

Uh oh!

grcevski commented Sep 22, 2022

Uh oh!

DaveCTurner commented Sep 23, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grcevski commented Sep 27, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Nov 2, 2022

💚 Backport successful

Uh oh!

Uh oh!

DaveCTurner left a comment •

edited

Loading