Merge pull request #18312 from vespa-engine/mpolden/faster-tests

Minor performance improvements to controller-server tests

6 files changed, 85 insertions, 59 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
index 2322b251fe0..4f01df21430 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
@@ -1,15 +1,14 @@
 // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.application;
 
-import com.google.common.collect.ImmutableList;
-
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
 import java.nio.file.Path;
-import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Predicate;
 import java.util.zip.ZipEntry;
@@ -21,19 +20,19 @@ import java.util.zip.ZipOutputStream;
  */
 public class ZipStreamReader {
 
-    private final ImmutableList<ZipEntryWithContent> entries;
+    private final List<ZipEntryWithContent> entries = new ArrayList<>();
     private final int maxEntrySizeInBytes;
 
     public ZipStreamReader(InputStream input, Predicate<String> entryNameMatcher, int maxEntrySizeInBytes) {
         this.maxEntrySizeInBytes = maxEntrySizeInBytes;
         try (ZipInputStream zipInput = new ZipInputStream(input)) {
-            ImmutableList.Builder<ZipEntryWithContent> builder = new ImmutableList.Builder<>();
             ZipEntry zipEntry;
+
             while (null != (zipEntry = zipInput.getNextEntry())) {
                 if (!entryNameMatcher.test(requireName(zipEntry.getName()))) continue;
-                builder.add(new ZipEntryWithContent(zipEntry, readContent(zipInput)));
+                entries.add(new ZipEntryWithContent(zipEntry, readContent(zipInput)));
             }
-            entries = builder.build();
+
         } catch (IOException e) {
             throw new UncheckedIOException("IO error reading zip content", e);
         }
@@ -79,10 +78,10 @@ public class ZipStreamReader {
         }
     }
 
-    public List<ZipEntryWithContent> entries() { return entries; }
+    public List<ZipEntryWithContent> entries() { return Collections.unmodifiableList(entries); }
 
     private static String requireName(String name) {
-        if (Arrays.asList(name.split("/")).contains("..") ||
+        if (List.of(name.split("/")).contains("..") ||
             !trimTrailingSlash(name).equals(Path.of(name).normalize().toString())) {
             throw new IllegalArgumentException("Unexpected non-normalized path found in zip content: '" + name + "'");
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index fc7a99eb2f0..78f688f545b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -27,6 +27,7 @@ import java.util.Date;
 import java.util.List;
 import java.util.OptionalInt;
 import java.util.StringJoiner;
+import java.util.zip.Deflater;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
 
@@ -273,27 +274,27 @@ public class ApplicationPackageBuilder {
         }
         ByteArrayOutputStream zip = new ByteArrayOutputStream();
         try (ZipOutputStream out = new ZipOutputStream(zip)) {
-            out.putNextEntry(new ZipEntry(dir + "deployment.xml"));
-            out.write(deploymentSpec());
-            out.closeEntry();
-            out.putNextEntry(new ZipEntry(dir + "validation-overrides.xml"));
-            out.write(validationOverrides());
-            out.closeEntry();
-            out.putNextEntry(new ZipEntry(dir + "search-definitions/test.sd"));
-            out.write(searchDefinition());
-            out.closeEntry();
-            out.putNextEntry(new ZipEntry(dir + "build-meta.json"));
-            out.write(buildMeta(compileVersion));
-            out.closeEntry();
-            out.putNextEntry(new ZipEntry(dir + "security/clients.pem"));
-            out.write(X509CertificateUtils.toPem(trustedCertificates).getBytes(UTF_8));
-            out.closeEntry();
+            out.setLevel(Deflater.NO_COMPRESSION); // This is for testing purposes so we skip compression for performance
+            writeZipEntry(out, dir + "deployment.xml", deploymentSpec());
+            writeZipEntry(out, dir + "validation-overrides.xml", validationOverrides());
+            writeZipEntry(out, dir + "search-definitions/test.sd", searchDefinition());
+            writeZipEntry(out, dir + "build-meta.json", buildMeta(compileVersion));
+            if (!trustedCertificates.isEmpty()) {
+                writeZipEntry(out, dir + "security/clients.pem", X509CertificateUtils.toPem(trustedCertificates).getBytes(UTF_8));
+            }
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }
         return new ApplicationPackage(zip.toByteArray());
     }
 
+    private void writeZipEntry(ZipOutputStream out, String name, byte[] content) throws IOException {
+        ZipEntry entry = new ZipEntry(name);
+        out.putNextEntry(entry);
+        out.write(content);
+        out.closeEntry();
+    }
+
     private static String asIso8601Date(Instant instant) {
         return new SimpleDateFormat("yyyy-MM-dd").format(Date.from(instant));
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
index 4203051965b..098282e4e89 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
@@ -64,7 +64,6 @@ import java.util.Set;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
-import java.util.stream.IntStream;
 
 import static com.yahoo.config.provision.NodeResources.DiskSpeed.slow;
 import static com.yahoo.config.provision.NodeResources.StorageType.remote;
@@ -168,18 +167,18 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     public void addNodes(List<ZoneId> zones, List<SystemApplication> applications) {
         for (ZoneId zone : zones) {
             for (SystemApplication application : applications) {
-                List<Node> nodes = IntStream.rangeClosed(1, 3)
-                                            .mapToObj(i -> new Node.Builder()
-                                                    .hostname(HostName.from("node-" + i + "-" + application.id().application()
-                                                            .value() + "-" + zone.value()))
-                                                    .state(Node.State.active)
-                                                    .type(application.nodeType())
-                                                    .owner(application.id())
-                                                    .currentVersion(initialVersion).wantedVersion(initialVersion)
-                                                    .currentOsVersion(Version.emptyVersion).wantedOsVersion(Version.emptyVersion)
-                                                    .build())
-                                            .collect(Collectors.toList());
-                nodeRepository().putNodes(zone, nodes);
+                for (int i = 1; i <= 3; i++) {
+                    Node node = new Node.Builder()
+                            .hostname(HostName.from("node-" + i + "-" + application.id().application()
+                                                                                   .value() + "-" + zone.value()))
+                            .state(Node.State.active)
+                            .type(application.nodeType())
+                            .owner(application.id())
+                            .currentVersion(initialVersion).wantedVersion(initialVersion)
+                            .currentOsVersion(Version.emptyVersion).wantedOsVersion(Version.emptyVersion)
+                            .build();
+                    nodeRepository().putNode(zone, node);
+                }
                 convergeServices(application.id(), zone);
             }
         }
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 afb56f10c38..4079591730d 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
@@ -59,9 +59,14 @@ public class NodeRepositoryMock implements NodeRepository {
 
     /** Add or update given nodes in zone */
     public void putNodes(ZoneId zone, List<Node> nodes) {
-        nodeRepository.putIfAbsent(zone, new HashMap<>());
-        nodeRepository.get(zone).putAll(nodes.stream().collect(Collectors.toMap(Node::hostname,
-                                                                                Function.identity())));
+        Map<HostName, Node> zoneNodes = nodeRepository.computeIfAbsent(zone, (k) -> new HashMap<>());
+        for (var node : nodes) {
+            zoneNodes.put(node.hostname(), node);
+        }
+    }
+
+    public void putNode(ZoneId zone, Node node) {
+        nodeRepository.computeIfAbsent(zone, (k) -> new HashMap<>()).put(node.hostname(), node);
     }
 
     public void putApplication(ZoneId zone, Application application) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
index 047a4461f7c..79b564eee52 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
@@ -66,9 +66,9 @@ public class RoutingPoliciesTest {
     private static final ZoneId zone3 = ZoneId.from("prod", "aws-us-east-1a");
     private static final ZoneId zone4 = ZoneId.from("prod", "aws-us-east-1b");
 
-    private final ApplicationPackage applicationPackage = applicationPackageBuilder().region(zone1.region())
-                                                                                     .region(zone2.region())
-                                                                                     .build();
+    private static final ApplicationPackage applicationPackage = applicationPackageBuilder().region(zone1.region())
+                                                                                            .region(zone2.region())
+                                                                                            .build();
 
     @Test
     public void global_routing_policies() {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
index 77ce86f1664..4dd283cf5d7 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
@@ -127,11 +127,7 @@ public class VersionStatusTest {
     @Test
     public void testVersionStatusAfterApplicationUpdates() {
         DeploymentTester tester = new DeploymentTester();
-        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
-                .upgradePolicy("default")
-                .region("us-west-1")
-                .region("us-east-3")
-                .build();
+        ApplicationPackage applicationPackage = applicationPackage("default");
 
         Version version1 = new Version("6.2");
         Version version2 = new Version("6.3");
@@ -216,10 +212,9 @@ public class VersionStatusTest {
         Version version0 = new Version("6.2");
         tester.controllerTester().upgradeSystem(version0);
         tester.upgrader().maintain();
-        var builder = new ApplicationPackageBuilder().region("us-west-1").region("us-east-3");
 
         // Setup applications - all running on version0
-        ApplicationPackage canaryPolicy = builder.upgradePolicy("canary").build();
+        ApplicationPackage canaryPolicy = applicationPackage("canary");
         var canary0 = tester.newDeploymentContext("tenant1", "canary0", "default")
                             .submit(canaryPolicy)
                             .deploy();
@@ -230,7 +225,7 @@ public class VersionStatusTest {
                             .submit(canaryPolicy)
                             .deploy();
 
-        ApplicationPackage defaultPolicy = builder.upgradePolicy("default").build();
+        ApplicationPackage defaultPolicy = applicationPackage("default");
         var default0 = tester.newDeploymentContext("tenant1", "default0", "default")
                              .submit(defaultPolicy)
                              .deploy();
@@ -262,7 +257,7 @@ public class VersionStatusTest {
                             .submit(defaultPolicy)
                             .deploy();
 
-        ApplicationPackage conservativePolicy = builder.upgradePolicy("conservative").build();
+        ApplicationPackage conservativePolicy = applicationPackage("conservative");
         var conservative0 = tester.newDeploymentContext("tenant1", "conservative0", "default")
                 .submit(conservativePolicy)
                 .deploy();
@@ -388,10 +383,10 @@ public class VersionStatusTest {
         Version version0 = new Version("6.2");
         tester.controllerTester().upgradeSystem(version0);
         tester.upgrader().maintain();
-        var appPackage = new ApplicationPackageBuilder().region("us-west-1").region("us-east-3").upgradePolicy("canary");
+        var appPackage = applicationPackage("canary");
 
         var canary0 = tester.newDeploymentContext("tenant1", "canary0", "default")
-                            .submit(appPackage.build())
+                            .submit(appPackage)
                             .deploy();
 
         assertEquals("All applications running on this version: High",
@@ -537,13 +532,13 @@ public class VersionStatusTest {
         Version version0 = Version.fromString("7.1");
         tester.controllerTester().upgradeSystem(version0);
         var canary0 = tester.newDeploymentContext("tenant1", "canary0", "default")
-                            .submit(new ApplicationPackageBuilder().upgradePolicy("canary").region("us-west-1").build())
+                            .submit(applicationPackage("canary"))
                             .deploy();
         var canary1 = tester.newDeploymentContext("tenant1", "canary1", "default")
-                            .submit(new ApplicationPackageBuilder().upgradePolicy("canary").region("us-west-1").build())
+                            .submit(applicationPackage("canary"))
                             .deploy();
         var default0 = tester.newDeploymentContext("tenant1", "default0", "default")
-                            .submit(new ApplicationPackageBuilder().upgradePolicy("default").region("us-west-1").build())
+                            .submit(applicationPackage("default"))
                             .deploy();
         tester.controllerTester().computeVersionStatus();
         assertSame(Confidence.high, tester.controller().readVersionStatus().version(version0).confidence());
@@ -609,12 +604,11 @@ public class VersionStatusTest {
     public void testStatusIncludesIncompleteUpgrades() {
         var tester = new DeploymentTester().atMondayMorning();
         var version0 = Version.fromString("7.1");
-        var applicationPackage = new ApplicationPackageBuilder().region("us-west-1").build();
 
         // Application deploys on initial version
         tester.controllerTester().upgradeSystem(version0);
         var context = tester.newDeploymentContext("tenant1", "default0", "default");
-        context.submit(applicationPackage).deploy();
+        context.submit(applicationPackage("default")).deploy();
 
         // System is upgraded and application starts upgrading to next version
         var version1 = Version.fromString("7.2");
@@ -688,4 +682,32 @@ public class VersionStatusTest {
                          .orElseThrow(() -> new IllegalArgumentException("Expected to find version: " + version));
     }
 
+    private static final ApplicationPackage canaryApplicationPackage =
+            new ApplicationPackageBuilder().upgradePolicy("canary")
+                                           .region("us-west-1")
+                                           .region("us-east-3")
+                                           .build();
+
+    private static final ApplicationPackage defaultApplicationPackage =
+            new ApplicationPackageBuilder().upgradePolicy("default")
+                                           .region("us-west-1")
+                                           .region("us-east-3")
+                                           .build();
+
+    private static final ApplicationPackage conservativeApplicationPackage =
+            new ApplicationPackageBuilder().upgradePolicy("conservative")
+                                           .region("us-west-1")
+                                           .region("us-east-3")
+                                           .build();
+
+    /** Returns empty prebuilt applications for efficiency */
+    private ApplicationPackage applicationPackage(String upgradePolicy) {
+        switch (upgradePolicy) {
+            case "canary" : return canaryApplicationPackage;
+            case "default" : return defaultApplicationPackage;
+            case "conservative" : return conservativeApplicationPackage;
+            default : throw new IllegalArgumentException("No upgrade policy '" + upgradePolicy + "'");
+        }
+    }
+
 }