Merge pull request #7113 from vespa-engine/jvenstad/validate-zones-have-jobs

Validate zones w.r.t. deployment jobs as well!

4 files changed, 37 insertions, 25 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
index a6c3f11470d..cd5178d80ac 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
@@ -199,6 +199,7 @@ public class Application {
         // Rotation status only contains VIP host names, one per zone in the system. The only way to map VIP hostname to
         // this deployment, and thereby determine rotation status, is to check if VIP hostname contains the
         // deployment's environment and region.
+        // TODO: change this map to be indexed by zones, then?
         return rotationStatus.entrySet().stream()
                              .filter(kv -> kv.getKey().value().contains(deployment.zone().value()))
                              .map(Map.Entry::getValue)
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 2dd3b1dda23..b3158102182 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
@@ -44,6 +44,7 @@ import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.JobStatus.JobRun;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.concurrent.Once;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentSteps;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.controller.rotation.Rotation;
@@ -330,13 +331,9 @@ public class ApplicationController {
                 validateRun(application.get(), zone, platformVersion, applicationVersion);
             }
 
-            validate(applicationPackage.deploymentSpec());
-
             // Update application with information from application package
-            if ( ! preferOldestVersion && ! application.get().deploymentJobs().builtInternally()) {
-                application = withUpdatedConfig(application, applicationPackage);
-                store(application); // store missing information even if we fail deployment below
-            }
+            if ( ! preferOldestVersion && ! application.get().deploymentJobs().builtInternally())
+                application = storeWithUpdatedConfig(application, applicationPackage);
 
             // Assign global rotation
             application = withRotation(application, zone);
@@ -359,8 +356,9 @@ public class ApplicationController {
     }
 
     /** Stores the deployment spec and validation overrides from the application package, and runs cleanup. */
-    public LockedApplication withUpdatedConfig(LockedApplication application, ApplicationPackage applicationPackage) {
-        // Store information about application package
+    public LockedApplication storeWithUpdatedConfig(LockedApplication application, ApplicationPackage applicationPackage) {
+        validate(applicationPackage.deploymentSpec());
+
         application = application.with(applicationPackage.deploymentSpec());
         application = application.with(applicationPackage.validationOverrides());
 
@@ -649,6 +647,7 @@ public class ApplicationController {
 
     /** Verify that each of the production zones listed in the deployment spec exist in this system. */
     private void validate(DeploymentSpec deploymentSpec) {
+        new DeploymentSteps(deploymentSpec, controller::system).jobs();
         deploymentSpec.zones().stream()
                 .filter(zone -> zone.environment() == Environment.prod)
                 .forEach(zone -> {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index e9b0267a443..3cff6ac2430 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -203,32 +203,31 @@ public class JobController {
      * Accepts and stores a new application package and test jar pair under a generated application version key.
      */
     public ApplicationVersion submit(ApplicationId id, SourceRevision revision,
-                                     byte[] applicationPackage, byte[] applicationTestPackage) {
+                                     byte[] packageBytes, byte[] testPackageBytes) {
         AtomicReference<ApplicationVersion> version = new AtomicReference<>();
         controller.applications().lockOrThrow(id, application -> {
+            if ( ! application.get().deploymentJobs().builtInternally()) {
+                // Copy all current packages to the new application store
+                application.get().deployments().values().stream()
+                           .map(Deployment::applicationVersion)
+                           .distinct()
+                           .forEach(appVersion -> {
+                               byte[] content = controller.applications().artifacts().getApplicationPackage(application.get().id(), appVersion.id());
+                               controller.applications().applicationStore().putApplicationPackage(application.get().id(), appVersion.id(), content);
+                           });
+            }
+
             long run = nextBuild(id);
             version.set(ApplicationVersion.from(revision, run));
 
             controller.applications().applicationStore().putApplicationPackage(id,
-                    version.get().id(),
-                    applicationPackage);
+                                                                               version.get().id(),
+                                                                               packageBytes);
             controller.applications().applicationStore().putTesterPackage(testerOf(id),
                                                                           version.get().id(),
-                                                                          applicationTestPackage);
-
-            if (!application.get().deploymentJobs().builtInternally()) {
-                // Copy all current packages to the new application store
-                application.get().deployments().values().stream()
-                        .map(Deployment::applicationVersion)
-                        .distinct()
-                        .forEach(appVersion -> {
-                            byte[] content = controller.applications().artifacts().getApplicationPackage(application.get().id(), appVersion.id());
-                            controller.applications().applicationStore().putApplicationPackage(application.get().id(), appVersion.id(), content);
-                        });
-            }
+                                                                          testPackageBytes);
 
-            controller.applications().store(controller.applications().withUpdatedConfig(application.withBuiltInternally(true),
-                    new ApplicationPackage(applicationPackage)));
+            controller.applications().storeWithUpdatedConfig(application.withBuiltInternally(true), new ApplicationPackage(packageBytes));
 
             notifyOfNewSubmission(id, revision, run);
         });
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 635e0c1fb26..a9353af1b20 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -147,6 +147,19 @@ public class ControllerTest {
 
         assertEquals(5, applications.get(app1.id()).get().deploymentJobs().jobStatus().size());
 
+        // Production zone for which there is no JobType is not allowed.
+        applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .region("deep-space-9")
+                .build();
+        try {
+            tester.controller().jobController().submit(app1.id(), BuildJob.defaultSourceRevision, applicationPackage.zippedContent(), new byte[0]);
+            fail("Expected exception due to illegal deployment spec.");
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("No job is known for zone prod.deep-space-9 in default.", e.getMessage());
+        }
+
         // prod zone removal is not allowed
         applicationPackage = new ApplicationPackageBuilder()
                 .environment(Environment.prod)