Revert "Retry deprovisioning if child is un-retired"

3 files changed, 18 insertions, 47 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
index 8c549c3ca53..57a3b436e37 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
@@ -244,9 +244,8 @@ public class Nodes {
         if ( ! zone.environment().isProduction() || zone.system().isCd())
             return deallocate(nodes, Agent.application, "Deactivated by application", transaction.nested());
 
-        NodeList nodeList = NodeList.copyOf(nodes);
-        NodeList stateless = nodeList.stateless();
-        NodeList stateful  = nodeList.stateful();
+        var stateless = NodeList.copyOf(nodes).stateless();
+        var stateful  = NodeList.copyOf(nodes).stateful();
         List<Node> written = new ArrayList<>();
         written.addAll(deallocate(stateless.asList(), Agent.application, "Deactivated by application", transaction.nested()));
         written.addAll(db.writeTo(Node.State.inactive, stateful.asList(), Agent.application, Optional.empty(), transaction.nested()));
@@ -471,7 +470,7 @@ public class Nodes {
      */
     public Node markNodeAvailableForNewAllocation(String hostname, Agent agent, String reason) {
         Node node = requireNode(hostname);
-        if (removeOnReadyingOf(node)) {
+        if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && node.type() == NodeType.tenant) {
             if (node.state() != Node.State.dirty)
                 illegal("Cannot make " + node  + " available for new allocation as it is not in state [dirty]");
             return removeRecursively(node, true).get(0);
@@ -846,12 +845,6 @@ public class Nodes {
                retirementRequestedByOperator;
     }
 
-    /** Returns whether node should be deleted when it's moved to ready */
-    private static boolean removeOnReadyingOf(Node node) {
-        if (node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER) return false;
-        return node.type() == NodeType.tenant || (node.status().wantToRetire() && node.status().wantToDeprovision());
-    }
-
     /** The different ways a host can be decommissioned */
     private enum DecommissionOperation {
         deprovision,
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 5229fbb8f37..ba59ff8e425 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
@@ -37,10 +37,10 @@ public class RetiringOsUpgrader implements OsUpgrader {
         NodeList allNodes = nodeRepository.nodes().list();
         Instant now = nodeRepository.clock().instant();
         NodeList candidates = candidates(now, target, allNodes);
-        candidates.not().matching(host -> deprovisioning(host, allNodes))
+        candidates.not().deprovisioning()
                   .byIncreasingOsVersion()
-                  .first()
-                  .ifPresent(node -> deprovision(node, target.version(), now));
+                  .first(1)
+                  .forEach(node -> deprovision(node, target.version(), now));
     }
 
     @Override
@@ -62,7 +62,7 @@ public class RetiringOsUpgrader implements OsUpgrader {
 
     /** Upgrade given host by retiring and deprovisioning it */
     private void deprovision(Node host, Version target, Instant now) {
-        LOG.info("Retiring and deprovisioning " + host + " and its children: On stale OS version " +
+        LOG.info("Retiring and deprovisioning " + host + ": On stale OS version " +
                  host.status().osVersion().current().map(Version::toFullString).orElse("<unset>") +
                  ", want " + target);
         nodeRepository.nodes().deprovision(host.hostname(), Agent.RetiringUpgrader, now);
@@ -70,10 +70,4 @@ public class RetiringOsUpgrader implements OsUpgrader {
         nodeRepository.osVersions().writeChange((change) -> change.withRetirementAt(now, host.type()));
     }
 
-    /** Returns whether host and all its children are deprovisioning */
-    private static boolean deprovisioning(Node host, NodeList allNodes) {
-        return host.status().wantToRetire() && host.status().wantToDeprovision() &&
-               allNodes.childrenOf(host).not().deprovisioning().isEmpty();
-    }
-
 }
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 5a64d99ea33..26be6d95336 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
@@ -227,41 +227,25 @@ public class OsVersionsTest {
 
     @Test
     public void upgrade_by_retiring_everything_at_once() {
-        OsVersions versions = new OsVersions(tester.nodeRepository(), true, Integer.MAX_VALUE);
-        int nodeCount = 3;
-        ApplicationId configServerApp = ApplicationId.from("hosted-vespa", "zone-config-servers", "default");
-        List<Node> configHosts = provisionInfraApplication(nodeCount, infraApplication, NodeType.confighost);
-        NodeResources resources = new NodeResources(2, 4, 8, 1);
-        for (int i = 0; i < nodeCount; i++) {
-            tester.makeReadyChildren(1, i, resources, NodeType.config, configHosts.get(i).hostname(), (index) -> "cfg" + index);
-        }
-        tester.prepareAndActivateInfraApplication(configServerApp, NodeType.config);
+        var versions = new OsVersions(tester.nodeRepository(), true, Integer.MAX_VALUE);
+        int hostCount = 3;
+        provisionInfraApplication(hostCount, infraApplication, NodeType.confighost);
+        Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list()
+                                                   .nodeType(NodeType.confighost)
+                                                   .not().state(Node.State.deprovisioned);
 
         // Target is set with zero budget and upgrade started
         var version1 = Version.fromString("7.1");
         versions.setTarget(NodeType.confighost, version1, Duration.ZERO,false);
-        for (int i = 0; i < nodeCount; i++) {
+        for (int i = 0; i < hostCount; i++) {
             versions.resumeUpgradeOf(NodeType.confighost, true);
         }
 
         // All hosts are deprovisioning
-        NodeList nodes = tester.nodeRepository().nodes().list().not().state(Node.State.deprovisioned);
-        NodeList configNodes = nodes.nodeType(NodeType.config);
-        assertEquals(nodeCount, nodes.nodeType(NodeType.confighost).deprovisioning().size());
-        assertEquals(nodeCount, configNodes.deprovisioning().size());
-
-        // One child is inadvertently unretired
-        tester.patchNode(configNodes.first().get(), (node) -> node.withWantToRetire(false, false,
-                                                                                    Agent.system, tester.clock().instant()));
-
-        // Resuming upgrade of host re-retires child
-        versions.resumeUpgradeOf(NodeType.confighost, true);
-        nodes = tester.nodeRepository().nodes().list().not().state(Node.State.deprovisioned);
-        assertEquals(nodeCount, nodes.nodeType(NodeType.config).deprovisioning().size());
-
+        assertEquals(hostCount, hostNodes.get().deprovisioning().size());
         // Nodes complete their upgrade by being reprovisioned
-        completeReprovisionOf(nodes.nodeType(NodeType.confighost).deprovisioning().asList(), NodeType.confighost);
-        assertEquals(nodeCount, tester.nodeRepository().nodes().list().nodeType(NodeType.confighost).onOsVersion(version1).size());
+        completeReprovisionOf(hostNodes.get().deprovisioning().asList(), NodeType.confighost);
+        assertEquals(hostCount, hostNodes.get().onOsVersion(version1).size());
     }
 
     @Test
@@ -544,7 +528,7 @@ public class OsVersionsTest {
                 ApplicationId application = node.allocation().get().owner();
                 tester.nodeRepository().nodes().park(node.hostname(), false, Agent.system,
                                                      getClass().getSimpleName());
-                tester.nodeRepository().nodes().removeRecursively(node, true);
+                tester.nodeRepository().nodes().removeRecursively(node.hostname());
                 node = provisionInfraApplication(1, application, nodeType).get(0);
             }
             return node.with(node.status().withOsVersion(node.status().osVersion().withCurrent(wantedOsVersion)));