Merge pull request #18632 from vespa-engine/revert-18627-mpolden/controller-node-cleanup-3

Revert "Support listing nodes for multiple applications"

4 files changed, 39 insertions, 41 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/NodeRepository.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/NodeRepository.java
index 62e87a3982b..60b24ad8c0b 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/NodeRepository.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/NodeRepository.java
@@ -14,6 +14,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * Node repository interface intended for use by the controller.
@@ -41,12 +42,14 @@ public interface NodeRepository {
     List<Node> list(ZoneId zone, List<HostName> hostnames);
 
     /** List all nodes in zone owned by given application */
-    default List<Node> list(ZoneId zone, ApplicationId application) {
-        return list(zone, Set.of(application), Set.of());
-    }
+    List<Node> list(ZoneId zone, ApplicationId application);
 
     /** List all nodes in states, in zone owned by given application */
-    List<Node> list(ZoneId zone, Set<ApplicationId> applications, Set<Node.State> states);
+    default List<Node> list(ZoneId zone, ApplicationId application, Set<Node.State> states) {
+        return list(zone, application).stream()
+                                      .filter(node -> states.contains(node.state()))
+                                      .collect(Collectors.toList());
+    }
 
     /** Get node repository's view of given application */
     Application getApplication(ZoneId zone, ApplicationId application);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 24d9db362ef..e9b4b25672a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -277,7 +277,7 @@ public class ApplicationController {
     /** Reads the oldest installed platform for the given application and zone from the node repo of that zone. */
     private Optional<Version> oldestInstalledPlatform(JobId job) {
         return configServer.nodeRepository().list(job.type().zone(controller.system()),
-                                                  Set.of(job.application()),
+                                                  job.application(),
                                                   EnumSet.of(active, reserved))
                            .stream()
                            .map(Node::currentVersion)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
index 7b17b3e44ff..c49e3c88df3 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
@@ -1,6 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.deployment;
 
+import com.google.common.collect.ImmutableSet;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentInstanceSpec;
 import com.yahoo.config.application.api.DeploymentSpec;
@@ -64,7 +65,6 @@ import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Date;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -313,8 +313,8 @@ public class InternalStepRunner implements StepRunner {
             return Optional.empty();
         }
         List<Node> nodes = controller.serviceRegistry().configServer().nodeRepository().list(id.type().zone(controller.system()),
-                                                                                             Set.of(id.application()),
-                                                                                             EnumSet.of(active));
+                                                                                             id.application(),
+                                                                                             Set.of(active));
         List<Node> parents = controller.serviceRegistry().configServer().nodeRepository().list(id.type().zone(controller.system()),
                                                                                                nodes.stream().map(node -> node.parentHostname().get()).collect(toList()));
         NodeList nodeList = NodeList.of(nodes, parents, services.get());
@@ -419,8 +419,8 @@ public class InternalStepRunner implements StepRunner {
                    : Optional.empty();
         }
         List<Node> nodes = controller.serviceRegistry().configServer().nodeRepository().list(zone,
-                                                                                             Set.of(testerId),
-                                                                                             EnumSet.of(active, reserved));
+                                                                                             testerId,
+                                                                                             ImmutableSet.of(active, reserved));
         List<Node> parents = controller.serviceRegistry().configServer().nodeRepository().list(zone,
                                                                                                nodes.stream().map(node -> node.parentHostname().get()).collect(toList()));
         NodeList nodeList = NodeList.of(nodes, parents, services.get());
@@ -447,13 +447,14 @@ public class InternalStepRunner implements StepRunner {
         if ( ! endpoints.containsKey(zoneId))
             return false;
 
-        return endpoints.get(zoneId).parallelStream().allMatch(endpoint -> {
+        return endpoints.get(zoneId).parallelStream().map(endpoint -> {
             boolean ready = controller.jobController().cloud().ready(endpoint.url());
-            if (!ready) {
+            if ( ! ready) {
                 logger.log("Failed to get 100 consecutive OKs from " + endpoint);
+                return Boolean.FALSE;
             }
-            return ready;
-        });
+            return Boolean.TRUE;
+        }).allMatch(Boolean.TRUE::equals);
     }
 
     /** Returns true iff all containers in the tester deployment give 100 consecutive 200 OK responses on /status.html. */
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
index e02cd25ff23..8a9aa6b4b1b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
@@ -20,13 +20,11 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.TargetVers
 
 import java.net.URI;
 import java.time.Duration;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
 
@@ -78,25 +76,23 @@ public class NodeRepositoryMock implements NodeRepository {
 
     @Override
     public List<Node> list(ZoneId zone, boolean includeDeprovisioned) {
-        return list(zone).stream()
-                         .filter(node -> includeDeprovisioned || node.state() != Node.State.deprovisioned)
-                         .collect(Collectors.toList());
+        return nodeRepository.getOrDefault(zone, Map.of()).values().stream()
+                             .filter(node -> includeDeprovisioned || node.state() != Node.State.deprovisioned)
+                             .collect(Collectors.toList());
     }
 
     @Override
-    public List<Node> list(ZoneId zone, List<HostName> hostnames) {
-        return list(zone).stream()
-                         .filter(node -> hostnames.contains(node.hostname()))
-                         .collect(Collectors.toList());
+    public List<Node> list(ZoneId zone, ApplicationId application) {
+        return nodeRepository.getOrDefault(zone, Map.of()).values().stream()
+                             .filter(node -> node.owner().map(application::equals).orElse(false))
+                             .collect(Collectors.toList());
     }
 
     @Override
-    public List<Node> list(ZoneId zone, Set<ApplicationId> applications, Set<Node.State> states) {
-        return list(zone).stream()
-                         .filter(node -> states.isEmpty() || states.contains(node.state()))
-                         .filter(node -> applications.isEmpty() ||
-                                         (node.owner().isPresent() && applications.contains(node.owner().get())))
-                         .collect(Collectors.toList());
+    public List<Node> list(ZoneId zone, List<HostName> hostnames) {
+        return nodeRepository.getOrDefault(zone, Map.of()).values().stream()
+                             .filter(node -> hostnames.contains(node.hostname()))
+                             .collect(Collectors.toList());
     }
 
     @Override
@@ -146,10 +142,11 @@ public class NodeRepositoryMock implements NodeRepository {
             return targetVersions.withVespaVersion(type, version);
         });
         // Bump wanted version of each node. This is done by InfrastructureProvisioner in a real node repository.
-        patchNodes(zone, Optional.empty(), node -> {
-            if (node.type() != type) return node;
-            return Node.builder(node).wantedVersion(version).build();
-        });
+        nodeRepository.getOrDefault(zone, Map.of()).values()
+                      .stream()
+                      .filter(node -> node.type() == type)
+                      .map(node -> Node.builder(node).wantedVersion(version).build())
+                      .forEach(node -> putNodes(zone, node));
     }
 
     @Override
@@ -162,10 +159,11 @@ public class NodeRepositoryMock implements NodeRepository {
             return targetVersions.withOsVersion(type, version);
         });
         // Bump wanted version of each node. This is done by OsUpgradeActivator in a real node repository.
-        patchNodes(zone, Optional.empty(), node -> {
-            if (node.type() != type) return node;
-            return Node.builder(node).wantedOsVersion(version).build();
-        });
+        nodeRepository.getOrDefault(zone, Map.of()).values()
+                      .stream()
+                      .filter(node -> node.type() == type)
+                      .map(node -> Node.builder(node).wantedOsVersion(version).build())
+                      .forEach(node -> putNodes(zone, node));
     }
 
     @Override
@@ -322,10 +320,6 @@ public class NodeRepositoryMock implements NodeRepository {
         this.hasSpareCapacity = hasSpareCapacity;
     }
 
-    private Collection<Node> list(ZoneId zone) {
-        return nodeRepository.getOrDefault(zone, Map.of()).values();
-    }
-
     private Node require(ZoneId zone, String hostname) {
         return require(zone, HostName.from(hostname));
     }