Merge pull request #32235 from vespa-engine/hmusum/cleanup-allocateHo…

…st-1 Start cleaning up HostProvisioner and subclasses
vespa-engine · Aug 27, 2024 · 5f73a17 · 5f73a17
2 parents 2783dd3 + 6ca08ac
commit 5f73a17
Show file tree

Hide file tree

Showing 10 changed files with 25 additions and 56 deletions.
diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
@@ -1262,7 +1262,7 @@
       "abstract"
     ],
     "methods" : [
-      "public abstract com.yahoo.config.provision.HostSpec allocateHost(java.lang.String)",
+      "public com.yahoo.config.provision.HostSpec allocateHost(java.lang.String)",
       "public abstract java.util.List prepare(com.yahoo.config.provision.ClusterSpec, com.yahoo.config.provision.Capacity, com.yahoo.config.provision.ProvisionLogger)"
     ],
     "fields" : [ ]

diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/HostProvisioner.java b/config-model-api/src/main/java/com/yahoo/config/model/api/HostProvisioner.java
@@ -19,7 +19,9 @@ public interface HostProvisioner {
 
     /** Allocates a single host for a service */
     // TODO: Remove
-    HostSpec allocateHost(String alias);
+    default HostSpec allocateHost(String alias) {
+        throw new UnsupportedOperationException("Allocating a single host is not supported");
+    }
 
     /**
      * Prepares allocation of a set of hosts with a given type, common id and the amount.

diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java
@@ -37,7 +37,7 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea
     private List<ConfigServerSpec> configServerSpecs = List.of();
     private boolean hostedVespa = false;
     private Zone zone = Zone.defaultZone();
-    private final Set<ContainerEndpoint> endpoints = Set.of();
+    private Set<ContainerEndpoint> endpoints = Set.of();
     private boolean useDedicatedNodeForLogserver = false;
     private double defaultTermwiseLimit = 1.0;
     private String jvmGCOptions = null;
@@ -403,6 +403,11 @@ public TestProperties setDistributionConfigFromClusterController(boolean configF
         return this;
     }
 
+    public TestProperties setContainerEndpoints(Set<ContainerEndpoint> containerEndpoints) {
+        this.endpoints = containerEndpoints;
+        return this;
+    }
+
     public static class Spec implements ConfigServerSpec {
 
         private final String hostName;

diff --git a/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java b/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java
@@ -152,6 +152,7 @@ private static Collection<Host> createHostInstances(int hostCount) {
     public HostSpec allocateHost(String alias) {
         List<Host> defaultHosts = freeNodes.get(defaultHostResources);
         if (defaultHosts.isEmpty()) throw new IllegalArgumentException("No more hosts with default resources available");
+
         Host newHost = freeNodes.removeValue(defaultHostResources, 0);
         return new HostSpec(newHost.hostname(), Optional.empty());
     }

diff --git a/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java b/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java
@@ -3,6 +3,8 @@
 
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.model.MockModelContext;
+import com.yahoo.config.model.api.ApplicationClusterEndpoint;
+import com.yahoo.config.model.api.ContainerEndpoint;
 import com.yahoo.config.model.api.HostInfo;
 import com.yahoo.config.model.api.HostProvisioner;
 import com.yahoo.config.model.api.Model;
@@ -24,6 +26,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 
 import static org.junit.jupiter.api.Assertions.*;
 
@@ -80,15 +83,6 @@ void testThatFactoryModelValidationCanBeIgnored() {
     void hostedVespaZoneApplicationAllocatesNodesFromNodeRepo() {
         String hostName = "test-host-name";
         String routingClusterName = "routing-cluster";
-
-        String hosts =
-                "<?xml version='1.0' encoding='utf-8' ?>\n" +
-                        "<hosts>\n" +
-                        "  <host name='" + hostName + "'>\n" +
-                        "    <alias>proxy1</alias>\n" +
-                        "  </host>\n" +
-                        "</hosts>";
-
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
                         "<services version='1.0' xmlns:deploy='vespa'>\n" +
@@ -100,25 +94,13 @@ void hostedVespaZoneApplicationAllocatesNodesFromNodeRepo() {
                         "    </container>\n" +
                         "</services>";
 
-        HostProvisioner provisionerToOverride = new HostProvisioner() {
-            @Override
-            public HostSpec allocateHost(String alias) {
-                return new HostSpec(hostName,
-                        NodeResources.unspecified(), NodeResources.unspecified(), NodeResources.unspecified(),
-                        ClusterMembership.from(ClusterSpec.request(ClusterSpec.Type.admin, new ClusterSpec.Id(routingClusterName)).vespaVersion("6.42").build(), 0),
-                        Optional.empty(), Optional.empty(), Optional.empty());
-            }
-
-            @Override
-            public List<HostSpec> prepare(ClusterSpec cluster, Capacity capacity, ProvisionLogger logger) {
-                return List.of(new HostSpec(hostName,
-                        NodeResources.unspecified(), NodeResources.unspecified(), NodeResources.unspecified(),
-                        ClusterMembership.from(ClusterSpec.request(ClusterSpec.Type.container, new ClusterSpec.Id(routingClusterName)).vespaVersion("6.42").build(), 0),
-                        Optional.empty(), Optional.empty(), Optional.empty()));
-            }
-        };
+        HostProvisioner provisionerToOverride = (cluster, capacity, logger) ->
+                List.of(new HostSpec(hostName,
+                                     NodeResources.unspecified(), NodeResources.unspecified(), NodeResources.unspecified(),
+                                     ClusterMembership.from(ClusterSpec.request(ClusterSpec.Type.container, new ClusterSpec.Id(routingClusterName)).vespaVersion("6.42").build(), 0),
+                                     Optional.empty(), Optional.empty(), Optional.empty()));
 
-        ModelContext modelContext = createMockModelContext(hosts, services, provisionerToOverride);
+        ModelContext modelContext = createMockModelContext(null, services, provisionerToOverride, routingClusterName);
         Model model = VespaModelFactory.createTestFactory().createModel(modelContext);
 
         List<HostInfo> allocatedHosts = new ArrayList<>(model.getHosts());
@@ -132,7 +114,7 @@ public List<HostSpec> prepare(ClusterSpec cluster, Capacity capacity, ProvisionL
                 "Routing service should run on host " + hostName);
     }
 
-    private ModelContext createMockModelContext(String hosts, String services, HostProvisioner provisionerToOverride) {
+    private ModelContext createMockModelContext(String hosts, String services, HostProvisioner provisionerToOverride, String clusterName) {
         return new MockModelContext() {
             @Override
             public ApplicationPackage applicationPackage() {
@@ -143,9 +125,9 @@ public ApplicationPackage applicationPackage() {
             public HostProvisioner getHostProvisioner() { return provisionerToOverride; }
 
             @Override
-            public Properties properties() {
-                return new TestProperties();
-            }
+            public Properties properties() { return new TestProperties()
+                    .setHostedVespa(true)
+                    .setContainerEndpoints(Set.of(new ContainerEndpoint(clusterName, ApplicationClusterEndpoint.Scope.zone, List.of("tc.example.com")))); }
         };
     }
 

diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java b/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java
@@ -3,7 +3,6 @@
 
 import com.yahoo.component.Version;
 
-import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 
@@ -38,14 +37,6 @@ public HostSpec(String hostname, Optional<NetworkPorts> networkPorts) {
              Optional.empty(), Optional.empty(), networkPorts, Optional.empty());
     }
 
-    // TODO: Remove when oldest model in use is 8.124
-    @Deprecated
-    public HostSpec(String hostname, List<String> ignored, Optional<NetworkPorts> networkPorts) {
-        this(hostname,
-             NodeResources.unspecified(), NodeResources.unspecified(), NodeResources.unspecified(),
-             Optional.empty(), Optional.empty(), networkPorts, Optional.empty());
-    }
-
     /** Create a host in a hosted system */
     public HostSpec(String hostname,
                     NodeResources realResources,

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/provision/ProvisionerAdapter.java b/configserver/src/main/java/com/yahoo/vespa/config/server/provision/ProvisionerAdapter.java
@@ -35,7 +35,7 @@ public ProvisionerAdapter(Provisioner provisioner, ApplicationId applicationId,
     @Override
     public HostSpec allocateHost(String alias) {
         // TODO: Remove this method since hosted/non-hosted needs different interfaces. See also ModelContextImpl.getHostProvisioner
-        // Illegal argument becaiuse we end here by following a path not suitable for the environment steered
+        // Illegal argument because we end here by following a path not suitable for the environment steered
         // by the content of the nodes tag in services
         throw new IllegalArgumentException("Clusters in hosted environments must have a <nodes count='N'> tag " +
                                                 "matching all zones, and having no <node> subtags, " +

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java b/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java
@@ -5,7 +5,6 @@
 import com.yahoo.config.provision.*;
 
 import java.util.List;
-import java.util.stream.Collectors;
 
 /**
  * Host provisioning from an existing {@link AllocatedHosts} instance.
@@ -28,11 +27,6 @@ public StaticProvisioner(AllocatedHosts allocatedHosts, HostProvisioner fallback
         this.fallback = fallback;
     }
 
-    @Override
-    public HostSpec allocateHost(String alias) {
-        throw new UnsupportedOperationException("Allocating a single host from provisioning info is not supported");
-    }
-
     @Override
     public List<HostSpec> prepare(ClusterSpec cluster, Capacity capacity, ProvisionLogger logger) {
         List<HostSpec> hostsAlreadyAllocatedToCluster = 

diff --git a/...er/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployNodeAllocationTest.java b/...er/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployNodeAllocationTest.java
@@ -103,11 +103,6 @@ private static class VersionProvisioner implements HostProvisioner {
 
         int invocation = 0;
 
-        @Override
-        public HostSpec allocateHost(String alias) {
-            throw new RuntimeException();
-        }
-
         @Override
         public List<HostSpec> prepare(ClusterSpec cluster, Capacity capacity, ProvisionLogger logger) {
             if (cluster.id().value().equals("container")) { // the container cluster from the app package: Use this to test

diff --git a/standalone-container/src/main/java/com/yahoo/container/standalone/LocalFileDb.java b/standalone-container/src/main/java/com/yahoo/container/standalone/LocalFileDb.java
@@ -15,7 +15,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
 
 /**
  * FileAcquirer and FileRegistry working on a local directory.