Merge pull request #27654 from vespa-engine/jonmv/restart-on-internal-deployment-with-node-changes

Restart also on internal redeployment if nodes change

3 files changed, 55 insertions, 12 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidator.java
index 0703d4fa3d6..f70b7f40421 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidator.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.model.application.validation.change;
 import com.yahoo.config.model.api.ConfigChangeAction;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.HostSpec;
 import com.yahoo.container.QrConfig;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.ApplicationContainer;
@@ -12,6 +13,10 @@ import com.yahoo.vespa.model.container.ContainerCluster;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toUnmodifiableSet;
 
 /**
  * Returns a restart action for each container that has turned on {@link QrConfig#restartOnDeploy()}.
@@ -22,19 +27,36 @@ public class ContainerRestartValidator implements ChangeValidator {
 
     @Override
     public List<ConfigChangeAction> validate(VespaModel currentModel, VespaModel nextModel, DeployState deployState) {
+        boolean nodesUnchanged = currentModel.allocatedHosts().equals(nextModel.allocatedHosts());
+        boolean contentUnchanged = contentHostsOf(currentModel).equals(contentHostsOf(nextModel));
         List<ConfigChangeAction> actions = new ArrayList<>();
         for (ContainerCluster<ApplicationContainer> cluster : nextModel.getContainerClusters().values()) {
             actions.addAll(cluster.getContainers().stream()
                                   .filter(container -> isExistingContainer(container, currentModel))
                                   .filter(container -> shouldContainerRestartOnDeploy(container, nextModel))
-                                  .map(container -> createConfigChangeAction(cluster.id(), container))
+                                  .map(container -> createConfigChangeAction(cluster.id(), container, nextModel, nodesUnchanged, contentUnchanged))
                                   .toList());
         }
         return actions;
     }
 
-    private static ConfigChangeAction createConfigChangeAction(ClusterSpec.Id id, Container container) {
-        return new VespaRestartAction(id, createMessage(container), container.getServiceInfo(), true);
+    private Set<HostSpec> contentHostsOf(VespaModel model) {
+        return model.allocatedHosts().getHosts().stream()
+                    .filter(spec -> spec.membership()
+                                        .map(membership -> membership.cluster().type().isContent())
+                                        .orElse(false))
+                    .collect(toUnmodifiableSet());
+    }
+
+    private static ConfigChangeAction createConfigChangeAction(ClusterSpec.Id id, Container container, VespaModel model,
+                                                               boolean nodesUnchanged, boolean contentUnchanged) {
+        boolean ignoreOnRedeploy = switch (model.getConfig(QrConfig.class, container.getConfigId()).restartOnInternalRedeploy()) {
+            case never -> true;
+            case content_changes -> contentUnchanged;
+            case node_changes -> nodesUnchanged;
+            case always -> false;
+        };
+        return new VespaRestartAction(id, createMessage(container), container.getServiceInfo(), ignoreOnRedeploy);
     }
 
     private static String createMessage(Container container) {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidatorTest.java
index c034944c8ae..d7697792d16 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ContainerRestartValidatorTest.java
@@ -19,36 +19,48 @@ public class ContainerRestartValidatorTest {
 
     @Test
     void validator_returns_action_for_containers_with_restart_on_deploy_enabled() {
-        VespaModel current = createModel(true);
-        VespaModel next = createModel(true);
+        VespaModel current = createModel(true, false);
+        VespaModel next = createModel(true, false);
         List<ConfigChangeAction> result = validateModel(current, next);
         assertEquals(2, result.size());
+        assertTrue(result.get(0).ignoreForInternalRedeploy());
+        assertTrue(result.get(1).ignoreForInternalRedeploy());
+    }
+
+    @Test
+    void validator_returns_not_ignored_action_for_containers_with_restart_on_internal_redeploy_always() {
+        VespaModel current = createModel(true, true);
+        VespaModel next = createModel(true, true);
+        List<ConfigChangeAction> result = validateModel(current, next);
+        assertEquals(2, result.size());
+        assertFalse(result.get(0).ignoreForInternalRedeploy());
+        assertFalse(result.get(1).ignoreForInternalRedeploy());
     }
 
     @Test
     void validator_returns_empty_list_for_containers_with_restart_on_deploy_disabled() {
-        VespaModel current = createModel(false);
-        VespaModel next = createModel(false);
+        VespaModel current = createModel(false, true);
+        VespaModel next = createModel(false, true);
         List<ConfigChangeAction> result = validateModel(current, next);
         assertTrue(result.isEmpty());
     }
 
     @Test
     void validator_returns_empty_list_for_containers_with_restart_on_deploy_disabled_where_previously_enabled() {
-        VespaModel current = createModel(true);
-        VespaModel next = createModel(false);
+        VespaModel current = createModel(true, true);
+        VespaModel next = createModel(false, true);
         List<ConfigChangeAction> result = validateModel(current, next);
         assertTrue(result.isEmpty());
     }
 
     @Test
     void restart_on_deploy_is_propagated_to_cluster() {
-        VespaModel model1 = createModel(false);
+        VespaModel model1 = createModel(false, false);
         assertFalse(model1.getContainerClusters().get("cluster1").getDeferChangesUntilRestart());
         assertFalse(model1.getContainerClusters().get("cluster2").getDeferChangesUntilRestart());
         assertFalse(model1.getContainerClusters().get("cluster3").getDeferChangesUntilRestart());
 
-        VespaModel model2 = createModel(true);
+        VespaModel model2 = createModel(true, false);
         assertTrue(model2.getContainerClusters().get("cluster1").getDeferChangesUntilRestart());
         assertTrue(model2.getContainerClusters().get("cluster2").getDeferChangesUntilRestart());
         assertFalse(model2.getContainerClusters().get("cluster3").getDeferChangesUntilRestart());
@@ -58,7 +70,7 @@ public class ContainerRestartValidatorTest {
         return new ContainerRestartValidator().validate(current, next, new DeployState.Builder().build());
     }
 
-    private static VespaModel createModel(boolean restartOnDeploy) {
+    private static VespaModel createModel(boolean restartOnDeploy, boolean alwaysRestart) {
         return new VespaModelCreatorWithMockPkg(
                 null,
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
@@ -69,6 +81,9 @@ public class ContainerRestartValidatorTest {
                 "       </http>\n" +
                 "       <config name='container.qr'>\n" +
                 "           <restartOnDeploy>" + restartOnDeploy + "</restartOnDeploy>\n" +
+              (alwaysRestart
+              ? "           <restartOnInternalRedeploy>always</restartOnInternalRedeploy>\n"
+              : "") +
                 "       </config>\n" +
                 "   </container>\n" +
                 "   <container id='cluster2' version='1.0'>\n" +
@@ -77,6 +92,9 @@ public class ContainerRestartValidatorTest {
                 "       </http>\n" +
                 "       <config name='container.qr'>\n" +
                 "           <restartOnDeploy>" + restartOnDeploy + "</restartOnDeploy>\n" +
+              (alwaysRestart
+              ? "           <restartOnInternalRedeploy>always</restartOnInternalRedeploy>\n"
+              : "") +
                 "       </config>\n" +
                 "   </container>\n" +
                 "   <container id='cluster3' version='1.0'>\n" +
diff --git a/container-core/src/main/resources/configdefinitions/container.qr.def b/container-core/src/main/resources/configdefinitions/container.qr.def
index f28b00883a2..ea638a9310d 100644
--- a/container-core/src/main/resources/configdefinitions/container.qr.def
+++ b/container-core/src/main/resources/configdefinitions/container.qr.def
@@ -34,6 +34,9 @@ nodeIndex int default=0
 ## Force restart of container on deploy, and defer any changes until restart
 restartOnDeploy bool default=false restart
 
+## Specifies under what circumstances restart on deploy should apply to internal redeployments
+restartOnInternalRedeploy enum { always, node_changes, content_changes, never } default=never restart
+
 ## Force heapdump if process is not able to stop within shutdown.timeout
 shutdown.dumpHeapOnTimeout bool default=false