[Deprecation API] Refactor resource deprecation checkers and add new resources. by gmarouli · Pull Request #120505 · elastic/elasticsearch

gmarouli · 2025-01-21T09:06:16Z

While preparing for 9.0, we want to add deprecation warnings when we encounter the usage of filtered allocation implementing tiers.

We encounter this setting in many places:

index settings
component templates
index templates
legacy templates
ILM policies.

The deprecation API already supports checking the index settings. In this PR:

We add another check to the index settings check.
We extend the response of the API to include templates which contains component and composable index templates, and ILM policies. We do not check the legacy templates considering that they are already deprecated.
We applied a refactoring to make the extension of the deprecation API easier. In this refactoring we introduced ResourceDeprecationChecker which is a checker that is dedicated to a resource type and groups the deprecation warnings by the resource name.

Considering that the component and composable templates have settings, we add them as well in the process of removing skipped settings.

Relates to ES-10298

Post-merge

Contact Kibana to ensure the UA gets updated to display things properly ([Upgrade Assistant] Add new deprecation types templates, ilm_policies kibana#208496).
Update the response spec in https://github.com/elastic/elasticsearch-specification/blob/main/specification/migration/deprecations/DeprecationInfoResponse.ts also

…and other deprecation warnings

martijnvg

This looks great 🚀 .
I think only an ResourceDeprecationChecker implementation for composable index templates is missing? And I'm wondering whether we should have something for legacy templates?

dakrone

Generally I think this is a good idea and fits well. I wonder though: should we have a common TemplateDeprecationChecker that works for the template: {…} section of both Composable and Component templates?

I'm not sure whether it's worth trying to inspect legacy templates, especially since we just want people to stop using them anyway.

gmarouli · 2025-01-22T12:13:33Z

Thank you for your feedback @martijnvg & @dakrone .

I also played around with the idea of grouping all the templates together, but then I was afraid for name conflicts. What if we have logs component template and logs composable index template, then the json won't be valid (not the way we have it now at least).

We could merge the two deprecation warnings under one resource logs, one for component template and one for the composable index template.

What I am proposing is:

{
  "index_settings": .....,
  "templates": {
     "deprecated_template_1": [
         {}, <- deprecation warning for component template
         {} <- deprecation warning for index template
     ]
  }
  "ilm_policies": {
     "deprecated_policy_1": [
     ],
    "deprecated_policy_2": [
     ]
  }
}

This could reduce the footprint of the API and it gives us more flexibility. What do you think?

dakrone

I left some minor comments, but I think this looks good. I think it would be good also to reach out to the Kibana team to make sure the Upgrade Assistant gets updated to also check the output additions.

Additionally, we need to update the response spec in https://github.com/elastic/elasticsearch-specification/blob/main/specification/migration/deprecations/DeprecationInfoResponse.ts also I think?

docs/changelog/120505.yaml

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationInfoAction.java

...precation/src/main/java/org/elasticsearch/xpack/deprecation/IlmPolicyDeprecationChecker.java

dakrone · 2025-01-23T23:10:58Z

...precation/src/main/java/org/elasticsearch/xpack/deprecation/IlmPolicyDeprecationChecker.java

+                    || containsDeprecatedFilteredAllocationConfig(allocateAction.getInclude())
+                    || containsDeprecatedFilteredAllocationConfig(allocateAction.getRequire())) {
+                    return new DeprecationIssue(
+                        DeprecationIssue.Level.WARNING,


Maybe out of scope for this, but should this be a critical warning, since failure to change this means that the index could fail to allocate on Cloud?

I was going back and forth on this.

For cloud, I agree this is critical. For self managed, assuming a user is using kibana and UA, I think it's a warning because the user might still be using the data node attribute (which is also a warning).

We could play it safe, and make them all of them critical (including the node attribute), on the combination: data: hot, warm, cold, frozen. It would be more disruptive to users but if the scope is this narrow maybe we can be more assertive. What do you think?

dakrone · 2025-01-23T23:11:28Z

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

+            return null;
+        }
+        return new DeprecationIssue(
+            DeprecationIssue.Level.WARNING,


Same here for critical level? What do you think?

…response format

martijnvg

Thanks @gmarouli for improving the deprecation API!

LGTM

… new resources (#120505) (#121016) * [Deprecation API] Refactor resource deprecation checkers and add new resources. #120505 * Fix test * Fix test --------- Co-authored-by: Lee Hinman <lee@writequit.org>

In #120505 we introduced a capabilities check in the yaml test `deprecation/10_basic/Test Deprecations` but we forgot to add them in the `RestDeprecationInfoAction`. In this PR we add the capabilities which will enable the test and we make the test resilient to the warning that occurs when the `.security-7` index is present.

In elastic#120505 we introduced a capabilities check in the yaml test `deprecation/10_basic/Test Deprecations` but we forgot to add them in the `RestDeprecationInfoAction`. In this PR we add the capabilities which will enable the test and we make the test resilient to the warning that occurs when the `.security-7` index is present.

In #120505 we introduced a capabilities check in the yaml test `deprecation/10_basic/Test Deprecations` but we forgot to add them in the `RestDeprecationInfoAction`. In this PR we add the capabilities which will enable the test and we make the test resilient to the warning that occurs when the `.security-7` index is present.

In elastic#120505 we introduced a capabilities check in the yaml test `deprecation/10_basic/Test Deprecations` but we forgot to add them in the `RestDeprecationInfoAction`. In this PR we add the capabilities which will enable the test and we make the test resilient to the warning that occurs when the `.security-7` index is present.

gmarouli added 3 commits January 21, 2025 09:33

Introduce ResourceDeprecationChecker to group all index, data stream …

d20a918

…and other deprecation warnings

Introduce ResourceDeprecationChecker to group all index, data stream …

da7a12a

…and other deprecation warnings

Move index settings also to ResourceDeprecationChecker

87c9cb3

elasticsearchmachine added the v9.0.0 label Jan 21, 2025

gmarouli added 3 commits January 21, 2025 11:20

Merge with main

83589f4

Fix XContent serialisation

49999c7

Introduce ComponentTemplateDeprecationChecks

41505ec

gmarouli mentioned this pull request Jan 21, 2025

[Synthetic _source] Deprecation warning to be emitted on Index Template & Component Template containing the deprecated settings #119993

Closed

martijnvg reviewed Jan 21, 2025

View reviewed changes

gmarouli and others added 4 commits January 21, 2025 14:58

Introduce ComponentTemplateDeprecationChecks

88271a0

Rename checkers

286d543

Add new checkers

0797a23

[CI] Auto commit changes from spotless

4457d09

dakrone reviewed Jan 21, 2025

View reviewed changes

gmarouli added 12 commits January 22, 2025 15:47

Merge with main

bb75c00

Update tests

6bda1dc

Unify component and index templates, remove legacy template check

5eeec51

Update tests

6df4812

Merge with main

9f0b516

Change the way we ensure the required fields exist in the response

48fee10

Remove skipped settings from templates

f2e2782

Fix test

c0f37b6

Extend the yaml tests to include the new resources in deprecation API

6e079d2

Polishing

349f831

Merge with main

733479b

Fix test

29e3693

gmarouli requested a review from dakrone January 23, 2025 16:38

gmarouli marked this pull request as ready for review January 23, 2025 16:38

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jan 23, 2025

gmarouli and others added 2 commits January 23, 2025 21:35

Update docs/changelog/120505.yaml

1098a27

Merge branch 'main' into handle-9/deprecate-node-attrs-v2

dbb0c1e

dakrone approved these changes Jan 23, 2025

View reviewed changes

gmarouli added 9 commits January 24, 2025 11:20

Merge with main

2c5da01

Process review comments

dc6ed49

Add missing check

cb7b097

Add new ILM template check for freeze action

facf5e1

Merge branch 'main' into handle-9/deprecate-node-attrs-v2

5284a61

Merge branch 'main' into handle-9/deprecate-node-attrs-v2

d5f9d8b

Adjust SourceModeRollingUpgradeIT to take into consideration the new …

d557621

…response format

Merge branch 'main' into handle-9/deprecate-node-attrs-v2

4ea9f78

Fix test

1b8c69d

martijnvg approved these changes Jan 28, 2025

View reviewed changes

Merge branch 'main' into handle-9/deprecate-node-attrs-v2

b898b33

gmarouli mentioned this pull request Jan 28, 2025

[Upgrade Assistant] Add new deprecation types templates, ilm_policies elastic/kibana#208496

Closed

gmarouli merged commit 7322015 into elastic:main Jan 28, 2025
16 checks passed

gmarouli deleted the handle-9/deprecate-node-attrs-v2 branch January 28, 2025 11:00

This was referenced Jan 28, 2025

[8.x][Deprecation API] Refactor resource deprecation checkers and add new resources (#120505) #121016

Merged

[Upgrade Assistant] Add resolutions for new templates and ilm_policies warnings elastic/kibana#208527

Closed

gmarouli mentioned this pull request Jan 29, 2025

Add templates and ilm_policies to the Deprecation API response. elastic/elasticsearch-specification#3648

Merged

gmarouli mentioned this pull request Mar 26, 2025

Fix p0=deprecation/10_basic/Test Deprecations #125687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Deprecation API] Refactor resource deprecation checkers and add new resources.#120505

[Deprecation API] Refactor resource deprecation checkers and add new resources.#120505
gmarouli merged 34 commits intoelastic:mainfrom
gmarouli:handle-9/deprecate-node-attrs-v2

gmarouli commented Jan 21, 2025 •

edited

Loading

martijnvg left a comment

dakrone left a comment

gmarouli commented Jan 22, 2025

dakrone left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dakrone Jan 23, 2025

gmarouli Jan 24, 2025

dakrone Jan 23, 2025

martijnvg left a comment

Uh oh!

Labels

4 participants

Conversation

gmarouli commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

gmarouli commented Jan 22, 2025

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dakrone Jan 23, 2025

Choose a reason for hiding this comment

gmarouli Jan 24, 2025

Choose a reason for hiding this comment

dakrone Jan 23, 2025

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

4 participants

gmarouli commented Jan 21, 2025 •

edited

Loading