Merge pull request #29766 from vespa-engine/jonmv/reapply-keep-config-changes-in-dev

Jonmv/reapply keep config changes in dev

9 files changed, 172 insertions, 102 deletions

diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
index 692780f3eb3..363ab3918d3 100644
--- a/config-model-api/abi-spec.json
+++ b/config-model-api/abi-spec.json
@@ -771,7 +771,9 @@
     "attributes" : [
       "public"
     ],
-    "methods" : [ ],
+    "methods" : [
+      "public java.util.Map messagesById()"
+    ],
     "fields" : [ ]
   },
   "com.yahoo.config.application.api.ValidationOverrides" : {
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
index 1edddb63e52..7b52d825473 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
@@ -15,6 +15,7 @@ import java.time.format.DateTimeFormatter;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -36,7 +37,7 @@ public class ValidationOverrides {
 
     private final String xmlForm;
 
-    /** Creates a validation overrides which does not have an xml form */
+    /** Creates a validation overrides which does not have an XML form */
     public ValidationOverrides(List<Allow> overrides) {
         this(overrides, null);
     }
@@ -163,10 +164,13 @@ public class ValidationOverrides {
      */
     public static class ValidationException extends IllegalArgumentException {
 
+        private final Map<ValidationId, Collection<String>> messagesById = new LinkedHashMap<>();
+
         static final long serialVersionUID = 789984668;
 
         private ValidationException(ValidationId validationId, String message) {
             super(validationId + ": " + message + ". " + toAllowMessage(validationId));
+            messagesById.put(validationId, List.of(message));
         }
 
         private ValidationException(Map<ValidationId, Collection<String>> messagesById) {
@@ -175,8 +179,11 @@ public class ValidationOverrides {
                                                String.join("\n\t", messages.getValue()) + "\n" +
                                                toAllowMessage(messages.getKey()))
                               .collect(Collectors.joining("\n")));
+            messagesById.forEach((id, messages) -> this.messagesById.put(id, List.copyOf(messages)));
         }
 
+        public Map<ValidationId, Collection<String>> messagesById() { return Map.copyOf(messagesById); }
+
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
index 33dfee58d1a..f19341098f4 100644
--- a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
+++ b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
@@ -172,7 +172,7 @@ public class DeployState implements ConfigDefinitionStore {
     /** Get the global rank profile registry for this application. */
     public final RankProfileRegistry rankProfileRegistry() { return rankProfileRegistry; }
 
-    /** Returns the validation overrides of this. This is never null */
+    /** Returns the validation overrides of this. This is never null. */
     public ValidationOverrides validationOverrides() { return validationOverrides; }
 
     @Override
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
index 903f1c06024..65572d07cc2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
@@ -217,13 +217,6 @@ public class VespaModelFactory implements ModelFactory {
     private List<ConfigChangeAction> validateModel(VespaModel model, DeployState deployState, ValidationParameters validationParameters) {
         try {
             return new Validation(additionalValidators).validate(model, validationParameters, deployState);
-        } catch (ValidationOverrides.ValidationException e) {
-            if (deployState.isHosted() && zone.environment().isManuallyDeployed())
-                deployState.getDeployLogger().logApplicationPackage(Level.WARNING,
-                                                                    "Auto-overriding validation which would be disallowed in production: " +
-                                                                    Exceptions.toMessageString(e));
-            else
-                rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors());
         } catch (IllegalArgumentException | TransientException | QuotaExceededException e) {
             rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors());
         } catch (Exception e) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
index d296015580f..0f7a415c33a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
@@ -3,8 +3,8 @@ package com.yahoo.vespa.model.application.validation;
 
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
+import com.yahoo.config.application.api.ValidationOverrides.ValidationException;
 import com.yahoo.config.model.api.ConfigChangeAction;
-import com.yahoo.config.model.api.Model;
 import com.yahoo.config.model.api.ValidationParameters;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.provision.ClusterSpec;
@@ -25,21 +25,17 @@ import com.yahoo.vespa.model.application.validation.change.RestartOnDeployForOnn
 import com.yahoo.vespa.model.application.validation.change.StartupCommandChangeValidator;
 import com.yahoo.vespa.model.application.validation.change.StreamingSearchClusterChangeValidator;
 import com.yahoo.vespa.model.application.validation.first.RedundancyValidator;
+import com.yahoo.yolean.Exceptions;
 
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.stream.Collectors;
 
-import static java.util.stream.Collectors.groupingBy;
-import static java.util.stream.Collectors.mapping;
-import static java.util.stream.Collectors.toCollection;
-
 /**
  * Executor of validators. This defines the right order of validator execution.
  *
@@ -51,7 +47,7 @@ public class Validation {
 
     public Validation() { this(List.of()); }
 
-    /** Create instance taking additional validators (e.g for cloud applications) */
+    /** Create instance taking additional validators (e.g., for cloud applications) */
     public Validation(List<Validator> additionalValidators) { this.additionalValidators = additionalValidators; }
 
     /**
@@ -62,86 +58,80 @@ public class Validation {
      * @throws ValidationOverrides.ValidationException if the change fails validation
      */
     public List<ConfigChangeAction> validate(VespaModel model, ValidationParameters validationParameters, DeployState deployState) {
+        Execution execution = new Execution(model, deployState);
         if (validationParameters.checkRouting()) {
-            new RoutingValidator().validate(model, deployState);
-            new RoutingSelectorValidator().validate(model, deployState);
+            validateRouting(execution);
         }
-        new SchemasDirValidator().validate(model, deployState);
-        new BundleValidator().validate(model, deployState);
-        new PublicApiBundleValidator().validate(model, deployState);
-        new SearchDataTypeValidator().validate(model, deployState);
-        new ComplexFieldsWithStructFieldAttributesValidator().validate(model, deployState);
-        new ComplexFieldsWithStructFieldIndexesValidator().validate(model, deployState);
-        new StreamingValidator().validate(model, deployState);
-        new RankSetupValidator(validationParameters.ignoreValidationErrors()).validate(model, deployState);
-        new NoPrefixForIndexes().validate(model, deployState);
-        new ContainerInCloudValidator().validate(model, deployState);
-        new DeploymentSpecValidator().validate(model, deployState);
-        new ValidationOverridesValidator().validate(model, deployState);
-        new ConstantValidator().validate(model, deployState);
-        new SecretStoreValidator().validate(model, deployState);
-        new EndpointCertificateSecretsValidator().validate(model, deployState);
-        new AccessControlFilterValidator().validate(model, deployState);
-        new QuotaValidator().validate(model, deployState);
-        new UriBindingsValidator().validate(model, deployState);
-        new CloudDataPlaneFilterValidator().validate(model, deployState);
-        new AccessControlFilterExcludeValidator().validate(model, deployState);
-        new CloudUserFilterValidator().validate(model, deployState);
-        new CloudHttpConnectorValidator().validate(model, deployState);
-        new UrlConfigValidator().validate(model, deployState);
-        new JvmHeapSizeValidator().validate(model, deployState);
-
-        additionalValidators.forEach(v -> v.validate(model, deployState));
-
-        List<ConfigChangeAction> result = Collections.emptyList();
+
+        validateModel(validationParameters, execution);
+
+        additionalValidators.forEach(execution::run);
+
         if (deployState.getProperties().isFirstTimeDeployment()) {
-            validateFirstTimeDeployment(model, deployState);
-        } else {
-            Optional<Model> currentActiveModel = deployState.getPreviousModel();
-            if (currentActiveModel.isPresent() && (currentActiveModel.get() instanceof VespaModel)) {
-                result = validateChanges((VespaModel) currentActiveModel.get(), model, deployState);
+            validateFirstTimeDeployment(execution);
+        }
+        else if (deployState.getPreviousModel().isPresent() && (deployState.getPreviousModel().get() instanceof VespaModel vespaModel)) {
+            validateChanges(vespaModel, execution);
                 // TODO: Why is this done here? It won't be done on more than one config server?
-                deferConfigChangesForClustersToBeRestarted(result, model);
-            }
+            deferConfigChangesForClustersToBeRestarted(execution.actions, model);
         }
-        return result;
+
+        execution.throwIfFailed();
+        return execution.actions;
     }
 
-    private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, VespaModel nextModel,
-                                                            DeployState deployState) {
-        ChangeValidator[] validators = new ChangeValidator[] {
-                new IndexingModeChangeValidator(),
-                new GlobalDocumentChangeValidator(),
-                new IndexedSearchClusterChangeValidator(),
-                new StreamingSearchClusterChangeValidator(),
-                new ConfigValueChangeValidator(),
-                new StartupCommandChangeValidator(),
-                new ContentTypeRemovalValidator(),
-                new ContentClusterRemovalValidator(),
-                new ResourcesReductionValidator(),
-                new ResourcesReductionValidator(),
-                new ContainerRestartValidator(),
-                new NodeResourceChangeValidator(),
-                new RedundancyIncreaseValidator(),
-                new CertificateRemovalChangeValidator(),
-                new RedundancyValidator(),
-                new RestartOnDeployForOnnxModelChangesValidator(),
-        };
-        List<ConfigChangeAction> actions = Arrays.stream(validators)
-                                                 .flatMap(v -> v.validate(currentModel, nextModel, deployState).stream())
-                                                 .toList();
-
-        Map<ValidationId, Collection<String>> disallowableActions = actions.stream()
-                                                                           .filter(action -> action.validationId().isPresent())
-                                                                           .collect(groupingBy(action -> action.validationId().orElseThrow(),
-                                                                                               mapping(ConfigChangeAction::getMessage,
-                                                                                                       toCollection(LinkedHashSet::new))));
-        deployState.validationOverrides().invalid(disallowableActions, deployState.now());
-        return actions;
+    private static void validateRouting(Execution execution) {
+        execution.run(new RoutingValidator());
+        execution.run(new RoutingSelectorValidator());
     }
 
-    private static void validateFirstTimeDeployment(VespaModel model, DeployState deployState) {
-        new RedundancyValidator().validate(model, deployState);
+    private static void validateModel(ValidationParameters validationParameters, Execution execution) {
+        execution.run(new SchemasDirValidator());
+        execution.run(new BundleValidator());
+        execution.run(new PublicApiBundleValidator());
+        execution.run(new SearchDataTypeValidator());
+        execution.run(new ComplexFieldsWithStructFieldAttributesValidator());
+        execution.run(new ComplexFieldsWithStructFieldIndexesValidator());
+        execution.run(new StreamingValidator());
+        execution.run(new RankSetupValidator(validationParameters.ignoreValidationErrors()));
+        execution.run(new NoPrefixForIndexes());
+        execution.run(new ContainerInCloudValidator());
+        execution.run(new DeploymentSpecValidator());
+        execution.run(new ValidationOverridesValidator());
+        execution.run(new ConstantValidator());
+        execution.run(new SecretStoreValidator());
+        execution.run(new EndpointCertificateSecretsValidator());
+        execution.run(new AccessControlFilterValidator());
+        execution.run(new QuotaValidator());
+        execution.run(new UriBindingsValidator());
+        execution.run(new CloudDataPlaneFilterValidator());
+        execution.run(new AccessControlFilterExcludeValidator());
+        execution.run(new CloudUserFilterValidator());
+        execution.run(new CloudHttpConnectorValidator());
+        execution.run(new UrlConfigValidator());
+        execution.run(new JvmHeapSizeValidator());
+    }
+
+    private static void validateFirstTimeDeployment(Execution execution) {
+        execution.run(new RedundancyValidator());
+    }
+
+    private static void validateChanges(VespaModel currentModel, Execution execution) {
+        execution.run(new IndexingModeChangeValidator(), currentModel);
+        execution.run(new GlobalDocumentChangeValidator(), currentModel);
+        execution.run(new IndexedSearchClusterChangeValidator(), currentModel);
+        execution.run(new StreamingSearchClusterChangeValidator(), currentModel);
+        execution.run(new ConfigValueChangeValidator(), currentModel);
+        execution.run(new StartupCommandChangeValidator(), currentModel);
+        execution.run(new ContentTypeRemovalValidator(), currentModel);
+        execution.run(new ContentClusterRemovalValidator(), currentModel);
+        execution.run(new ResourcesReductionValidator(), currentModel);
+        execution.run(new ContainerRestartValidator(), currentModel);
+        execution.run(new NodeResourceChangeValidator(), currentModel);
+        execution.run(new RedundancyIncreaseValidator(), currentModel);
+        execution.run(new CertificateRemovalChangeValidator(), currentModel);
+        execution.run(new RedundancyValidator(), currentModel);
+        execution.run(new RestartOnDeployForOnnxModelChangesValidator(), currentModel);
     }
 
     private static void deferConfigChangesForClustersToBeRestarted(List<ConfigChangeAction> actions, VespaModel model) {
@@ -160,4 +150,61 @@ public class Validation {
         }
     }
 
+
+    private static class Execution {
+
+        private final Map<ValidationId, List<String>> failures = new LinkedHashMap<>();
+        private final VespaModel model;
+        private final DeployState deployState;
+        private final List<ConfigChangeAction> actions = new ArrayList<>();
+
+        private Execution(VespaModel model, DeployState deployState) {
+            this.model = model;
+            this.deployState = deployState;
+        }
+
+        private void run(Validator validator) {
+            try {
+                validator.validate(model, deployState);
+            }
+            catch (ValidationException e) {
+                e.messagesById().forEach((id, messages) -> failures.computeIfAbsent(id, __ -> new ArrayList<>()).addAll(messages));
+            }
+        }
+
+        private void run(ChangeValidator validator, VespaModel previousModel) {
+            try {
+                // Some change validators throw, while some return a list of changes that may again be disallowed.
+                for (ConfigChangeAction action : validator.validate(previousModel, model, deployState)) {
+                    actions.add(action);
+                    if (action.validationId().isPresent()) run(new Validator() { // Changes without a validation ID are always allowed.
+                        @Override public void validate(VespaModel model, DeployState deployState) {
+                            deployState.validationOverrides().invalid(action.validationId().get(), action.getMessage(), deployState.now());
+                        }
+                    });
+                }
+            }
+            catch (ValidationException e) {
+                e.messagesById().forEach((id, messages) -> failures.computeIfAbsent(id, __ -> new ArrayList<>()).addAll(messages));
+            }
+        }
+
+        private void throwIfFailed() {
+            try {
+                if (failures.size() == 1 && failures.values().iterator().next().size() == 1) // Retain single-form exception message when possible.
+                    deployState.validationOverrides().invalid(failures.keySet().iterator().next(), failures.values().iterator().next().get(0), deployState.now());
+                else
+                    deployState.validationOverrides().invalid(failures, deployState.now());
+            }
+            catch (ValidationException e) {
+                if (deployState.isHosted() && deployState.zone().environment().isManuallyDeployed())
+                    deployState.getDeployLogger().logApplicationPackage(Level.WARNING,
+                                                                        "Auto-overriding validation which would be disallowed in production: " +
+                                                                        Exceptions.toMessageString(e));
+                else throw e;
+            }
+        }
+
+    }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
index fb07f65b5f4..e2dd3aca0b9 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
@@ -40,7 +40,7 @@ public class IndexingScriptChangeValidator {
             String fieldName = nextField.getName();
             ImmutableSDField currentField = currentSchema.getConcreteField(fieldName);
             if (currentField != null) {
-                validateScripts(currentField, nextField).ifPresent(r -> result.add(r));
+                validateScripts(currentField, nextField).ifPresent(result::add);
             }
             else if (nextField.isExtraField()) {
                 result.add(VespaReindexAction.of(id,
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
index 6b7df8871aa..bc36b800bfb 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
@@ -23,9 +23,11 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 public class CertificateRemovalChangeValidatorTest {
 
     private static final String validationOverrides =
-            "<validation-overrides>\n" +
-            "    <allow until='2000-01-14' comment='test override'>certificate-removal</allow>\n" +
-            "</validation-overrides>\n";
+            """
+            <validation-overrides>
+                <allow until='2000-01-14' comment='test override'>certificate-removal</allow>
+            </validation-overrides>
+            """;
 
     @Test
     void validate() {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
index 3fd3180b37e..9e0eab9aba7 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
@@ -22,19 +22,28 @@ import static org.junit.jupiter.api.Assertions.fail;
 public class IndexingModeChangeValidatorTest {
 
     @Test
+    void testChangingIndexModeFromIndexedToStreamingWhenDisallowedButInDev() {
+        ValidationTester tester = new ValidationTester();
+
+        VespaModel oldModel =
+                tester.deploy(null, getServices("index"), Environment.dev, "<validation-overrides />").getFirst();
+        List<ConfigChangeAction> actions = tester.deploy(oldModel, getServices("streaming"), Environment.dev, "<calidation-overrides />").getSecond();
+        assertReindexingChange("Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'", actions);
+    }
+
+    @Test
     void testChangingIndexModeFromIndexedToStreamingWhenDisallowed() {
         ValidationTester tester = new ValidationTester();
 
         VespaModel oldModel =
                 tester.deploy(null, getServices("index"), Environment.prod, "<validation-overrides />").getFirst();
         try {
-            List<ConfigChangeAction> changeActions =
-                    tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond();
+            tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond();
             fail("Should throw on disallowed config change action");
         }
         catch (ValidationException e) {
-            assertEquals("indexing-mode-change:\n" +
-                    "\tDocument type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'\n" +
+            assertEquals("indexing-mode-change: " +
+                    "Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'. " +
                     "To allow this add <allow until='yyyy-mm-dd'>indexing-mode-change</allow> to validation-overrides.xml, see https://docs.vespa.ai/en/reference/validation-overrides.html",
                     e.getMessage());
         }
@@ -94,8 +103,10 @@ public class IndexingModeChangeValidatorTest {
     }
 
     private static final String validationOverrides =
-            "<validation-overrides>\n" +
-            "    <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>\n" +
-            "</validation-overrides>\n";
+            """
+            <validation-overrides>
+                <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>
+            </validation-overrides>
+            """;
 
 }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ResourcesReductionValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ResourcesReductionValidatorTest.java
index f9c0d00ac2c..b7e612127c1 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ResourcesReductionValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ResourcesReductionValidatorTest.java
@@ -29,6 +29,14 @@ public class ResourcesReductionValidatorTest {
     private final ValidationTester tester = new ValidationTester(provisioner);
 
     @Test
+    void ok_when_reduction_by_over_50_percent_in_hosted_dev() {
+        var fromResources = new NodeResources(8, 64, 800, 1);
+        var toResources = new NodeResources(8, 16, 800, 1);
+        VespaModel previous = tester.deploy(null, contentServices(6, fromResources), Environment.dev, null, CONTAINER_CLUSTER).getFirst();
+        tester.deploy(previous, contentServices(6, toResources), Environment.dev, null, CONTAINER_CLUSTER);
+    }
+
+    @Test
     void fail_when_reduction_by_over_50_percent() {
         var fromResources = new NodeResources(8, 64, 800, 1);
         var toResources = new NodeResources(8, 16, 800, 1);