Limit grace period to RetiringOsUpgrader

8 files changed, 28 insertions, 62 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingOsUpgrader.java
index 9c938dd87ec..4178d4a6328 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingOsUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingOsUpgrader.java
@@ -7,6 +7,7 @@ import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter;
 
+import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.logging.Logger;
@@ -39,9 +40,10 @@ public class DelegatingOsUpgrader implements OsUpgrader {
     public void upgradeTo(OsVersionTarget target) {
         NodeList activeNodes = nodeRepository.nodes().list(Node.State.active).nodeType(target.nodeType());
         int numberToUpgrade = Math.max(0, maxActiveUpgrades - activeNodes.changingOsVersionTo(target.version()).size());
+        Instant now = nodeRepository.clock().instant();
         NodeList nodesToUpgrade = activeNodes.not().changingOsVersionTo(target.version())
                                              .osVersionIsBefore(target.version())
-                                             .matching(node -> shouldUpgrade(node, nodeRepository.clock().instant()))
+                                             .matching(node -> canUpgradeAt(now, node))
                                              .byIncreasingOsVersion()
                                              .first(numberToUpgrade);
         if (nodesToUpgrade.size() == 0) return;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsUpgrader.java
index 9abb0a29628..4140de76368 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsUpgrader.java
@@ -3,9 +3,7 @@ package com.yahoo.vespa.hosted.provision.os;
 
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.node.History;
 
-import java.time.Duration;
 import java.time.Instant;
 
 /**
@@ -15,17 +13,15 @@ import java.time.Instant;
  */
 public interface OsUpgrader {
 
-    /** The duration we should leave new nodes along before scheduling OS upgrades */
-    Duration gracePeriod = Duration.ofDays(30);
-
     /** Trigger upgrade to given target */
     void upgradeTo(OsVersionTarget target);
 
     /** Disable OS upgrade for all nodes of given type */
     void disableUpgrade(NodeType type);
 
-    default boolean shouldUpgrade(Node node, Instant now) {
-        return node.history().age(now).toSeconds() > gracePeriod.toSeconds();
+    /** Returns whether node can upgrade at given instant */
+    default boolean canUpgradeAt(Instant instant, Node node) {
+        return true;
     }
 
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java
index ffedbd241d7..f96effe9e10 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java
@@ -47,7 +47,7 @@ public class RebuildingOsUpgrader implements OsUpgrader {
     public void upgradeTo(OsVersionTarget target) {
         NodeList allNodes = nodeRepository.nodes().list();
         Instant now = nodeRepository.clock().instant();
-        rebuildableHosts(target, allNodes).forEach(host -> rebuild(host, target.version(), now));
+        rebuildableHosts(target, allNodes, now).forEach(host -> rebuild(host, target.version(), now));
     }
 
     @Override
@@ -62,7 +62,7 @@ public class RebuildingOsUpgrader implements OsUpgrader {
         return Math.max(0, limit - hostsOfType.rebuilding().size());
     }
 
-    private List<Node> rebuildableHosts(OsVersionTarget target, NodeList allNodes) {
+    private List<Node> rebuildableHosts(OsVersionTarget target, NodeList allNodes, Instant now) {
         NodeList hostsOfTargetType = allNodes.nodeType(target.nodeType());
         int rebuildLimit = rebuildLimit(target.nodeType(), hostsOfTargetType);
 
@@ -76,7 +76,7 @@ public class RebuildingOsUpgrader implements OsUpgrader {
         NodeList candidates = hostsOfTargetType.state(Node.State.active)
                                                .not().rebuilding()
                                                .osVersionIsBefore(target.version())
-                                               .matching(node -> shouldUpgrade(node, nodeRepository.clock().instant()))
+                                               .matching(node -> canUpgradeAt(now, node))
                                                .byIncreasingOsVersion();
         for (Node host : candidates) {
             if (hostsToRebuild.size() == rebuildLimit) break;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java
index dba79cd70ca..79b7441cc34 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java
@@ -26,6 +26,9 @@ public class RetiringOsUpgrader implements OsUpgrader {
 
     private static final Logger LOG = Logger.getLogger(RetiringOsUpgrader.class.getName());
 
+    /** The duration this leaves new nodes alone before scheduling any upgrade */
+    static final Duration GRACE_PERIOD = Duration.ofDays(30);
+
     protected final NodeRepository nodeRepository;
 
     public RetiringOsUpgrader(NodeRepository nodeRepository) {
@@ -33,22 +36,27 @@ public class RetiringOsUpgrader implements OsUpgrader {
     }
 
     @Override
-    public final void upgradeTo(OsVersionTarget target) {
+    public void upgradeTo(OsVersionTarget target) {
         NodeList allNodes = nodeRepository.nodes().list();
         Instant now = nodeRepository.clock().instant();
         NodeList candidates = candidates(now, target, allNodes);
         candidates.not().deprovisioning()
-                  .matching(node -> shouldUpgrade(node, nodeRepository.clock().instant()))
+                  .matching(node -> canUpgradeAt(now, node))
                   .byIncreasingOsVersion()
                   .first(1)
                   .forEach(node -> deprovision(node, target.version(), now));
     }
 
     @Override
-    public final void disableUpgrade(NodeType type) {
+    public void disableUpgrade(NodeType type) {
         // No action needed in this implementation.
     }
 
+    @Override
+    public boolean canUpgradeAt(Instant instant, Node node) {
+        return node.history().age(instant).compareTo(GRACE_PERIOD) > 0;
+    }
+
     /** Returns nodes that are candidates for upgrade */
     private NodeList candidates(Instant instant, OsVersionTarget target, NodeList allNodes) {
         NodeList activeNodes = allNodes.state(Node.State.active).nodeType(target.nodeType());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
index f9b5e383ea0..ff2ab44ad4a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
@@ -10,7 +10,6 @@ import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.Allocation;
 import com.yahoo.vespa.hosted.provision.node.Status;
-import com.yahoo.vespa.hosted.provision.os.OsUpgrader;
 import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
 import org.junit.Test;
 
@@ -50,8 +49,6 @@ public class OsUpgradeActivatorTest {
         var allNodes = new ArrayList<>(configHostNodes);
         allNodes.addAll(tenantHostNodes);
 
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
-
         // All nodes are on initial version
         assertEquals(version0, minWantedVersion(allNodes));
         completeUpgradeOf(configHostNodes);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
index c3a5d7efd2f..af2a215dae0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
@@ -65,13 +65,6 @@ public class OsVersionsTest {
         // Resume upgrade
         versions.resumeUpgradeOf(NodeType.host, true);
         NodeList allHosts = hostNodes.get();
-        assertFalse("No upgrades of new hosts", allHosts.except(hostOnLaterVersion).stream()
-                                                        .anyMatch(node -> node.status().osVersion().wanted().isPresent()));
-
-        // Resume upgrade after sufficient time
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
-        versions.resumeUpgradeOf(NodeType.host, true);
-        allHosts = hostNodes.get();
         assertTrue("Wanted version is set", allHosts.except(hostOnLaterVersion).stream()
                                                     .allMatch(node -> node.status().osVersion().wanted().isPresent()));
         assertTrue("Wanted version is not set for host on later version",
@@ -106,8 +99,6 @@ public class OsVersionsTest {
         provisionInfraApplication(totalNodes);
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list().state(Node.State.active).hosts();
 
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
-
         // 5 nodes have no version. The other 15 are spread across different versions
         var hostNodesList = hostNodes.get().asList();
         for (int i = totalNodes - maxActiveUpgrades - 1; i >= 0; i--) {
@@ -153,7 +144,6 @@ public class OsVersionsTest {
         var versions = new OsVersions(tester.nodeRepository());
         provisionInfraApplication(10);
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list().hosts();
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
 
         // Some nodes are targeting an older version
         var version1 = Version.fromString("7.1");
@@ -182,7 +172,7 @@ public class OsVersionsTest {
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list()
                                                    .hosts()
                                                    .not().state(Node.State.deprovisioned);
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
+        tester.clock().advance(RetiringOsUpgrader.GRACE_PERIOD.plusDays(1));
 
         // Target is set and upgrade started
         var version1 = Version.fromString("7.1");
@@ -244,7 +234,7 @@ public class OsVersionsTest {
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list()
                                                    .nodeType(NodeType.confighost)
                                                    .not().state(Node.State.deprovisioned);
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
+        tester.clock().advance(RetiringOsUpgrader.GRACE_PERIOD.plusDays(1));
 
         // Target is set with zero budget and upgrade started
         var version1 = Version.fromString("7.1");
@@ -267,7 +257,6 @@ public class OsVersionsTest {
         int hostCount = 10;
         provisionInfraApplication(hostCount + 1);
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list().nodeType(NodeType.host);
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
 
         // All hosts upgrade to first version. Upgrades are delegated
         var version0 = Version.fromString("7.0");
@@ -315,7 +304,6 @@ public class OsVersionsTest {
         assertEquals(0, hostNodes.get().rebuilding().size());
 
         // Next version is within same major. Upgrade mechanism switches to delegated
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
         var version2 = Version.fromString("8.1");
         versions.setTarget(NodeType.host, version2, Duration.ZERO, false);
         versions.resumeUpgradeOf(NodeType.host, true);
@@ -346,8 +334,6 @@ public class OsVersionsTest {
         Supplier<NodeList> hosts = () -> tester.nodeRepository().nodes().list().nodeType(NodeType.host,
                                                                                          NodeType.confighost);
 
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
-
         // All hosts upgrade to first version. Upgrades are delegated
         var version0 = Version.fromString("7.0");
         versions.setTarget(NodeType.host, version0, Duration.ZERO, false);
@@ -382,7 +368,6 @@ public class OsVersionsTest {
         deployApplication(app1);
         deployApplication(app2);
         Supplier<NodeList> hosts = () -> tester.nodeRepository().nodes().list().nodeType(NodeType.host);
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
 
         // All hosts are on initial version
         var version0 = Version.fromString("7.0");
@@ -456,7 +441,6 @@ public class OsVersionsTest {
         var versions = new OsVersions(tester.nodeRepository(), false, Integer.MAX_VALUE);
         provisionInfraApplication(hostCount, infraApplication, NodeType.proxyhost);
         Supplier<NodeList> hosts = () -> tester.nodeRepository().nodes().list().nodeType(NodeType.proxyhost);
-        tester.clock().advance(OsUpgrader.gracePeriod.plusDays(1));
 
         // All hosts are on initial version
         var version0 = Version.fromString("7.0");
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java
index eb540426ce0..19af4d00e54 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java
@@ -6,17 +6,11 @@ import com.yahoo.application.container.handler.Response;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.test.ManualClock;
 import com.yahoo.text.Utf8;
 import com.yahoo.vespa.applicationmodel.HostName;
-import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.NodeMutex;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.maintenance.OsUpgradeActivator;
 import com.yahoo.vespa.hosted.provision.maintenance.TestMetric;
-import com.yahoo.vespa.hosted.provision.node.Agent;
-import com.yahoo.vespa.hosted.provision.node.History;
-import com.yahoo.vespa.hosted.provision.os.OsUpgrader;
 import com.yahoo.vespa.hosted.provision.testutils.MockNodeRepository;
 import com.yahoo.vespa.hosted.provision.testutils.OrchestratorMock;
 import org.junit.After;
@@ -776,17 +770,7 @@ public class NodesV2ApiTest {
                                    Request.Method.PATCH),
                        "{\"message\":\"Set osVersion to 7.5.2, upgradeBudget to PT0S for nodes of type host\"}");
 
-        var nodeRepository = (NodeRepository)tester.container().components().getComponent(MockNodeRepository.class.getName());
-
-        // Age nodes to pass OS upgrade grace period for new nodes
-        for (Node node : nodeRepository.nodes().list()) {
-            try (NodeMutex lockedNode = nodeRepository.nodes().lockAndGet(node).orElseThrow()) {
-                Node agedNode = node.with(node.history().with(new History.Event(History.Event.Type.provisioned,
-                                                                                Agent.system,
-                                                                                nodeRepository.clock().instant().minus(OsUpgrader.gracePeriod.plusDays(1)))));
-                nodeRepository.nodes().write(agedNode, lockedNode);
-            }
-        }
+        var nodeRepository = (NodeRepository) tester.container().components().getComponent(MockNodeRepository.class.getName());
 
         // Activate target
         var osUpgradeActivator = new OsUpgradeActivator(nodeRepository, Duration.ofDays(1), new TestMetric());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/docker-node1-os-upgrade-complete.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/docker-node1-os-upgrade-complete.json
index 322f7cd0cad..b885f7bd7fc 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/docker-node1-os-upgrade-complete.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/docker-node1-os-upgrade-complete.json
@@ -38,6 +38,11 @@
   "down": false,
   "history": [
     {
+      "event": "provisioned",
+      "at": 123,
+      "agent": "system"
+    },
+    {
       "event": "readied",
       "at": 123,
       "agent": "system"
@@ -51,21 +56,11 @@
       "event": "activated",
       "at": 123,
       "agent": "application"
-    },
-    {
-      "event": "provisioned",
-      "at": -2678399877,
-      "agent": "system"
     }
   ],
   "log": [
     {
       "event": "provisioned",
-      "at": -2678399877,
-      "agent": "system"
-    },
-    {
-      "event": "provisioned",
       "at": 123,
       "agent": "system"
     },