[7.x] [ML] Assert mappings match templates in Upgrade tests (#61905)

At the end of the rolling upgrade tests check the mappings of the concrete
.ml and .transform-internal indices match the mappings in the templates.
When the templates change, the tests should prove that the mappings have
been updated in the new cluster.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/persistence/InferenceIndexConstants.java

@@ -13,6 +13,9 @@ import org.elasticsearch.common.ParseField;
 public final class InferenceIndexConstants {
 
     /**
+     * When incrementing the index version please also update
+     * any use of the constant in x-pack/qa/
+     *
      * version: 7.8.0:
      *  - adds inference_config definition to trained model config
      *

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/persistence/TransformInternalIndexConstants.java

@@ -19,6 +19,7 @@ public final class TransformInternalIndexConstants {
      *
      *    - XPackRestTestConstants
      *    - yaml tests under x-pack/qa/
+     *    - upgrade tests under x-pack/qa/rolling-upgrade
      *
      * (pro-tip: grep for the constant)
      */

x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/config_index_mappings.json

@@ -334,6 +334,9 @@
       "max_num_threads" : {
         "type" : "integer"
       },
+      "model_memory_limit": {
+        "type" : "keyword"
+      },
       "model_plot_config" : {
         "properties" : {
           "enabled" : {

x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/persistence/TransformInternalIndex.java

@@ -288,7 +288,7 @@ public final class TransformInternalIndex {
             .field(TYPE, KEYWORD)
             .endObject()
             .startObject(SourceConfig.QUERY.getPreferredName())
-            .field(ENABLED, "false")
+            .field(ENABLED, false)
             .endObject()
             .endObject()
             .endObject()

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/AbstractUpgradeTestCase.java

@@ -8,17 +8,28 @@ package org.elasticsearch.upgrades;
 import org.elasticsearch.Version;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
+import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
+import org.elasticsearch.common.xcontent.support.XContentMapValues;
 import org.elasticsearch.test.rest.ESRestTestCase;
 import org.elasticsearch.xpack.test.SecuritySettingsSourceField;
 import org.junit.Before;
 
+import java.io.IOException;
+import java.util.AbstractMap;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeMap;
+import java.util.TreeSet;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static org.elasticsearch.xpack.test.SecuritySettingsSourceField.basicAuthHeaderValue;
 
@@ -116,4 +127,199 @@ public abstract class AbstractUpgradeTestCase extends ESRestTestCase {
             }
         });
     }
+
+    /**
+     * Compares the mappings from the template and the index and asserts they
+     * are the same.
+     *
+     * The test is intended to catch cases where an index mapping has been
+     * updated dynamically or a write occurred before the template was put.
+     * The assertion error message details the differences in the mappings.
+     *
+     * The Mappings, which are maps of maps, are flattened with the keys built
+     * from the keys of the sub-maps appended to the parent key.
+     * This makes diffing the 2 maps easier and diffs more comprehensible.
+     *
+     * The _meta field is not compared as it contains version numbers that
+     * change even when the mappings don't.
+     *
+     * Mistakes happen and some indices may be stuck with the incorrect mappings
+     * that cannot be fixed without re-index. In this case use the {@code exceptions}
+     * parameter to filter out fields in the index mapping that are not in the
+     * template. Each exception should be a '.' separated path to the value
+     * e.g. {@code properties.analysis.analysis_field.type}.
+     *
+     * @param templateName The template
+     * @param indexName The index
+     * @param notAnErrorIfIndexDoesNotExist The index may or may not have been created from
+     *                                      the template. If {@code true} then the missing
+     *                                      index does not cause an error
+     * @param exceptions List of keys to ignore in the index mappings.
+     *                   The key is a '.' separated path.
+     * @throws IOException Yes
+     */
+    @SuppressWarnings("unchecked")
+    protected void assertLegacyTemplateMatchesIndexMappings(String templateName,
+                                                            String indexName,
+                                                            boolean notAnErrorIfIndexDoesNotExist,
+                                                            Set<String> exceptions) throws IOException {
+
+        Request getTemplate = new Request("GET", "_template/" + templateName);
+        Response templateResponse = client().performRequest(getTemplate);
+        assertEquals("missing template [" + templateName + "]", 200, templateResponse.getStatusLine().getStatusCode());
+
+        Map<String, Object> templateMappings = (Map<String, Object>) XContentMapValues.extractValue(entityAsMap(templateResponse),
+            templateName, "mappings");
+        assertNotNull(templateMappings);
+
+        Request getIndexMapping = new Request("GET", indexName + "/_mapping");
+        Response indexMappingResponse;
+        try {
+            indexMappingResponse = client().performRequest(getIndexMapping);
+        } catch (ResponseException e) {
+            if (e.getResponse().getStatusLine().getStatusCode() == 404 && notAnErrorIfIndexDoesNotExist) {
+                return;
+            } else {
+                throw e;
+            }
+        }
+        assertEquals("error getting mappings for index [" + indexName + "]",
+            200, indexMappingResponse.getStatusLine().getStatusCode());
+
+        Map<String, Object> indexMappings = (Map<String, Object>) XContentMapValues.extractValue(entityAsMap(indexMappingResponse),
+            indexName, "mappings");
+        assertNotNull(indexMappings);
+
+        // ignore the _meta field
+        indexMappings.remove("_meta");
+        templateMappings.remove("_meta");
+
+        // We cannot do a simple comparison of mappings e.g
+        // Objects.equals(indexMappings, templateMappings) because some
+        // templates use strings for the boolean values - "true" and "false"
+        // which are automatically converted to Booleans causing the equality
+        // to fail.
+        boolean mappingsAreTheSame = true;
+
+        // flatten the map of maps
+        Map<String, Object> flatTemplateMap = flattenMap(templateMappings);
+        Map<String, Object> flatIndexMap = flattenMap(indexMappings);
+
+        SortedSet<String> keysInTemplateMissingFromIndex = new TreeSet<>(flatTemplateMap.keySet());
+        keysInTemplateMissingFromIndex.removeAll(flatIndexMap.keySet());
+
+        SortedSet<String> keysInIndexMissingFromTemplate = new TreeSet<>(flatIndexMap.keySet());
+        keysInIndexMissingFromTemplate.removeAll(flatTemplateMap.keySet());
+
+        // In the case of object fields the 'type: object' mapping is set by default.
+        // If this does not explicitly appear in the template it is not an error
+        // as ES has added the default to the index mappings
+        keysInIndexMissingFromTemplate.removeIf(key -> key.endsWith("type") && "object".equals(flatIndexMap.get(key)));
+
+        // Remove the exceptions
+        keysInIndexMissingFromTemplate.removeAll(exceptions);
+
+        StringBuilder errorMesssage = new StringBuilder("Error the template mappings [")
+            .append(templateName)
+            .append("] and index mappings [")
+            .append(indexName)
+            .append("] are not the same")
+            .append(System.lineSeparator());
+
+        if (keysInTemplateMissingFromIndex.isEmpty() == false) {
+            mappingsAreTheSame = false;
+            errorMesssage.append("Keys in the template missing from the index mapping: ")
+                .append(keysInTemplateMissingFromIndex)
+                .append(System.lineSeparator());
+        }
+
+        if (keysInIndexMissingFromTemplate.isEmpty() == false) {
+            mappingsAreTheSame = false;
+            errorMesssage.append("Keys in the index missing from the template mapping: ")
+                .append(keysInIndexMissingFromTemplate)
+                .append(System.lineSeparator());
+        }
+
+        // find values that are different for the same key
+        Set<String> commonKeys = new TreeSet<>(flatIndexMap.keySet());
+        commonKeys.retainAll(flatTemplateMap.keySet());
+        for (String key : commonKeys) {
+            Object template = flatTemplateMap.get(key);
+            Object index = flatIndexMap.get(key);
+            if (Objects.equals(template, index) ==  false) {
+                // Both maybe be booleans but different representations
+                if (areBooleanObjectsAndEqual(index, template)) {
+                    continue;
+                }
+
+                mappingsAreTheSame = false;
+
+                errorMesssage.append("Values for key [").append(key).append("] are different").append(System.lineSeparator());
+                errorMesssage.append("    template value [").append(template).append("] ").append(template.getClass().getSimpleName())
+                    .append(System.lineSeparator());
+                errorMesssage.append("    index value [").append(index).append("] ").append(index.getClass().getSimpleName())
+                    .append(System.lineSeparator());
+            }
+        }
+
+        if (mappingsAreTheSame == false) {
+            fail(errorMesssage.toString());
+        }
+    }
+
+    protected void assertLegacyTemplateMatchesIndexMappings(String templateName,
+                                                            String indexName) throws IOException {
+        assertLegacyTemplateMatchesIndexMappings(templateName, indexName, false, Collections.emptySet());
+    }
+
+    private boolean areBooleanObjectsAndEqual(Object a, Object b) {
+        Boolean left;
+        Boolean right;
+
+        if (a instanceof Boolean) {
+            left = (Boolean)a;
+        } else if (a instanceof String && isBooleanValueString((String)a)) {
+            left = Boolean.parseBoolean((String)a);
+        } else {
+            return false;
+        }
+
+        if (b instanceof Boolean) {
+            right = (Boolean)b;
+        } else if (b instanceof String && isBooleanValueString((String)b)) {
+            right = Boolean.parseBoolean((String)b);
+        } else {
+            return false;
+        }
+
+        return left.equals(right);
+    }
+
+    /* Boolean.parseBoolean is not strict anything that isn't
+     * "true" is returned as false. Here we want to know if
+     * s is a boolean.
+     */
+    private boolean isBooleanValueString(String s) {
+        return s.equalsIgnoreCase("true") || s.equalsIgnoreCase("false");
+    }
+
+    private Map<String, Object> flattenMap(Map<String, Object> map) {
+        return new TreeMap<>(flatten("", map).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
+    }
+
+    private Stream<Map.Entry<String, Object>> flatten(String path, Map<String, Object> map) {
+        return map.entrySet()
+            .stream()
+            .flatMap((e) -> extractValue(path, e));
+    }
+
+    @SuppressWarnings("unchecked")
+    private Stream<Map.Entry<String, Object>> extractValue(String path, Map.Entry<String, Object> entry) {
+        String nextPath = path.isEmpty() ? entry.getKey() : path + "." + entry.getKey();
+        if (entry.getValue() instanceof Map<?, ?>) {
+            return flatten(nextPath, (Map<String, Object>) entry.getValue());
+        } else {
+            return Stream.of(new AbstractMap.SimpleEntry<>(nextPath, entry.getValue()));
+        }
+    }
 }

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/MlMappingsUpgradeIT.java

@@ -20,7 +20,9 @@ import org.elasticsearch.xpack.test.rest.XPackRestTestHelper;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -53,6 +55,7 @@ public class MlMappingsUpgradeIT extends AbstractUpgradeTestCase {
                 assertUpgradedResultsMappings();
                 closeAndReopenTestJob();
                 assertUpgradedConfigMappings();
+                assertMappingsMatchTemplates();
                 break;
             default:
                 throw new UnsupportedOperationException("Unknown cluster type [" + CLUSTER_TYPE + "]");
@@ -144,4 +147,45 @@ public class MlMappingsUpgradeIT extends AbstractUpgradeTestCase {
                 extractValue("mappings.properties.model_plot_config.properties.annotations_enabled.type", indexLevel));
         });
     }
+
+    /**
+     * Assert that the mappings of the ml indices are the same as in the
+     * templates. If different this is either a consequence of an unintended
+     * write (dynamic update) or the mappings have not been updated after
+     * upgrade.
+     *
+     * A failure here will be very difficult to reproduce as it may be a side
+     * effect of one of the other tests running in the cluster.
+     *
+     * @throws IOException On error
+     */
+    private void assertMappingsMatchTemplates() throws IOException {
+        // Keys that have been dynamically mapped in the .ml-config index
+        // but are not in the template. These can only be fixed with
+        // re-index and should be addressed at the next major upgrade.
+        // For now this serves as documentation of the missing fields
+        Set<String> configIndexExceptions = new HashSet<>();
+        configIndexExceptions.add("properties.allow_lazy_start.type");
+        configIndexExceptions.add("properties.analysis.properties.classification.properties.randomize_seed.type");
+        configIndexExceptions.add("properties.analysis.properties.outlier_detection.properties.compute_feature_influence.type");
+        configIndexExceptions.add("properties.analysis.properties.outlier_detection.properties.outlier_fraction.type");
+        configIndexExceptions.add("properties.analysis.properties.outlier_detection.properties.standardization_enabled.type");
+        configIndexExceptions.add("properties.analysis.properties.regression.properties.randomize_seed.type");
+        configIndexExceptions.add("properties.deleting.type");
+        configIndexExceptions.add("properties.model_memory_limit.type");
+        // fields from previous versions that have been removed
+        configIndexExceptions.add("properties.established_model_memory.type");
+        configIndexExceptions.add("properties.last_data_time.type");
+        configIndexExceptions.add("properties.types.type");
+
+        assertLegacyTemplateMatchesIndexMappings(".ml-config", ".ml-config", false, configIndexExceptions);
+        // the true parameter means the index may not have been created
+        assertLegacyTemplateMatchesIndexMappings(".ml-meta", ".ml-meta", true, Collections.emptySet());
+        assertLegacyTemplateMatchesIndexMappings(".ml-stats", ".ml-stats-000001", true, Collections.emptySet());
+        assertLegacyTemplateMatchesIndexMappings(".ml-state", ".ml-state-000001");
+        assertLegacyTemplateMatchesIndexMappings(".ml-notifications-000001", ".ml-notifications-000001");
+        assertLegacyTemplateMatchesIndexMappings(".ml-inference-000003", ".ml-inference-000003", true, Collections.emptySet());
+        // .ml-annotations-6 does not use a template
+        // .ml-anomalies-shared uses a template but will have dynamically updated mappings as new jobs are opened
+    }
 }

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TransformSurvivesUpgradeIT.java

@@ -157,6 +157,7 @@ public class TransformSurvivesUpgradeIT extends AbstractUpgradeTestCase {
             case UPGRADED:
                 client().performRequest(waitForYellow);
                 verifyContinuousTransformHandlesData(3);
+                assertLegacyTemplateMatchesIndexMappings(".transform-internal-005", ".transform-internal-005");
                 cleanUpTransforms();
                 break;
             default: