Merge pull request #12397 from vespa-engine/jvenstad/integration-tests-with-new-deploy-path

Jvenstad/integration tests with new deploy path

6 files changed, 75 insertions, 81 deletions

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 914eadc90e9..0264f92d389 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
@@ -20,6 +20,7 @@ import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.flags.FlagSource;
 import com.yahoo.vespa.hosted.controller.api.ActivateResult;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
+import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeploymentData;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.ConfigChangeActions;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.Hostname;
@@ -346,9 +347,8 @@ public class ApplicationController {
             } // Release application lock while doing the deployment, which is a lengthy task.
 
             // Carry out deployment without holding the application lock.
-            options = withVersion(platformVersion, options);
-            ActivateResult result = deploy(instanceId, applicationPackage, zone, options, endpoints,
-                                           endpointCertificateMetadata);
+            ActivateResult result = deploy(instanceId, applicationPackage, zone, platformVersion, options.ignoreValidationErrors,
+                                           endpoints, endpointCertificateMetadata);
 
             lockApplicationOrThrow(applicationId, application ->
                     store(application.with(instanceId.instance(),
@@ -420,25 +420,24 @@ public class ApplicationController {
             ApplicationPackage applicationPackage = new ApplicationPackage(
                     artifactRepository.getSystemApplicationPackage(application.id(), zone, version)
             );
-            DeployOptions options = withVersion(version, DeployOptions.none());
-            return deploy(application.id(), applicationPackage, zone, options, Set.of(), /* No application cert */ Optional.empty());
+            return deploy(application.id(), applicationPackage, zone, version, false, Set.of(), /* No application cert */ Optional.empty());
         } else {
            throw new RuntimeException("This system application does not have an application package: " + application.id().toShortString());
         }
     }
 
     /** Deploys the given tester application to the given zone. */
-    public ActivateResult deployTester(TesterId tester, ApplicationPackage applicationPackage, ZoneId zone, DeployOptions options) {
-        return deploy(tester.id(), applicationPackage, zone, options, Set.of(), /* No application cert for tester*/ Optional.empty());
+    public ActivateResult deployTester(TesterId tester, ApplicationPackage applicationPackage, ZoneId zone, Version platform) {
+        return deploy(tester.id(), applicationPackage, zone, platform, false, Set.of(), /* No application cert for tester*/ Optional.empty());
     }
 
     private ActivateResult deploy(ApplicationId application, ApplicationPackage applicationPackage,
-                                  ZoneId zone, DeployOptions deployOptions, Set<ContainerEndpoint> endpoints,
+                                  ZoneId zone, Version platform, boolean ignoreValidationErrors, Set<ContainerEndpoint> endpoints,
                                   Optional<EndpointCertificateMetadata> endpointCertificateMetadata) {
-        DeploymentId deploymentId = new DeploymentId(application, zone);
         try {
             ConfigServer.PreparedApplication preparedApplication =
-                    configServer.deploy(deploymentId, deployOptions, endpoints, endpointCertificateMetadata, applicationPackage.zippedContent());
+                    configServer.deploy(new DeploymentData(application, zone, applicationPackage.zippedContent(), platform,
+                                                           ignoreValidationErrors, endpoints, endpointCertificateMetadata));
             return new ActivateResult(new RevisionId(applicationPackage.hash()), preparedApplication.prepareResponse(),
                                       applicationPackage.zippedContent().length);
         } finally {
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 dedabd8caf2..861c8089ed5 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
@@ -159,7 +159,7 @@ public class InternalStepRunner implements StepRunner {
         }
         catch (RuntimeException e) {
             logger.log(WARNING, "Unexpected exception running " + id, e);
-            if (JobProfile.of(id.type()).alwaysRun().contains(step.get())) {
+            if (step.get().alwaysRun()) {
                 logger.log("Will keep trying, as this is a cleanup step.");
                 return Optional.empty();
             }
@@ -190,7 +190,7 @@ public class InternalStepRunner implements StepRunner {
                 : Optional.empty();
 
         Optional<Version> vespaVersion = id.type().environment().isManuallyDeployed()
-                ? Optional.of(versions.targetPlatform())
+                ? Optional.of(versions.targetPlatform()) // TODO jonmv: This makes it impossible to deploy on older majors — fix.
                 : Optional.empty();
         return deploy(id.application(),
                       id.type(),
@@ -215,10 +215,7 @@ public class InternalStepRunner implements StepRunner {
                       () -> controller.applications().deployTester(id.tester(),
                                                                    testerPackage(id),
                                                                    id.type().zone(controller.system()),
-                                                                   new DeployOptions(true,
-                                                                                     Optional.of(platform),
-                                                                                     false,
-                                                                                     false)),
+                                                                   platform),
                       controller.jobController().run(id).get()
                                 .stepInfo(deployTester).get()
                                 .startTime().get(),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
index 88b3442bd6e..1c1d60a2cf0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
@@ -34,8 +34,8 @@ public enum JobProfile {
                           deployTester,
                           installTester,
                           startTests,
-                          endTests),
-               EnumSet.of(copyVespaLogs,
+                          endTests,
+                          copyVespaLogs,
                           deactivateTester,
                           deactivateReal,
                           report)),
@@ -49,37 +49,35 @@ public enum JobProfile {
                            deployReal,
                            installReal,
                            startTests,
-                           endTests),
-                EnumSet.of(copyVespaLogs,
+                           endTests,
+                           copyVespaLogs,
                            deactivateTester,
                            deactivateReal,
                            report)),
 
     production(EnumSet.of(deployReal,
-                          installReal),
-               EnumSet.of(report)),
+                          installReal,
+                          report)),
 
     productionTest(EnumSet.of(deployTester,
                               installTester,
                               startTests,
-                              endTests),
-                   EnumSet.of(deactivateTester,
+                              endTests,
+                              deactivateTester,
                               report)),
 
     development(EnumSet.of(deployReal,
-                           installReal),
-                EnumSet.of(copyVespaLogs));
+                           installReal,
+                           copyVespaLogs));
 
 
     private final Set<Step> steps;
-    private final Set<Step> alwaysRun;
 
-    JobProfile(Set<Step> runWhileSuccess, Set<Step> alwaysRun) {
-        runWhileSuccess.addAll(alwaysRun);
-        this.steps = Collections.unmodifiableSet(runWhileSuccess);
-        this.alwaysRun = Collections.unmodifiableSet(alwaysRun);
+    JobProfile(Set<Step> steps) {
+        this.steps = Collections.unmodifiableSet(steps);
     }
 
+    // TODO jonmv: Let caller decide profile, and store with run?
     public static JobProfile of(JobType type) {
         switch (type.environment()) {
             case test: return systemTest;
@@ -94,7 +92,4 @@ public enum JobProfile {
     /** Returns all steps in this profile, the default for which is to run only when all prerequisites are successes. */
     public Set<Step> steps() { return steps; }
 
-    /** Returns the set of steps that should always be run, regardless of outcome. */
-    public Set<Step> alwaysRun() { return alwaysRun; }
-
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Run.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Run.java
index 8cd57fa7d3a..9bef405e0c7 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Run.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Run.java
@@ -268,9 +268,9 @@ public class Run {
     private List<Step> forcedSteps() {
         return ImmutableList.copyOf(steps.entrySet().stream()
                                          .filter(entry ->    entry.getValue().status() == unfinished
-                                                          && JobProfile.of(id.type()).alwaysRun().contains(entry.getKey())
+                                                          && entry.getKey().alwaysRun()
                                                           && entry.getKey().prerequisites().stream()
-                                                                  .filter(JobProfile.of(id.type()).alwaysRun()::contains)
+                                                                  .filter(Step::alwaysRun)
                                                                   .allMatch(step ->    steps.get(step) == null
                                                                                     || steps.get(step).status() != unfinished))
                                          .map(Map.Entry::getKey)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
index 44a93a655a8..75d875a29c5 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
@@ -26,54 +26,59 @@ import java.util.List;
 public enum Step {
 
     /** Download test-jar and assemble and deploy tester application. */
-    deployTester,
+    deployTester(false),
 
     /** See that tester is done deploying, and is ready to serve. */
-    installTester(deployTester),
+    installTester(false, deployTester),
 
     /** Download and deploy the initial real application, for staging tests. */
-    deployInitialReal(deployTester),
+    deployInitialReal(false, deployTester),
 
     /** See that the real application has had its nodes converge to the initial state. */
-    installInitialReal(deployInitialReal),
+    installInitialReal(false, deployInitialReal),
 
     /** Ask the tester to run its staging setup. */
-    startStagingSetup(installInitialReal, installTester),
+    startStagingSetup(false, installInitialReal, installTester),
 
     /** See that the staging setup is done. */
-    endStagingSetup(startStagingSetup),
+    endStagingSetup(false, startStagingSetup),
 
     /** Download and deploy real application, restarting services if required. */
-    deployReal(endStagingSetup, deployTester),
+    deployReal(false, endStagingSetup, deployTester),
 
     /** See that real application has had its nodes converge to the wanted version and generation. */
-    installReal(deployReal),
+    installReal(false, deployReal),
 
     /** Ask the tester to run its tests. */
-    startTests(installReal, installTester),
+    startTests(false, installReal, installTester),
 
     /** See that the tests are done running. */
-    endTests(startTests),
+    endTests(false, startTests),
 
     /** Fetch and store Vespa logs from the log server cluster of the deployment -- used for test and dev deployments. */
-    copyVespaLogs(installReal, endTests),
+    copyVespaLogs(true, installReal, endTests),
 
     /** Delete the real application -- used for test deployments. */
-    deactivateReal(deployInitialReal, deployReal, endTests, copyVespaLogs),
+    deactivateReal(true, deployInitialReal, deployReal, endTests, copyVespaLogs),
 
     /** Deactivate the tester. */
-    deactivateTester(deployTester, endTests),
+    deactivateTester(true, deployTester, endTests),
 
     /** Report completion to the deployment orchestration machinery. */
-    report(deactivateReal, deactivateTester);
+    report(true, deactivateReal, deactivateTester);
 
 
+    private final boolean alwaysRun;
     private final List<Step> prerequisites;
 
-    Step(Step... prerequisites) {
+    Step(boolean alwaysRun, Step... prerequisites) {
+        this.alwaysRun = alwaysRun;
         this.prerequisites = ImmutableList.copyOf(prerequisites);
     }
 
+    /** Returns whether this is a cleanup-step, and should always run, regardless of job outcome, when specified in a job. */
+    public boolean alwaysRun() { return alwaysRun; }
+
     /** Returns the prerequisite steps that must be successfully completed before this, assuming the job contains these steps. */
     public List<Step> prerequisites() { return prerequisites; }
 
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 42e4df7f565..204e0a1c1b8 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
@@ -13,7 +13,7 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.flags.json.FlagData;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.ClusterMetrics;
-import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
+import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeploymentData;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.ConfigChangeActions;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -21,7 +21,6 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.Hostname;
 import com.yahoo.vespa.hosted.controller.api.identifiers.Identifier;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.api.integration.LogEntry;
-import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServer;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.ContainerEndpoint;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer;
@@ -317,52 +316,51 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     }
 
     @Override
-    public PreparedApplication deploy(DeploymentId deployment, DeployOptions deployOptions,
-                                      Set<ContainerEndpoint> containerEndpoints,
-                                      Optional<EndpointCertificateMetadata> endpointCertificateMetadata, byte[] content) {
-        lastPrepareVersion = deployOptions.vespaVersion.map(Version::fromString).orElse(null);
+    public PreparedApplication deploy(DeploymentData deployment) {
+        lastPrepareVersion = deployment.platform();
         if (prepareException != null) {
             RuntimeException prepareException = this.prepareException;
             this.prepareException = null;
             throw prepareException;
         }
-        applications.put(deployment, new Application(deployment.applicationId(), lastPrepareVersion, new ApplicationPackage(content)));
+        DeploymentId id = new DeploymentId(deployment.instance(), deployment.zone());
+        applications.put(id, new Application(id.applicationId(), lastPrepareVersion, new ApplicationPackage(deployment.applicationPackage())));
 
-        if (nodeRepository().list(deployment.zoneId(), deployment.applicationId()).isEmpty())
-            provision(deployment.zoneId(), deployment.applicationId());
+        if (nodeRepository().list(id.zoneId(), id.applicationId()).isEmpty())
+            provision(id.zoneId(), id.applicationId());
 
         this.rotationNames.put(
-                deployment,
-                containerEndpoints.stream()
-                                  .map(ContainerEndpoint::names)
-                                  .flatMap(Collection::stream)
-                                  .collect(Collectors.toSet())
+                id,
+                deployment.containerEndpoints().stream()
+                          .map(ContainerEndpoint::names)
+                          .flatMap(Collection::stream)
+                          .collect(Collectors.toSet())
         );
 
-        if (!deferLoadBalancerProvisioning.contains(deployment.zoneId().environment())) {
-            putLoadBalancers(deployment.zoneId(), List.of(new LoadBalancer(UUID.randomUUID().toString(),
-                                                                           deployment.applicationId(),
-                                                                           ClusterSpec.Id.from("default"),
-                                                                           HostName.from("lb-0--" + deployment.applicationId().serializedForm() + "--" + deployment.zoneId().toString()),
-                                                                           LoadBalancer.State.active,
-                                                                           Optional.of("dns-zone-1"))));
+        if (!deferLoadBalancerProvisioning.contains(id.zoneId().environment())) {
+            putLoadBalancers(id.zoneId(), List.of(new LoadBalancer(UUID.randomUUID().toString(),
+                                                                   id.applicationId(),
+                                                                   ClusterSpec.Id.from("default"),
+                                                                   HostName.from("lb-0--" + id.applicationId().serializedForm() + "--" + id.zoneId().toString()),
+                                                                   LoadBalancer.State.active,
+                                                                   Optional.of("dns-zone-1"))));
         }
 
         return () -> {
-            Application application = applications.get(deployment);
+            Application application = applications.get(id);
             application.activate();
-            List<Node> nodes = nodeRepository.list(deployment.zoneId(), deployment.applicationId());
+            List<Node> nodes = nodeRepository.list(id.zoneId(), id.applicationId());
             for (Node node : nodes) {
-                nodeRepository.putByHostname(deployment.zoneId(), new Node.Builder(node)
+                nodeRepository.putByHostname(id.zoneId(), new Node.Builder(node)
                         .state(Node.State.active)
                         .wantedVersion(application.version().get())
                         .build());
             }
-            serviceStatus.put(deployment, new ServiceConvergence(deployment.applicationId(),
-                                                                 deployment.zoneId(),
-                                                                 false,
-                                                                 2,
-                                                                 nodes.stream()
+            serviceStatus.put(id, new ServiceConvergence(id.applicationId(),
+                                                         id.zoneId(),
+                                                         false,
+                                                         2,
+                                                         nodes.stream()
                                                                       .map(node -> new ServiceConvergence.Status(node.hostname(),
                                                                                                                  43,
                                                                                                                  "container",
@@ -377,7 +375,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
                                               Collections.emptyList());
             setConfigChangeActions(null);
             prepareResponse.tenant = new TenantId("tenant");
-            prepareResponse.log = warnings.getOrDefault(deployment, Collections.emptyList());
+            prepareResponse.log = warnings.getOrDefault(id, Collections.emptyList());
             return prepareResponse;
         };
     }