Add deprecation info and warnings for an empty TIER_PREFERENCE #79305

joegallo · 2021-10-16T05:14:42Z

On 7.x, for clusters that have data nodes that don't have all data roles (e.g. a cluster with a dedicated hot tier would mean those nodes had data_hot but not all the other data_... roles), we want to emit deprecation warnings when an index is created that doesn't have a tier preference, and raise any such previously created indices with deprecation info.

Note: I've got the logic mostly nailed down here, or so I think, and there are some reasonable tests as well. Clearly the strings for the warnings and info are placeholders -- I'll take another swing at them on Monday. For everything else, though, feel free to ignore the 'WIP' tag to the right, this is intended to be mostly reviewable.

so that it's available to use for a check that wants to use it

Strings and other details obviously placeholder

They don't like the new warning, but we can shortcut it by adding a tier preference.

elasticmachine · 2021-10-16T05:14:46Z

Pinging @elastic/es-data-management (Team:Data Management)

henningandersen

I left some comments, otherwise this looks good.

henningandersen · 2021-10-16T07:04:28Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

+                // if we're not enforcing the tier preference, then maybe warn
+                if (preferredTiers.isEmpty()) {
+                    if (DataTier.dataNodesWithoutAllDataRoles(currentState).isEmpty() == false) {
+                        DEPRECATION_LOGGER.warn(DeprecationCategory.INDICES /*maybe OTHER? */, "index_without_tier_preference",


I think INDICES is fine here, though we could also consider SETTINGS.

👍, 14f1984

henningandersen · 2021-10-16T07:09:12Z

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java

@@ -1102,6 +1103,16 @@ public void testDeprecateSimpleFS() {
            "as it offers superior or equivalent performance to [simplefs].");
    }

+    public void testDeprecateNoTierPreference() {


I wonder if you should bake this into testEnforceDefaultTierPreference instead. I think that test method will have to deal with deprecation warnings anyway and it sort of tests all the relevant cases (also want to ensure you get no deprecation warnings when there is a resulting tier preference).

Fair point, I think I've accomplished what you're getting at via 2b11293.

henningandersen · 2021-10-16T07:10:06Z

server/src/test/java/org/elasticsearch/cluster/routing/allocation/DataTierTests.java

+        return ClusterState.builder(ClusterState.EMPTY_STATE).nodes(discoBuilder.build()).build();
+    }
+
+    public void testDataNodesWithoutAllDataRoles() {


nit: can we move this test method before the helper method above? That feels like a more natural reading order.

Sure thing, 2a16334

henningandersen · 2021-10-16T07:14:26Z

server/src/test/java/org/elasticsearch/cluster/routing/allocation/DataTierTests.java

+            newNode(0, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.MASTER_ROLE)),
+            newNode(1, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.DATA_ROLE)),
+            newNode(2, org.elasticsearch.core.Map.of(), allDataRoles),
+            newNode(3, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE))


Perhaps add one with nearly all roles, something like:

newNode(4, org.elasticsearch.core.Map.of(), randomValueOtherThan(ALL_DATA_TIERS, () -> randomSubSetOf(between(1, ALL_DATA_TIERS.size()-1), ALL_DATA_TIERS)));

You can then validate that you get the node out but no need to validate the roles (just the id/name).

Agreed, thanks for the suggestion, 0065a23

henningandersen · 2021-10-16T07:18:28Z

.../plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java

@@ -174,4 +175,9 @@ private DeprecationChecks() {
    public interface NodeDeprecationCheck<A, B, C, D, R> {
        R apply(A first, B second, C third, D fourth);
    }
+
+    @FunctionalInterface
+    public interface IndexDeprecationCheck<A, B, R> {


I am not sure I follow why it is beneficial to have this be generic, can we not just make apply take cluster-state and index-metadata as args end return an IndexDeprecationIssue?

Ohhhhh, yup, I see it now. I tried to mirror what was in place for the NodeDeprecationCheck, but indeed that's only the way it is because there's not a java.util.QuadFunction. Replaced with BiFunction in a963286.

henningandersen · 2021-10-16T07:20:22Z

...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java

+            if (tierPreference.isEmpty()) {
+                return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
+                    "some message here",
+                    "https://www.elastic.co/some/url/here.html",


We should ensure this URL points to a good page describing the mandatory tier preference migration. I think just getting some page link to tier preference in for now is fine though, we can then iterate on the docs for this and update the link.

902f998, good suggestion, I've added a link to the data tiers page for now, because there's some discussion of the setting there (incidentally, some discussion that will need to get updated, in light of the changes from this and the other related PRs).

henningandersen · 2021-10-16T07:21:13Z

...precation/src/test/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecksTests.java

@@ -671,4 +697,43 @@ public void testFrozenIndex() {
            )
        ));
    }
+
+    public static ClusterState clusterStateWithoutAllDataRoles() {


nit: private?

✅, 2f5d9cd

henningandersen · 2021-10-16T07:21:26Z

...precation/src/test/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecksTests.java

+        return ClusterState.builder(ClusterState.EMPTY_STATE).nodes(discoBuilder.build()).build();
+    }
+
+    public void testEmptyDataTierPreference() {


nit: move test method before helper method.

Done, 2a16334

henningandersen · 2021-10-16T07:22:13Z

...precation/src/test/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecksTests.java

+    public void testEmptyDataTierPreference() {
+        Settings.Builder settings = settings(Version.CURRENT);
+        settings.put(DataTier.TIER_PREFERENCE_SETTING.getKey(), "  ");
+        IndexMetadata indexMetadata = IndexMetadata.builder("test").settings(settings).numberOfShards(1).numberOfReplicas(0).build();


Can we randomize shard and replica count? Since they are irrelevant here, we might as well signal that by randomizing them.

Absolutely, 0812b95

henningandersen · 2021-10-16T07:22:57Z

...precation/src/test/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecksTests.java

+                    "some message here",
+                    "https://www.elastic.co/some/url/here.html",
+                    "some details here", false, null)


Some of this should contain the index name I think (once you adjust the message).

902f998, we can still iterate on the exact words, but indeed the index name is there.

and rewrite the set matching to use hamcrest more heavily -- it was easy enough to handle a set of one item manually with an iterator, but with two items in the picture it's a bit annoying to do 'by hand'

We don't need a purpose-built interface for these.

If the aggregated settings end up not containing a tier preference, and the cluster has a node without all the data roles, then we should emit a warning. For convenience, the test randomly chooses between an empty cluster state and one that doesn't have all the data roles, so the final condition is written in terms of merely not being the empty state (since the alternative is always one that doesn't have all the data roles, at least in the universe of this test).

We can iterate on these until we get to something that's just right, but this at least gives us a start.

masseyke · 2021-10-18T22:19:14Z

...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java

+            if (tierPreference.isEmpty()) {
+                String indexName = indexMetadata.getIndex().getName();
+                return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
+                    "index [" + indexName + "] does not have a [" + DataTier.TIER_PREFERENCE + "] setting, " +


It would be good to get @debadair or @jrodewig or someone from our docs team to weigh in on the wording here. They've recently had an effort to standardize the wording.

+1, this isn't the final wording -- there's still an implied run through the overall 7.x and 8.0 docs for this that is coming, and I think rewording these messages to their 100% final form fits in with the intent of that (as yet to be written) PR.

masseyke · 2021-10-18T22:25:34Z

...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java

+                return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
+                    "index [" + indexName + "] does not have a [" + DataTier.TIER_PREFERENCE + "] setting, " +
+                        "in 8.0 this setting will be required for all indices and may not be empty or null.",
+                    "https://www.elastic.co/guide/en/elasticsearch/reference/current/data-tiers.html",


Also we're generating shortened URLs for all of the deprecation info API URLs like this. The pattern we've been following is "https://ela.st/es-deprecation-7-", and then we'll map that to a concrete URL. Also, we're trying to make sure that all of the concrete URLs go to something fixed in 7, like "https://www.elastic.co/guide/en/elasticsearch/reference/7.16/data-tiers.html"

+1, this isn't the final URL, it's just a glorified placeholder -- the right docs don't really exist yet. #79305 (comment)

masseyke

Looks good to me, with the caveat that we need to make sure to return to the deprecation info URL and message at some point soon.

joegallo · 2021-10-19T17:13:41Z

@elasticmachine update branch

joegallo added 6 commits October 15, 2021 21:50

Organize imports

3f800c4

Add a utility method data nodes that are missing roles

7cfc8f1

Thread the ClusterState into IndexDeprecationChecks

b3e7ee9

so that it's available to use for a check that wants to use it

New index deprecation check for empty tier preference

cfbd76e

Strings and other details obviously placeholder

New deprecation log for empty tier preference

76090df

Strings and other details obviously placeholder

Add a tier preference for these tests

675fd52

They don't like the new warning, but we can shortcut it by adding a tier preference.

joegallo added WIP :Data Management/ILM+SLM Index and Snapshot lifecycle management v7.16.0 labels Oct 16, 2021

joegallo requested a review from henningandersen October 16, 2021 05:14

elasticmachine added the Team:Data Management Meta label for data/management team label Oct 16, 2021

henningandersen reviewed Oct 16, 2021

View reviewed changes

joegallo added 11 commits October 18, 2021 10:33

Merge branch '7.x' into enforce-default-tier-preference-checks

89c2527

INDICES is fine

14f1984

Put helpers after tests, rather than before

2a16334

This helper can be private

2f5d9cd

Randomize shard and replica counts

0812b95

Add an all-but-one data roles element to this test

0065a23

and rewrite the set matching to use hamcrest more heavily -- it was easy enough to handle a set of one item manually with an iterator, but with two items in the picture it's a bit annoying to do 'by hand'

Oops, unused import

cc28dab

This can just be BiFunction

a963286

We don't need a purpose-built interface for these.

Merge branch '7.x' into enforce-default-tier-preference-checks

156775d

Merge branch '7.x' into enforce-default-tier-preference-checks

c10b720

joegallo requested a review from masseyke October 18, 2021 19:29

joegallo added 2 commits October 18, 2021 17:40

Non-final (but also non-placeholder) messages

ac950dc

We can iterate on these until we get to something that's just right, but this at least gives us a start.

Merge branch '7.x' into enforce-default-tier-preference-checks

902f998

masseyke reviewed Oct 18, 2021

View reviewed changes

joegallo removed the WIP label Oct 18, 2021

joegallo mentioned this pull request Oct 18, 2021

Mandatory data tier preference #76147

Open

7 tasks

joegallo requested review from henningandersen and masseyke October 18, 2021 23:34

masseyke mentioned this pull request Oct 19, 2021

Fixing a unit test #79470

Merged

masseyke approved these changes Oct 19, 2021

View reviewed changes

Merge branch '7.x' into enforce-default-tier-preference-checks

9a25c04

joegallo merged commit 679beda into elastic:7.x Oct 19, 2021

joegallo deleted the enforce-default-tier-preference-checks branch October 19, 2021 18:17

joegallo mentioned this pull request Oct 19, 2021

Add missing _tier_preference on these indices #79509

Merged

danhermann added the >deprecation label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation info and warnings for an empty TIER_PREFERENCE #79305

Add deprecation info and warnings for an empty TIER_PREFERENCE #79305

joegallo commented Oct 16, 2021 •

edited

Loading

elasticmachine commented Oct 16, 2021

henningandersen left a comment

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

henningandersen Oct 16, 2021

joegallo Oct 18, 2021

masseyke Oct 18, 2021

joegallo Oct 18, 2021

masseyke Oct 18, 2021

joegallo Oct 18, 2021

masseyke left a comment

joegallo commented Oct 19, 2021

Add deprecation info and warnings for an empty TIER_PREFERENCE #79305

Add deprecation info and warnings for an empty TIER_PREFERENCE #79305

Conversation

joegallo commented Oct 16, 2021 • edited Loading

elasticmachine commented Oct 16, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masseyke left a comment

Choose a reason for hiding this comment

joegallo commented Oct 19, 2021

joegallo commented Oct 16, 2021 •

edited

Loading