From eb85852a188492bfc8da51ea34bfa1aaa28b4868 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 13 Jun 2018 10:53:18 +0100 Subject: [PATCH 01/11] index_prefixes back-compat should test 6.3 (#30951) From 5c77ebe89d2c0246c0c505f8e9466c69490eebe5 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Wed, 13 Jun 2018 11:20:38 +0100 Subject: [PATCH 02/11] [ML] Implement new rules design (#31110) Rules allow users to supply a detector with domain knowledge that can improve the quality of the results. The model detects statistically anomalous results but it has no knowledge of the meaning of the values being modelled. For example, a detector that performs a population analysis over IP addresses could benefit from a list of IP addresses that the user knows to be safe. Then anomalous results for those IP addresses will not be created and will not affect the quantiles either. Another example would be a detector looking for anomalies in the median value of CPU utilization. A user might want to inform the detector that any results where the actual value is less than 5 is not interesting. This commit introduces a `custom_rules` field to the `Detector`. A detector may have multiple rules which are combined with `or`. A rule has 3 fields: `actions`, `scope` and `conditions`. Actions is a list of what should happen when the rule applies. The current options include `skip_result` and `skip_model_update`. The default value for `actions` is the `skip_result` action. Scope is optional and allows for applying filters on any of the partition/over/by field. When not defined the rule applies to all series. The `filter_id` needs to be specified to match the id of the filter to be used. Optionally, the `filter_type` can be specified as either `include` (default) or `exclude`. When set to `include` the rule applies to entities that are in the filter. When set to `exclude` the rule only applies to entities not in the filter. There may be zero or more conditions. A condition requires `applies_to`, `operator` and `value` to be specified. The `applies_to` value can be either `actual`, `typical` or `diff_from_typical` and it specifies the numerical value to which the condition applies. The `operator` (`lt`, `lte`, `gt`, `gte`) and `value` complete the definition. Conditions are combined with `and` and allow to specify numerical conditions for when a rule applies. A rule must either have a scope or one or more conditions. Finally, a rule with scope and conditions applies when all of them apply. --- .../core/ml/calendars/ScheduledEvent.java | 4 +- .../xpack/core/ml/job/config/Condition.java | 132 ---------- .../xpack/core/ml/job/config/Connective.java | 42 ---- .../core/ml/job/config/DetectionRule.java | 151 +++-------- .../xpack/core/ml/job/config/Detector.java | 151 ++++------- .../xpack/core/ml/job/config/FilterRef.java | 123 +++++++++ .../xpack/core/ml/job/config/JobUpdate.java | 4 +- .../xpack/core/ml/job/config/Operator.java | 29 +-- .../xpack/core/ml/job/config/RuleAction.java | 4 +- .../core/ml/job/config/RuleCondition.java | 237 +++++------------- .../core/ml/job/config/RuleConditionType.java | 69 ----- .../xpack/core/ml/job/config/RuleScope.java | 143 +++++++++++ .../xpack/core/ml/job/messages/Messages.java | 33 +-- .../ml/calendars/ScheduledEventTests.java | 19 +- .../ml/job/config/AnalysisConfigTests.java | 8 +- .../ml/job/config/DetectionRuleTests.java | 157 +++++------- .../core/ml/job/config/DetectorTests.java | 205 ++++++++------- .../core/ml/job/config/FilterRefTests.java | 30 +++ .../core/ml/job/config/JobUpdateTests.java | 12 +- .../ml/job/config/RuleConditionTests.java | 234 +++-------------- .../core/ml/job/config/RuleScopeTests.java | 81 ++++++ .../ml/action/TransportOpenJobAction.java | 17 +- .../xpack/ml/job/JobManager.java | 1 + .../action/TransportOpenJobActionTests.java | 80 ++++++ .../xpack/ml/integration/JobProviderIT.java | 28 +-- .../xpack/ml/job/JobManagerTests.java | 8 +- .../xpack/ml/job/config/ConditionTests.java | 114 --------- .../xpack/ml/job/config/ConnectiveTests.java | 79 ------ .../xpack/ml/job/config/OperatorTests.java | 76 +----- .../xpack/ml/job/config/RuleActionTests.java | 14 +- .../ml/job/config/RuleConditionTypeTests.java | 140 ----------- .../AutodetectCommunicatorTests.java | 7 +- .../ControlMsgToProcessWriterTests.java | 43 ++-- .../writer/FieldConfigWriterTests.java | 21 +- .../writer/ScheduledEventsWriterTests.java | 12 +- .../rest-api-spec/test/ml/filter_crud.yml | 12 +- .../rest-api-spec/test/ml/jobs_crud.yml | 131 ++++------ .../ml/integration/DetectionRulesIT.java | 149 ++++++----- .../ml/integration/ScheduledEventsIT.java | 4 - .../test/mixed_cluster/30_ml_jobs_crud.yml | 4 +- .../test/old_cluster/30_ml_jobs_crud.yml | 32 +-- .../test/upgraded_cluster/30_ml_jobs_crud.yml | 23 +- 42 files changed, 1062 insertions(+), 1801 deletions(-) delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java index 68e1201816dc4..79e569987fa02 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java @@ -16,7 +16,6 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlMetaIndex; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; @@ -148,8 +147,7 @@ public DetectionRule toDetectionRule(TimeValue bucketSpan) { conditions.add(RuleCondition.createTime(Operator.LT, bucketEndTime)); DetectionRule.Builder builder = new DetectionRule.Builder(conditions); - builder.setActions(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING); - builder.setConditionsConnective(Connective.AND); + builder.setActions(RuleAction.SKIP_RESULT, RuleAction.SKIP_MODEL_UPDATE); return builder.build(); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java deleted file mode 100644 index 7d3074df0ae28..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java +++ /dev/null @@ -1,132 +0,0 @@ -/* - * 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.core.ml.job.config; - -import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.ConstructingObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; -import org.elasticsearch.common.xcontent.ToXContentObject; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; -import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; - -import java.io.IOException; -import java.util.Objects; -import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; - -/** - * A class that describes a condition. - * The {@linkplain Operator} enum defines the available - * comparisons a condition can use. - */ -public class Condition implements ToXContentObject, Writeable { - public static final ParseField CONDITION_FIELD = new ParseField("condition"); - public static final ParseField VALUE_FIELD = new ParseField("value"); - - public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - CONDITION_FIELD.getPreferredName(), a -> new Condition((Operator) a[0], (String) a[1])); - - static { - PARSER.declareField(ConstructingObjectParser.constructorArg(), p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return Operator.fromString(p.text()); - } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, Operator.OPERATOR_FIELD, ValueType.STRING); - PARSER.declareField(ConstructingObjectParser.constructorArg(), p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return p.text(); - } - if (p.currentToken() == XContentParser.Token.VALUE_NULL) { - return null; - } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, VALUE_FIELD, ValueType.STRING_OR_NULL); - } - - private final Operator op; - private final String value; - - public Condition(StreamInput in) throws IOException { - op = Operator.readFromStream(in); - value = in.readOptionalString(); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - op.writeTo(out); - out.writeOptionalString(value); - } - - public Condition(Operator op, String value) { - if (value == null) { - throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NULL)); - } - - if (op.expectsANumericArgument()) { - try { - Double.parseDouble(value); - } catch (NumberFormatException nfe) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, value); - throw ExceptionsHelper.badRequestException(msg); - } - } else { - try { - Pattern.compile(value); - } catch (PatternSyntaxException e) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX, value); - throw ExceptionsHelper.badRequestException(msg); - } - } - this.op = op; - this.value = value; - } - - public Operator getOperator() { - return op; - } - - public String getValue() { - return value; - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.field(Operator.OPERATOR_FIELD.getPreferredName(), op); - builder.field(VALUE_FIELD.getPreferredName(), value); - builder.endObject(); - return builder; - } - - @Override - public int hashCode() { - return Objects.hash(op, value); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null) { - return false; - } - - if (getClass() != obj.getClass()) { - return false; - } - - Condition other = (Condition) obj; - return Objects.equals(this.op, other.op) && - Objects.equals(this.value, other.value); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java deleted file mode 100644 index 0b4ad010fdd32..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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.core.ml.job.config; - -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; - -import java.io.IOException; -import java.util.Locale; - -public enum Connective implements Writeable { - OR, AND; - - /** - * Case-insensitive from string method. - * - * @param value - * String representation - * @return The connective type - */ - public static Connective fromString(String value) { - return Connective.valueOf(value.toUpperCase(Locale.ROOT)); - } - - public static Connective readFromStream(StreamInput in) throws IOException { - return in.readEnum(Connective.class); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeEnum(this); - } - - @Override - public String toString() { - return name().toLowerCase(Locale.ROOT); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java index 0948e978c886e..fbdb2f6662a77 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java @@ -6,22 +6,18 @@ package org.elasticsearch.xpack.core.ml.job.config; import org.elasticsearch.Version; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlParserType; import org.elasticsearch.xpack.core.ml.job.messages.Messages; import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.EnumMap; @@ -30,16 +26,15 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.stream.Collectors; public class DetectionRule implements ToXContentObject, Writeable { + public static final Version VERSION_INTRODUCED = Version.V_6_4_0; + public static final ParseField DETECTION_RULE_FIELD = new ParseField("detection_rule"); - public static final ParseField ACTIONS_FIELD = new ParseField("actions", "rule_action"); - public static final ParseField TARGET_FIELD_NAME_FIELD = new ParseField("target_field_name"); - public static final ParseField TARGET_FIELD_VALUE_FIELD = new ParseField("target_field_value"); - public static final ParseField CONDITIONS_CONNECTIVE_FIELD = new ParseField("conditions_connective"); - public static final ParseField CONDITIONS_FIELD = new ParseField("conditions", "rule_conditions"); + public static final ParseField ACTIONS_FIELD = new ParseField("actions"); + public static final ParseField SCOPE_FIELD = new ParseField("scope"); + public static final ParseField CONDITIONS_FIELD = new ParseField("conditions"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly public static final ObjectParser METADATA_PARSER = @@ -55,87 +50,44 @@ public class DetectionRule implements ToXContentObject, Writeable { ObjectParser parser = PARSERS.get(parserType); assert parser != null; parser.declareStringArray(Builder::setActions, ACTIONS_FIELD); - parser.declareString(Builder::setTargetFieldName, TARGET_FIELD_NAME_FIELD); - parser.declareString(Builder::setTargetFieldValue, TARGET_FIELD_VALUE_FIELD); - parser.declareField(Builder::setConditionsConnective, p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return Connective.fromString(p.text()); - } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, CONDITIONS_CONNECTIVE_FIELD, ValueType.STRING); + parser.declareObject(Builder::setScope, RuleScope.parser(parserType), SCOPE_FIELD); parser.declareObjectArray(Builder::setConditions, (p, c) -> RuleCondition.PARSERS.get(parserType).apply(p, c), CONDITIONS_FIELD); } } private final EnumSet actions; - private final String targetFieldName; - private final String targetFieldValue; - private final Connective conditionsConnective; + private final RuleScope scope; private final List conditions; - private DetectionRule(EnumSet actions, @Nullable String targetFieldName, @Nullable String targetFieldValue, - Connective conditionsConnective, List conditions) { + private DetectionRule(EnumSet actions, RuleScope scope, List conditions) { this.actions = Objects.requireNonNull(actions); - this.targetFieldName = targetFieldName; - this.targetFieldValue = targetFieldValue; - this.conditionsConnective = Objects.requireNonNull(conditionsConnective); + this.scope = Objects.requireNonNull(scope); this.conditions = Collections.unmodifiableList(conditions); } public DetectionRule(StreamInput in) throws IOException { - actions = EnumSet.noneOf(RuleAction.class); - if (in.getVersion().before(Version.V_6_2_0)) { - actions.add(RuleAction.readFromStream(in)); - } else { - int actionsCount = in.readVInt(); - for (int i = 0; i < actionsCount; ++i) { - actions.add(RuleAction.readFromStream(in)); - } - } - - conditionsConnective = Connective.readFromStream(in); - int size = in.readVInt(); - conditions = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - conditions.add(new RuleCondition(in)); - } - targetFieldName = in.readOptionalString(); - targetFieldValue = in.readOptionalString(); + actions = in.readEnumSet(RuleAction.class); + scope = new RuleScope(in); + conditions = in.readList(RuleCondition::new); } @Override public void writeTo(StreamOutput out) throws IOException { - if (out.getVersion().before(Version.V_6_2_0)) { - // Only filter_results is supported prior to 6.2.0 - RuleAction.FILTER_RESULTS.writeTo(out); - } else { - out.writeVInt(actions.size()); - for (RuleAction action : actions) { - action.writeTo(out); - } - } - - conditionsConnective.writeTo(out); - out.writeVInt(conditions.size()); - for (RuleCondition condition : conditions) { - condition.writeTo(out); - } - out.writeOptionalString(targetFieldName); - out.writeOptionalString(targetFieldValue); + out.writeEnumSet(actions); + scope.writeTo(out); + out.writeList(conditions); } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(ACTIONS_FIELD.getPreferredName(), actions); - builder.field(CONDITIONS_CONNECTIVE_FIELD.getPreferredName(), conditionsConnective); - builder.field(CONDITIONS_FIELD.getPreferredName(), conditions); - if (targetFieldName != null) { - builder.field(TARGET_FIELD_NAME_FIELD.getPreferredName(), targetFieldName); + if (scope.isEmpty() == false) { + builder.field(SCOPE_FIELD.getPreferredName(), scope); } - if (targetFieldValue != null) { - builder.field(TARGET_FIELD_VALUE_FIELD.getPreferredName(), targetFieldValue); + if (conditions.isEmpty() == false) { + builder.field(CONDITIONS_FIELD.getPreferredName(), conditions); } builder.endObject(); return builder; @@ -145,18 +97,8 @@ public EnumSet getActions() { return actions; } - @Nullable - public String getTargetFieldName() { - return targetFieldName; - } - - @Nullable - public String getTargetFieldValue() { - return targetFieldValue; - } - - public Connective getConditionsConnective() { - return conditionsConnective; + public RuleScope getScope() { + return scope; } public List getConditions() { @@ -164,7 +106,7 @@ public List getConditions() { } public Set extractReferencedFilters() { - return conditions.stream().map(RuleCondition::getFilterId).filter(Objects::nonNull).collect(Collectors.toSet()); + return scope.getReferencedFilters(); } @Override @@ -179,29 +121,29 @@ public boolean equals(Object obj) { DetectionRule other = (DetectionRule) obj; return Objects.equals(actions, other.actions) - && Objects.equals(targetFieldName, other.targetFieldName) - && Objects.equals(targetFieldValue, other.targetFieldValue) - && Objects.equals(conditionsConnective, other.conditionsConnective) + && Objects.equals(scope, other.scope) && Objects.equals(conditions, other.conditions); } @Override public int hashCode() { - return Objects.hash(actions, targetFieldName, targetFieldValue, conditionsConnective, conditions); + return Objects.hash(actions, scope, conditions); } public static class Builder { - private EnumSet actions = EnumSet.of(RuleAction.FILTER_RESULTS); - private String targetFieldName; - private String targetFieldValue; - private Connective conditionsConnective = Connective.OR; + private EnumSet actions = EnumSet.of(RuleAction.SKIP_RESULT); + private RuleScope scope = new RuleScope(); private List conditions = Collections.emptyList(); + public Builder(RuleScope.Builder scope) { + this.scope = scope.build(); + } + public Builder(List conditions) { this.conditions = ExceptionsHelper.requireNonNull(conditions, CONDITIONS_FIELD.getPreferredName()); } - private Builder() { + Builder() { } public Builder setActions(List actions) { @@ -221,18 +163,8 @@ public Builder setActions(RuleAction... actions) { return this; } - public Builder setTargetFieldName(String targetFieldName) { - this.targetFieldName = targetFieldName; - return this; - } - - public Builder setTargetFieldValue(String targetFieldValue) { - this.targetFieldValue = targetFieldValue; - return this; - } - - public Builder setConditionsConnective(Connective connective) { - this.conditionsConnective = ExceptionsHelper.requireNonNull(connective, CONDITIONS_CONNECTIVE_FIELD.getPreferredName()); + public Builder setScope(RuleScope scope) { + this.scope = Objects.requireNonNull(scope); return this; } @@ -242,22 +174,11 @@ public Builder setConditions(List conditions) { } public DetectionRule build() { - if (targetFieldValue != null && targetFieldName == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_MISSING_TARGET_FIELD_NAME, targetFieldValue); + if (scope.isEmpty() && conditions.isEmpty()) { + String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_REQUIRES_SCOPE_OR_CONDITION); throw ExceptionsHelper.badRequestException(msg); } - if (conditions == null || conditions.isEmpty()) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_REQUIRES_AT_LEAST_ONE_CONDITION); - throw ExceptionsHelper.badRequestException(msg); - } - for (RuleCondition condition : conditions) { - if (condition.getType().isCategorical() && targetFieldName != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION, - DetectionRule.TARGET_FIELD_NAME_FIELD.getPreferredName()); - throw ExceptionsHelper.badRequestException(msg); - } - } - return new DetectionRule(actions, targetFieldName, targetFieldValue, conditionsConnective, conditions); + return new DetectionRule(actions, scope, conditions); } } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java index e5cf4b16f6e73..bae5e654ba4fa 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.TreeSet; import java.util.stream.Collectors; @@ -84,8 +85,7 @@ public String toString() { public static final ParseField PARTITION_FIELD_NAME_FIELD = new ParseField("partition_field_name"); public static final ParseField USE_NULL_FIELD = new ParseField("use_null"); public static final ParseField EXCLUDE_FREQUENT_FIELD = new ParseField("exclude_frequent"); - // TODO: Remove the deprecated detector_rules setting in 7.0 - public static final ParseField RULES_FIELD = new ParseField("rules", "detector_rules"); + public static final ParseField CUSTOM_RULES_FIELD = new ParseField("custom_rules"); public static final ParseField DETECTOR_INDEX = new ParseField("detector_index"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly @@ -113,7 +113,7 @@ public String toString() { throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, EXCLUDE_FREQUENT_FIELD, ObjectParser.ValueType.STRING); parser.declareObjectArray(Builder::setRules, (p, c) -> - DetectionRule.PARSERS.get(parserType).apply(p, c).build(), RULES_FIELD); + DetectionRule.PARSERS.get(parserType).apply(p, c).build(), CUSTOM_RULES_FIELD); parser.declareInt(Builder::setDetectorIndex, DETECTOR_INDEX); } } @@ -209,6 +209,19 @@ public String toString() { DetectorFunction.TIME_OF_WEEK ); + /** + * Functions that do not support rule conditions: + *
    + *
  • lat_long - because it is a multivariate feature + *
  • metric - because having the same conditions on min,max,mean is + * error-prone + *
  • rare - because the actual/typical value is not something a user can anticipate + *
  • freq_rare - because the actual/typical value is not something a user can anticipate + *
+ */ + static final EnumSet FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT = EnumSet.of( + DetectorFunction.LAT_LONG, DetectorFunction.METRIC, DetectorFunction.RARE, DetectorFunction.FREQ_RARE); + /** * field names cannot contain any of these characters * ", \ @@ -263,7 +276,11 @@ public void writeTo(StreamOutput out) throws IOException { } else { out.writeBoolean(false); } - out.writeList(rules); + if (out.getVersion().onOrAfter(DetectionRule.VERSION_INTRODUCED)) { + out.writeList(rules); + } else { + out.writeList(Collections.emptyList()); + } if (out.getVersion().onOrAfter(Version.V_5_5_0)) { out.writeInt(detectorIndex); } @@ -293,7 +310,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(EXCLUDE_FREQUENT_FIELD.getPreferredName(), excludeFrequent); } if (rules.isEmpty() == false) { - builder.field(RULES_FIELD.getPreferredName(), rules); + builder.field(CUSTOM_RULES_FIELD.getPreferredName(), rules); } // negative means "unknown", which should only happen for a 5.4 job if (detectorIndex >= 0 @@ -467,17 +484,6 @@ public int hashCode() { public static class Builder { - /** - * Functions that do not support rules: - *
    - *
  • lat_long - because it is a multivariate feature - *
  • metric - because having the same conditions on min,max,mean is - * error-prone - *
- */ - static final EnumSet FUNCTIONS_WITHOUT_RULE_SUPPORT = EnumSet.of( - DetectorFunction.LAT_LONG, DetectorFunction.METRIC); - private String detectorDescription; private DetectorFunction function; private String fieldName; @@ -598,14 +604,8 @@ public Detector build() { } DetectorFunction function = this.function == null ? DetectorFunction.METRIC : this.function; - if (rules.isEmpty() == false) { - if (FUNCTIONS_WITHOUT_RULE_SUPPORT.contains(function)) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION, function); - throw ExceptionsHelper.badRequestException(msg); - } - for (DetectionRule rule : rules) { - checkScoping(rule); - } + for (DetectionRule rule : rules) { + validateRule(rule, function); } // partition, by and over field names cannot be duplicates @@ -691,96 +691,37 @@ private static boolean containsInvalidChar(String field) { return field.chars().anyMatch(Character::isISOControl); } - private void checkScoping(DetectionRule rule) throws ElasticsearchParseException { - String targetFieldName = rule.getTargetFieldName(); - checkTargetFieldNameIsValid(extractAnalysisFields(), targetFieldName); - for (RuleCondition condition : rule.getConditions()) { - List validOptions = Collections.emptyList(); - switch (condition.getType()) { - case CATEGORICAL: - case CATEGORICAL_COMPLEMENT: - validOptions = extractAnalysisFields(); - break; - case NUMERICAL_ACTUAL: - case NUMERICAL_TYPICAL: - case NUMERICAL_DIFF_ABS: - validOptions = getValidFieldNameOptionsForNumeric(rule); - break; - case TIME: - default: - break; - } - if (!validOptions.contains(condition.getFieldName())) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME, validOptions, - condition.getFieldName()); - throw ExceptionsHelper.badRequestException(msg); - } - } + private void validateRule(DetectionRule rule, DetectorFunction function) { + checkFunctionHasRuleSupport(rule, function); + checkScoping(rule); } - private void checkTargetFieldNameIsValid(List analysisFields, String targetFieldName) - throws ElasticsearchParseException { - if (targetFieldName != null && !analysisFields.contains(targetFieldName)) { - String msg = - Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME, analysisFields, targetFieldName); + private void checkFunctionHasRuleSupport(DetectionRule rule, DetectorFunction function) { + if (ruleHasConditionOnResultValue(rule) && FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function)) { + String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION, function); throw ExceptionsHelper.badRequestException(msg); } } - private List getValidFieldNameOptionsForNumeric(DetectionRule rule) { - List result = new ArrayList<>(); - if (overFieldName != null) { - result.add(byFieldName == null ? overFieldName : byFieldName); - } else if (byFieldName != null) { - result.add(byFieldName); - } - - if (rule.getTargetFieldName() != null) { - ScopingLevel targetLevel = ScopingLevel.from(this, rule.getTargetFieldName()); - result = result.stream().filter(field -> targetLevel.isHigherThan(ScopingLevel.from(this, field))) - .collect(Collectors.toList()); - } - - if (isEmptyFieldNameAllowed(rule)) { - result.add(null); - } - return result; - } - - private boolean isEmptyFieldNameAllowed(DetectionRule rule) { - List analysisFields = extractAnalysisFields(); - return analysisFields.isEmpty() || (rule.getTargetFieldName() != null && analysisFields.size() == 1); - } - - enum ScopingLevel { - PARTITION(3), - OVER(2), - BY(1); - - int level; - - ScopingLevel(int level) { - this.level = level; - } - - boolean isHigherThan(ScopingLevel other) { - return level > other.level; - } - - static ScopingLevel from(Detector.Builder detector, String fieldName) { - if (fieldName.equals(detector.partitionFieldName)) { - return ScopingLevel.PARTITION; - } - if (fieldName.equals(detector.overFieldName)) { - return ScopingLevel.OVER; - } - if (fieldName.equals(detector.byFieldName)) { - return ScopingLevel.BY; + private static boolean ruleHasConditionOnResultValue(DetectionRule rule) { + for (RuleCondition condition : rule.getConditions()) { + switch (condition.getAppliesTo()) { + case ACTUAL: + case TYPICAL: + case DIFF_FROM_TYPICAL: + return true; + case TIME: + return false; + default: + throw new IllegalStateException("Unknown applies_to value [" + condition.getAppliesTo() + "]"); } - throw ExceptionsHelper.badRequestException( - "fieldName '" + fieldName + "' does not match an analysis field"); } + return false; } + private void checkScoping(DetectionRule rule) { + Set analysisFields = new TreeSet<>(extractAnalysisFields()); + rule.getScope().validate(analysisFields); + } } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java new file mode 100644 index 0000000000000..7f3fb56287965 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java @@ -0,0 +1,123 @@ +/* + * 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.core.ml.job.config; + +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.xpack.core.ml.MlParserType; + +import java.io.IOException; +import java.util.EnumMap; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; + +public class FilterRef implements ToXContentObject, Writeable { + + public static final ParseField FILTER_REF_FIELD = new ParseField("filter_ref"); + public static final ParseField FILTER_ID = new ParseField("filter_id"); + public static final ParseField FILTER_TYPE = new ParseField("filter_type"); + + public enum FilterType { + INCLUDE, EXCLUDE; + + public static FilterType fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); + } + + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); + } + } + + // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly + public static final ConstructingObjectParser METADATA_PARSER = + new ConstructingObjectParser<>(FILTER_REF_FIELD.getPreferredName(), true, + a -> new FilterRef((String) a[0], (FilterType) a[1])); + public static final ConstructingObjectParser CONFIG_PARSER = + new ConstructingObjectParser<>(FILTER_REF_FIELD.getPreferredName(), false, + a -> new FilterRef((String) a[0], (FilterType) a[1])); + public static final Map> PARSERS = new EnumMap<>(MlParserType.class); + + static { + PARSERS.put(MlParserType.METADATA, METADATA_PARSER); + PARSERS.put(MlParserType.CONFIG, CONFIG_PARSER); + for (MlParserType parserType : MlParserType.values()) { + ConstructingObjectParser parser = PARSERS.get(parserType); + assert parser != null; + parser.declareString(ConstructingObjectParser.constructorArg(), FILTER_ID); + parser.declareField(ConstructingObjectParser.optionalConstructorArg(), p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return FilterType.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, FILTER_TYPE, ObjectParser.ValueType.STRING); + } + } + + private final String filterId; + private final FilterType filterType; + + public FilterRef(String filterId, FilterType filterType) { + this.filterId = Objects.requireNonNull(filterId); + this.filterType = filterType == null ? FilterType.INCLUDE : filterType; + } + + public FilterRef(StreamInput in) throws IOException { + filterId = in.readString(); + filterType = in.readEnum(FilterType.class); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(filterId); + out.writeEnum(filterType); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + builder.field(FILTER_ID.getPreferredName(), filterId); + builder.field(FILTER_TYPE.getPreferredName(), filterType); + builder.endObject(); + return builder; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj instanceof FilterRef == false) { + return false; + } + + FilterRef other = (FilterRef) obj; + return Objects.equals(filterId, other.filterId) && Objects.equals(filterType, other.filterType); + } + + @Override + public int hashCode() { + return Objects.hash(filterId, filterType); + } + + public String getFilterId() { + return filterId; + } + + public FilterType getFilterType() { + return filterType; + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java index 2c7ee538485b9..16243ed16edd4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java @@ -496,7 +496,7 @@ public static class DetectorUpdate implements Writeable, ToXContentObject { PARSER.declareInt(ConstructingObjectParser.optionalConstructorArg(), Detector.DETECTOR_INDEX); PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), Job.DESCRIPTION); PARSER.declareObjectArray(ConstructingObjectParser.optionalConstructorArg(), (parser, parseFieldMatcher) -> - DetectionRule.CONFIG_PARSER.apply(parser, parseFieldMatcher).build(), Detector.RULES_FIELD); + DetectionRule.CONFIG_PARSER.apply(parser, parseFieldMatcher).build(), Detector.CUSTOM_RULES_FIELD); } private int detectorIndex; @@ -550,7 +550,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(Job.DESCRIPTION.getPreferredName(), description); } if (rules != null) { - builder.field(Detector.RULES_FIELD.getPreferredName(), rules); + builder.field(Detector.CUSTOM_RULES_FIELD.getPreferredName(), rules); } builder.endObject(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java index 5813a10c93bb3..bfe9b0e3589ba 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java @@ -19,12 +19,6 @@ * Enum representing logical comparisons on doubles */ public enum Operator implements Writeable { - EQ { - @Override - public boolean test(double lhs, double rhs) { - return Double.compare(lhs, rhs) == 0; - } - }, GT { @Override public boolean test(double lhs, double rhs) { @@ -48,19 +42,10 @@ public boolean test(double lhs, double rhs) { public boolean test(double lhs, double rhs) { return Double.compare(lhs, rhs) <= 0; } - }, - MATCH { - @Override - public boolean match(Pattern pattern, String field) { - Matcher match = pattern.matcher(field); - return match.matches(); - } - - @Override - public boolean expectsANumericArgument() { - return false; - } }; + // EQ was considered but given the oddity of such a + // condition and the fact that it would be a numerically + // unstable condition, it was rejected. public static final ParseField OPERATOR_FIELD = new ParseField("operator"); @@ -68,14 +53,6 @@ public boolean test(double lhs, double rhs) { return false; } - public boolean match(Pattern pattern, String field) { - return false; - } - - public boolean expectsANumericArgument() { - return true; - } - public static Operator fromString(String name) { return valueOf(name.trim().toUpperCase(Locale.ROOT)); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java index 607961140be4e..499f3a47f0e06 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java @@ -13,8 +13,8 @@ import java.util.Locale; public enum RuleAction implements Writeable { - FILTER_RESULTS, - SKIP_SAMPLING; + SKIP_RESULT, + SKIP_MODEL_UPDATE; /** * Case-insensitive from string method. diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java index 6ca24c518d8fa..378ceaca6c476 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.core.ml.job.config; -import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -16,29 +15,27 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlParserType; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; -import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; import java.io.IOException; import java.util.EnumMap; -import java.util.EnumSet; +import java.util.Locale; import java.util.Map; import java.util.Objects; public class RuleCondition implements ToXContentObject, Writeable { - public static final ParseField TYPE_FIELD = new ParseField("type", "condition_type"); + public static final ParseField RULE_CONDITION_FIELD = new ParseField("rule_condition"); - public static final ParseField FIELD_NAME_FIELD = new ParseField("field_name"); - public static final ParseField FIELD_VALUE_FIELD = new ParseField("field_value"); - public static final ParseField FILTER_ID_FIELD = new ParseField(MlFilter.ID.getPreferredName(), "value_filter"); + + public static final ParseField APPLIES_TO_FIELD = new ParseField("applies_to"); + public static final ParseField VALUE_FIELD = new ParseField("value"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly public static final ConstructingObjectParser METADATA_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), true, - a -> new RuleCondition((RuleConditionType) a[0], (String) a[1], (String) a[2], (Condition) a[3], (String) a[4])); + a -> new RuleCondition((AppliesTo) a[0], (Operator) a[1], (double) a[2])); public static final ConstructingObjectParser CONFIG_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), false, - a -> new RuleCondition((RuleConditionType) a[0], (String) a[1], (String) a[2], (Condition) a[3], (String) a[4])); + a -> new RuleCondition((AppliesTo) a[0], (Operator) a[1], (double) a[2])); public static final Map> PARSERS = new EnumMap<>(MlParserType.class); @@ -50,111 +47,63 @@ public class RuleCondition implements ToXContentObject, Writeable { assert parser != null; parser.declareField(ConstructingObjectParser.constructorArg(), p -> { if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return RuleConditionType.fromString(p.text()); + return AppliesTo.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, APPLIES_TO_FIELD, ValueType.STRING); + parser.declareField(ConstructingObjectParser.constructorArg(), p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return Operator.fromString(p.text()); } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, TYPE_FIELD, ValueType.STRING); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FIELD_NAME_FIELD); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FIELD_VALUE_FIELD); - parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), Condition.PARSER, Condition.CONDITION_FIELD); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FILTER_ID_FIELD); + }, Operator.OPERATOR_FIELD, ValueType.STRING); + parser.declareDouble(ConstructingObjectParser.constructorArg(), VALUE_FIELD); } } - private final RuleConditionType type; - private final String fieldName; - private final String fieldValue; - private final Condition condition; - private final String filterId; + private final AppliesTo appliesTo; + private final Operator operator; + private final double value; public RuleCondition(StreamInput in) throws IOException { - type = RuleConditionType.readFromStream(in); - condition = in.readOptionalWriteable(Condition::new); - fieldName = in.readOptionalString(); - fieldValue = in.readOptionalString(); - filterId = in.readOptionalString(); + appliesTo = AppliesTo.readFromStream(in); + operator = Operator.readFromStream(in); + value = in.readDouble(); } @Override public void writeTo(StreamOutput out) throws IOException { - type.writeTo(out); - out.writeOptionalWriteable(condition); - out.writeOptionalString(fieldName); - out.writeOptionalString(fieldValue); - out.writeOptionalString(filterId); + appliesTo.writeTo(out); + operator.writeTo(out); + out.writeDouble(value); } - RuleCondition(RuleConditionType type, String fieldName, String fieldValue, Condition condition, String filterId) { - this.type = type; - this.fieldName = fieldName; - this.fieldValue = fieldValue; - this.condition = condition; - this.filterId = filterId; - - verifyFieldsBoundToType(this); - verifyFieldValueRequiresFieldName(this); - } - - public RuleCondition(RuleCondition ruleCondition) { - this.type = ruleCondition.type; - this.fieldName = ruleCondition.fieldName; - this.fieldValue = ruleCondition.fieldValue; - this.condition = ruleCondition.condition; - this.filterId = ruleCondition.filterId; + public RuleCondition(AppliesTo appliesTo, Operator operator, double value) { + this.appliesTo = appliesTo; + this.operator = operator; + this.value = value; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(TYPE_FIELD.getPreferredName(), type); - if (condition != null) { - builder.field(Condition.CONDITION_FIELD.getPreferredName(), condition); - } - if (fieldName != null) { - builder.field(FIELD_NAME_FIELD.getPreferredName(), fieldName); - } - if (fieldValue != null) { - builder.field(FIELD_VALUE_FIELD.getPreferredName(), fieldValue); - } - if (filterId != null) { - builder.field(FILTER_ID_FIELD.getPreferredName(), filterId); - } + builder.field(APPLIES_TO_FIELD.getPreferredName(), appliesTo); + builder.field(Operator.OPERATOR_FIELD.getPreferredName(), operator); + builder.field(VALUE_FIELD.getPreferredName(), value); builder.endObject(); return builder; } - public RuleConditionType getType() { - return type; - } - - /** - * The field name for which the rule applies. Can be null, meaning rule - * applies to all results. - */ - public String getFieldName() { - return fieldName; - } - - /** - * The value of the field name for which the rule applies. When set, the - * rule applies only to the results that have the fieldName/fieldValue pair. - * When null, the rule applies to all values for of the specified field - * name. Only applicable when fieldName is not null. - */ - public String getFieldValue() { - return fieldValue; + public AppliesTo getAppliesTo() { + return appliesTo; } - public Condition getCondition() { - return condition; + public Operator getOperator() { + return operator; } - /** - * The unique identifier of a filter. Required when the rule type is - * categorical. Should be null for all other types. - */ - public String getFilterId() { - return filterId; + public double getValue() { + return value; } @Override @@ -168,114 +117,40 @@ public boolean equals(Object obj) { } RuleCondition other = (RuleCondition) obj; - return Objects.equals(type, other.type) && Objects.equals(fieldName, other.fieldName) - && Objects.equals(fieldValue, other.fieldValue) && Objects.equals(condition, other.condition) - && Objects.equals(filterId, other.filterId); + return appliesTo == other.appliesTo && operator == other.operator && value == other.value; } @Override public int hashCode() { - return Objects.hash(type, fieldName, fieldValue, condition, filterId); - } - - public static RuleCondition createCategorical(String fieldName, String filterId) { - return new RuleCondition(RuleConditionType.CATEGORICAL, fieldName, null, null, filterId); - } - - public static RuleCondition createNumerical(RuleConditionType conditionType, String fieldName, String fieldValue, - Condition condition ) { - if (conditionType.isNumerical() == false) { - throw new IllegalStateException("Rule condition type [" + conditionType + "] not valid for a numerical condition"); - } - return new RuleCondition(conditionType, fieldName, fieldValue, condition, null); + return Objects.hash(appliesTo, operator, value); } public static RuleCondition createTime(Operator operator, long epochSeconds) { - return new RuleCondition(RuleConditionType.TIME, null, null, new Condition(operator, Long.toString(epochSeconds)), null); - } - - private static void verifyFieldsBoundToType(RuleCondition ruleCondition) throws ElasticsearchParseException { - switch (ruleCondition.getType()) { - case CATEGORICAL: - case CATEGORICAL_COMPLEMENT: - verifyCategorical(ruleCondition); - break; - case NUMERICAL_ACTUAL: - case NUMERICAL_TYPICAL: - case NUMERICAL_DIFF_ABS: - verifyNumerical(ruleCondition); - break; - case TIME: - verifyTimeRule(ruleCondition); - break; - default: - throw new IllegalStateException(); - } - } - - private static void verifyCategorical(RuleCondition ruleCondition) throws ElasticsearchParseException { - checkCategoricalHasNoField(Condition.CONDITION_FIELD.getPreferredName(), ruleCondition.getCondition()); - checkCategoricalHasNoField(RuleCondition.FIELD_VALUE_FIELD.getPreferredName(), ruleCondition.getFieldValue()); - checkCategoricalHasField(FILTER_ID_FIELD.getPreferredName(), ruleCondition.getFilterId()); - } - - private static void checkCategoricalHasNoField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); - } - } - - private static void checkCategoricalHasField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_MISSING_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); - } + return new RuleCondition(AppliesTo.TIME, operator, epochSeconds); } - private static void verifyNumerical(RuleCondition ruleCondition) throws ElasticsearchParseException { - checkNumericalHasNoField(FILTER_ID_FIELD.getPreferredName(), ruleCondition.getFilterId()); - checkNumericalHasField(Condition.CONDITION_FIELD.getPreferredName(), ruleCondition.getCondition()); - if (ruleCondition.getFieldName() != null && ruleCondition.getFieldValue() == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_WITH_FIELD_NAME_REQUIRES_FIELD_VALUE); - throw ExceptionsHelper.badRequestException(msg); - } - checkNumericalConditionOparatorsAreValid(ruleCondition); - } + public enum AppliesTo implements Writeable { + ACTUAL, + TYPICAL, + DIFF_FROM_TYPICAL, + TIME; - private static void checkNumericalHasNoField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); + public static AppliesTo fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); } - } - private static void checkNumericalHasField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_MISSING_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); + public static AppliesTo readFromStream(StreamInput in) throws IOException { + return in.readEnum(AppliesTo.class); } - } - private static void verifyFieldValueRequiresFieldName(RuleCondition ruleCondition) throws ElasticsearchParseException { - if (ruleCondition.getFieldValue() != null && ruleCondition.getFieldName() == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_MISSING_FIELD_NAME, - ruleCondition.getFieldValue()); - throw ExceptionsHelper.badRequestException(msg); + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeEnum(this); } - } - - static EnumSet VALID_CONDITION_OPERATORS = EnumSet.of(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE); - private static void checkNumericalConditionOparatorsAreValid(RuleCondition ruleCondition) throws ElasticsearchParseException { - Operator operator = ruleCondition.getCondition().getOperator(); - if (!VALID_CONDITION_OPERATORS.contains(operator)) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPERATOR, operator); - throw ExceptionsHelper.badRequestException(msg); + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); } } - - private static void verifyTimeRule(RuleCondition ruleCondition) { - checkNumericalConditionOparatorsAreValid(ruleCondition); - } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java deleted file mode 100644 index aa563d001b5ca..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * 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.core.ml.job.config; - - -import org.elasticsearch.Version; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; - -import java.io.IOException; -import java.util.Locale; - -public enum RuleConditionType implements Writeable { - CATEGORICAL(false, true), - NUMERICAL_ACTUAL(true, false), - NUMERICAL_TYPICAL(true, false), - NUMERICAL_DIFF_ABS(true, false), - TIME(false, false), - CATEGORICAL_COMPLEMENT(false, true); - - private final boolean isNumerical; - private final boolean isCategorical; - - RuleConditionType(boolean isNumerical, boolean isCategorical) { - this.isNumerical = isNumerical; - this.isCategorical = isCategorical; - } - - public boolean isNumerical() { - return isNumerical; - } - - public boolean isCategorical() { - return isCategorical; - } - - /** - * Case-insensitive from string method. - * - * @param value - * String representation - * @return The condition type - */ - public static RuleConditionType fromString(String value) { - return RuleConditionType.valueOf(value.toUpperCase(Locale.ROOT)); - } - - public static RuleConditionType readFromStream(StreamInput in) throws IOException { - return in.readEnum(RuleConditionType.class); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - if (this == CATEGORICAL_COMPLEMENT && out.getVersion().before(Version.V_6_3_0)) { - out.writeEnum(CATEGORICAL); - } else { - out.writeEnum(this); - } - } - - @Override - public String toString() { - return name().toLowerCase(Locale.ROOT); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java new file mode 100644 index 0000000000000..b6b3b4e061bdd --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java @@ -0,0 +1,143 @@ +/* + * 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.core.ml.job.config; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ContextParser; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.xpack.core.ml.MlParserType; +import org.elasticsearch.xpack.core.ml.job.messages.Messages; +import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +public class RuleScope implements ToXContentObject, Writeable { + + public static ContextParser parser(MlParserType parserType) { + return (p, c) -> { + Map unparsedScope = p.map(); + if (unparsedScope.isEmpty()) { + return new RuleScope(); + } + ConstructingObjectParser filterRefParser = FilterRef.PARSERS.get(parserType); + Map scope = new HashMap<>(); + for (Map.Entry entry : unparsedScope.entrySet()) { + try (XContentBuilder builder = XContentFactory.jsonBuilder()) { + builder.map((Map) entry.getValue()); + try (XContentParser scopeParser = XContentFactory.xContent(builder.contentType()).createParser( + NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, Strings.toString(builder))) { + scope.put(entry.getKey(), filterRefParser.parse(scopeParser, null)); + } + } + } + return new RuleScope(scope); + }; + } + + private final Map scope; + + public RuleScope() { + scope = Collections.emptyMap(); + } + + public RuleScope(Map scope) { + this.scope = Objects.requireNonNull(scope); + } + + public RuleScope(StreamInput in) throws IOException { + scope = in.readMap(StreamInput::readString, FilterRef::new); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeMap(scope, StreamOutput::writeString, (out1, value) -> value.writeTo(out1)); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return builder.map(scope); + } + + public boolean isEmpty() { + return scope.isEmpty(); + } + + public void validate(Set validKeys) { + Optional invalidKey = scope.keySet().stream().filter(k -> !validKeys.contains(k)).findFirst(); + if (invalidKey.isPresent()) { + throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD, + invalidKey.get(), validKeys)); + } + } + + public Set getReferencedFilters() { + return scope.values().stream().map(FilterRef::getFilterId).collect(Collectors.toSet()); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj instanceof RuleScope == false) { + return false; + } + + RuleScope other = (RuleScope) obj; + return Objects.equals(scope, other.scope); + } + + @Override + public int hashCode() { + return Objects.hash(scope); + } + + public static Builder builder() { + return new Builder(); + } + + public static class Builder { + + private Map scope = new HashMap<>(); + + public Builder() {} + + public Builder(RuleScope otherScope) { + scope = new HashMap<>(otherScope.scope); + } + + public Builder exclude(String field, String filterId) { + scope.put(field, new FilterRef(filterId, FilterRef.FilterType.EXCLUDE)); + return this; + } + + public Builder include(String field, String filterId) { + scope.put(field, new FilterRef(filterId, FilterRef.FilterType.INCLUDE)); + return this; + } + + public RuleScope build() { + return new RuleScope(scope); + } + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java index 7e5dc231e057a..79d8f068d91f8 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java @@ -88,35 +88,12 @@ public final class Messages { "categorization_filters require setting categorization_field_name"; public static final String JOB_CONFIG_CATEGORIZATION_ANALYZER_REQUIRES_CATEGORIZATION_FIELD_NAME = "categorization_analyzer requires setting categorization_field_name"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_NULL = "Invalid condition: the value field cannot be null"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER = - "Invalid condition value: cannot parse a double from string ''{0}''"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX = - "Invalid condition value: ''{0}'' is not a valid regular expression"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION = - "Invalid detector rule: a categorical rule_condition does not support {0}"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_MISSING_OPTION = - "Invalid detector rule: a categorical rule_condition requires {0} to be set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME = - "Invalid detector rule: field_name has to be one of {0}; actual was ''{1}''"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_MISSING_FIELD_NAME = - "Invalid detector rule: missing field_name in rule_condition where field_value ''{0}'' is set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPERATOR = - "Invalid detector rule: operator ''{0}'' is not allowed"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPTION = - "Invalid detector rule: a numerical rule_condition does not support {0}"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_MISSING_OPTION = - "Invalid detector rule: a numerical rule_condition requires {0} to be set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_WITH_FIELD_NAME_REQUIRES_FIELD_VALUE = - "Invalid detector rule: a numerical rule_condition with field_name requires that field_value is set"; - public static final String JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME = - "Invalid detector rule: target_field_name has to be one of {0}; actual was ''{1}''"; - public static final String JOB_CONFIG_DETECTION_RULE_MISSING_TARGET_FIELD_NAME = - "Invalid detector rule: missing target_field_name where target_field_value ''{0}'' is set"; public static final String JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION = - "Invalid detector rule: function {0} does not support rules"; - public static final String JOB_CONFIG_DETECTION_RULE_REQUIRES_AT_LEAST_ONE_CONDITION = - "Invalid detector rule: at least one rule_condition is required"; + "Invalid detector rule: function {0} does not support rules with conditions"; + public static final String JOB_CONFIG_DETECTION_RULE_REQUIRES_SCOPE_OR_CONDITION = + "Invalid detector rule: at least scope or a condition is required"; + public static final String JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD = + "Invalid detector rule: scope field ''{0}'' is invalid; select from {1}"; public static final String JOB_CONFIG_FIELDNAME_INCOMPATIBLE_FUNCTION = "field_name cannot be used with function ''{0}''"; public static final String JOB_CONFIG_FIELD_VALUE_TOO_LOW = "{0} cannot be less than {1,number}. Value = {2,number}"; public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW = "model_memory_limit must be at least 1 MiB. Value = {0,number}"; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java index f98eb9d5dcecc..3d6c317542279 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java @@ -11,12 +11,10 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; import org.joda.time.DateTime; import java.io.IOException; @@ -56,25 +54,22 @@ public void testToDetectionRule() { ScheduledEvent event = createTestInstance(); DetectionRule rule = event.toDetectionRule(TimeValue.timeValueSeconds(bucketSpanSecs)); - assertEquals(Connective.AND, rule.getConditionsConnective()); - assertEquals(rule.getActions(), EnumSet.of(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING)); - assertNull(rule.getTargetFieldName()); - assertNull(rule.getTargetFieldValue()); + assertEquals(rule.getActions(), EnumSet.of(RuleAction.SKIP_RESULT, RuleAction.SKIP_MODEL_UPDATE)); List conditions = rule.getConditions(); assertEquals(2, conditions.size()); - assertEquals(RuleConditionType.TIME, conditions.get(0).getType()); - assertEquals(RuleConditionType.TIME, conditions.get(1).getType()); - assertEquals(Operator.GTE, conditions.get(0).getCondition().getOperator()); - assertEquals(Operator.LT, conditions.get(1).getCondition().getOperator()); + assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(0).getAppliesTo()); + assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(1).getAppliesTo()); + assertEquals(Operator.GTE, conditions.get(0).getOperator()); + assertEquals(Operator.LT, conditions.get(1).getOperator()); // Check times are aligned with the bucket - long conditionStartTime = Long.parseLong(conditions.get(0).getCondition().getValue()); + long conditionStartTime = (long) conditions.get(0).getValue(); assertEquals(0, conditionStartTime % bucketSpanSecs); long bucketCount = conditionStartTime / bucketSpanSecs; assertEquals(bucketSpanSecs * bucketCount, conditionStartTime); - long conditionEndTime = Long.parseLong(conditions.get(1).getCondition().getValue()); + long conditionEndTime = (long) conditions.get(1).getValue(); assertEquals(0, conditionEndTime % bucketSpanSecs); long eventTime = event.getEndTime().toEpochSecond() - conditionStartTime; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java index e64c01d7d0c3b..6c54eb78189a4 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java @@ -486,11 +486,9 @@ public void testEquals_GivenDifferentCategorizationFilters() { assertFalse(config2.equals(config1)); } - public void testExtractReferencedLists() { - DetectionRule rule1 = new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("foo", - "filter1"))).build(); - DetectionRule rule2 = new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("foo", - "filter2"))).build(); + public void testExtractReferencedFilters() { + DetectionRule rule1 = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "filter1")).build(); + DetectionRule rule2 = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "filter2")).build(); Detector.Builder detector1 = new Detector.Builder("count", null); detector1.setByFieldName("foo"); detector1.setRules(Collections.singletonList(rule1)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java index 3aaf99ab730f8..a57bd24eda0e1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java @@ -5,100 +5,40 @@ */ package org.elasticsearch.xpack.core.ml.job.config; +import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; -import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; -public class DetectionRuleTests extends AbstractSerializingTestCase { - - public void testExtractReferencedLists() { - RuleCondition numericalCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "field", "value", new Condition(Operator.GT, "5"), null); - List conditions = Arrays.asList( - numericalCondition, - RuleCondition.createCategorical("foo", "filter1"), - RuleCondition.createCategorical("bar", "filter2")); - - DetectionRule rule = new DetectionRule.Builder(conditions).build(); - - assertEquals(new HashSet<>(Arrays.asList("filter1", "filter2")), rule.extractReferencedFilters()); - } - - public void testEqualsGivenSameObject() { - DetectionRule rule = createFullyPopulated().build(); - assertTrue(rule.equals(rule)); - } - - public void testEqualsGivenString() { - assertFalse(createFullyPopulated().build().equals("a string")); - } +import static org.hamcrest.Matchers.equalTo; - public void testEqualsGivenDifferentTargetFieldName() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setTargetFieldName("targetField2").build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenDifferentTargetFieldValue() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setTargetFieldValue("targetValue2").build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenDifferentConnective() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setConditionsConnective(Connective.OR).build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenRules() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setConditions(createRule("10")).build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } +public class DetectionRuleTests extends AbstractSerializingTestCase { - public void testEqualsGivenEqual() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().build(); - assertTrue(rule1.equals(rule2)); - assertTrue(rule2.equals(rule1)); - assertEquals(rule1.hashCode(), rule2.hashCode()); + public void testBuildWithNeitherScopeNorCondition() { + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, () -> new DetectionRule.Builder().build()); + assertThat(e.getMessage(), equalTo("Invalid detector rule: at least scope or a condition is required")); } - private static DetectionRule.Builder createFullyPopulated() { - return new DetectionRule.Builder(createRule("5")) - .setActions(EnumSet.of(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING)) - .setTargetFieldName("targetField") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND); - } + public void testExtractReferencedLists() { + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder() + .exclude("foo", "filter1").include("bar", "filter2")) + .build(); - private static List createRule(String value) { - Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); + assertEquals(new HashSet<>(Arrays.asList("filter1", "filter2")), rule.extractReferencedFilters()); } @Override protected DetectionRule createTestInstance() { - int size = 1 + randomInt(20); - List ruleConditions = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - // no need for random condition (it is already tested) - ruleConditions.addAll(createRule(Double.toString(randomDouble()))); - } - DetectionRule.Builder builder = new DetectionRule.Builder(ruleConditions); + DetectionRule.Builder builder = new DetectionRule.Builder(); if (randomBoolean()) { EnumSet actions = EnumSet.noneOf(RuleAction.class); @@ -109,13 +49,35 @@ protected DetectionRule createTestInstance() { builder.setActions(actions); } - if (randomBoolean()) { - builder.setConditionsConnective(randomFrom(Connective.values())); + boolean hasScope = randomBoolean(); + boolean hasConditions = randomBoolean(); + + if (!hasScope && !hasConditions) { + // at least one of the two should be present + if (randomBoolean()) { + hasScope = true; + } else { + hasConditions = true; + } } - if (randomBoolean()) { - builder.setTargetFieldName(randomAlphaOfLengthBetween(1, 20)); - builder.setTargetFieldValue(randomAlphaOfLengthBetween(1, 20)); + if (hasScope) { + Map scope = new HashMap<>(); + int scopeSize = randomIntBetween(1, 3); + for (int i = 0; i < scopeSize; i++) { + scope.put(randomAlphaOfLength(20), new FilterRef(randomAlphaOfLength(20), randomFrom(FilterRef.FilterType.values()))); + } + builder.setScope(new RuleScope(scope)); + } + + if (hasConditions) { + int size = randomIntBetween(1, 5); + List ruleConditions = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + // no need for random condition (it is already tested) + ruleConditions.addAll(createCondition(randomDouble())); + } + builder.setConditions(ruleConditions); } return builder.build(); @@ -132,39 +94,34 @@ protected DetectionRule doParseInstance(XContentParser parser) { } @Override - protected DetectionRule mutateInstance(DetectionRule instance) throws IOException { + protected DetectionRule mutateInstance(DetectionRule instance) { List conditions = instance.getConditions(); + RuleScope scope = instance.getScope(); EnumSet actions = instance.getActions(); - String targetFieldName = instance.getTargetFieldName(); - String targetFieldValue = instance.getTargetFieldValue(); - Connective connective = instance.getConditionsConnective(); - switch (between(0, 3)) { + switch (between(0, 2)) { case 0: - conditions = new ArrayList<>(conditions); - conditions.addAll(createRule(Double.toString(randomDouble()))); + if (actions.size() == RuleAction.values().length) { + actions = EnumSet.of(randomFrom(RuleAction.values())); + } else { + actions = EnumSet.allOf(RuleAction.class); + } break; case 1: - targetFieldName = randomAlphaOfLengthBetween(5, 10); + conditions = new ArrayList<>(conditions); + conditions.addAll(createCondition(randomDouble())); break; case 2: - targetFieldValue = randomAlphaOfLengthBetween(5, 10); - if (targetFieldName == null) { - targetFieldName = randomAlphaOfLengthBetween(5, 10); - } - break; - case 3: - if (connective == Connective.AND) { - connective = Connective.OR; - } else { - connective = Connective.AND; - } + scope = new RuleScope.Builder(scope).include("another_field", "another_filter").build(); break; default: throw new AssertionError("Illegal randomisation branch"); } - return new DetectionRule.Builder(conditions).setActions(actions).setTargetFieldName(targetFieldName) - .setTargetFieldValue(targetFieldValue).setConditionsConnective(connective).build(); + return new DetectionRule.Builder(conditions).setActions(actions).setScope(scope).build(); + } + + private static List createCondition(double value) { + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, value)); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java index 1296928d68478..c9be0d3b89248 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java @@ -63,55 +63,46 @@ public void testEquals_GivenDifferentByFieldName() { Detector.Builder builder = new Detector.Builder(detector2); builder.setByFieldName("by2"); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by2", "val", condition, null))) - .setActions(RuleAction.FILTER_RESULTS).setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); detector2 = builder.build(); assertFalse(detector1.equals(detector2)); } public void testExtractAnalysisFields() { - Detector detector = createDetector().build(); + DetectionRule rule = new DetectionRule.Builder( + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))) + .setActions(RuleAction.SKIP_RESULT) + .build(); + Detector.Builder builder = createDetector(); + builder.setRules(Collections.singletonList(rule)); + Detector detector = builder.build(); assertEquals(Arrays.asList("by_field", "over_field", "partition"), detector.extractAnalysisFields()); - Detector.Builder builder = new Detector.Builder(detector); + builder.setPartitionFieldName(null); + detector = builder.build(); + assertEquals(Arrays.asList("by_field", "over_field"), detector.extractAnalysisFields()); + builder = new Detector.Builder(detector); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); builder.setByFieldName(null); + detector = builder.build(); + assertEquals(Collections.singletonList("over_field"), detector.extractAnalysisFields()); + builder = new Detector.Builder(detector); - rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); builder.setOverFieldName(null); + detector = builder.build(); + assertTrue(detector.extractAnalysisFields().isEmpty()); } public void testExtractReferencedLists() { Detector.Builder builder = createDetector(); builder.setRules(Arrays.asList( - new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("by_field", "list1"))).build(), - new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("by_field", "list2"))).build())); + new DetectionRule.Builder(RuleScope.builder().exclude("by_field", "list1")).build(), + new DetectionRule.Builder(RuleScope.builder().exclude("by_field", "list2")).build())); Detector detector = builder.build(); assertEquals(new HashSet<>(Arrays.asList("list1", "list2")), detector.extractReferencedFilters()); @@ -139,13 +130,8 @@ private Detector.Builder createDetector() { detector.setOverFieldName("over_field"); detector.setPartitionFieldName("partition"); detector.setUseNull(true); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by_field", "val", condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("partition", "partition_filter")) + .setActions(RuleAction.SKIP_RESULT) .build(); detector.setRules(Collections.singletonList(rule)); return detector; @@ -159,32 +145,27 @@ protected Detector createTestInstance() { detector = new Detector.Builder(function = randomFrom(Detector.COUNT_WITHOUT_FIELD_FUNCTIONS), null); } else { EnumSet functions = EnumSet.copyOf(Detector.FIELD_NAME_FUNCTIONS); - functions.removeAll(Detector.Builder.FUNCTIONS_WITHOUT_RULE_SUPPORT); detector = new Detector.Builder(function = randomFrom(functions), randomAlphaOfLengthBetween(1, 20)); } if (randomBoolean()) { detector.setDetectorDescription(randomAlphaOfLengthBetween(1, 20)); } - String fieldName = null; if (randomBoolean()) { - detector.setPartitionFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setPartitionFieldName(randomAlphaOfLengthBetween(6, 20)); } else if (randomBoolean() && Detector.NO_OVER_FIELD_NAME_FUNCTIONS.contains(function) == false) { - detector.setOverFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setOverFieldName(randomAlphaOfLengthBetween(6, 20)); } else if (randomBoolean()) { - detector.setByFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setByFieldName(randomAlphaOfLengthBetween(6, 20)); } if (randomBoolean()) { detector.setExcludeFrequent(randomFrom(Detector.ExcludeFrequent.values())); } - if (randomBoolean()) { + if (Detector.FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function) == false && randomBoolean()) { int size = randomInt(10); List rules = new ArrayList<>(size); for (int i = 0; i < size; i++) { // no need for random DetectionRule (it is already tested) - Condition condition = new Condition(Operator.GT, "5"); - rules.add(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setTargetFieldName(fieldName).build()); + rules.add(new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())).build()); } detector.setRules(rules); } @@ -462,63 +443,20 @@ public void testVerify_GivenFunctionsThatCanHaveByField() { } } - public void testVerify_GivenInvalidRuleTargetFieldName() { - Detector.Builder detector = new Detector.Builder("mean", "metricVale"); - detector.setByFieldName("metricName"); - detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "metricVale", new Condition(Operator.LT, "5"), null); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instancE").build(); - detector.setRules(Collections.singletonList(rule)); - - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); - - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME, - "[metricName, instance]", "instancE"), - e.getMessage()); - } - - public void testVerify_GivenValidRule() { - Detector.Builder detector = new Detector.Builder("mean", "metricVale"); - detector.setByFieldName("metricName"); - detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "CPU", new Condition(Operator.LT, "5"), null); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instance").build(); - detector.setRules(Collections.singletonList(rule)); - detector.build(); - } - - public void testVerify_GivenCategoricalRuleOnAllPartitioningFields() { + public void testVerify_GivenAllPartitioningFieldsAreScoped() { Detector.Builder detector = new Detector.Builder("count", null); detector.setPartitionFieldName("my_partition"); detector.setOverFieldName("my_over"); detector.setByFieldName("my_by"); - DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - RuleCondition.createCategorical("my_partition", "my_filter_id"), - RuleCondition.createCategorical("my_over", "my_filter_id"), - RuleCondition.createCategorical("my_by", "my_filter_id") - )).build(); - detector.setRules(Collections.singletonList(rule)); - detector.build(); - } - - public void testVerify_GivenCategoricalRuleOnInvalidField() { - Detector.Builder detector = new Detector.Builder("mean", "my_metric"); - detector.setPartitionFieldName("my_partition"); - detector.setOverFieldName("my_over"); - detector.setByFieldName("my_by"); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList( - RuleCondition.createCategorical("my_metric", "my_filter_id") - )).build(); + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder() + .exclude("my_partition", "my_filter_id") + .exclude("my_over", "my_filter_id") + .exclude("my_by", "my_filter_id")) + .build(); detector.setRules(Collections.singletonList(rule)); - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); - - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME, - "[my_by, my_over, my_partition]", "my_metric"), - e.getMessage()); + detector.build(); } public void testVerify_GivenSameByAndPartition() { @@ -596,6 +534,85 @@ public void testVerify_GivenOverIsOver() { assertEquals("'over' is not a permitted value for over_field_name", e.getMessage()); } + public void testVerify_GivenRulesAndFunctionIsLatLong() { + Detector.Builder detector = new Detector.Builder("lat_long", "geo"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function lat_long does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsMetric() { + Detector.Builder detector = new Detector.Builder("metric", "some_metric"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function metric does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsRare() { + Detector.Builder detector = new Detector.Builder("rare", null); + detector.setByFieldName("some_field"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function rare does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsFreqRare() { + Detector.Builder detector = new Detector.Builder("freq_rare", null); + detector.setByFieldName("some_field"); + detector.setOverFieldName("some_field2"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function freq_rare does not support rules with conditions")); + } + + public void testVerify_GivenTimeConditionRuleAndFunctionIsLatLong() { + Detector.Builder detector = new Detector.Builder("lat_long", "geo"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.TIME, Operator.GT, 42.0))).build())); + detector.build(); + } + + public void testVerify_GivenScopeRuleOnInvalidField() { + Detector.Builder detector = new Detector.Builder("mean", "my_metric"); + detector.setPartitionFieldName("my_partition"); + detector.setOverFieldName("my_over"); + detector.setByFieldName("my_by"); + + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("my_metric", "my_filter_id")).build(); + detector.setRules(Collections.singletonList(rule)); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD, + "my_metric", "[my_by, my_over, my_partition]"), e.getMessage()); + } + + public void testVerify_GivenValidRule() { + Detector.Builder detector = new Detector.Builder("mean", "metricVale"); + detector.setByFieldName("metricName"); + detector.setPartitionFieldName("instance"); + DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())) + .setScope(RuleScope.builder() + .include("metricName", "f1") + .exclude("instance", "f2") + .build()) + .build(); + detector.setRules(Collections.singletonList(rule)); + detector.build(); + } + public void testExcludeFrequentForString() { assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("all")); assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("ALL")); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java new file mode 100644 index 0000000000000..241bf6593320c --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java @@ -0,0 +1,30 @@ +/* + * 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.core.ml.job.config; + +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.test.AbstractSerializingTestCase; + +import java.io.IOException; + +public class FilterRefTests extends AbstractSerializingTestCase { + + @Override + protected FilterRef createTestInstance() { + return new FilterRef(randomAlphaOfLength(20), randomFrom(FilterRef.FilterType.values())); + } + + @Override + protected FilterRef doParseInstance(XContentParser parser) throws IOException { + return FilterRef.CONFIG_PARSER.parse(parser, null); + } + + @Override + protected Writeable.Reader instanceReader() { + return FilterRef::new; + } +} \ No newline at end of file diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java index 3663ff14e6302..c529d6ebfb368 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java @@ -53,10 +53,8 @@ protected JobUpdate createTestInstance() { List detectionRules = null; if (randomBoolean()) { detectionRules = new ArrayList<>(); - Condition condition = new Condition(Operator.GT, "5"); detectionRules.add(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setTargetFieldName("foo").build()); + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))).build()); } detectorUpdates.add(new JobUpdate.DetectorUpdate(i, detectorDescription, detectionRules)); } @@ -119,13 +117,11 @@ protected JobUpdate doParseInstance(XContentParser parser) { public void testMergeWithJob() { List detectorUpdates = new ArrayList<>(); List detectionRules1 = Collections.singletonList(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, new Condition(Operator.GT, "5") - , null))) - .setTargetFieldName("mlcategory").build()); + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))) + .build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(0, "description-1", detectionRules1)); List detectionRules2 = Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, new Condition(Operator.GT, "5"), null))) - .setTargetFieldName("host").build()); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))).build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(1, "description-2", detectionRules2)); ModelPlotConfig modelPlotConfig = new ModelPlotConfig(randomBoolean(), randomAlphaOfLength(10)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java index 882c590983aae..07122818d5506 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java @@ -5,36 +5,21 @@ */ package org.elasticsearch.xpack.core.ml.job.config; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; public class RuleConditionTests extends AbstractSerializingTestCase { @Override protected RuleCondition createTestInstance() { - Condition condition = null; - String fieldName = null; - String valueFilter = null; - String fieldValue = null; - RuleConditionType type = randomFrom(RuleConditionType.values()); - if (type.isCategorical()) { - valueFilter = randomAlphaOfLengthBetween(1, 20); - if (randomBoolean()) { - fieldName = randomAlphaOfLengthBetween(1, 20); - } - } else { - // no need to randomize, it is properly randomly tested in - // ConditionTest - condition = new Condition(Operator.LT, Long.toString(randomLong())); - if (randomBoolean()) { - fieldName = randomAlphaOfLengthBetween(1, 20); - fieldValue = randomAlphaOfLengthBetween(1, 20); - } - } - return new RuleCondition(type, fieldName, fieldValue, condition, valueFilter); + return createRandom(); + } + + public static RuleCondition createRandom() { + RuleCondition.AppliesTo appliesTo = randomFrom(RuleCondition.AppliesTo.values()); + Operator operator = randomFrom(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE); + return new RuleCondition(appliesTo, operator, randomDouble()); } @Override @@ -47,199 +32,52 @@ protected RuleCondition doParseInstance(XContentParser parser) { return RuleCondition.CONFIG_PARSER.apply(parser, null); } - public void testConstructor() { - RuleCondition condition = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); - assertEquals(RuleConditionType.CATEGORICAL, condition.getType()); - assertNull(condition.getFieldName()); - assertNull(condition.getFieldValue()); - assertNull(condition.getCondition()); - } - public void testEqualsGivenSameObject() { - RuleCondition condition = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); + RuleCondition condition = createRandom(); assertTrue(condition.equals(condition)); } public void testEqualsGivenString() { - assertFalse(new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "filter").equals("a string")); - } - - public void testEqualsGivenDifferentType() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentFieldName() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricNameaaa", "cpu", - new Condition(Operator.LT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentFieldValue() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpuaaa", - new Condition(Operator.LT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentCondition() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpu", - new Condition(Operator.GT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentValueFilter() { - RuleCondition condition1 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "myFilter"); - RuleCondition condition2 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "myFilteraaa"); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - private static RuleCondition createFullyPopulated() { - return new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpu", new Condition(Operator.LT, "5"), null); - } - - public void testVerify_GivenCategoricalWithCondition() { - Condition condition = new Condition(Operator.MATCH, "text"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, null, null, condition, null)); - assertEquals("Invalid detector rule: a categorical rule_condition does not support condition", e.getMessage()); - } - - public void testVerify_GivenCategoricalWithFieldValue() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, "metric", "CPU", null, null)); - assertEquals("Invalid detector rule: a categorical rule_condition does not support field_value", e.getMessage()); - } - - public void testVerify_GivenCategoricalWithoutFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, null)); - assertEquals("Invalid detector rule: a categorical rule_condition requires filter_id to be set", e.getMessage()); - } - - public void testVerify_GivenNumericalActualWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalActualWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); + assertFalse(createRandom().equals("a string")); } - public void testVerify_GivenNumericalActualWithFieldNameButNoFieldValue() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", null, new Condition(Operator.LT, "5"), null)); - assertEquals("Invalid detector rule: a numerical rule_condition with field_name requires that field_value is set", e.getMessage()); - } - - public void testVerify_GivenNumericalTypicalWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalTypicalWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); - } - - public void testVerify_GivenNumericalDiffAbsWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalDiffAbsWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); - } - - public void testVerify_GivenFieldValueWithoutFieldName() { - Condition condition = new Condition(Operator.LTE, "5"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, "foo", condition, null)); - assertEquals("Invalid detector rule: missing field_name in rule_condition where field_value 'foo' is set", e.getMessage()); - } - - public void testVerify_GivenNumericalAndOperatorEquals() { - Condition condition = new Condition(Operator.EQ, "5"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); - assertEquals("Invalid detector rule: operator 'eq' is not allowed", e.getMessage()); - } - - public void testVerify_GivenNumericalAndOperatorMatch() { - Condition condition = new Condition(Operator.MATCH, "aaa"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); - assertEquals("Invalid detector rule: operator 'match' is not allowed", e.getMessage()); - } - - public void testVerify_GivenDetectionRuleWithInvalidCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "CPU", new Condition(Operator.LT, "invalid"), - null)); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, "invalid"), e.getMessage()); - } - - public void testVerify_GivenValidCategorical() { - // no validation error: - new RuleCondition(RuleConditionType.CATEGORICAL, "metric", null, null, "myFilter"); - new RuleCondition(RuleConditionType.CATEGORICAL_COMPLEMENT, "metric", null, null, "myFilter"); - } - - public void testVerify_GivenValidNumericalActual() { + public void testVerify_GivenValidActual() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", "cpu", new Condition(Operator.GT, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5.0); } - public void testVerify_GivenValidNumericalTypical() { + public void testVerify_GivenValidTypical() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", "cpu", new Condition(Operator.GTE, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.GTE, 5.0); } - public void testVerify_GivenValidNumericalDiffAbs() { + public void testVerify_GivenValidDiffFromTypical() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, "metric", "cpu", new Condition(Operator.LT, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, Operator.LT, 5.0); } public void testCreateTimeBased() { RuleCondition timeBased = RuleCondition.createTime(Operator.GTE, 100L); - assertEquals(RuleConditionType.TIME, timeBased.getType()); - assertEquals(Operator.GTE, timeBased.getCondition().getOperator()); - assertEquals("100", timeBased.getCondition().getValue()); - assertNull(timeBased.getFieldName()); - assertNull(timeBased.getFieldValue()); - assertNull(timeBased.getFilterId()); - } - - public void testCreateTimeBased_GivenOperatorMatch() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> RuleCondition.createTime(Operator.MATCH, 100L)); - assertEquals("Invalid detector rule: operator 'match' is not allowed", e.getMessage()); - } - - public void testCreateNumerical() { - RuleCondition ruleCondition = RuleCondition.createNumerical(RuleConditionType.NUMERICAL_ACTUAL, "foo", "bar", - new Condition(Operator.GTE, "100")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, ruleCondition.getType()); - assertEquals(Operator.GTE, ruleCondition.getCondition().getOperator()); - assertEquals("100", ruleCondition.getCondition().getValue()); - assertEquals("foo", ruleCondition.getFieldName()); - assertEquals("bar", ruleCondition.getFieldValue()); - assertNull(ruleCondition.getFilterId()); + assertEquals(RuleCondition.AppliesTo.TIME, timeBased.getAppliesTo()); + assertEquals(Operator.GTE, timeBased.getOperator()); + assertEquals(100.0, timeBased.getValue(), 0.000001); + } + + public void testAppliesToFromString() { + assertEquals(RuleCondition.AppliesTo.ACTUAL, RuleCondition.AppliesTo.fromString("actual")); + assertEquals(RuleCondition.AppliesTo.ACTUAL, RuleCondition.AppliesTo.fromString("ACTUAL")); + assertEquals(RuleCondition.AppliesTo.TYPICAL, RuleCondition.AppliesTo.fromString("typical")); + assertEquals(RuleCondition.AppliesTo.TYPICAL, RuleCondition.AppliesTo.fromString("TYPICAL")); + assertEquals(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, RuleCondition.AppliesTo.fromString("diff_from_typical")); + assertEquals(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, RuleCondition.AppliesTo.fromString("DIFF_FROM_TYPICAL")); + assertEquals(RuleCondition.AppliesTo.TIME, RuleCondition.AppliesTo.fromString("time")); + assertEquals(RuleCondition.AppliesTo.TIME, RuleCondition.AppliesTo.fromString("TIME")); + } + + public void testAppliesToToString() { + assertEquals("actual", RuleCondition.AppliesTo.ACTUAL.toString()); + assertEquals("typical", RuleCondition.AppliesTo.TYPICAL.toString()); + assertEquals("diff_from_typical", RuleCondition.AppliesTo.DIFF_FROM_TYPICAL.toString()); + assertEquals("time", RuleCondition.AppliesTo.TIME.toString()); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java new file mode 100644 index 0000000000000..10b9c29aba7ef --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java @@ -0,0 +1,81 @@ +/* + * 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.core.ml.job.config; + +import org.elasticsearch.ElasticsearchStatusException; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.test.AbstractWireSerializingTestCase; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +public class RuleScopeTests extends AbstractWireSerializingTestCase { + + @Override + protected RuleScope createTestInstance() { + RuleScope.Builder scope = RuleScope.builder(); + int count = randomIntBetween(0, 3); + for (int i = 0; i < count; ++i) { + if (randomBoolean()) { + scope.include(randomAlphaOfLength(20), randomAlphaOfLength(20)); + } else { + scope.exclude(randomAlphaOfLength(20), randomAlphaOfLength(20)); + } + } + return scope.build(); + } + + @Override + protected Writeable.Reader instanceReader() { + return RuleScope::new; + } + + public void testValidate_GivenEmpty() { + RuleScope scope = RuleScope.builder().build(); + assertThat(scope.isEmpty(), is(true)); + + scope.validate(Sets.newHashSet("a", "b")); + } + + public void testValidate_GivenMultipleValidFields() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.isEmpty(), is(false)); + + scope.validate(Sets.newHashSet("foo", "bar", "foobar")); + } + + public void testValidate_GivenMultipleFieldsIncludingInvalid() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.isEmpty(), is(false)); + + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, + () -> scope.validate(Sets.newHashSet("foo", "foobar"))); + assertThat(e.getMessage(), equalTo("Invalid detector rule: scope field 'bar' is invalid; select from [foo, foobar]")); + } + + public void testGetReferencedFilters_GivenEmpty() { + assertThat(RuleScope.builder().build().getReferencedFilters().isEmpty(), is(true)); + } + + public void testGetReferencedFilters_GivenMultipleFields() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.getReferencedFilters(), contains("filter1", "filter2", "filter3")); + } +} \ No newline at end of file diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java index f783dfbddc35a..5de7962169279 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java @@ -54,6 +54,7 @@ import org.elasticsearch.xpack.core.ml.action.OpenJobAction; import org.elasticsearch.xpack.core.ml.action.PutJobAction; import org.elasticsearch.xpack.core.ml.action.UpdateJobAction; +import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.JobTaskStatus; @@ -190,6 +191,14 @@ static PersistentTasksCustomMetaData.Assignment selectLeastLoadedMlNode(String j continue; } + if (jobHasRules(job) && node.getVersion().before(DetectionRule.VERSION_INTRODUCED)) { + String reason = "Not opening job [" + jobId + "] on node [" + nodeNameAndVersion(node) + "], because jobs using " + + "custom_rules require a node of version [" + DetectionRule.VERSION_INTRODUCED + "] or higher"; + logger.trace(reason); + reasons.add(reason); + continue; + } + long numberOfAssignedJobs = 0; int numberOfAllocatingJobs = 0; long assignedJobMemory = 0; @@ -373,6 +382,10 @@ private static boolean nodeSupportsModelSnapshotVersion(DiscoveryNode node, Job return node.getVersion().onOrAfter(job.getModelSnapshotMinVersion()); } + private static boolean jobHasRules(Job job) { + return job.getAnalysisConfig().getDetectors().stream().anyMatch(d -> d.getRules().isEmpty() == false); + } + static String[] mappingRequiresUpdate(ClusterState state, String[] concreteIndices, Version minVersion, Logger logger) throws IOException { List indicesToUpdate = new ArrayList<>(); @@ -646,9 +659,11 @@ public PersistentTasksCustomMetaData.Assignment getAssignment(OpenJobAction.JobP @Override public void validate(OpenJobAction.JobParams params, ClusterState clusterState) { + + TransportOpenJobAction.validate(params.getJobId(), MlMetadata.getMlMetadata(clusterState)); + // If we already know that we can't find an ml node because all ml nodes are running at capacity or // simply because there are no ml nodes in the cluster then we fail quickly here: - TransportOpenJobAction.validate(params.getJobId(), MlMetadata.getMlMetadata(clusterState)); PersistentTasksCustomMetaData.Assignment assignment = selectLeastLoadedMlNode(params.getJobId(), clusterState, maxConcurrentJobAllocations, fallbackMaxNumberOfOpenJobs, maxMachineMemoryPercent, logger); if (assignment.getExecutorNode() == null) { diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java index 2042d917f6057..1fd73b96667b2 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java @@ -181,6 +181,7 @@ public void putJob(PutJobAction.Request request, AnalysisRegistry analysisRegist request.getJobBuilder().validateCategorizationAnalyzer(analysisRegistry, environment); Job job = request.getJobBuilder().build(new Date()); + if (job.getDataDescription() != null && job.getDataDescription().getFormat() == DataDescription.DataFormat.DELIMITED) { DEPRECATION_LOGGER.deprecated("Creating jobs with delimited data format is deprecated. Please use xcontent instead."); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java index 5ee07d63a968b..6ef2d92d9c7c6 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java @@ -36,9 +36,15 @@ import org.elasticsearch.xpack.core.ml.MlMetaIndex; import org.elasticsearch.xpack.core.ml.MlMetadata; import org.elasticsearch.xpack.core.ml.action.OpenJobAction; +import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; +import org.elasticsearch.xpack.core.ml.job.config.DataDescription; +import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; +import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.JobTaskStatus; +import org.elasticsearch.xpack.core.ml.job.config.Operator; +import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndex; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndexFields; import org.elasticsearch.xpack.core.ml.job.persistence.ElasticsearchMappings; @@ -49,6 +55,7 @@ import java.io.IOException; import java.net.InetAddress; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.HashMap; @@ -427,6 +434,62 @@ public void testSelectLeastLoadedMlNode_noNodesMatchingModelSnapshotMinVersion() assertNull(result.getExecutorNode()); } + public void testSelectLeastLoadedMlNode_jobWithRulesButNoNodeMeetsRequiredVersion() { + Map nodeAttr = new HashMap<>(); + nodeAttr.put(MachineLearning.ML_ENABLED_NODE_ATTR, "true"); + DiscoveryNodes nodes = DiscoveryNodes.builder() + .add(new DiscoveryNode("_node_name1", "_node_id1", new TransportAddress(InetAddress.getLoopbackAddress(), 9300), + nodeAttr, Collections.emptySet(), Version.V_6_2_0)) + .add(new DiscoveryNode("_node_name2", "_node_id2", new TransportAddress(InetAddress.getLoopbackAddress(), 9301), + nodeAttr, Collections.emptySet(), Version.V_6_3_0)) + .build(); + + PersistentTasksCustomMetaData.Builder tasksBuilder = PersistentTasksCustomMetaData.builder(); + addJobTask("job_with_rules", "_node_id1", null, tasksBuilder); + PersistentTasksCustomMetaData tasks = tasksBuilder.build(); + + ClusterState.Builder cs = ClusterState.builder(new ClusterName("_name")); + MetaData.Builder metaData = MetaData.builder(); + RoutingTable.Builder routingTable = RoutingTable.builder(); + addJobAndIndices(metaData, routingTable, jobWithRulesCreator(), "job_with_rules"); + cs.nodes(nodes); + metaData.putCustom(PersistentTasksCustomMetaData.TYPE, tasks); + cs.metaData(metaData); + cs.routingTable(routingTable.build()); + Assignment result = TransportOpenJobAction.selectLeastLoadedMlNode("job_with_rules", cs.build(), + 2, 10, 30, logger); + assertThat(result.getExplanation(), containsString( + "because jobs using custom_rules require a node of version [6.4.0] or higher")); + assertNull(result.getExecutorNode()); + } + + public void testSelectLeastLoadedMlNode_jobWithRulesAndNodeMeetsRequiredVersion() { + Map nodeAttr = new HashMap<>(); + nodeAttr.put(MachineLearning.ML_ENABLED_NODE_ATTR, "true"); + DiscoveryNodes nodes = DiscoveryNodes.builder() + .add(new DiscoveryNode("_node_name1", "_node_id1", new TransportAddress(InetAddress.getLoopbackAddress(), 9300), + nodeAttr, Collections.emptySet(), Version.V_6_2_0)) + .add(new DiscoveryNode("_node_name2", "_node_id2", new TransportAddress(InetAddress.getLoopbackAddress(), 9301), + nodeAttr, Collections.emptySet(), Version.V_6_4_0)) + .build(); + + PersistentTasksCustomMetaData.Builder tasksBuilder = PersistentTasksCustomMetaData.builder(); + addJobTask("job_with_rules", "_node_id1", null, tasksBuilder); + PersistentTasksCustomMetaData tasks = tasksBuilder.build(); + + ClusterState.Builder cs = ClusterState.builder(new ClusterName("_name")); + MetaData.Builder metaData = MetaData.builder(); + RoutingTable.Builder routingTable = RoutingTable.builder(); + addJobAndIndices(metaData, routingTable, jobWithRulesCreator(), "job_with_rules"); + cs.nodes(nodes); + metaData.putCustom(PersistentTasksCustomMetaData.TYPE, tasks); + cs.metaData(metaData); + cs.routingTable(routingTable.build()); + Assignment result = TransportOpenJobAction.selectLeastLoadedMlNode("job_with_rules", cs.build(), + 2, 10, 30, logger); + assertNotNull(result.getExecutorNode()); + } + public void testVerifyIndicesPrimaryShardsAreActive() { MetaData.Builder metaData = MetaData.builder(); RoutingTable.Builder routingTable = RoutingTable.builder(); @@ -645,4 +708,21 @@ private ClusterState getClusterStateWithMappingsWithMetaData(Map return csBuilder.build(); } + private static Function jobWithRulesCreator() { + return jobId -> { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) + )).build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder(jobId); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + return job.build(new Date()); + }; + } + } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index 120f04e95e70b..7e0dc453f07ee 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -27,29 +27,28 @@ import org.elasticsearch.xpack.core.ml.calendars.Calendar; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DataDescription; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndex; -import org.elasticsearch.xpack.ml.job.persistence.CalendarQueryBuilder; -import org.elasticsearch.xpack.ml.job.persistence.ScheduledEventsQueryBuilder; -import org.elasticsearch.xpack.ml.job.process.autodetect.params.AutodetectParams; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCounts; +import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCountsTests; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSizeStats; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSnapshot; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.Quantiles; import org.elasticsearch.xpack.ml.LocalStateMachineLearning; import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.ml.MlSingleNodeTestCase; +import org.elasticsearch.xpack.ml.job.persistence.CalendarQueryBuilder; import org.elasticsearch.xpack.ml.job.persistence.JobDataCountsPersister; import org.elasticsearch.xpack.ml.job.persistence.JobProvider; import org.elasticsearch.xpack.ml.job.persistence.JobResultsPersister; -import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCountsTests; +import org.elasticsearch.xpack.ml.job.persistence.ScheduledEventsQueryBuilder; +import org.elasticsearch.xpack.ml.job.process.autodetect.params.AutodetectParams; import org.junit.Before; import java.io.IOException; @@ -511,20 +510,15 @@ private Job.Builder createJob(String jobId, List filterIds, List private AnalysisConfig.Builder createAnalysisConfig(List filterIds) { Detector.Builder detector = new Detector.Builder("mean", "field"); detector.setByFieldName("by_field"); + List rules = new ArrayList<>(); - if (!filterIds.isEmpty()) { - List conditions = new ArrayList<>(); - - for (String filterId : filterIds) { - conditions.add(RuleCondition.createCategorical("by_field", filterId)); - } - - DetectionRule.Builder rule = new DetectionRule.Builder(conditions) - .setActions(RuleAction.FILTER_RESULTS) - .setConditionsConnective(Connective.OR); + for (String filterId : filterIds) { + RuleScope.Builder ruleScope = RuleScope.builder(); + ruleScope.include("by_field", filterId); - detector.setRules(Collections.singletonList(rule.build())); + rules.add(new DetectionRule.Builder(ruleScope).setActions(RuleAction.SKIP_RESULT).build()); } + detector.setRules(rules); return new AnalysisConfig.Builder(Collections.singletonList(detector.build())); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java index 64b3bfbab45c8..454f941d6c8b0 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java @@ -19,6 +19,7 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.analysis.AnalysisRegistry; +import org.elasticsearch.persistent.PersistentTasksCustomMetaData; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.MLMetadataField; import org.elasticsearch.xpack.core.ml.MachineLearningField; @@ -32,8 +33,7 @@ import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.persistent.PersistentTasksCustomMetaData; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.ml.job.categorization.CategorizationAnalyzerTests; import org.elasticsearch.xpack.ml.job.persistence.JobProvider; import org.elasticsearch.xpack.ml.job.process.autodetect.UpdateParams; @@ -177,9 +177,7 @@ public void onFailure(Exception e) { public void testUpdateProcessOnFilterChanged() { Detector.Builder detectorReferencingFilter = new Detector.Builder("count", null); detectorReferencingFilter.setByFieldName("foo"); - RuleCondition.createCategorical("foo", "foo_filter"); - DetectionRule filterRule = new DetectionRule.Builder(Collections.singletonList( - RuleCondition.createCategorical("foo", "foo_filter"))).build(); + DetectionRule filterRule = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "foo_filter")).build(); detectorReferencingFilter.setRules(Collections.singletonList(filterRule)); AnalysisConfig.Builder filterAnalysisConfig = new AnalysisConfig.Builder(Collections.singletonList( detectorReferencingFilter.build())); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java deleted file mode 100644 index 09bae9fb8cb9b..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java +++ /dev/null @@ -1,114 +0,0 @@ -/* - * 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.ml.job.config; - -import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.common.io.stream.Writeable.Reader; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Condition; -import org.elasticsearch.xpack.core.ml.job.config.Operator; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; - -import java.io.IOException; - -public class ConditionTests extends AbstractSerializingTestCase { - - public void testSetValues() { - Condition cond = new Condition(Operator.EQ, "5"); - assertEquals(Operator.EQ, cond.getOperator()); - assertEquals("5", cond.getValue()); - } - - public void testHashCodeAndEquals() { - Condition cond1 = new Condition(Operator.MATCH, "regex"); - Condition cond2 = new Condition(Operator.MATCH, "regex"); - - assertEquals(cond1, cond2); - assertEquals(cond1.hashCode(), cond2.hashCode()); - - Condition cond3 = new Condition(Operator.EQ, "5"); - assertFalse(cond1.equals(cond3)); - assertFalse(cond1.hashCode() == cond3.hashCode()); - } - - @Override - protected Condition createTestInstance() { - Operator op = randomFrom(Operator.values()); - Condition condition; - switch (op) { - case EQ: - case GT: - case GTE: - case LT: - case LTE: - condition = new Condition(op, Double.toString(randomDouble())); - break; - case MATCH: - condition = new Condition(op, randomAlphaOfLengthBetween(1, 20)); - break; - default: - throw new AssertionError("Unknown operator selected: " + op); - } - return condition; - } - - @Override - protected Reader instanceReader() { - return Condition::new; - } - - @Override - protected Condition doParseInstance(XContentParser parser) { - return Condition.PARSER.apply(parser, null); - } - - public void testVerifyArgsNumericArgs() { - new Condition(Operator.LTE, "100"); - new Condition(Operator.GT, "10.0"); - } - - public void testVerify_GivenEmptyValue() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, () -> new Condition(Operator.LT, "")); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, ""), e.getMessage()); - } - - public void testVerify_GivenInvalidRegex() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, () -> new Condition(Operator.MATCH, "[*")); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX, "[*"), e.getMessage()); - } - - public void testVerify_GivenNullRegex() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, - () -> new Condition(Operator.MATCH, null)); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NULL, "[*"), e.getMessage()); - } - - @Override - protected Condition mutateInstance(Condition instance) throws IOException { - Operator op = instance.getOperator(); - String value = instance.getValue(); - switch (between(0, 1)) { - case 0: - Operator newOp = op; - while (newOp == op) { - newOp = randomFrom(Operator.values()); - } - if (op == Operator.MATCH && newOp != Operator.MATCH) { - value = Double.toString(randomDouble()); - } - op = newOp; - break; - case 1: - value = Double.toString(randomDouble()); - break; - default: - throw new AssertionError("Illegal randomisation branch"); - } - return new Condition(op, value); - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java deleted file mode 100644 index c8f9b6d36cbca..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java +++ /dev/null @@ -1,79 +0,0 @@ -/* - * 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.ml.job.config; - -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Connective; - -import java.io.IOException; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; - -public class ConnectiveTests extends ESTestCase { - - public void testForString() { - assertEquals(Connective.OR, Connective.fromString("or")); - assertEquals(Connective.OR, Connective.fromString("OR")); - assertEquals(Connective.AND, Connective.fromString("and")); - assertEquals(Connective.AND, Connective.fromString("AND")); - } - - public void testToString() { - assertEquals("or", Connective.OR.toString()); - assertEquals("and", Connective.AND.toString()); - } - - public void testValidOrdinals() { - assertThat(Connective.OR.ordinal(), equalTo(0)); - assertThat(Connective.AND.ordinal(), equalTo(1)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - Connective.OR.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - Connective.AND.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); - } - } - } - - public void testReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Connective.readFromStream(in), equalTo(Connective.OR)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Connective.readFromStream(in), equalTo(Connective.AND)); - } - } - } - - public void testInvalidReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(randomIntBetween(2, Integer.MAX_VALUE)); - try (StreamInput in = out.bytes().streamInput()) { - Connective.readFromStream(in); - fail("Expected IOException"); - } catch (IOException e) { - assertThat(e.getMessage(), containsString("Unknown Connective ordinal [")); - } - } - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java index 07f4d4c5edfa2..469dbf822e005 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.xpack.core.ml.job.config.Operator; import java.io.IOException; -import java.util.regex.Pattern; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -19,110 +18,63 @@ public class OperatorTests extends ESTestCase { public void testFromString() { - assertEquals(Operator.fromString("eq"), Operator.EQ); assertEquals(Operator.fromString("gt"), Operator.GT); assertEquals(Operator.fromString("gte"), Operator.GTE); assertEquals(Operator.fromString("lte"), Operator.LTE); assertEquals(Operator.fromString("lt"), Operator.LT); - assertEquals(Operator.fromString("match"), Operator.MATCH); assertEquals(Operator.fromString("Gt"), Operator.GT); - assertEquals(Operator.fromString("EQ"), Operator.EQ); assertEquals(Operator.fromString("GTE"), Operator.GTE); - assertEquals(Operator.fromString("Match"), Operator.MATCH); } public void testToString() { - assertEquals("eq", Operator.EQ.toString()); assertEquals("gt", Operator.GT.toString()); assertEquals("gte", Operator.GTE.toString()); assertEquals("lte", Operator.LTE.toString()); assertEquals("lt", Operator.LT.toString()); - assertEquals("match", Operator.MATCH.toString()); } public void testTest() { - assertTrue(Operator.GT.expectsANumericArgument()); assertTrue(Operator.GT.test(1.0, 0.0)); assertFalse(Operator.GT.test(0.0, 1.0)); - assertTrue(Operator.GTE.expectsANumericArgument()); assertTrue(Operator.GTE.test(1.0, 0.0)); assertTrue(Operator.GTE.test(1.0, 1.0)); assertFalse(Operator.GTE.test(0.0, 1.0)); - assertTrue(Operator.EQ.expectsANumericArgument()); - assertTrue(Operator.EQ.test(0.0, 0.0)); - assertFalse(Operator.EQ.test(1.0, 0.0)); - - assertTrue(Operator.LT.expectsANumericArgument()); assertTrue(Operator.LT.test(0.0, 1.0)); assertFalse(Operator.LT.test(0.0, 0.0)); - assertTrue(Operator.LTE.expectsANumericArgument()); assertTrue(Operator.LTE.test(0.0, 1.0)); assertTrue(Operator.LTE.test(1.0, 1.0)); assertFalse(Operator.LTE.test(1.0, 0.0)); } - public void testMatch() { - assertFalse(Operator.MATCH.expectsANumericArgument()); - assertFalse(Operator.MATCH.test(0.0, 1.0)); - - Pattern pattern = Pattern.compile("^aa.*"); - - assertTrue(Operator.MATCH.match(pattern, "aaaaa")); - assertFalse(Operator.MATCH.match(pattern, "bbaaa")); - } - - public void testValidOrdinals() { - assertThat(Operator.EQ.ordinal(), equalTo(0)); - assertThat(Operator.GT.ordinal(), equalTo(1)); - assertThat(Operator.GTE.ordinal(), equalTo(2)); - assertThat(Operator.LT.ordinal(), equalTo(3)); - assertThat(Operator.LTE.ordinal(), equalTo(4)); - assertThat(Operator.MATCH.ordinal(), equalTo(5)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - Operator.EQ.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - + public void testWriteTo() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.GT.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); + assertThat(in.readVInt(), equalTo(0)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.GTE.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(2)); + assertThat(in.readVInt(), equalTo(1)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.LT.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(3)); + assertThat(in.readVInt(), equalTo(2)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.LTE.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(4)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - Operator.MATCH.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(5)); + assertThat(in.readVInt(), equalTo(3)); } } } @@ -130,40 +82,28 @@ public void testwriteTo() throws Exception { public void testReadFrom() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Operator.readFromStream(in), equalTo(Operator.EQ)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.GT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(2); + out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.GTE)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(3); + out.writeVInt(2); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.LT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(4); + out.writeVInt(3); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.LTE)); } } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(5); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Operator.readFromStream(in), equalTo(Operator.MATCH)); - } - } } public void testInvalidReadFrom() throws Exception { diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java index 1aeefc47ac61c..fadf81693934b 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java @@ -15,27 +15,29 @@ public class RuleActionTests extends ESTestCase { public void testForString() { - assertEquals(RuleAction.FILTER_RESULTS, RuleAction.fromString("filter_results")); - assertEquals(RuleAction.FILTER_RESULTS, RuleAction.fromString("FILTER_RESULTS")); - assertEquals(RuleAction.SKIP_SAMPLING, RuleAction.fromString("SKip_sampLing")); + assertEquals(RuleAction.SKIP_RESULT, RuleAction.fromString("skip_result")); + assertEquals(RuleAction.SKIP_RESULT, RuleAction.fromString("SKIP_RESULT")); + assertEquals(RuleAction.SKIP_MODEL_UPDATE, RuleAction.fromString("skip_model_update")); + assertEquals(RuleAction.SKIP_MODEL_UPDATE, RuleAction.fromString("SKIP_MODEL_UPDATE")); } public void testToString() { - assertEquals("filter_results", RuleAction.FILTER_RESULTS.toString()); + assertEquals("skip_result", RuleAction.SKIP_RESULT.toString()); + assertEquals("skip_model_update", RuleAction.SKIP_MODEL_UPDATE.toString()); } public void testReadFrom() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(0); try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.FILTER_RESULTS)); + assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_RESULT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_SAMPLING)); + assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_MODEL_UPDATE)); } } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java deleted file mode 100644 index bec7ca24fbe68..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * 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.ml.job.config; - -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; - -import java.io.IOException; -import java.util.EnumSet; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; - -public class RuleConditionTypeTests extends ESTestCase { - - public void testFromString() { - assertEquals(RuleConditionType.CATEGORICAL, RuleConditionType.fromString("categorical")); - assertEquals(RuleConditionType.CATEGORICAL, RuleConditionType.fromString("CATEGORICAL")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, RuleConditionType.fromString("numerical_actual")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, RuleConditionType.fromString("NUMERICAL_ACTUAL")); - assertEquals(RuleConditionType.NUMERICAL_TYPICAL, RuleConditionType.fromString("numerical_typical")); - assertEquals(RuleConditionType.NUMERICAL_TYPICAL, RuleConditionType.fromString("NUMERICAL_TYPICAL")); - assertEquals(RuleConditionType.NUMERICAL_DIFF_ABS, RuleConditionType.fromString("numerical_diff_abs")); - assertEquals(RuleConditionType.NUMERICAL_DIFF_ABS, RuleConditionType.fromString("NUMERICAL_DIFF_ABS")); - } - - public void testToString() { - assertEquals("categorical", RuleConditionType.CATEGORICAL.toString()); - assertEquals("categorical_complement", RuleConditionType.CATEGORICAL_COMPLEMENT.toString()); - assertEquals("numerical_actual", RuleConditionType.NUMERICAL_ACTUAL.toString()); - assertEquals("numerical_typical", RuleConditionType.NUMERICAL_TYPICAL.toString()); - assertEquals("numerical_diff_abs", RuleConditionType.NUMERICAL_DIFF_ABS.toString()); - } - - public void testValidOrdinals() { - assertThat(RuleConditionType.CATEGORICAL.ordinal(), equalTo(0)); - assertThat(RuleConditionType.NUMERICAL_ACTUAL.ordinal(), equalTo(1)); - assertThat(RuleConditionType.NUMERICAL_TYPICAL.ordinal(), equalTo(2)); - assertThat(RuleConditionType.NUMERICAL_DIFF_ABS.ordinal(), equalTo(3)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.CATEGORICAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_ACTUAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_TYPICAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(2)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_DIFF_ABS.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(3)); - } - } - } - - public void testReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.CATEGORICAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_ACTUAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(2); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_TYPICAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(3); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_DIFF_ABS)); - } - } - } - - public void testInvalidReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(randomIntBetween(4, Integer.MAX_VALUE)); - try (StreamInput in = out.bytes().streamInput()) { - RuleConditionType.readFromStream(in); - fail("Expected IOException"); - } catch (IOException e) { - assertThat(e.getMessage(), containsString("Unknown RuleConditionType ordinal [")); - } - } - } - - public void testIsNumerical() { - for (RuleConditionType type : EnumSet.allOf(RuleConditionType.class)) { - boolean isNumerical = type.isNumerical(); - if (type == RuleConditionType.NUMERICAL_ACTUAL || - type == RuleConditionType.NUMERICAL_DIFF_ABS || - type == RuleConditionType.NUMERICAL_TYPICAL) { - assertTrue(isNumerical); - } else { - assertFalse(isNumerical); - } - } - } - - public void testIsCategorical() { - for (RuleConditionType type : EnumSet.allOf(RuleConditionType.class)) { - boolean isCategorical = type.isCategorical(); - if (type == RuleConditionType.CATEGORICAL || - type == RuleConditionType.CATEGORICAL_COMPLEMENT) { - assertTrue(isCategorical); - } else { - assertFalse(isCategorical); - } - } - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java index 0aecad2b3a634..cedc65c2ee225 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java @@ -22,7 +22,7 @@ import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobUpdate; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.process.autodetect.output.FlushAcknowledgement; import org.elasticsearch.xpack.ml.job.categorization.CategorizationAnalyzerTests; import org.elasticsearch.xpack.ml.job.persistence.StateStreamer; @@ -91,10 +91,7 @@ public void testWriteUpdateProcessMessage() throws IOException { when(process.isReady()).thenReturn(true); AutodetectCommunicator communicator = createAutodetectCommunicator(process, mock(AutoDetectResultProcessor.class)); - List conditions = Collections.singletonList( - RuleCondition.createCategorical("foo", "bar")); - - DetectionRule updatedRule = new DetectionRule.Builder(conditions).build(); + DetectionRule updatedRule = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "bar")).build(); List detectorUpdates = Collections.singletonList( new JobUpdate.DetectorUpdate(0, "updated description", Collections.singletonList(updatedRule))); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java index 130476951abbd..8c32a5bb40d46 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java @@ -8,18 +8,16 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; -import org.elasticsearch.xpack.core.ml.job.config.Condition; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.ModelPlotConfig; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; import org.elasticsearch.xpack.ml.job.process.autodetect.params.DataLoadParams; import org.elasticsearch.xpack.ml.job.process.autodetect.params.FlushJobParams; import org.elasticsearch.xpack.ml.job.process.autodetect.params.TimeRange; import org.junit.Before; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.mockito.Mockito; @@ -31,6 +29,7 @@ import java.util.Optional; import java.util.stream.IntStream; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -190,22 +189,18 @@ public void testWriteUpdateModelPlotMessage() throws IOException { public void testWriteUpdateDetectorRulesMessage() throws IOException { ControlMsgToProcessWriter writer = new ControlMsgToProcessWriter(lengthEncodedWriter, 4); - DetectionRule rule1 = new DetectionRule.Builder(createRule("5")).setTargetFieldName("targetField1") - .setTargetFieldValue("targetValue").setConditionsConnective(Connective.AND).build(); - DetectionRule rule2 = new DetectionRule.Builder(createRule("5")).setTargetFieldName("targetField2") - .setTargetFieldValue("targetValue").setConditionsConnective(Connective.AND).build(); + DetectionRule rule1 = new DetectionRule.Builder(createRule(5)).build(); + DetectionRule rule2 = new DetectionRule.Builder(createRule(5)).build(); writer.writeUpdateDetectorRulesMessage(2, Arrays.asList(rule1, rule2)); InOrder inOrder = inOrder(lengthEncodedWriter); inOrder.verify(lengthEncodedWriter).writeNumFields(4); inOrder.verify(lengthEncodedWriter, times(3)).writeField(""); inOrder.verify(lengthEncodedWriter).writeField("u[detectorRules]\ndetectorIndex=2\n" + - "rulesJson=[{\"actions\":[\"filter_results\"],\"conditions_connective\":\"and\",\"conditions\":" + - "[{\"type\":\"numerical_actual\",\"condition\":{\"operator\":\"gt\",\"value\":\"5\"}}]," + - "\"target_field_name\":\"targetField1\",\"target_field_value\":\"targetValue\"}," + - "{\"actions\":[\"filter_results\"],\"conditions_connective\":\"and\",\"conditions\":[" + - "{\"type\":\"numerical_actual\",\"condition\":{\"operator\":\"gt\",\"value\":\"5\"}}]," + - "\"target_field_name\":\"targetField2\",\"target_field_value\":\"targetValue\"}]"); + "rulesJson=[{\"actions\":[\"skip_result\"],\"conditions\":" + + "[{\"applies_to\":\"actual\",\"operator\":\"gt\",\"value\":5.0}]}," + + "{\"actions\":[\"skip_result\"],\"conditions\":[" + + "{\"applies_to\":\"actual\",\"operator\":\"gt\",\"value\":5.0}]}]"); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -244,16 +239,17 @@ public void testWriteUpdateScheduledEventsMessage() throws IOException { InOrder inOrder = inOrder(lengthEncodedWriter); inOrder.verify(lengthEncodedWriter).writeNumFields(2); inOrder.verify(lengthEncodedWriter, times(1)).writeField(""); - inOrder.verify(lengthEncodedWriter).writeField("u[scheduledEvents]\n" + ArgumentCaptor capturedMessage = ArgumentCaptor.forClass(String.class); + inOrder.verify(lengthEncodedWriter).writeField(capturedMessage.capture()); + assertThat(capturedMessage.getValue(), equalTo("u[scheduledEvents]\n" + "scheduledevent.0.description = new year\n" - + "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," - + "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1514764800\"}}," - + "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1514851200\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5147648E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5148512E9}]}]\n" + "scheduledevent.1.description = Jan maintenance day\n" - + "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," - + "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1515196800\"}}," - + "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515283200\"}}]}]\n" - ); + + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5151968E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5152832E9}]}]\n")); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -288,8 +284,7 @@ public void testWriteStartBackgroundPersistMessage() throws IOException { verifyNoMoreInteractions(lengthEncodedWriter); } - private static List createRule(String value) { - Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(RuleCondition.createNumerical(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition)); + private static List createRule(double value) { + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, value)); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java index 57917f6761bb7..bf08d09bf090c 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java @@ -10,16 +10,14 @@ import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; +import org.elasticsearch.xpack.ml.MachineLearning; import org.ini4j.Config; import org.ini4j.Ini; import org.ini4j.Profile.Section; @@ -193,9 +191,8 @@ public void testWrite_GivenDetectorWithRules() throws IOException { Detector.Builder detector = new Detector.Builder("mean", "metricValue"); detector.setByFieldName("metricName"); detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = RuleCondition.createNumerical - (RuleConditionType.NUMERICAL_ACTUAL, "metricName", "metricValue", new Condition(Operator.LT, "5")); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instance").build(); + RuleCondition ruleCondition = new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 5); + DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).build(); detector.setRules(Collections.singletonList(rule)); AnalysisConfig.Builder builder = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); @@ -255,14 +252,12 @@ public void testWrite_GivenScheduledEvents() throws IOException { verify(writer).write("detector.0.clause = count\n" + "scheduledevent.0.description = The Ashes\n" + - "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1511395200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515369600\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"],\"conditions\":[{\"applies_to\":\"time\"," + + "\"operator\":\"gte\",\"value\":1.5113952E9},{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5153696E9}]}]\n" + "scheduledevent.1.description = elasticon\n" + - "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1519603200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1519862400\"}}]}]" + - "\n"); + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5196032E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5198624E9}]}]\n"); verifyNoMoreInteractions(writer); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java index ae1b77b34089c..2806aef128575 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java @@ -44,13 +44,13 @@ public void testWrite() throws IOException { new ScheduledEventsWriter(events, TimeValue.timeValueHours(1), buffer).write(); String expectedString = "scheduledevent.0.description = Black Friday\n" + - "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1511395200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515369600\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5113952E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5153696E9}]}]\n" + "scheduledevent.1.description = Blue Monday\n" + - "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1519603200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1519862400\"}}]}]" + + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5196032E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5198624E9}]}]" + "\n"; assertThat(buffer.toString(), equalTo(expectedString)); } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml index 5203ead3ce8a9..d3165260f4b95 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml @@ -152,15 +152,11 @@ setup: "analysis_config" : { "bucket_span": "3600s", "detectors" :[{"function":"mean","field_name":"responsetime", "by_field_name": "airline", - "rules": [ + "custom_rules": [ { - "conditions": [ - { - "type": "categorical", - "field_name": "airline", - "filter_id": "filter-foo" - } - ] + "scope": { + "airline": {"filter_id": "filter-foo"} + } } ]}] }, diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml index ed6b9ead1a3b1..df505176ae739 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml @@ -301,10 +301,26 @@ { "groups": ["group-1", "group-2"], "description":"Post update description", - "detectors": [{"detector_index": 0, "rules": {"target_field_name": "airline", - "conditions": [ { "type": "numerical_actual", - "condition": {"operator": "gt", "value": "10" } } ] } }, - {"detector_index": 1, "description": "updated description"}], + "detectors": [ + { + "detector_index": 0, + "custom_rules":[ + { + "conditions": [ + { + "applies_to": "actual", + "operator": "gt", + "value": 10 + } + ] + } + ] + }, + { + "detector_index": 1, + "description": "updated description" + } + ], "model_plot_config": { "enabled": false, "terms": "foobar" @@ -324,7 +340,8 @@ - match: { model_plot_config.enabled: false } - match: { model_plot_config.terms: "foobar" } - match: { analysis_config.categorization_filters: ["cat3.*"] } - - match: { analysis_config.detectors.0.rules.0.target_field_name: "airline" } + - match: { analysis_config.detectors.0.custom_rules.0.actions: ["skip_result"] } + - length: { analysis_config.detectors.0.custom_rules.0.conditions: 1 } - match: { analysis_config.detectors.0.detector_index: 0 } - match: { analysis_config.detectors.1.detector_description: "updated description" } - match: { analysis_config.detectors.1.detector_index: 1 } @@ -1223,17 +1240,23 @@ { "function": "count", "by_field_name": "country", - "rules": [ + "custom_rules": [ { - "actions": ["filter_results", "skip_sampling"], + "actions": ["skip_result", "skip_model_update"], + "scope": { + "country": {"filter_id": "safe_countries"} + }, "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} + "applies_to":"actual", + "operator":"lt", + "value": 33.3 }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} + { + "applies_to":"typical", + "operator":"lte", + "value": 42.0 + } ] } ] @@ -1248,83 +1271,23 @@ job_id: jobs-crud-rules - match: { count: 1 } - match: { - jobs.0.analysis_config.detectors.0.rules: [ + jobs.0.analysis_config.detectors.0.custom_rules: [ { - "actions": ["filter_results", "skip_sampling"], - "conditions_connective": "or", + "actions": ["skip_result", "skip_model_update"], + "scope": { + "country": {"filter_id": "safe_countries", "filter_type": "include"} + }, "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] - } - ---- -"Test job with pre 6.2 rules": - - - skip: - features: "warnings" - reason: certain rule fields were renamed in 6.2.0 - - - do: - warnings: - - Deprecated field [detector_rules] used, expected [rules] instead - - Deprecated field [rule_action] used, expected [actions] instead - - Deprecated field [rule_conditions] used, expected [conditions] instead - - Deprecated field [condition_type] used, expected [type] instead - - Deprecated field [value_filter] used, expected [filter_id] instead - xpack.ml.put_job: - job_id: jobs-crud-pre-6-2-rules - body: > - { - "analysis_config": { - "detectors": [ - { - "function": "count", - "by_field_name": "country", - "detector_rules": [ - { - "rule_action": "filter_results", - "rule_conditions": [ - { - "condition_type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "value_filter": "foo"} - ] - } - ] - } - ] + "applies_to":"actual", + "operator":"lt", + "value": 33.3 }, - "data_description" : {} - } - - - do: - xpack.ml.get_jobs: - job_id: jobs-crud-pre-6-2-rules - - match: { count: 1 } - - match: { - jobs.0.analysis_config.detectors.0.rules: [ - { - "actions": ["filter_results"], - "conditions_connective": "or", - "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} + "applies_to":"typical", + "operator":"lte", + "value": 42.0 + } ] } ] diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java index 47ced4a96dde8..aa53d6255cb8e 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java @@ -12,7 +12,6 @@ import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.xpack.core.ml.action.GetRecordsAction; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DataDescription; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; @@ -21,10 +20,9 @@ import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.results.AnomalyRecord; import org.junit.After; -import org.junit.Ignore; import java.io.IOException; import java.util.ArrayList; @@ -35,9 +33,7 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isOneOf; @@ -48,38 +44,22 @@ public class DetectionRulesIT extends MlNativeAutodetectIntegTestCase { @After - public void cleanUpTest() throws Exception { + public void cleanUpTest() { cleanUp(); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") - public void testNumericalRule() throws Exception { - RuleCondition condition1 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_1", - new Condition(Operator.LT, "1000")); - RuleCondition condition2 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_2", - new Condition(Operator.LT, "500")); - RuleCondition condition3 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_3", - new Condition(Operator.LT, "100")); - DetectionRule rule = new DetectionRule.Builder(Arrays.asList(condition1, condition2, condition3)).build(); - - Detector.Builder detector = new Detector.Builder("max", "value"); - detector.setRules(Arrays.asList(rule)); - detector.setByFieldName("by_field"); + public void testCondition() throws Exception { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 100.0) + )).build(); - AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder( - Arrays.asList(detector.build())); + Detector.Builder detector = new Detector.Builder("mean", "value"); + detector.setByFieldName("by_field"); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("detection-rule-numeric-test"); + Job.Builder job = new Job.Builder("detection-rules-it-test-condition"); job.setAnalysisConfig(analysisConfig); job.setDataDescription(dataDescription); @@ -91,12 +71,11 @@ public void testNumericalRule() throws Exception { int totalBuckets = 2 * 24; // each half of the buckets contains one anomaly for each by field value Set anomalousBuckets = new HashSet<>(Arrays.asList(20, 44)); - List byFieldValues = Arrays.asList("by_field_value_1", "by_field_value_2", "by_field_value_3"); + List byFieldValues = Arrays.asList("low", "high"); Map anomalousValues = new HashMap<>(); - anomalousValues.put("by_field_value_1", 800); - anomalousValues.put("by_field_value_2", 400); - anomalousValues.put("by_field_value_3", 400); - int normalValue = 1; + anomalousValues.put("low", 99); + anomalousValues.put("high", 701); + int normalValue = 400; List data = new ArrayList<>(); for (int bucket = 0; bucket < totalBuckets; bucket++) { for (String byFieldValue : byFieldValues) { @@ -115,27 +94,14 @@ public void testNumericalRule() throws Exception { List records = getRecords(job.getId()); assertThat(records.size(), equalTo(1)); - assertThat(records.get(0).getByFieldValue(), equalTo("by_field_value_3")); + assertThat(records.get(0).getByFieldValue(), equalTo("high")); long firstRecordTimestamp = records.get(0).getTimestamp().getTime(); { // Update rules so that the anomalies suppression is inverted - RuleCondition newCondition1 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_1", - new Condition(Operator.GT, "1000")); - RuleCondition newCondition2 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_2", - new Condition(Operator.GT, "500")); - RuleCondition newCondition3 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_3", - new Condition(Operator.GT, "0")); - DetectionRule newRule = new DetectionRule.Builder(Arrays.asList(newCondition1, newCondition2, newCondition3)).build(); + DetectionRule newRule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 700.0) + )).build(); JobUpdate.Builder update = new JobUpdate.Builder(job.getId()); update.setDetectorUpdates(Arrays.asList(new JobUpdate.DetectorUpdate(0, null, Arrays.asList(newRule)))); updateJob(job.getId(), update.build()); @@ -149,18 +115,15 @@ public void testNumericalRule() throws Exception { GetRecordsAction.Request recordsAfterFirstHalf = new GetRecordsAction.Request(job.getId()); recordsAfterFirstHalf.setStart(String.valueOf(firstRecordTimestamp + 1)); records = getRecords(recordsAfterFirstHalf); - assertThat(records.size(), equalTo(2)); - Set secondHaldRecordByFieldValues = records.stream().map(AnomalyRecord::getByFieldValue).collect(Collectors.toSet()); - assertThat(secondHaldRecordByFieldValues, contains("by_field_value_1", "by_field_value_2")); + assertThat(records.size(), equalTo(1)); + assertThat(records.get(0).getByFieldValue(), equalTo("low")); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") - public void testCategoricalRule() throws Exception { + public void testScope() throws Exception { MlFilter safeIps = new MlFilter("safe_ips", Arrays.asList("111.111.111.111", "222.222.222.222")); assertThat(putMlFilter(safeIps), is(true)); - RuleCondition condition = RuleCondition.createCategorical("ip", safeIps.getId()); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(condition)).build(); + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")).build(); Detector.Builder detector = new Detector.Builder("count", null); detector.setRules(Arrays.asList(rule)); @@ -169,7 +132,7 @@ public void testCategoricalRule() throws Exception { AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("detection-rule-categorical-test"); + Job.Builder job = new Job.Builder("detection-rules-it-test-scope"); job.setAnalysisConfig(analysisConfig); job.setDataDescription(dataDescription); @@ -263,6 +226,70 @@ public void testCategoricalRule() throws Exception { closeJob(job.getId()); } + public void testScopeAndCondition() throws IOException { + // We have 2 IPs and they're both safe-listed. + List ips = Arrays.asList("111.111.111.111", "222.222.222.222"); + MlFilter safeIps = new MlFilter("safe_ips", ips); + assertThat(putMlFilter(safeIps), is(true)); + + // Ignore if ip in safe list AND actual < 10. + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")) + .setConditions(Arrays.asList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 10.0))) + .build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + detector.setOverFieldName("ip"); + + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); + analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder("detection-rules-it-test-scope-and-condition"); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + + registerJob(job); + putJob(job); + openJob(job.getId()); + + long timestamp = 1509062400000L; + List data = new ArrayList<>(); + + // First, 20 buckets with a count of 1 for both IPs + for (int bucket = 0; bucket < 20; bucket++) { + for (String ip : ips) { + data.add(createIpRecord(timestamp, ip)); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + } + + // Now send anomalous count of 9 for 111.111.111.111 + for (int i = 0; i < 9; i++) { + data.add(createIpRecord(timestamp, "111.111.111.111")); + } + + // and 10 for 222.222.222.222 + for (int i = 0; i < 10; i++) { + data.add(createIpRecord(timestamp, "222.222.222.222")); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + + // Some more normal buckets + for (int bucket = 0; bucket < 3; bucket++) { + for (String ip : ips) { + data.add(createIpRecord(timestamp, ip)); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + } + + postData(job.getId(), joinBetween(0, data.size(), data)); + closeJob(job.getId()); + + List records = getRecords(job.getId()); + assertThat(records.size(), equalTo(1)); + assertThat(records.get(0).getOverFieldValue(), equalTo("222.222.222.222")); + } + private String createIpRecord(long timestamp, String ip) throws IOException { Map record = new HashMap<>(); record.put("time", timestamp); diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java index 8410075ff5e27..6703e4ef2365b 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java @@ -20,7 +20,6 @@ import org.elasticsearch.xpack.core.ml.job.results.AnomalyRecord; import org.elasticsearch.xpack.core.ml.job.results.Bucket; import org.junit.After; -import org.junit.Ignore; import java.io.IOException; import java.time.Instant; @@ -42,7 +41,6 @@ public void cleanUpTest() { cleanUp(); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testScheduledEvents() throws IOException { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); @@ -154,7 +152,6 @@ public void testScheduledEvents() throws IOException { assertThat(records, is(empty())); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testScheduledEventWithInterimResults() throws IOException { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); Job.Builder job = createJob("scheduled-events-interim-results", bucketSpan); @@ -196,7 +193,6 @@ public void testScheduledEventWithInterimResults() throws IOException { /** * Test an open job picks up changes to scheduled events/calendars */ - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testOnlineUpdate() throws Exception { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); Job.Builder job = createJob("scheduled-events-online-update", bucketSpan); diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml index 6ea8771c2374e..c1b238422e92e 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml @@ -91,9 +91,9 @@ wait_for_status: green --- -"Test get job with rules": +"Test job with pre 6.4 rules": - do: xpack.ml.get_jobs: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules - match: { count: 1 } diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml index da36da301c1f8..3a3334f6907e9 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml @@ -134,15 +134,15 @@ } --- -"Test job with pre 6.2 rules": +"Test job with pre 6.4 rules": - skip: - version: "6.2.0 - " - reason: "Rules fields were renamed on 6.2.0" + version: "6.4.0 - " + reason: "Rules were replaced by custom_rules on 6.4.0" - do: xpack.ml.put_job: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules body: > { "analysis_config": { @@ -171,36 +171,22 @@ } --- -"Test job with post 6.2 rules": +"Test job with pre 6.4 rules - dummy job 6.4 onwards": - skip: - version: " - 6.1.99" - reason: "Rules fields were renamed on 6.2.0" + version: " - 6.3.99" + reason: "Rules replaced by custom_rules on 6.4.0" - do: xpack.ml.put_job: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules body: > { "analysis_config": { "detectors": [ { "function": "count", - "by_field_name": "country", - "rules": [ - { - "actions": ["filter_results"], - "conditions": [ - { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] + "by_field_name": "country" } ] }, diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml index 91d294572894d..6634722fac977 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml @@ -100,29 +100,14 @@ setup: - match: { acknowledged: true } --- -"Test get job with rules": +"Test job with pre 6.4 rules": - do: xpack.ml.get_jobs: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules - match: { count: 1 } - - match: { - jobs.0.analysis_config.detectors.0.rules: [ - { - "actions": ["filter_results"], - "conditions_connective": "or", - "conditions": [ - { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] - } + - is_false: jobs.0.analysis_config.detectors.0.rules + - is_false: jobs.0.analysis_config.detectors.0.custom_rules --- "Test get job with function shortcut should expand": From 489db54e5745dc375bdb399aa3e7c37783a6b9ec Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 13 Jun 2018 11:25:26 +0100 Subject: [PATCH 03/11] Ignore numeric shard count if waiting for ALL (#31265) Today, if GET /_cluster/health?wait_for_active_shards=all does not immediately succeed then it throws an exception due to an erroneous and unnecessary call to ActiveShardCount#enoughShardsActive(). This commit fixes this logic. Fixes #31151 --- .../health/TransportClusterHealthAction.java | 10 +++++----- .../health/TransportClusterHealthActionTests.java | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java index 697849985afeb..255f70c56fe6b 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java @@ -234,11 +234,11 @@ static int prepareResponse(final ClusterHealthRequest request, final ClusterHeal ActiveShardCount waitForActiveShards = request.waitForActiveShards(); assert waitForActiveShards.equals(ActiveShardCount.DEFAULT) == false : "waitForActiveShards must not be DEFAULT on the request object, instead it should be NONE"; - if (waitForActiveShards.equals(ActiveShardCount.ALL) - && response.getUnassignedShards() == 0 - && response.getInitializingShards() == 0) { - // if we are waiting for all shards to be active, then the num of unassigned and num of initializing shards must be 0 - waitForCounter++; + if (waitForActiveShards.equals(ActiveShardCount.ALL)) { + if (response.getUnassignedShards() == 0 && response.getInitializingShards() == 0) { + // if we are waiting for all shards to be active, then the num of unassigned and num of initializing shards must be 0 + waitForCounter++; + } } else if (waitForActiveShards.enoughShardsActive(response.getActiveShards())) { // there are enough active shards to meet the requirements of the request waitForCounter++; diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthActionTests.java index cac5bed4033ac..8601687b04a23 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthActionTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.action.admin.cluster.health; import org.elasticsearch.Version; +import org.elasticsearch.action.support.ActiveShardCount; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexMetaData; @@ -61,6 +62,20 @@ public void testWaitForInitializingShards() throws Exception { assertThat(TransportClusterHealthAction.prepareResponse(request, response, clusterState, null), equalTo(0)); } + public void testWaitForAllShards() { + final String[] indices = {"test"}; + final ClusterHealthRequest request = new ClusterHealthRequest(); + request.waitForActiveShards(ActiveShardCount.ALL); + + ClusterState clusterState = randomClusterStateWithInitializingShards("test", 1); + ClusterHealthResponse response = new ClusterHealthResponse("", indices, clusterState); + assertThat(TransportClusterHealthAction.prepareResponse(request, response, clusterState, null), equalTo(0)); + + clusterState = ClusterState.builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)).build(); + response = new ClusterHealthResponse("", indices, clusterState); + assertThat(TransportClusterHealthAction.prepareResponse(request, response, clusterState, null), equalTo(1)); + } + ClusterState randomClusterStateWithInitializingShards(String index, final int initializingShards) { final IndexMetaData indexMetaData = IndexMetaData .builder(index) From 8b4d80ad09261d5db2595ded3ef68032c6898414 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 13 Jun 2018 12:40:22 +0200 Subject: [PATCH 04/11] Fix AntFixture waiting condition (#31272) The AntFixture waiting condition is evaluated to false but it should be true. --- .../gradle/test/AntFixture.groovy | 4 +-- modules/reindex/build.gradle | 5 +++ .../repositories/url/URLFixture.java | 16 ++++++--- .../example/resthandler/ExampleFixtureIT.java | 24 ++++++++++--- .../azure/AzureStorageFixture.java | 15 +++++++- .../gcs/GoogleCloudStorageFixture.java | 14 +++++++- plugins/repository-hdfs/build.gradle | 5 +++ .../repositories/s3/AmazonS3Fixture.java | 14 +++++++- .../main/java/example/ExampleTestFixture.java | 35 ++++++++----------- 9 files changed, 98 insertions(+), 34 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/AntFixture.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/AntFixture.groovy index 039bce052263c..8dcb862064ec9 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/AntFixture.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/AntFixture.groovy @@ -149,11 +149,11 @@ public class AntFixture extends AntTask implements Fixture { } // the process is started (has a pid) and is bound to a network interface - // so now wait undil the waitCondition has been met + // so now evaluates if the waitCondition is successful // TODO: change this to a loop? boolean success try { - success = waitCondition(this, ant) == false + success = waitCondition(this, ant) } catch (Exception e) { String msg = "Wait condition caught exception for ${name}" logger.error(msg, e) diff --git a/modules/reindex/build.gradle b/modules/reindex/build.gradle index 765d55dd095c7..8870e21858d18 100644 --- a/modules/reindex/build.gradle +++ b/modules/reindex/build.gradle @@ -121,6 +121,11 @@ if (Os.isFamily(Os.FAMILY_WINDOWS)) { baseDir, unzip.temporaryDir, version == '090' + waitCondition = { fixture, ant -> + // the fixture writes the ports file when Elasticsearch's HTTP service + // is ready, so we can just wait for the file to exist + return fixture.portsFile.exists() + } } integTest.dependsOn fixture integTestRunner { diff --git a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLFixture.java b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLFixture.java index c9a36ec859021..353a0d895c2c7 100644 --- a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLFixture.java +++ b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLFixture.java @@ -39,6 +39,7 @@ import java.util.Map; import java.util.Objects; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static java.util.Collections.singletonMap; @@ -67,7 +68,6 @@ public static void main(String[] args) throws Exception { writeFile(workingDirectory, "ports", addressAndPort); // Exposes the repository over HTTP - final String url = "http://" + addressAndPort; httpServer.createContext("/", new ResponseHandler(dir(args[1]))); httpServer.start(); @@ -110,7 +110,13 @@ static class ResponseHandler implements HttpHandler { @Override public void handle(HttpExchange exchange) throws IOException { Response response; - if ("GET".equalsIgnoreCase(exchange.getRequestMethod())) { + + final String userAgent = exchange.getRequestHeaders().getFirst("User-Agent"); + if (userAgent != null && userAgent.startsWith("Apache Ant")) { + // This is a request made by the AntFixture, just reply "OK" + response = new Response(RestStatus.OK, emptyMap(), "text/plain; charset=utf-8", "OK".getBytes(UTF_8)); + + } else if ("GET".equalsIgnoreCase(exchange.getRequestMethod())) { String path = exchange.getRequestURI().toString(); if (path.length() > 0 && path.charAt(0) == '/') { path = path.substring(1); @@ -125,13 +131,13 @@ public void handle(HttpExchange exchange) throws IOException { Map headers = singletonMap("Content-Length", String.valueOf(content.length)); response = new Response(RestStatus.OK, headers, "application/octet-stream", content); } else { - response = new Response(RestStatus.NOT_FOUND, emptyMap(), "text/plain", new byte[0]); + response = new Response(RestStatus.NOT_FOUND, emptyMap(), "text/plain; charset=utf-8", new byte[0]); } } else { - response = new Response(RestStatus.FORBIDDEN, emptyMap(), "text/plain", new byte[0]); + response = new Response(RestStatus.FORBIDDEN, emptyMap(), "text/plain; charset=utf-8", new byte[0]); } } else { - response = new Response(RestStatus.INTERNAL_SERVER_ERROR, emptyMap(), "text/plain", + response = new Response(RestStatus.INTERNAL_SERVER_ERROR, emptyMap(), "text/plain; charset=utf-8", "Unsupported HTTP method".getBytes(StandardCharsets.UTF_8)); } exchange.sendResponseHeaders(response.status.getStatus(), response.body.length); diff --git a/plugins/examples/rest-handler/src/test/java/org/elasticsearch/example/resthandler/ExampleFixtureIT.java b/plugins/examples/rest-handler/src/test/java/org/elasticsearch/example/resthandler/ExampleFixtureIT.java index 97fc6b241ea5a..522e67b512d04 100644 --- a/plugins/examples/rest-handler/src/test/java/org/elasticsearch/example/resthandler/ExampleFixtureIT.java +++ b/plugins/examples/rest-handler/src/test/java/org/elasticsearch/example/resthandler/ExampleFixtureIT.java @@ -23,25 +23,41 @@ import org.elasticsearch.test.ESTestCase; import java.io.BufferedReader; +import java.io.BufferedWriter; import java.io.InputStreamReader; +import java.io.OutputStreamWriter; import java.net.InetAddress; import java.net.Socket; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.util.Objects; +import java.util.ArrayList; +import java.util.List; + +import static org.hamcrest.Matchers.hasItems; public class ExampleFixtureIT extends ESTestCase { public void testExample() throws Exception { - final String stringAddress = Objects.requireNonNull(System.getProperty("external.address")); - final URL url = new URL("http://" + stringAddress); + final String externalAddress = System.getProperty("external.address"); + assertNotNull("External address must not be null", externalAddress); + final URL url = new URL("http://" + externalAddress); final InetAddress address = InetAddress.getByName(url.getHost()); try ( Socket socket = new MockSocket(address, url.getPort()); + BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8)); BufferedReader reader = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)) ) { - assertEquals("TEST", reader.readLine()); + writer.write("GET / HTTP/1.1\r\n"); + writer.write("Host: elastic.co\r\n\r\n"); + writer.flush(); + + final List lines = new ArrayList<>(); + String line; + while ((line = reader.readLine()) != null) { + lines.add(line); + } + assertThat(lines, hasItems("HTTP/1.1 200 OK", "TEST")); } } } diff --git a/plugins/repository-azure/qa/microsoft-azure-storage/src/test/java/org/elasticsearch/repositories/azure/AzureStorageFixture.java b/plugins/repository-azure/qa/microsoft-azure-storage/src/test/java/org/elasticsearch/repositories/azure/AzureStorageFixture.java index ebd8241e710ea..2f74c00ef92e2 100644 --- a/plugins/repository-azure/qa/microsoft-azure-storage/src/test/java/org/elasticsearch/repositories/azure/AzureStorageFixture.java +++ b/plugins/repository-azure/qa/microsoft-azure-storage/src/test/java/org/elasticsearch/repositories/azure/AzureStorageFixture.java @@ -24,6 +24,8 @@ import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.Streams; import org.elasticsearch.mocksocket.MockHttpServer; +import org.elasticsearch.repositories.azure.AzureStorageTestServer.Response; +import org.elasticsearch.rest.RestStatus; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -39,6 +41,8 @@ import java.util.List; import java.util.Map; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; @@ -121,7 +125,16 @@ public void handle(HttpExchange exchange) throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); Streams.copy(exchange.getRequestBody(), out); - final AzureStorageTestServer.Response response = server.handle(method, path, query, headers, out.toByteArray()); + Response response = null; + + final String userAgent = exchange.getRequestHeaders().getFirst("User-Agent"); + if (userAgent != null && userAgent.startsWith("Apache Ant")) { + // This is a request made by the AntFixture, just reply "OK" + response = new Response(RestStatus.OK, emptyMap(), "text/plain; charset=utf-8", "OK".getBytes(UTF_8)); + } else { + // Otherwise simulate a S3 response + response = server.handle(method, path, query, headers, out.toByteArray()); + } Map> responseHeaders = exchange.getResponseHeaders(); responseHeaders.put("Content-Type", singletonList(response.contentType)); diff --git a/plugins/repository-gcs/qa/google-cloud-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageFixture.java b/plugins/repository-gcs/qa/google-cloud-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageFixture.java index 31c85d35f3fe8..6175e581e4fd0 100644 --- a/plugins/repository-gcs/qa/google-cloud-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageFixture.java +++ b/plugins/repository-gcs/qa/google-cloud-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageFixture.java @@ -25,6 +25,7 @@ import org.elasticsearch.core.internal.io.Streams; import org.elasticsearch.mocksocket.MockHttpServer; import org.elasticsearch.repositories.gcs.GoogleCloudStorageTestServer.Response; +import org.elasticsearch.rest.RestStatus; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -40,6 +41,8 @@ import java.util.List; import java.util.Map; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; @@ -123,7 +126,16 @@ public void handle(HttpExchange exchange) throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); Streams.copy(exchange.getRequestBody(), out); - final Response storageResponse = storageServer.handle(method, path, query, headers, out.toByteArray()); + Response storageResponse = null; + + final String userAgent = exchange.getRequestHeaders().getFirst("User-Agent"); + if (userAgent != null && userAgent.startsWith("Apache Ant")) { + // This is a request made by the AntFixture, just reply "OK" + storageResponse = new Response(RestStatus.OK, emptyMap(), "text/plain; charset=utf-8", "OK".getBytes(UTF_8)); + } else { + // Otherwise simulate a S3 response + storageResponse = storageServer.handle(method, path, query, headers, out.toByteArray()); + } Map> responseHeaders = exchange.getResponseHeaders(); responseHeaders.put("Content-Type", singletonList(storageResponse.contentType)); diff --git a/plugins/repository-hdfs/build.gradle b/plugins/repository-hdfs/build.gradle index 3c94f4ace7759..304e0f4ae0e1f 100644 --- a/plugins/repository-hdfs/build.gradle +++ b/plugins/repository-hdfs/build.gradle @@ -116,6 +116,11 @@ for (String fixtureName : ['hdfsFixture', 'haHdfsFixture', 'secureHdfsFixture', dependsOn project.configurations.hdfsFixture executable = new File(project.runtimeJavaHome, 'bin/java') env 'CLASSPATH', "${ -> project.configurations.hdfsFixture.asPath }" + waitCondition = { fixture, ant -> + // the hdfs.MiniHDFS fixture writes the ports file when + // it's ready, so we can just wait for the file to exist + return fixture.portsFile.exists() + } final List miniHDFSArgs = [] diff --git a/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java b/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java index c8321e83d1390..cf123f85d98a9 100644 --- a/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java +++ b/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.io.Streams; import org.elasticsearch.mocksocket.MockHttpServer; import org.elasticsearch.repositories.s3.AmazonS3TestServer.Response; +import org.elasticsearch.rest.RestStatus; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -40,6 +41,8 @@ import java.util.List; import java.util.Map; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; @@ -122,7 +125,16 @@ public void handle(HttpExchange exchange) throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); Streams.copy(exchange.getRequestBody(), out); - final Response storageResponse = storageServer.handle(method, path, query, headers, out.toByteArray()); + Response storageResponse = null; + + final String userAgent = exchange.getRequestHeaders().getFirst("User-Agent"); + if (userAgent != null && userAgent.startsWith("Apache Ant")) { + // This is a request made by the AntFixture, just reply "OK" + storageResponse = new Response(RestStatus.OK, emptyMap(), "text/plain; charset=utf-8", "OK".getBytes(UTF_8)); + } else { + // Otherwise simulate a S3 response + storageResponse = storageServer.handle(method, path, query, headers, out.toByteArray()); + } Map> responseHeaders = exchange.getResponseHeaders(); responseHeaders.put("Content-Type", singletonList(storageResponse.contentType)); diff --git a/test/fixtures/example-fixture/src/main/java/example/ExampleTestFixture.java b/test/fixtures/example-fixture/src/main/java/example/ExampleTestFixture.java index 603aba1fc639b..96103d8eaa900 100644 --- a/test/fixtures/example-fixture/src/main/java/example/ExampleTestFixture.java +++ b/test/fixtures/example-fixture/src/main/java/example/ExampleTestFixture.java @@ -19,14 +19,12 @@ package example; +import com.sun.net.httpserver.HttpServer; + import java.lang.management.ManagementFactory; import java.net.Inet6Address; import java.net.InetAddress; import java.net.InetSocketAddress; -import java.nio.ByteBuffer; -import java.nio.channels.AsynchronousServerSocketChannel; -import java.nio.channels.AsynchronousSocketChannel; -import java.nio.channels.CompletionHandler; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -41,9 +39,9 @@ public static void main(String args[]) throws Exception { throw new IllegalArgumentException("ExampleTestFixture "); } Path dir = Paths.get(args[0]); - AsynchronousServerSocketChannel server = AsynchronousServerSocketChannel - .open() - .bind(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0)); + + final InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), 0); + final HttpServer httpServer = HttpServer.create(socketAddress, 0); // write pid file Path tmp = Files.createTempFile(dir, null, null); @@ -53,7 +51,7 @@ public static void main(String args[]) throws Exception { // write port file tmp = Files.createTempFile(dir, null, null); - InetSocketAddress bound = (InetSocketAddress) server.getLocalAddress(); + InetSocketAddress bound = httpServer.getAddress(); if (bound.getAddress() instanceof Inet6Address) { Files.write(tmp, Collections.singleton("[" + bound.getHostString() + "]:" + bound.getPort())); } else { @@ -61,21 +59,18 @@ public static void main(String args[]) throws Exception { } Files.move(tmp, dir.resolve("ports"), StandardCopyOption.ATOMIC_MOVE); + final byte[] response = "TEST\n".getBytes(StandardCharsets.UTF_8); + // go time - server.accept(null, new CompletionHandler() { - @Override - public void completed(AsynchronousSocketChannel socket, Void attachment) { - server.accept(null, this); - try (AsynchronousSocketChannel ch = socket) { - ch.write(ByteBuffer.wrap("TEST\n".getBytes(StandardCharsets.UTF_8))).get(); - } catch (Exception e) { - throw new RuntimeException(e); - } + httpServer.createContext("/", exchange -> { + try { + exchange.sendResponseHeaders(200, response.length); + exchange.getResponseBody().write(response); + } finally { + exchange.close(); } - - @Override - public void failed(Throwable exc, Void attachment) {} }); + httpServer.start(); // wait forever, until you kill me Thread.sleep(Long.MAX_VALUE); From 8c9360b5a1356a5c3f5d8ee2be9bd0cdf552216b Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Wed, 13 Jun 2018 13:22:34 +0200 Subject: [PATCH 05/11] Log warnings when cluster state publication failed to some nodes (#31233) If the publishing of a cluster state to a node fails, we currently only log it as debug information and only on the master. This makes it hard to see the cause of (test) failures when logging is set to default levels. This PR adds a warn level log on the node receiving the cluster state when it fails to deserialise the cluster state and a warn level log on the master with a list of nodes for which publication failed. --- ...ingClusterStatePublishResponseHandler.java | 12 ++++++ .../zen/PublishClusterStateAction.java | 41 +++++++++++-------- ...usterStatePublishResponseHandlerTests.java | 19 +++++++-- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandler.java b/server/src/main/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandler.java index 72e59675c1bfa..8d5ef1926cdd5 100644 --- a/server/src/main/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandler.java +++ b/server/src/main/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandler.java @@ -22,6 +22,7 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; +import java.util.Collections; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -35,6 +36,7 @@ public class BlockingClusterStatePublishResponseHandler { private final CountDownLatch latch; private final Set pendingNodes; + private final Set failedNodes; /** * Creates a new BlockingClusterStatePublishResponseHandler @@ -44,6 +46,7 @@ public BlockingClusterStatePublishResponseHandler(Set publishingT this.pendingNodes = ConcurrentCollections.newConcurrentSet(); this.pendingNodes.addAll(publishingToNodes); this.latch = new CountDownLatch(pendingNodes.size()); + this.failedNodes = ConcurrentCollections.newConcurrentSet(); } /** @@ -64,6 +67,8 @@ public void onResponse(DiscoveryNode node) { public void onFailure(DiscoveryNode node, Exception e) { boolean found = pendingNodes.remove(node); assert found : "node [" + node + "] already responded or failed"; + boolean added = failedNodes.add(node); + assert added : "duplicate failures for " + node; latch.countDown(); } @@ -86,4 +91,11 @@ public DiscoveryNode[] pendingNodes() { // nulls if some nodes responded in the meanwhile return pendingNodes.toArray(new DiscoveryNode[0]); } + + /** + * returns a set of nodes for which publication has failed. + */ + public Set getFailedNodes() { + return Collections.unmodifiableSet(failedNodes); + } } diff --git a/server/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java b/server/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java index 382a42141d83a..cd87a41526313 100644 --- a/server/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java +++ b/server/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java @@ -20,7 +20,6 @@ package org.elasticsearch.discovery.zen; import org.apache.logging.log4j.message.ParameterizedMessage; -import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; @@ -41,6 +40,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.discovery.AckClusterStatePublishResponseHandler; import org.elasticsearch.discovery.BlockingClusterStatePublishResponseHandler; import org.elasticsearch.discovery.Discovery; @@ -207,6 +207,12 @@ private void innerPublish(final ClusterChangedEvent clusterChangedEvent, final S clusterState.version(), publishTimeout, pendingNodes); } } + // The failure is logged under debug when a sending failed. we now log a summary. + Set failedNodes = publishResponseHandler.getFailedNodes(); + if (failedNodes.isEmpty() == false) { + logger.warn("publishing cluster state with version [{}] failed for the following nodes: [{}]", + clusterChangedEvent.state().version(), failedNodes); + } } catch (InterruptedException e) { // ignore & restore interrupt Thread.currentThread().interrupt(); @@ -367,14 +373,14 @@ public static BytesReference serializeDiffClusterState(Diff diff, Version nodeVe protected void handleIncomingClusterStateRequest(BytesTransportRequest request, TransportChannel channel) throws IOException { Compressor compressor = CompressorFactory.compressor(request.bytes()); StreamInput in = request.bytes().streamInput(); - try { - if (compressor != null) { - in = compressor.streamInput(in); - } - in = new NamedWriteableAwareStreamInput(in, namedWriteableRegistry); - in.setVersion(request.version()); - synchronized (lastSeenClusterStateMutex) { - final ClusterState incomingState; + final ClusterState incomingState; + synchronized (lastSeenClusterStateMutex) { + try { + if (compressor != null) { + in = compressor.streamInput(in); + } + in = new NamedWriteableAwareStreamInput(in, namedWriteableRegistry); + in.setVersion(request.version()); // If true we received full cluster state - otherwise diffs if (in.readBoolean()) { incomingState = ClusterState.readFrom(in, transportService.getLocalNode()); @@ -391,14 +397,17 @@ protected void handleIncomingClusterStateRequest(BytesTransportRequest request, logger.debug("received diff for but don't have any local cluster state - requesting full state"); throw new IncompatibleClusterStateVersionException("have no local cluster state"); } - incomingClusterStateListener.onIncomingClusterState(incomingState); - lastSeenClusterState = incomingState; + } catch (IncompatibleClusterStateVersionException e) { + incompatibleClusterStateDiffReceivedCount.incrementAndGet(); + throw e; + } catch (Exception e) { + logger.warn("unexpected error while deserializing an incoming cluster state", e); + throw e; + } finally { + IOUtils.close(in); } - } catch (IncompatibleClusterStateVersionException e) { - incompatibleClusterStateDiffReceivedCount.incrementAndGet(); - throw e; - } finally { - IOUtils.close(in); + incomingClusterStateListener.onIncomingClusterState(incomingState); + lastSeenClusterState = incomingState; } channel.sendResponse(TransportResponse.Empty.INSTANCE); } diff --git a/server/src/test/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandlerTests.java b/server/src/test/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandlerTests.java index 6d0ee8a97821e..9504344236b86 100644 --- a/server/src/test/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandlerTests.java +++ b/server/src/test/java/org/elasticsearch/discovery/BlockingClusterStatePublishResponseHandlerTests.java @@ -85,10 +85,16 @@ public void testConcurrentAccess() throws InterruptedException { int firstRound = randomIntBetween(5, nodeCount - 1); Thread[] threads = new Thread[firstRound]; CyclicBarrier barrier = new CyclicBarrier(firstRound); + Set expectedFailures = new HashSet<>(); Set completedNodes = new HashSet<>(); for (int i = 0; i < threads.length; i++) { - completedNodes.add(allNodes[i]); - threads[i] = new Thread(new PublishResponder(randomBoolean(), allNodes[i], barrier, logger, handler)); + final DiscoveryNode node = allNodes[i]; + completedNodes.add(node); + final boolean fail = randomBoolean(); + if (fail) { + expectedFailures.add(node); + } + threads[i] = new Thread(new PublishResponder(fail, node, barrier, logger, handler)); threads[i].start(); } // wait on the threads to finish @@ -105,7 +111,12 @@ public void testConcurrentAccess() throws InterruptedException { barrier = new CyclicBarrier(secondRound); for (int i = 0; i < threads.length; i++) { - threads[i] = new Thread(new PublishResponder(randomBoolean(), allNodes[firstRound + i], barrier, logger, handler)); + final DiscoveryNode node = allNodes[firstRound + i]; + final boolean fail = randomBoolean(); + if (fail) { + expectedFailures.add(node); + } + threads[i] = new Thread(new PublishResponder(fail, node, barrier, logger, handler)); threads[i].start(); } // wait on the threads to finish @@ -114,6 +125,6 @@ public void testConcurrentAccess() throws InterruptedException { } assertTrue("expected handler not to timeout as all nodes responded", handler.awaitAllNodes(new TimeValue(10))); assertThat(handler.pendingNodes(), arrayWithSize(0)); - + assertThat(handler.getFailedNodes(), equalTo(expectedFailures)); } } From 66f7dd2c4d2b3400f196228d730d92b7b522bde6 Mon Sep 17 00:00:00 2001 From: Tom Veasey Date: Wed, 13 Jun 2018 13:12:53 +0100 Subject: [PATCH 06/11] [ML] Update test thresholds to account for changes to memory control (#31289) To avoid temporary failures, this also disables these tests until elastic/ml-cpp#122 is committed. --- .../integration/AutodetectMemoryLimitIT.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/AutodetectMemoryLimitIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/AutodetectMemoryLimitIT.java index 4e0aa9c7e0613..e9ba002779e37 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/AutodetectMemoryLimitIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/AutodetectMemoryLimitIT.java @@ -37,6 +37,7 @@ public void cleanUpTest() { cleanUp(); } + @AwaitsFix(bugUrl = "https://github.com/elastic/ml-cpp/pulls/122") public void testTooManyPartitions() throws Exception { Detector.Builder detector = new Detector.Builder("count", null); detector.setPartitionFieldName("user"); @@ -77,11 +78,12 @@ public void testTooManyPartitions() throws Exception { // Assert we haven't violated the limit too much GetJobsStatsAction.Response.JobStats jobStats = getJobStats(job.getId()).get(0); ModelSizeStats modelSizeStats = jobStats.getModelSizeStats(); - assertThat(modelSizeStats.getModelBytes(), lessThan(35000000L)); - assertThat(modelSizeStats.getModelBytes(), greaterThan(30000000L)); + assertThat(modelSizeStats.getModelBytes(), lessThan(31500000L)); + assertThat(modelSizeStats.getModelBytes(), greaterThan(24000000L)); assertThat(modelSizeStats.getMemoryStatus(), equalTo(ModelSizeStats.MemoryStatus.HARD_LIMIT)); } + @AwaitsFix(bugUrl = "https://github.com/elastic/ml-cpp/pulls/122") public void testTooManyByFields() throws Exception { Detector.Builder detector = new Detector.Builder("count", null); detector.setByFieldName("user"); @@ -122,11 +124,12 @@ public void testTooManyByFields() throws Exception { // Assert we haven't violated the limit too much GetJobsStatsAction.Response.JobStats jobStats = getJobStats(job.getId()).get(0); ModelSizeStats modelSizeStats = jobStats.getModelSizeStats(); - assertThat(modelSizeStats.getModelBytes(), lessThan(36000000L)); - assertThat(modelSizeStats.getModelBytes(), greaterThan(30000000L)); + assertThat(modelSizeStats.getModelBytes(), lessThan(31500000L)); + assertThat(modelSizeStats.getModelBytes(), greaterThan(25000000L)); assertThat(modelSizeStats.getMemoryStatus(), equalTo(ModelSizeStats.MemoryStatus.HARD_LIMIT)); } + @AwaitsFix(bugUrl = "https://github.com/elastic/ml-cpp/pulls/122") public void testTooManyByAndOverFields() throws Exception { Detector.Builder detector = new Detector.Builder("count", null); detector.setByFieldName("department"); @@ -171,11 +174,12 @@ public void testTooManyByAndOverFields() throws Exception { // Assert we haven't violated the limit too much GetJobsStatsAction.Response.JobStats jobStats = getJobStats(job.getId()).get(0); ModelSizeStats modelSizeStats = jobStats.getModelSizeStats(); - assertThat(modelSizeStats.getModelBytes(), lessThan(36000000L)); + assertThat(modelSizeStats.getModelBytes(), lessThan(31500000L)); assertThat(modelSizeStats.getModelBytes(), greaterThan(24000000L)); assertThat(modelSizeStats.getMemoryStatus(), equalTo(ModelSizeStats.MemoryStatus.HARD_LIMIT)); } + @AwaitsFix(bugUrl = "https://github.com/elastic/ml-cpp/pulls/122") public void testManyDistinctOverFields() throws Exception { Detector.Builder detector = new Detector.Builder("sum", "value"); detector.setOverFieldName("user"); @@ -221,9 +225,9 @@ public void testManyDistinctOverFields() throws Exception { // Assert we haven't violated the limit too much GetJobsStatsAction.Response.JobStats jobStats = getJobStats(job.getId()).get(0); ModelSizeStats modelSizeStats = jobStats.getModelSizeStats(); - assertThat(modelSizeStats.getModelBytes(), lessThan(90000000L)); - assertThat(modelSizeStats.getModelBytes(), greaterThan(75000000L)); - assertThat(modelSizeStats.getMemoryStatus(), equalTo(ModelSizeStats.MemoryStatus.OK)); + assertThat(modelSizeStats.getModelBytes(), lessThan(116000000L)); + assertThat(modelSizeStats.getModelBytes(), greaterThan(90000000L)); + assertThat(modelSizeStats.getMemoryStatus(), equalTo(ModelSizeStats.MemoryStatus.HARD_LIMIT)); } private static Map createRecord(long timestamp, String user, String department) { From 24163d10b7639ea8f66ec5fbf52a80ab90b0c356 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 13 Jun 2018 15:06:13 +0200 Subject: [PATCH 07/11] REST hl client: cluster health to default to cluster level (#31268) With #29331 we added support for the cluster health API to the high-level REST client. The transport client does not support the level parameter, and it always returns all the info needed for shards level rendering. We have maintained that behaviour when adding support for cluster health to the high-level REST client, to ease migration, but the correct thing to do is to default the high-level REST client to `cluster` level, which is the same default as when going through the Elasticsearch REST layer. --- .../java/org/elasticsearch/client/ClusterClientIT.java | 2 +- .../org/elasticsearch/client/RequestConvertersTests.java | 2 +- docs/java-rest/high-level/cluster/health.asciidoc | 1 + docs/reference/migration/migrate_7_0/restclient.asciidoc | 9 ++++++++- .../admin/cluster/health/ClusterHealthRequest.java | 8 +++----- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java index 2ae6f9dc186ef..2a870ec65eea1 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java @@ -129,7 +129,6 @@ public void testClusterHealthYellowClusterLevel() throws IOException { createIndex("index2", Settings.EMPTY); ClusterHealthRequest request = new ClusterHealthRequest(); request.timeout("5s"); - request.level(ClusterHealthRequest.Level.CLUSTER); ClusterHealthResponse response = execute(request, highLevelClient().cluster()::health, highLevelClient().cluster()::healthAsync); assertYellowShards(response); @@ -170,6 +169,7 @@ public void testClusterHealthYellowSpecificIndex() throws IOException { createIndex("index", Settings.EMPTY); createIndex("index2", Settings.EMPTY); ClusterHealthRequest request = new ClusterHealthRequest("index"); + request.level(ClusterHealthRequest.Level.SHARDS); request.timeout("5s"); ClusterHealthResponse response = execute(request, highLevelClient().cluster()::health, highLevelClient().cluster()::healthAsync); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java index 5bc9bac96dcd7..aa8221f30991e 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java @@ -1568,7 +1568,7 @@ public void testClusterHealth() { healthRequest.level(level); expectedParams.put("level", level.name().toLowerCase(Locale.ROOT)); } else { - expectedParams.put("level", "shards"); + expectedParams.put("level", "cluster"); } if (randomBoolean()) { Priority priority = randomFrom(Priority.values()); diff --git a/docs/java-rest/high-level/cluster/health.asciidoc b/docs/java-rest/high-level/cluster/health.asciidoc index 6c0f926f15f42..192880849e26d 100644 --- a/docs/java-rest/high-level/cluster/health.asciidoc +++ b/docs/java-rest/high-level/cluster/health.asciidoc @@ -67,6 +67,7 @@ include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[health-request-wai include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[health-request-level] -------------------------------------------------- <1> The level of detail of the returned health information. Accepts a `ClusterHealthRequest.Level` value. +Default value is `cluster`. ["source","java",subs="attributes,callouts,macros"] -------------------------------------------------- diff --git a/docs/reference/migration/migrate_7_0/restclient.asciidoc b/docs/reference/migration/migrate_7_0/restclient.asciidoc index 146b0d7338640..470996cfeffec 100644 --- a/docs/reference/migration/migrate_7_0/restclient.asciidoc +++ b/docs/reference/migration/migrate_7_0/restclient.asciidoc @@ -10,4 +10,11 @@ header, e.g. `client.index(indexRequest)` becomes `client.index(indexRequest, RequestOptions.DEFAULT)`. In case you are specifying headers e.g. `client.index(indexRequest, new Header("name" "value"))` becomes -`client.index(indexRequest, RequestOptions.DEFAULT.toBuilder().addHeader("name", "value").build());` \ No newline at end of file +`client.index(indexRequest, RequestOptions.DEFAULT.toBuilder().addHeader("name", "value").build());` + +==== Cluster Health API default to `cluster` level + +The Cluster Health API used to default to `shards` level to ease migration +from transport client that doesn't support the `level` parameter and always +returns information including indices and shards details. The level default +value has been aligned with the Elasticsearch default level: `cluster`. \ No newline at end of file diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java index 59a291888d09e..0b9bcbf11b9a1 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java @@ -48,9 +48,9 @@ public class ClusterHealthRequest extends MasterNodeReadRequest Date: Wed, 13 Jun 2018 09:33:06 -0400 Subject: [PATCH 08/11] Test: Remove broken yml test feature (#31255) The `requires_replica` yaml test feature hasn't worked for years. This is what happens if you try to use it: ``` > Throwable #1: java.lang.NullPointerException > at __randomizedtesting.SeedInfo.seed([E6602FB306244B12:6E341069A8D826EA]:0) > at org.elasticsearch.test.rest.yaml.Features.areAllSupported(Features.java:58) > at org.elasticsearch.test.rest.yaml.section.SkipSection.skip(SkipSection.java:144) > at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:321) ``` None of our tests use it. --- .../main/java/org/elasticsearch/test/rest/yaml/Features.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java index bb885d3f181b3..757fc2218d51c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java @@ -19,8 +19,6 @@ package org.elasticsearch.test.rest.yaml; -import org.elasticsearch.test.ESIntegTestCase; - import java.util.Arrays; import java.util.List; @@ -56,9 +54,6 @@ private Features() { */ public static boolean areAllSupported(List features) { for (String feature : features) { - if ("requires_replica".equals(feature) && ESIntegTestCase.cluster().numDataNodes() >= 2) { - continue; - } if (!SUPPORTED.contains(feature)) { return false; } From 7199d5f0e680182bdabba80c9976c6b74f00bedd Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 13 Jun 2018 10:16:46 -0400 Subject: [PATCH 09/11] Add notion of internal index settings (#31286) We have some use cases for an index setting to only be manageable by dedicated APIs rather than be updateable via the update settings API. This commit adds the notion of an internal index setting. Such settings can be set on create index requests, they can not be changed via the update settings API, yet they can be changed by action on behalf of or triggered by the user via dedicated APIs. --- .../MetaDataUpdateSettingsService.java | 6 +- .../settings/AbstractScopedSettings.java | 60 ++++- .../common/settings/Setting.java | 20 +- .../common/settings/ScopedSettingsTests.java | 24 ++ .../common/settings/SettingTests.java | 7 + .../indices/settings/UpdateSettingsIT.java | 205 +++++++++++++++++- 6 files changed, 311 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java index ce5ad12a53d6a..38766c08e08a3 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java @@ -82,8 +82,10 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request Settings.Builder settingsForOpenIndices = Settings.builder(); final Set skippedSettings = new HashSet<>(); - indexScopedSettings.validate(normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false /* don't validate wildcards */), - false); //don't validate dependencies here we check it below never allow to change the number of shards + indexScopedSettings.validate( + normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false), // don't validate wildcards + false, // don't validate dependencies here we check it below never allow to change the number of shards + true); // validate internal index settings for (String key : normalizedSettings.keySet()) { Setting setting = indexScopedSettings.get(key); boolean isWildcard = setting == null && Regex.isSimpleMatchPattern(key); diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index e8bb946c8a795..eb4e294642417 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -282,6 +282,18 @@ public final void validate(final Settings settings, final boolean validateDepend validate(settings, validateDependencies, false, false); } + /** + * Validates that all settings are registered and valid. + * + * @param settings the settings to validate + * @param validateDependencies true if dependent settings should be validated + * @param validateInternalIndex true if internal index settings should be validated + * @see Setting#getSettingsDependencies(String) + */ + public final void validate(final Settings settings, final boolean validateDependencies, final boolean validateInternalIndex) { + validate(settings, validateDependencies, false, false, validateInternalIndex); + } + /** * Validates that all settings are registered and valid. * @@ -296,6 +308,25 @@ public final void validate( final boolean validateDependencies, final boolean ignorePrivateSettings, final boolean ignoreArchivedSettings) { + validate(settings, validateDependencies, ignorePrivateSettings, ignoreArchivedSettings, false); + } + + /** + * Validates that all settings are registered and valid. + * + * @param settings the settings + * @param validateDependencies true if dependent settings should be validated + * @param ignorePrivateSettings true if private settings should be ignored during validation + * @param ignoreArchivedSettings true if archived settings should be ignored during validation + * @param validateInternalIndex true if index internal settings should be validated + * @see Setting#getSettingsDependencies(String) + */ + public final void validate( + final Settings settings, + final boolean validateDependencies, + final boolean ignorePrivateSettings, + final boolean ignoreArchivedSettings, + final boolean validateInternalIndex) { final List exceptions = new ArrayList<>(); for (final String key : settings.keySet()) { // settings iterate in deterministic fashion if (isPrivateSetting(key) && ignorePrivateSettings) { @@ -305,7 +336,7 @@ public final void validate( continue; } try { - validate(key, settings, validateDependencies); + validate(key, settings, validateDependencies, validateInternalIndex); } catch (final RuntimeException ex) { exceptions.add(ex); } @@ -314,9 +345,27 @@ public final void validate( } /** - * Validates that the setting is valid + * Validates that the settings is valid. + * + * @param key the key of the setting to validate + * @param settings the settings + * @param validateDependencies true if dependent settings should be validated + * @throws IllegalArgumentException if the setting is invalid */ - void validate(String key, Settings settings, boolean validateDependencies) { + void validate(final String key, final Settings settings, final boolean validateDependencies) { + validate(key, settings, validateDependencies, false); + } + + /** + * Validates that the settings is valid. + * + * @param key the key of the setting to validate + * @param settings the settings + * @param validateDependencies true if dependent settings should be validated + * @param validateInternalIndex true if internal index settings should be validated + * @throws IllegalArgumentException if the setting is invalid + */ + void validate(final String key, final Settings settings, final boolean validateDependencies, final boolean validateInternalIndex) { Setting setting = getRaw(key); if (setting == null) { LevensteinDistance ld = new LevensteinDistance(); @@ -356,6 +405,11 @@ void validate(String key, Settings settings, boolean validateDependencies) { } } } + // the only time that validateInternalIndex should be true is if this call is coming via the update settings API + if (validateInternalIndex && setting.getProperties().contains(Setting.Property.InternalIndex)) { + throw new IllegalArgumentException( + "can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API"); + } } setting.get(settings); } diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index f45f4bda9c9fe..7f3906ff5a251 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -120,7 +120,13 @@ public enum Property { * Mark this setting as not copyable during an index resize (shrink or split). This property can only be applied to settings that * also have {@link Property#IndexScope}. */ - NotCopyableOnResize + NotCopyableOnResize, + + /** + * Indicates an index-level setting that is managed internally. Such a setting can only be added to an index on index creation but + * can not be updated via the update API. + */ + InternalIndex } private final Key key; @@ -152,14 +158,18 @@ private Setting(Key key, @Nullable Setting fallbackSetting, Function properties, final Property property) { + if (properties.contains(property) && properties.contains(Property.IndexScope) == false) { + throw new IllegalArgumentException("non-index-scoped setting [" + key + "] can not have property [" + property + "]"); + } + } + /** * Creates a new Setting instance * @param key the settings key for this setting. diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index f00768651f917..2376d5663402e 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -876,4 +876,28 @@ public void testFinalSettingUpdateFail() { Settings.builder().put(currentSettings), Settings.builder(), "node")); assertThat(exc.getMessage(), containsString("final node setting [some.final.group.foo]")); } + + public void testInternalIndexSettingsFailsValidation() { + final Setting indexInternalSetting = Setting.simpleString("index.internal", Property.InternalIndex, Property.IndexScope); + final IndexScopedSettings indexScopedSettings = + new IndexScopedSettings(Settings.EMPTY, Collections.singleton(indexInternalSetting)); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> { + final Settings settings = Settings.builder().put("index.internal", "internal").build(); + indexScopedSettings.validate(settings, false, /* validateInternalIndex */ true); + }); + final String message = "can not update internal setting [index.internal]; this setting is managed via a dedicated API"; + assertThat(e, hasToString(containsString(message))); + } + + public void testInternalIndexSettingsSkipValidation() { + final Setting internalIndexSetting = Setting.simpleString("index.internal", Property.InternalIndex, Property.IndexScope); + final IndexScopedSettings indexScopedSettings = + new IndexScopedSettings(Settings.EMPTY, Collections.singleton(internalIndexSetting)); + // nothing should happen, validation should not throw an exception + final Settings settings = Settings.builder().put("index.internal", "internal").build(); + indexScopedSettings.validate(settings, false, /* validateInternalIndex */ false); + } + } diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 1ab92526e3130..c32037f44525e 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -735,6 +735,13 @@ public void testRejectNonIndexScopedNotCopyableOnResizeSetting() { assertThat(e, hasToString(containsString("non-index-scoped setting [foo.bar] can not have property [NotCopyableOnResize]"))); } + public void testRejectNonIndexScopedIndexInternalSetting() { + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> Setting.simpleString("foo.bar", Property.InternalIndex)); + assertThat(e, hasToString(containsString("non-index-scoped setting [foo.bar] can not have property [InternalIndex]"))); + } + public void testTimeValue() { final TimeValue random = TimeValue.parseTimeValue(randomTimeValue(), "test"); diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 51c073c607e22..8093e7d38a14d 100644 --- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -19,19 +19,40 @@ package org.elasticsearch.indices.settings; +import org.elasticsearch.action.Action; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionRequest; +import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.master.MasterNodeRequest; +import org.elasticsearch.action.support.master.TransportMasterNodeAction; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateUpdateTask; +import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Priority; +import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.indices.IndicesService; +import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import java.io.IOException; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -46,6 +67,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.nullValue; public class UpdateSettingsIT extends ESIntegTestCase { @@ -79,7 +101,12 @@ public void testInvalidDynamicUpdate() { @Override protected Collection> nodePlugins() { - return Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class); + return Arrays.asList(DummySettingPlugin.class, FinalSettingPlugin.class, InternalIndexSettingsPlugin.class); + } + + @Override + protected Collection> transportClientPlugins() { + return Collections.singletonList(InternalIndexSettingsPlugin.class); } public static class DummySettingPlugin extends Plugin { @@ -124,6 +151,151 @@ public List> getSettings() { } } + public static class InternalIndexSettingsPlugin extends Plugin implements ActionPlugin { + + public static final Setting INDEX_INTERNAL_SETTING = + Setting.simpleString("index.internal", Setting.Property.IndexScope, Setting.Property.InternalIndex); + + @Override + public List> getSettings() { + return Collections.singletonList(INDEX_INTERNAL_SETTING); + } + + public static class UpdateInternalIndexAction + extends Action { + + private static final UpdateInternalIndexAction INSTANCE = new UpdateInternalIndexAction(); + private static final String NAME = "indices:admin/settings/update-internal-index"; + + public UpdateInternalIndexAction() { + super(NAME); + } + + static class Request extends MasterNodeRequest { + + private String index; + private String value; + + Request() { + + } + + Request(final String index, final String value) { + this.index = index; + this.value = value; + } + + @Override + public ActionRequestValidationException validate() { + return null; + } + + @Override + public void readFrom(final StreamInput in) throws IOException { + super.readFrom(in); + index = in.readString(); + value = in.readString(); + } + + @Override + public void writeTo(final StreamOutput out) throws IOException { + super.writeTo(out); + out.writeString(index); + out.writeString(value); + } + + } + + static class Response extends ActionResponse { + + } + + @Override + public Response newResponse() { + return new Response(); + } + + } + + public static class TransportUpdateInternalIndexAction + extends TransportMasterNodeAction { + + @Inject + public TransportUpdateInternalIndexAction( + final Settings settings, + final TransportService transportService, + final ClusterService clusterService, + final ThreadPool threadPool, + final ActionFilters actionFilters, + final IndexNameExpressionResolver indexNameExpressionResolver) { + super( + settings, + UpdateInternalIndexAction.NAME, + transportService, + clusterService, + threadPool, + actionFilters, + indexNameExpressionResolver, + UpdateInternalIndexAction.Request::new); + } + + @Override + protected String executor() { + return ThreadPool.Names.SAME; + } + + @Override + protected UpdateInternalIndexAction.Response newResponse() { + return new UpdateInternalIndexAction.Response(); + } + + @Override + protected void masterOperation( + final UpdateInternalIndexAction.Request request, + final ClusterState state, + final ActionListener listener) throws Exception { + clusterService.submitStateUpdateTask("update-index-internal", new ClusterStateUpdateTask() { + @Override + public ClusterState execute(final ClusterState currentState) throws Exception { + final MetaData.Builder builder = MetaData.builder(currentState.metaData()); + final IndexMetaData.Builder imdBuilder = IndexMetaData.builder(currentState.metaData().index(request.index)); + final Settings.Builder settingsBuilder = + Settings.builder() + .put(currentState.metaData().index(request.index).getSettings()) + .put("index.internal", request.value); + imdBuilder.settings(settingsBuilder); + builder.put(imdBuilder.build(), true); + return ClusterState.builder(currentState).metaData(builder).build(); + } + + @Override + public void clusterStateProcessed(final String source, final ClusterState oldState, final ClusterState newState) { + listener.onResponse(new UpdateInternalIndexAction.Response()); + } + + @Override + public void onFailure(final String source, final Exception e) { + listener.onFailure(e); + } + + }); + } + + @Override + protected ClusterBlockException checkBlock(UpdateInternalIndexAction.Request request, ClusterState state) { + return null; + } + + } + + @Override + public List> getActions() { + return Collections.singletonList( + new ActionHandler<>(UpdateInternalIndexAction.INSTANCE, TransportUpdateInternalIndexAction.class)); + } + + } + public void testUpdateDependentClusterSettings() { IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> client().admin().cluster().prepareUpdateSettings().setPersistentSettings(Settings.builder() @@ -474,4 +646,35 @@ public void testUpdateSettingsWithBlocks() { } } + public void testUpdateInternalIndexSettingViaSettingsAPI() { + final Settings settings = Settings.builder().put("index.internal", "internal").build(); + createIndex("test", settings); + final GetSettingsResponse response = client().admin().indices().prepareGetSettings("test").get(); + assertThat(response.getSetting("test", "index.internal"), equalTo("internal")); + // we can not update the setting via the update settings API + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> client().admin() + .indices() + .prepareUpdateSettings("test") + .setSettings(Settings.builder().put("index.internal", "internal-update")) + .get()); + final String message = "can not update internal setting [index.internal]; this setting is managed via a dedicated API"; + assertThat(e, hasToString(containsString(message))); + final GetSettingsResponse responseAfterAttemptedUpdate = client().admin().indices().prepareGetSettings("test").get(); + assertThat(responseAfterAttemptedUpdate.getSetting("test", "index.internal"), equalTo("internal")); + } + + public void testUpdateInternalIndexSettingViaDedicatedAPI() { + final Settings settings = Settings.builder().put("index.internal", "internal").build(); + createIndex("test", settings); + final GetSettingsResponse response = client().admin().indices().prepareGetSettings("test").get(); + assertThat(response.getSetting("test", "index.internal"), equalTo("internal")); + client().execute( + InternalIndexSettingsPlugin.UpdateInternalIndexAction.INSTANCE, + new InternalIndexSettingsPlugin.UpdateInternalIndexAction.Request("test", "internal-update")) + .actionGet(); + final GetSettingsResponse responseAfterUpdate = client().admin().indices().prepareGetSettings("test").get(); + assertThat(responseAfterUpdate.getSetting("test", "index.internal"), equalTo("internal-update")); + } + } From 88f44a9f66b61fb158b3f4605a57909c86037489 Mon Sep 17 00:00:00 2001 From: David Kyle Date: Wed, 13 Jun 2018 15:42:18 +0100 Subject: [PATCH 10/11] [ML] Check licence when datafeeds use cross cluster search (#31247) This change prevents a datafeed using cross cluster search from starting if the remote cluster does not have x-pack installed and a sufficient license. The check is made only when starting a datafeed. --- .../core/ml/datafeed/DatafeedConfigTests.java | 25 ++- .../action/TransportStartDatafeedAction.java | 127 +++++++---- .../ml/datafeed/DatafeedNodeSelector.java | 6 +- .../ml/datafeed/MlRemoteLicenseChecker.java | 192 +++++++++++++++++ .../process/autodetect/AutodetectProcess.java | 2 +- .../datafeed/MlRemoteLicenseCheckerTests.java | 200 ++++++++++++++++++ 6 files changed, 493 insertions(+), 59 deletions(-) create mode 100644 x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseChecker.java create mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseCheckerTests.java diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java index 6aa987fc0e932..d59ef16dfdf2c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java @@ -40,7 +40,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.TimeZone; @@ -193,11 +192,11 @@ public void testDefaults() { public void testDefaultQueryDelay() { DatafeedConfig.Builder feedBuilder1 = new DatafeedConfig.Builder("datafeed1", "job1"); - feedBuilder1.setIndices(Arrays.asList("foo")); + feedBuilder1.setIndices(Collections.singletonList("foo")); DatafeedConfig.Builder feedBuilder2 = new DatafeedConfig.Builder("datafeed2", "job1"); - feedBuilder2.setIndices(Arrays.asList("foo")); + feedBuilder2.setIndices(Collections.singletonList("foo")); DatafeedConfig.Builder feedBuilder3 = new DatafeedConfig.Builder("datafeed3", "job2"); - feedBuilder3.setIndices(Arrays.asList("foo")); + feedBuilder3.setIndices(Collections.singletonList("foo")); DatafeedConfig feed1 = feedBuilder1.build(); DatafeedConfig feed2 = feedBuilder2.build(); DatafeedConfig feed3 = feedBuilder3.build(); @@ -208,19 +207,19 @@ public void testDefaultQueryDelay() { assertThat(feed1.getQueryDelay(), not(equalTo(feed3.getQueryDelay()))); } - public void testCheckValid_GivenNullIndices() throws IOException { + public void testCheckValid_GivenNullIndices() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); expectThrows(IllegalArgumentException.class, () -> conf.setIndices(null)); } - public void testCheckValid_GivenEmptyIndices() throws IOException { + public void testCheckValid_GivenEmptyIndices() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); conf.setIndices(Collections.emptyList()); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, conf::build); assertEquals(Messages.getMessage(Messages.DATAFEED_CONFIG_INVALID_OPTION_VALUE, "indices", "[]"), e.getMessage()); } - public void testCheckValid_GivenIndicesContainsOnlyNulls() throws IOException { + public void testCheckValid_GivenIndicesContainsOnlyNulls() { List indices = new ArrayList<>(); indices.add(null); indices.add(null); @@ -230,7 +229,7 @@ public void testCheckValid_GivenIndicesContainsOnlyNulls() throws IOException { assertEquals(Messages.getMessage(Messages.DATAFEED_CONFIG_INVALID_OPTION_VALUE, "indices", "[null, null]"), e.getMessage()); } - public void testCheckValid_GivenIndicesContainsOnlyEmptyStrings() throws IOException { + public void testCheckValid_GivenIndicesContainsOnlyEmptyStrings() { List indices = new ArrayList<>(); indices.add(""); indices.add(""); @@ -240,27 +239,27 @@ public void testCheckValid_GivenIndicesContainsOnlyEmptyStrings() throws IOExcep assertEquals(Messages.getMessage(Messages.DATAFEED_CONFIG_INVALID_OPTION_VALUE, "indices", "[, ]"), e.getMessage()); } - public void testCheckValid_GivenNegativeQueryDelay() throws IOException { + public void testCheckValid_GivenNegativeQueryDelay() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, () -> conf.setQueryDelay(TimeValue.timeValueMillis(-10))); assertEquals("query_delay cannot be less than 0. Value = -10", e.getMessage()); } - public void testCheckValid_GivenZeroFrequency() throws IOException { + public void testCheckValid_GivenZeroFrequency() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, () -> conf.setFrequency(TimeValue.ZERO)); assertEquals("frequency cannot be less or equal than 0. Value = 0s", e.getMessage()); } - public void testCheckValid_GivenNegativeFrequency() throws IOException { + public void testCheckValid_GivenNegativeFrequency() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, () -> conf.setFrequency(TimeValue.timeValueMinutes(-1))); assertEquals("frequency cannot be less or equal than 0. Value = -1", e.getMessage()); } - public void testCheckValid_GivenNegativeScrollSize() throws IOException { + public void testCheckValid_GivenNegativeScrollSize() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, () -> conf.setScrollSize(-1000)); assertEquals(Messages.getMessage(Messages.DATAFEED_CONFIG_INVALID_OPTION_VALUE, "scroll_size", -1000L), e.getMessage()); @@ -414,7 +413,7 @@ public void testDefaultFrequency_GivenNegative() { public void testDefaultFrequency_GivenNoAggregations() { DatafeedConfig.Builder datafeedBuilder = new DatafeedConfig.Builder("feed", "job"); - datafeedBuilder.setIndices(Arrays.asList("my_index")); + datafeedBuilder.setIndices(Collections.singletonList("my_index")); DatafeedConfig datafeed = datafeedBuilder.build(); assertEquals(TimeValue.timeValueMinutes(1), datafeed.defaultFrequency(TimeValue.timeValueSeconds(1))); diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java index bed83ed82c1c9..3d261864ab409 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java @@ -43,10 +43,12 @@ import org.elasticsearch.persistent.PersistentTasksExecutor; import org.elasticsearch.persistent.PersistentTasksService; import org.elasticsearch.xpack.ml.MachineLearning; +import org.elasticsearch.xpack.ml.datafeed.MlRemoteLicenseChecker; import org.elasticsearch.xpack.ml.datafeed.DatafeedManager; import org.elasticsearch.xpack.ml.datafeed.DatafeedNodeSelector; import org.elasticsearch.xpack.ml.datafeed.extractor.DataExtractorFactory; +import java.util.List; import java.util.Map; import java.util.function.Predicate; @@ -111,23 +113,25 @@ protected void masterOperation(StartDatafeedAction.Request request, ClusterState ActionListener listener) { StartDatafeedAction.DatafeedParams params = request.getParams(); if (licenseState.isMachineLearningAllowed()) { - ActionListener> finalListener = + + ActionListener> waitForTaskListener = new ActionListener>() { - @Override - public void onResponse(PersistentTasksCustomMetaData.PersistentTask persistentTask) { - waitForDatafeedStarted(persistentTask.getId(), params, listener); - } + @Override + public void onResponse(PersistentTasksCustomMetaData.PersistentTask + persistentTask) { + waitForDatafeedStarted(persistentTask.getId(), params, listener); + } - @Override - public void onFailure(Exception e) { - if (e instanceof ResourceAlreadyExistsException) { - logger.debug("datafeed already started", e); - e = new ElasticsearchStatusException("cannot start datafeed [" + params.getDatafeedId() + - "] because it has already been started", RestStatus.CONFLICT); - } - listener.onFailure(e); - } - }; + @Override + public void onFailure(Exception e) { + if (e instanceof ResourceAlreadyExistsException) { + logger.debug("datafeed already started", e); + e = new ElasticsearchStatusException("cannot start datafeed [" + params.getDatafeedId() + + "] because it has already been started", RestStatus.CONFLICT); + } + listener.onFailure(e); + } + }; // Verify data extractor factory can be created, then start persistent task MlMetadata mlMetadata = MlMetadata.getMlMetadata(state); @@ -135,16 +139,39 @@ public void onFailure(Exception e) { validate(params.getDatafeedId(), mlMetadata, tasks); DatafeedConfig datafeed = mlMetadata.getDatafeed(params.getDatafeedId()); Job job = mlMetadata.getJobs().get(datafeed.getJobId()); - DataExtractorFactory.create(client, datafeed, job, ActionListener.wrap( - dataExtractorFactory -> - persistentTasksService.sendStartRequest(MLMetadataField.datafeedTaskId(params.getDatafeedId()), - StartDatafeedAction.TASK_NAME, params, finalListener) - , listener::onFailure)); + + if (MlRemoteLicenseChecker.containsRemoteIndex(datafeed.getIndices())) { + MlRemoteLicenseChecker remoteLicenseChecker = new MlRemoteLicenseChecker(client); + remoteLicenseChecker.checkRemoteClusterLicenses(MlRemoteLicenseChecker.remoteClusterNames(datafeed.getIndices()), + ActionListener.wrap( + response -> { + if (response.isViolated()) { + listener.onFailure(createUnlicensedError(datafeed.getId(), response)); + } else { + createDataExtractor(job, datafeed, params, waitForTaskListener); + } + }, + e -> listener.onFailure(createUnknownLicenseError(datafeed.getId(), + MlRemoteLicenseChecker.remoteIndices(datafeed.getIndices()), e)) + )); + } else { + createDataExtractor(job, datafeed, params, waitForTaskListener); + } } else { listener.onFailure(LicenseUtils.newComplianceException(XPackField.MACHINE_LEARNING)); } } + private void createDataExtractor(Job job, DatafeedConfig datafeed, StartDatafeedAction.DatafeedParams params, + ActionListener> + listener) { + DataExtractorFactory.create(client, datafeed, job, ActionListener.wrap( + dataExtractorFactory -> + persistentTasksService.sendStartRequest(MLMetadataField.datafeedTaskId(params.getDatafeedId()), + StartDatafeedAction.TASK_NAME, params, listener) + , listener::onFailure)); + } + @Override protected ClusterBlockException checkBlock(StartDatafeedAction.Request request, ClusterState state) { // We only delegate here to PersistentTasksService, but if there is a metadata writeblock, @@ -158,28 +185,29 @@ private void waitForDatafeedStarted(String taskId, StartDatafeedAction.DatafeedP DatafeedPredicate predicate = new DatafeedPredicate(); persistentTasksService.waitForPersistentTaskCondition(taskId, predicate, params.getTimeout(), new PersistentTasksService.WaitForPersistentTaskListener() { - @Override - public void onResponse(PersistentTasksCustomMetaData.PersistentTask persistentTask) { - if (predicate.exception != null) { - // We want to return to the caller without leaving an unassigned persistent task, to match - // what would have happened if the error had been detected in the "fast fail" validation - cancelDatafeedStart(persistentTask, predicate.exception, listener); - } else { - listener.onResponse(new StartDatafeedAction.Response(true)); - } - } + @Override + public void onResponse(PersistentTasksCustomMetaData.PersistentTask + persistentTask) { + if (predicate.exception != null) { + // We want to return to the caller without leaving an unassigned persistent task, to match + // what would have happened if the error had been detected in the "fast fail" validation + cancelDatafeedStart(persistentTask, predicate.exception, listener); + } else { + listener.onResponse(new StartDatafeedAction.Response(true)); + } + } - @Override - public void onFailure(Exception e) { - listener.onFailure(e); - } + @Override + public void onFailure(Exception e) { + listener.onFailure(e); + } - @Override - public void onTimeout(TimeValue timeout) { - listener.onFailure(new ElasticsearchException("Starting datafeed [" - + params.getDatafeedId() + "] timed out after [" + timeout + "]")); - } - }); + @Override + public void onTimeout(TimeValue timeout) { + listener.onFailure(new ElasticsearchException("Starting datafeed [" + + params.getDatafeedId() + "] timed out after [" + timeout + "]")); + } + }); } private void cancelDatafeedStart(PersistentTasksCustomMetaData.PersistentTask persistentTask, @@ -203,6 +231,25 @@ public void onFailure(Exception e) { ); } + private ElasticsearchStatusException createUnlicensedError(String datafeedId, + MlRemoteLicenseChecker.LicenseViolation licenseViolation) { + String message = "Cannot start datafeed [" + datafeedId + "] as it is configured to use " + + "indices on a remote cluster [" + licenseViolation.get().getClusterName() + + "] that is not licensed for Machine Learning. " + + MlRemoteLicenseChecker.buildErrorMessage(licenseViolation.get()); + + return new ElasticsearchStatusException(message, RestStatus.BAD_REQUEST); + } + + private ElasticsearchStatusException createUnknownLicenseError(String datafeedId, List remoteIndices, + Exception cause) { + String message = "Cannot start datafeed [" + datafeedId + "] as it is configured to use" + + " indices on a remote cluster " + remoteIndices + + " but the license type could not be verified"; + + return new ElasticsearchStatusException(message, RestStatus.BAD_REQUEST, new Exception(cause.getMessage())); + } + public static class StartDatafeedPersistentTasksExecutor extends PersistentTasksExecutor { private final DatafeedManager datafeedManager; private final IndexNameExpressionResolver resolver; diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedNodeSelector.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedNodeSelector.java index 37f9715d09464..0eb57ab79be5d 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedNodeSelector.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedNodeSelector.java @@ -91,7 +91,7 @@ private AssignmentFailure verifyIndicesActive(DatafeedConfig datafeed) { List indices = datafeed.getIndices(); for (String index : indices) { - if (isRemoteIndex(index)) { + if (MlRemoteLicenseChecker.isRemoteIndex(index)) { // We cannot verify remote indices continue; } @@ -122,10 +122,6 @@ private AssignmentFailure verifyIndicesActive(DatafeedConfig datafeed) { return null; } - private boolean isRemoteIndex(String index) { - return index.indexOf(':') != -1; - } - private static class AssignmentFailure { private final String reason; private final boolean isCriticalForTaskCreation; diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseChecker.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseChecker.java new file mode 100644 index 0000000000000..b55713f6d0ab7 --- /dev/null +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseChecker.java @@ -0,0 +1,192 @@ +/* + * 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.ml.datafeed; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.client.Client; +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.license.License; +import org.elasticsearch.license.XPackInfoResponse; +import org.elasticsearch.transport.ActionNotFoundTransportException; +import org.elasticsearch.transport.RemoteClusterAware; +import org.elasticsearch.xpack.core.action.XPackInfoAction; +import org.elasticsearch.xpack.core.action.XPackInfoRequest; + +import java.util.EnumSet; +import java.util.Iterator; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; + +/** + * ML datafeeds can use cross cluster search to access data in a remote cluster. + * The remote cluster should be licenced for ML this class performs that check + * using the _xpack (info) endpoint. + */ +public class MlRemoteLicenseChecker { + + private final Client client; + + public static class RemoteClusterLicenseInfo { + private final String clusterName; + private final XPackInfoResponse.LicenseInfo licenseInfo; + + RemoteClusterLicenseInfo(String clusterName, XPackInfoResponse.LicenseInfo licenseInfo) { + this.clusterName = clusterName; + this.licenseInfo = licenseInfo; + } + + public String getClusterName() { + return clusterName; + } + + public XPackInfoResponse.LicenseInfo getLicenseInfo() { + return licenseInfo; + } + } + + public class LicenseViolation { + private final RemoteClusterLicenseInfo licenseInfo; + + private LicenseViolation(@Nullable RemoteClusterLicenseInfo licenseInfo) { + this.licenseInfo = licenseInfo; + } + + public boolean isViolated() { + return licenseInfo != null; + } + + public RemoteClusterLicenseInfo get() { + return licenseInfo; + } + } + + public MlRemoteLicenseChecker(Client client) { + this.client = client; + } + + /** + * Check each cluster is licensed for ML. + * This function evaluates lazily and will terminate when the first cluster + * that is not licensed is found or an error occurs. + * + * @param clusterNames List of remote cluster names + * @param listener Response listener + */ + public void checkRemoteClusterLicenses(List clusterNames, ActionListener listener) { + final Iterator itr = clusterNames.iterator(); + if (itr.hasNext() == false) { + listener.onResponse(new LicenseViolation(null)); + return; + } + + final AtomicReference clusterName = new AtomicReference<>(itr.next()); + + ActionListener infoListener = new ActionListener() { + @Override + public void onResponse(XPackInfoResponse xPackInfoResponse) { + if (licenseSupportsML(xPackInfoResponse.getLicenseInfo()) == false) { + listener.onResponse(new LicenseViolation( + new RemoteClusterLicenseInfo(clusterName.get(), xPackInfoResponse.getLicenseInfo()))); + return; + } + + if (itr.hasNext()) { + clusterName.set(itr.next()); + remoteClusterLicense(clusterName.get(), this); + } else { + listener.onResponse(new LicenseViolation(null)); + } + } + + @Override + public void onFailure(Exception e) { + String message = "Could not determine the X-Pack licence type for cluster [" + clusterName.get() + "]"; + if (e instanceof ActionNotFoundTransportException) { + // This is likely to be because x-pack is not installed in the target cluster + message += ". Is X-Pack installed on the target cluster?"; + } + listener.onFailure(new ElasticsearchException(message, e)); + } + }; + + remoteClusterLicense(clusterName.get(), infoListener); + } + + private void remoteClusterLicense(String clusterName, ActionListener listener) { + Client remoteClusterClient = client.getRemoteClusterClient(clusterName); + ThreadContext threadContext = remoteClusterClient.threadPool().getThreadContext(); + try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + // we stash any context here since this is an internal execution and should not leak any + // existing context information. + threadContext.markAsSystemContext(); + + XPackInfoRequest request = new XPackInfoRequest(); + request.setCategories(EnumSet.of(XPackInfoRequest.Category.LICENSE)); + remoteClusterClient.execute(XPackInfoAction.INSTANCE, request, listener); + } + } + + static boolean licenseSupportsML(XPackInfoResponse.LicenseInfo licenseInfo) { + License.OperationMode mode = License.OperationMode.resolve(licenseInfo.getMode()); + return licenseInfo.getStatus() == License.Status.ACTIVE && + (mode == License.OperationMode.PLATINUM || mode == License.OperationMode.TRIAL); + } + + public static boolean isRemoteIndex(String index) { + return index.indexOf(RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR) != -1; + } + + public static boolean containsRemoteIndex(List indices) { + return indices.stream().anyMatch(MlRemoteLicenseChecker::isRemoteIndex); + } + + /** + * Get any remote indices used in cross cluster search. + * Remote indices are of the form {@code cluster_name:index_name} + * @return List of remote cluster indices + */ + public static List remoteIndices(List indices) { + return indices.stream().filter(MlRemoteLicenseChecker::isRemoteIndex).collect(Collectors.toList()); + } + + /** + * Extract the list of remote cluster names from the list of indices. + * @param indices List of indices. Remote cluster indices are prefixed + * with {@code cluster-name:} + * @return Every cluster name found in {@code indices} + */ + public static List remoteClusterNames(List indices) { + return indices.stream() + .filter(MlRemoteLicenseChecker::isRemoteIndex) + .map(index -> index.substring(0, index.indexOf(RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR))) + .distinct() + .collect(Collectors.toList()); + } + + public static String buildErrorMessage(RemoteClusterLicenseInfo clusterLicenseInfo) { + StringBuilder error = new StringBuilder(); + if (clusterLicenseInfo.licenseInfo.getStatus() != License.Status.ACTIVE) { + error.append("The license on cluster [").append(clusterLicenseInfo.clusterName) + .append("] is not active. "); + } else { + License.OperationMode mode = License.OperationMode.resolve(clusterLicenseInfo.licenseInfo.getMode()); + if (mode != License.OperationMode.PLATINUM && mode != License.OperationMode.TRIAL) { + error.append("The license mode [").append(mode) + .append("] on cluster [") + .append(clusterLicenseInfo.clusterName) + .append("] does not enable Machine Learning. "); + } + } + + error.append(Strings.toString(clusterLicenseInfo.licenseInfo)); + return error.toString(); + } +} diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcess.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcess.java index 049880b1ac224..21be815d561a8 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcess.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcess.java @@ -117,7 +117,7 @@ void writeUpdateDetectorRulesMessage(int detectorIndex, List rule /** * Ask the job to start persisting model state in the background - * @throws IOException + * @throws IOException If writing the request fails */ void persistJob() throws IOException; diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseCheckerTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseCheckerTests.java new file mode 100644 index 0000000000000..47d4d30a7c6e4 --- /dev/null +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/MlRemoteLicenseCheckerTests.java @@ -0,0 +1,200 @@ +/* + * 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.ml.datafeed; + +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.client.Client; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.license.License; +import org.elasticsearch.license.XPackInfoResponse; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.xpack.core.action.XPackInfoAction; +import org.elasticsearch.xpack.core.ml.datafeed.DatafeedConfig; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.is; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.same; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class MlRemoteLicenseCheckerTests extends ESTestCase { + + public void testIsRemoteIndex() { + List indices = Arrays.asList("local-index1", "local-index2"); + assertFalse(MlRemoteLicenseChecker.containsRemoteIndex(indices)); + indices = Arrays.asList("local-index1", "remote-cluster:remote-index2"); + assertTrue(MlRemoteLicenseChecker.containsRemoteIndex(indices)); + } + + public void testRemoteIndices() { + List indices = Collections.singletonList("local-index"); + assertThat(MlRemoteLicenseChecker.remoteIndices(indices), is(empty())); + indices = Arrays.asList("local-index", "remote-cluster:index1", "local-index2", "remote-cluster2:index1"); + assertThat(MlRemoteLicenseChecker.remoteIndices(indices), containsInAnyOrder("remote-cluster:index1", "remote-cluster2:index1")); + } + + public void testRemoteClusterNames() { + List indices = Arrays.asList("local-index1", "local-index2"); + assertThat(MlRemoteLicenseChecker.remoteClusterNames(indices), empty()); + indices = Arrays.asList("local-index1", "remote-cluster1:remote-index2"); + assertThat(MlRemoteLicenseChecker.remoteClusterNames(indices), contains("remote-cluster1")); + indices = Arrays.asList("remote-cluster1:index2", "index1", "remote-cluster2:index1"); + assertThat(MlRemoteLicenseChecker.remoteClusterNames(indices), contains("remote-cluster1", "remote-cluster2")); + indices = Arrays.asList("remote-cluster1:index2", "index1", "remote-cluster2:index1", "remote-cluster2:index2"); + assertThat(MlRemoteLicenseChecker.remoteClusterNames(indices), contains("remote-cluster1", "remote-cluster2")); + } + + public void testLicenseSupportsML() { + XPackInfoResponse.LicenseInfo licenseInfo = new XPackInfoResponse.LicenseInfo("uid", "trial", "trial", + License.Status.ACTIVE, randomNonNegativeLong()); + assertTrue(MlRemoteLicenseChecker.licenseSupportsML(licenseInfo)); + + licenseInfo = new XPackInfoResponse.LicenseInfo("uid", "trial", "trial", License.Status.EXPIRED, randomNonNegativeLong()); + assertFalse(MlRemoteLicenseChecker.licenseSupportsML(licenseInfo)); + + licenseInfo = new XPackInfoResponse.LicenseInfo("uid", "GOLD", "GOLD", License.Status.ACTIVE, randomNonNegativeLong()); + assertFalse(MlRemoteLicenseChecker.licenseSupportsML(licenseInfo)); + + licenseInfo = new XPackInfoResponse.LicenseInfo("uid", "PLATINUM", "PLATINUM", License.Status.ACTIVE, randomNonNegativeLong()); + assertTrue(MlRemoteLicenseChecker.licenseSupportsML(licenseInfo)); + } + + public void testCheckRemoteClusterLicenses_givenValidLicenses() { + final AtomicInteger index = new AtomicInteger(0); + final List responses = new ArrayList<>(); + + Client client = createMockClient(); + doAnswer(invocationMock -> { + @SuppressWarnings("raw_types") + ActionListener listener = (ActionListener) invocationMock.getArguments()[2]; + listener.onResponse(responses.get(index.getAndIncrement())); + return null; + }).when(client).execute(same(XPackInfoAction.INSTANCE), any(), any()); + + + List remoteClusterNames = Arrays.asList("valid1", "valid2", "valid3"); + responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null)); + responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null)); + responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null)); + + MlRemoteLicenseChecker licenseChecker = new MlRemoteLicenseChecker(client); + AtomicReference licCheckResponse = new AtomicReference<>(); + + licenseChecker.checkRemoteClusterLicenses(remoteClusterNames, + new ActionListener() { + @Override + public void onResponse(MlRemoteLicenseChecker.LicenseViolation response) { + licCheckResponse.set(response); + } + + @Override + public void onFailure(Exception e) { + fail(e.getMessage()); + } + }); + + verify(client, times(3)).execute(same(XPackInfoAction.INSTANCE), any(), any()); + assertNotNull(licCheckResponse.get()); + assertFalse(licCheckResponse.get().isViolated()); + assertNull(licCheckResponse.get().get()); + } + + public void testCheckRemoteClusterLicenses_givenInvalidLicense() { + final AtomicInteger index = new AtomicInteger(0); + List remoteClusterNames = Arrays.asList("good", "cluster-with-basic-license", "good2"); + final List responses = new ArrayList<>(); + responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null)); + responses.add(new XPackInfoResponse(null, createBasicLicenseResponse(), null)); + responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null)); + + Client client = createMockClient(); + doAnswer(invocationMock -> { + @SuppressWarnings("raw_types") + ActionListener listener = (ActionListener) invocationMock.getArguments()[2]; + listener.onResponse(responses.get(index.getAndIncrement())); + return null; + }).when(client).execute(same(XPackInfoAction.INSTANCE), any(), any()); + + MlRemoteLicenseChecker licenseChecker = new MlRemoteLicenseChecker(client); + AtomicReference licCheckResponse = new AtomicReference<>(); + + licenseChecker.checkRemoteClusterLicenses(remoteClusterNames, + new ActionListener() { + @Override + public void onResponse(MlRemoteLicenseChecker.LicenseViolation response) { + licCheckResponse.set(response); + } + + @Override + public void onFailure(Exception e) { + fail(e.getMessage()); + } + }); + + verify(client, times(2)).execute(same(XPackInfoAction.INSTANCE), any(), any()); + assertNotNull(licCheckResponse.get()); + assertTrue(licCheckResponse.get().isViolated()); + assertEquals("cluster-with-basic-license", licCheckResponse.get().get().getClusterName()); + assertEquals("BASIC", licCheckResponse.get().get().getLicenseInfo().getType()); + } + + public void testBuildErrorMessage() { + XPackInfoResponse.LicenseInfo platinumLicence = createPlatinumLicenseResponse(); + MlRemoteLicenseChecker.RemoteClusterLicenseInfo info = + new MlRemoteLicenseChecker.RemoteClusterLicenseInfo("platinum-cluster", platinumLicence); + assertEquals(Strings.toString(platinumLicence), MlRemoteLicenseChecker.buildErrorMessage(info)); + + XPackInfoResponse.LicenseInfo basicLicense = createBasicLicenseResponse(); + info = new MlRemoteLicenseChecker.RemoteClusterLicenseInfo("basic-cluster", basicLicense); + String expected = "The license mode [BASIC] on cluster [basic-cluster] does not enable Machine Learning. " + + Strings.toString(basicLicense); + assertEquals(expected, MlRemoteLicenseChecker.buildErrorMessage(info)); + + XPackInfoResponse.LicenseInfo expiredLicense = createExpiredLicenseResponse(); + info = new MlRemoteLicenseChecker.RemoteClusterLicenseInfo("expired-cluster", expiredLicense); + expected = "The license on cluster [expired-cluster] is not active. " + Strings.toString(expiredLicense); + assertEquals(expected, MlRemoteLicenseChecker.buildErrorMessage(info)); + } + + private Client createMockClient() { + Client client = mock(Client.class); + ThreadPool threadPool = mock(ThreadPool.class); + when(client.threadPool()).thenReturn(threadPool); + when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY)); + when(client.getRemoteClusterClient(anyString())).thenReturn(client); + return client; + } + + private XPackInfoResponse.LicenseInfo createPlatinumLicenseResponse() { + return new XPackInfoResponse.LicenseInfo("uid", "PLATINUM", "PLATINUM", License.Status.ACTIVE, randomNonNegativeLong()); + } + + private XPackInfoResponse.LicenseInfo createBasicLicenseResponse() { + return new XPackInfoResponse.LicenseInfo("uid", "BASIC", "BASIC", License.Status.ACTIVE, randomNonNegativeLong()); + } + + private XPackInfoResponse.LicenseInfo createExpiredLicenseResponse() { + return new XPackInfoResponse.LicenseInfo("uid", "PLATINUM", "PLATINUM", License.Status.EXPIRED, randomNonNegativeLong()); + } +} From a486177a19171ab408cb88c1b1ef8d75ef2427fd Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Wed, 13 Jun 2018 11:31:04 -0400 Subject: [PATCH 11/11] [Rollup] Metric config parser must use builder so validation runs (#31159) The parser for the Metric config was directly instantiating the config object, rather than using the builder. That means it was bypassing the validation logic built into the builder, and would allow users to create invalid metric configs (like using unsupported metrics). The job would later blow up and abort due to bad configs, but this isn't immediately obvious to the user since the PutJob API succeeded. --- .../xpack/core/rollup/job/MetricConfig.java | 11 ++++--- .../core/rollup/job/RollupJobConfig.java | 2 +- .../job/MetricsConfigSerializingTests.java | 4 +-- .../rest-api-spec/test/rollup/put_job.yml | 30 +++++++++++++++++++ 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java index f26c67935edff..67b83646c4237 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java @@ -12,7 +12,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.mapper.NumberFieldMapper; @@ -75,12 +75,11 @@ public class MetricConfig implements Writeable, ToXContentFragment { MAPPER_TYPES = types; } - public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - NAME, a -> new MetricConfig((String)a[0], (List) a[1])); + public static final ObjectParser PARSER = new ObjectParser<>(NAME, MetricConfig.Builder::new); static { - PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELD); - PARSER.declareStringArray(ConstructingObjectParser.constructorArg(), METRICS); + PARSER.declareString(MetricConfig.Builder::setField, FIELD); + PARSER.declareStringArray(MetricConfig.Builder::setMetrics, METRICS); } MetricConfig(String name, List metrics) { @@ -257,4 +256,4 @@ public MetricConfig build() { return new MetricConfig(field, metrics); } } -} \ No newline at end of file +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java index 3818ebcf44758..422ecdd5fd9fb 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java @@ -63,7 +63,7 @@ public class RollupJobConfig implements NamedWriteable, ToXContentObject { static { PARSER.declareString(RollupJobConfig.Builder::setId, RollupField.ID); PARSER.declareObject(RollupJobConfig.Builder::setGroupConfig, (p, c) -> GroupConfig.PARSER.apply(p,c).build(), GROUPS); - PARSER.declareObjectArray(RollupJobConfig.Builder::setMetricsConfig, MetricConfig.PARSER, METRICS); + PARSER.declareObjectArray(RollupJobConfig.Builder::setMetricsConfig, (p, c) -> MetricConfig.PARSER.apply(p, c).build(), METRICS); PARSER.declareString((params, val) -> params.setTimeout(TimeValue.parseTimeValue(val, TIMEOUT.getPreferredName())), TIMEOUT); PARSER.declareString(RollupJobConfig.Builder::setIndexPattern, INDEX_PATTERN); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/MetricsConfigSerializingTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/MetricsConfigSerializingTests.java index 92a0976f532b7..9b330e7165093 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/MetricsConfigSerializingTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/MetricsConfigSerializingTests.java @@ -24,7 +24,7 @@ public class MetricsConfigSerializingTests extends AbstractSerializingTestCase { @Override protected MetricConfig doParseInstance(XContentParser parser) throws IOException { - return MetricConfig.PARSER.apply(parser, null); + return MetricConfig.PARSER.apply(parser, null).build(); } @Override @@ -36,7 +36,7 @@ protected Writeable.Reader instanceReader() { protected MetricConfig createTestInstance() { return ConfigTestHelpers.getMetricConfig().build(); } - + public void testValidateNoMapping() throws IOException { ActionRequestValidationException e = new ActionRequestValidationException(); Map> responseMap = new HashMap<>(); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml index 717be0d6b250f..98ef9b32e3d29 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml @@ -188,3 +188,33 @@ setup: ] } +--- +"Unknown Metric": + + - do: + catch: /Unsupported metric \[does_not_exist\]/ + headers: + Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser + xpack.rollup.put_job: + id: foo + body: > + { + "index_pattern": "foo", + "rollup_index": "foo_rollup", + "cron": "*/30 * * * * ?", + "page_size" :10, + "groups" : { + "date_histogram": { + "field": "the_field", + "interval": "1h" + } + }, + "metrics": [ + { + "field": "value_field", + "metrics": ["min", "max", "sum", "does_not_exist"] + } + ] + } + +