From be788160efd23fed3eba5ef1d431cb1cb3e88cf6 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Wed, 30 Jan 2019 20:12:20 +0000 Subject: [PATCH] [ML] Datafeed deprecation checks (#38026) Deprecation checks for the ML datafeed query and aggregations. --- .../deprecation/DeprecationInfoAction.java | 48 +++++++++++++++---- .../core/ml/datafeed/DatafeedConfig.java | 2 +- .../DeprecationInfoActionResponseTests.java | 22 +++++++-- .../xpack/deprecation/DeprecationChecks.java | 7 +++ .../deprecation/MlDeprecationChecks.java | 44 +++++++++++++++++ .../TransportDeprecationInfoAction.java | 42 +++++++++++++--- .../deprecation/MlDeprecationChecksTests.java | 48 +++++++++++++++++++ .../test/deprecation/10_basic.yml | 46 ++++++++++++++++++ 8 files changed, 239 insertions(+), 20 deletions(-) create mode 100644 x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecks.java create mode 100644 x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecksTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoAction.java index 09c6a0d5752..1241b136c7a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoAction.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.core.deprecation; +import org.elasticsearch.Version; import org.elasticsearch.action.Action; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.ActionResponse; @@ -23,9 +24,12 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -67,16 +71,19 @@ public class DeprecationInfoAction extends Action clusterSettingsIssues; private List nodeSettingsIssues; private Map> indexSettingsIssues; + private List mlSettingsIssues; public Response() { } public Response(List clusterSettingsIssues, List nodeSettingsIssues, - Map> indexSettingsIssues) { + Map> indexSettingsIssues, + List mlSettingsIssues) { this.clusterSettingsIssues = clusterSettingsIssues; this.nodeSettingsIssues = nodeSettingsIssues; this.indexSettingsIssues = indexSettingsIssues; + this.mlSettingsIssues = mlSettingsIssues; } public List getClusterSettingsIssues() { @@ -91,12 +98,21 @@ public class DeprecationInfoAction extends Action getMlSettingsIssues() { + return mlSettingsIssues; + } + @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); clusterSettingsIssues = in.readList(DeprecationIssue::new); nodeSettingsIssues = in.readList(DeprecationIssue::new); indexSettingsIssues = in.readMapOfLists(StreamInput::readString, DeprecationIssue::new); + if (in.getVersion().onOrAfter(Version.V_6_7_0)) { + mlSettingsIssues = in.readList(DeprecationIssue::new); + } else { + mlSettingsIssues = Collections.emptyList(); + } } @Override @@ -105,6 +121,9 @@ public class DeprecationInfoAction extends Action v.writeTo(o)); + if (out.getVersion().onOrAfter(Version.V_6_7_0)) { + out.writeList(mlSettingsIssues); + } } @Override @@ -114,10 +133,10 @@ public class DeprecationInfoAction extends Action nodesInfo, List nodesStats, ClusterState state, - IndexNameExpressionResolver indexNameExpressionResolver, - String[] indices, IndicesOptions indicesOptions, - List>clusterSettingsChecks, - List, List, DeprecationIssue>> nodeSettingsChecks, - List> indexSettingsChecks) { + IndexNameExpressionResolver indexNameExpressionResolver, + String[] indices, IndicesOptions indicesOptions, + List datafeeds, + List>clusterSettingsChecks, + List, List, DeprecationIssue>> nodeSettingsChecks, + List> indexSettingsChecks, + List> mlSettingsCheck) { List clusterSettingsIssues = filterChecks(clusterSettingsChecks, (c) -> c.apply(state)); List nodeSettingsIssues = filterChecks(nodeSettingsChecks, (c) -> c.apply(nodesInfo, nodesStats)); + List mlSettingsIssues = new ArrayList<>(); + for (DatafeedConfig config : datafeeds) { + mlSettingsIssues.addAll(filterChecks(mlSettingsCheck, (c) -> c.apply(config))); + } String[] concreteIndexNames = indexNameExpressionResolver.concreteIndexNames(state, indicesOptions, indices); @@ -174,7 +202,7 @@ public class DeprecationInfoAction extends Action implements } } - void setQuery(Map query) { + public void setQuery(Map query) { this.query = ExceptionsHelper.requireNonNull(query, QUERY.getPreferredName()); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoActionResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoActionResponseTests.java index 5267e5dc2ff..b878f1c5d40 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoActionResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/deprecation/DeprecationInfoActionResponseTests.java @@ -22,6 +22,8 @@ import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.test.AbstractStreamableTestCase; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfigTests; import java.io.IOException; import java.util.Arrays; @@ -45,13 +47,15 @@ public class DeprecationInfoActionResponseTests extends AbstractStreamableTestCa .limit(randomIntBetween(0, 10)).collect(Collectors.toList()); List nodeIssues = Stream.generate(DeprecationIssueTests::createTestInstance) .limit(randomIntBetween(0, 10)).collect(Collectors.toList()); + List mlIssues = Stream.generate(DeprecationIssueTests::createTestInstance) + .limit(randomIntBetween(0, 10)).collect(Collectors.toList()); Map> indexIssues = new HashMap<>(); for (int i = 0; i < randomIntBetween(0, 10); i++) { List perIndexIssues = Stream.generate(DeprecationIssueTests::createTestInstance) .limit(randomIntBetween(0, 10)).collect(Collectors.toList()); indexIssues.put(randomAlphaOfLength(10), perIndexIssues); } - return new DeprecationInfoAction.Response(clusterIssues, nodeIssues, indexIssues); + return new DeprecationInfoAction.Response(clusterIssues, nodeIssues, indexIssues, mlIssues); } @Override @@ -80,12 +84,14 @@ public class DeprecationInfoActionResponseTests extends AbstractStreamableTestCa List nodeStats = Collections.singletonList(new NodeStats(discoveryNode, 0L, null, null, null, null, null, null, null, null, null, null, null, null, null)); + List datafeeds = Collections.singletonList(DatafeedConfigTests.createRandomizedDatafeedConfig("foo")); IndexNameExpressionResolver resolver = new IndexNameExpressionResolver(); IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, true); boolean clusterIssueFound = randomBoolean(); boolean nodeIssueFound = randomBoolean(); boolean indexIssueFound = randomBoolean(); + boolean mlIssueFound = randomBoolean(); DeprecationIssue foundIssue = DeprecationIssueTests.createTestInstance(); List> clusterSettingsChecks = Collections.unmodifiableList(Arrays.asList( @@ -100,10 +106,14 @@ public class DeprecationInfoActionResponseTests extends AbstractStreamableTestCa Collections.unmodifiableList(Arrays.asList( (idx) -> indexIssueFound ? foundIssue : null )); + List> mlSettingsChecks = + Collections.unmodifiableList(Arrays.asList( + (idx) -> mlIssueFound ? foundIssue : null + )); DeprecationInfoAction.Response response = DeprecationInfoAction.Response.from(nodeInfos, nodeStats, state, - resolver, Strings.EMPTY_ARRAY, indicesOptions, - clusterSettingsChecks, nodeSettingsChecks, indexSettingsChecks); + resolver, Strings.EMPTY_ARRAY, indicesOptions, datafeeds, + clusterSettingsChecks, nodeSettingsChecks, indexSettingsChecks, mlSettingsChecks); if (clusterIssueFound) { assertThat(response.getClusterSettingsIssues(), equalTo(Collections.singletonList(foundIssue))); @@ -123,5 +133,11 @@ public class DeprecationInfoActionResponseTests extends AbstractStreamableTestCa } else { assertTrue(response.getIndexSettingsIssues().isEmpty()); } + + if (mlIssueFound) { + assertThat(response.getMlSettingsIssues(), equalTo(Collections.singletonList(foundIssue))); + } else { + assertTrue(response.getMlSettingsIssues().isEmpty()); + } } } diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java index 1363d3a09a0..c6c3d5fd840 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java @@ -11,6 +11,7 @@ import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.xpack.core.deprecation.DeprecationInfoAction; import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; import java.util.Arrays; import java.util.Collections; @@ -41,6 +42,12 @@ public class DeprecationChecks { Collections.unmodifiableList(Arrays.asList( IndexDeprecationChecks::oldIndicesCheck)); + static List> ML_SETTINGS_CHECKS = + Collections.unmodifiableList(Arrays.asList( + MlDeprecationChecks::checkDataFeedAggregations, + MlDeprecationChecks::checkDataFeedQuery + )); + /** * helper utility function to reduce repeat of running a specific {@link List} of checks. * diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecks.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecks.java new file mode 100644 index 00000000000..187a8669574 --- /dev/null +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecks.java @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.deprecation; + +import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; + +import java.util.List; + +/** + * Check the {@link DatafeedConfig} query and aggregations for deprecated usages. + */ +final class MlDeprecationChecks { + + private MlDeprecationChecks() { + } + + static DeprecationIssue checkDataFeedQuery(DatafeedConfig datafeedConfig) { + List deprecations = datafeedConfig.getQueryDeprecations(); + if (deprecations.isEmpty()) { + return null; + } else { + return new DeprecationIssue(DeprecationIssue.Level.WARNING, + "Datafeed [" + datafeedConfig.getId() + "] uses deprecated query options", + "https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html#breaking_70_search_changes", + deprecations.toString()); + } + } + + static DeprecationIssue checkDataFeedAggregations(DatafeedConfig datafeedConfig) { + List deprecations = datafeedConfig.getAggDeprecations(); + if (deprecations.isEmpty()) { + return null; + } else { + return new DeprecationIssue(DeprecationIssue.Level.WARNING, + "Datafeed [" + datafeedConfig.getId() + "] uses deprecated aggregation options", + "https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html" + + "#breaking_70_aggregations_changes", deprecations.toString()); + } + } +} diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java index 29bd948ab6c..6ae416248e9 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java @@ -19,6 +19,7 @@ import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.license.LicenseUtils; import org.elasticsearch.license.XPackLicenseState; @@ -26,7 +27,13 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.XPackField; +import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.deprecation.DeprecationInfoAction; +import org.elasticsearch.xpack.core.ml.action.GetDatafeedsAction; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; + +import java.util.Collections; +import java.util.List; public class TransportDeprecationInfoAction extends TransportMasterNodeReadAction { @@ -34,9 +41,10 @@ public class TransportDeprecationInfoAction extends TransportMasterNodeReadActio private final XPackLicenseState licenseState; private final NodeClient client; private final IndexNameExpressionResolver indexNameExpressionResolver; + private final Settings settings; @Inject - public TransportDeprecationInfoAction(TransportService transportService, ClusterService clusterService, + public TransportDeprecationInfoAction(Settings settings, TransportService transportService, ClusterService clusterService, ThreadPool threadPool, ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver, XPackLicenseState licenseState, NodeClient client) { @@ -45,6 +53,7 @@ public class TransportDeprecationInfoAction extends TransportMasterNodeReadActio this.licenseState = licenseState; this.client = client; this.indexNameExpressionResolver = indexNameExpressionResolver; + this.settings = settings; } @Override @@ -83,11 +92,20 @@ public class TransportDeprecationInfoAction extends TransportMasterNodeReadActio if (nodesStatsResponse.hasFailures()) { throw nodesStatsResponse.failures().get(0); } - listener.onResponse(DeprecationInfoAction.Response.from(nodesInfoResponse.getNodes(), - nodesStatsResponse.getNodes(), state, indexNameExpressionResolver, - request.indices(), request.indicesOptions(), - DeprecationChecks.CLUSTER_SETTINGS_CHECKS, DeprecationChecks.NODE_SETTINGS_CHECKS, - DeprecationChecks.INDEX_SETTINGS_CHECKS)); + + getDatafeedConfigs(ActionListener.wrap( + datafeeds -> { + listener.onResponse( + DeprecationInfoAction.Response.from(nodesInfoResponse.getNodes(), + nodesStatsResponse.getNodes(), state, indexNameExpressionResolver, + request.indices(), request.indicesOptions(), datafeeds, + DeprecationChecks.CLUSTER_SETTINGS_CHECKS, + DeprecationChecks.NODE_SETTINGS_CHECKS, + DeprecationChecks.INDEX_SETTINGS_CHECKS, + DeprecationChecks.ML_SETTINGS_CHECKS)); + }, + listener::onFailure + )); }, listener::onFailure), client.admin().cluster()::nodesStats); }, listener::onFailure), client.admin().cluster()::nodesInfo); @@ -95,4 +113,16 @@ public class TransportDeprecationInfoAction extends TransportMasterNodeReadActio listener.onFailure(LicenseUtils.newComplianceException(XPackField.DEPRECATION)); } } + + private void getDatafeedConfigs(ActionListener> listener) { + if (XPackSettings.MACHINE_LEARNING_ENABLED.get(settings) == false) { + listener.onResponse(Collections.emptyList()); + } else { + ClientHelper.executeAsyncWithOrigin(client, ClientHelper.DEPRECATION_ORIGIN, GetDatafeedsAction.INSTANCE, + new GetDatafeedsAction.Request(GetDatafeedsAction.ALL), ActionListener.wrap( + datafeedsResponse -> listener.onResponse(datafeedsResponse.getResponse().results()), + listener::onFailure + )); + } + } } diff --git a/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecksTests.java b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecksTests.java new file mode 100644 index 00000000000..6d93ed18731 --- /dev/null +++ b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/MlDeprecationChecksTests.java @@ -0,0 +1,48 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.deprecation; + +import org.elasticsearch.index.query.TermQueryBuilder; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; + +import java.util.Collections; + +public class MlDeprecationChecksTests extends ESTestCase { + + @Override + protected boolean enableWarningsCheck() { + return false; + } + + public void testCheckDataFeedQuery() { + DatafeedConfig.Builder goodDatafeed = new DatafeedConfig.Builder("good-df", "job-id"); + goodDatafeed.setIndices(Collections.singletonList("some-index")); + goodDatafeed.setParsedQuery(new TermQueryBuilder("foo", "bar")); + assertNull(MlDeprecationChecks.checkDataFeedQuery(goodDatafeed.build())); + + DatafeedConfig.Builder deprecatedDatafeed = new DatafeedConfig.Builder("df-with-deprecated-query", "job-id"); + deprecatedDatafeed.setIndices(Collections.singletonList("some-index")); + // TODO: once some query syntax has been removed from 8.0 and deprecated in 7.x reinstate this test + // to check that particular query syntax causes a deprecation warning + /* + Map qs = new HashMap<>(); + qs.put("query", "foo"); + qs.put("use_dis_max", true); + Map query = Collections.singletonMap("query_string", qs); + deprecatedDatafeed.setQuery(query); + + DeprecationIssue issue = MlDeprecationChecks.checkDataFeedQuery(deprecatedDatafeed.build()); + assertNotNull(issue); + assertThat(issue.getDetails(), equalTo("[Deprecated field [use_dis_max] used, replaced by [Set [tie_breaker] to 1 instead]]")); + assertThat(issue.getLevel(), equalTo(DeprecationIssue.Level.WARNING)); + assertThat(issue.getMessage(), equalTo("Datafeed [df-with-deprecated-query] uses deprecated query options")); + assertThat(issue.getUrl(), equalTo("https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html" + + "#breaking_80_search_changes")); + */ + } +} diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml index dad0c3b08eb..1cbb310bb4a 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml @@ -12,4 +12,50 @@ setup: - length: { cluster_settings: 0 } - length: { node_settings: 0 } - length: { index_settings: 0 } + - length: { ml_settings: 0 } +--- +"Test ml": + - skip: + version: "7.0.0 - " + features: ["headers", "warnings"] + reason: this test needs adjusting to contain syntax deprecated in 7.x and removed in 8.0 + +# Index the config directly to prevent the deprecated +# use_dis_max field being rewritten by the parser. This +# simulates the config being created in an older version +# of elasticsearch + - do: + headers: + Content-Type: application/json + index: + index: .ml-config + type: doc + id: deprecation-datafeed-datafeed + body: > + { + "datafeed_id" : "deprecation-datafeed", + "config_type" : "datafeed", + "job_id" : "deprecation-job", + "indices" : ["index-foo"], + "query" : { + "query_string" : { + "query" : "foo", + "use_dis_max" : true + } + } + } + + - do: + indices.refresh: + index: [.ml-config] + +# TODO: change the query and expected warnings to one that makes sense for 7.x + - do: + warnings: + - Deprecated field [use_dis_max] used, replaced by [Set [tie_breaker] to 1 instead] + xpack.migration.deprecations: + index: "*" + - length: { ml_settings: 1 } + - match: { ml_settings.0.level : warning } + - match: { ml_settings.0.message : "Datafeed [deprecation-datafeed] uses deprecated query options" }