Log model build errors to the deploy log

8 files changed, 48 insertions, 35 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index d1cf011d33a..52bb8442982 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -838,15 +838,14 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     public void validateThatSessionIsNotActive(Tenant tenant, long sessionId) {
         Session session = getRemoteSession(tenant, sessionId);
-        if (Session.Status.ACTIVATE.equals(session.getStatus())) {
-            throw new IllegalStateException("Session is active: " + sessionId);
-        }
+        if (Session.Status.ACTIVATE == session.getStatus())
+            throw new IllegalArgumentException("Session is active: " + sessionId);
     }
 
     public void validateThatSessionIsPrepared(Tenant tenant, long sessionId) {
         Session session = getRemoteSession(tenant, sessionId);
-        if ( ! Session.Status.PREPARE.equals(session.getStatus()))
-            throw new IllegalStateException("Session not prepared: " + sessionId);
+        if ( Session.Status.PREPARE != session.getStatus())
+            throw new IllegalArgumentException("Session not prepared: " + sessionId);
     }
 
     public long createSessionFromExisting(ApplicationId applicationId, boolean internalRedeploy, TimeoutBudget timeoutBudget) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/BadRequestException.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/BadRequestException.java
index 50b494d2d56..ca2ab456564 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/BadRequestException.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/BadRequestException.java
@@ -5,11 +5,11 @@ package com.yahoo.vespa.config.server.http;
  * Exception that will create a http response with BAD_REQUEST response code (400)
  *
  * @author hmusum
- * @since 5.1.17
  */
-public class BadRequestException extends RuntimeException {
+public class BadRequestException extends IllegalArgumentException {
 
     public BadRequestException(String message) {
         super(message);
     }
+
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java
index 78a7b8aaa8f..510fb3f4cc2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java
@@ -50,12 +50,12 @@ public class HttpHandler extends LoggingRequestHandler {
             return HttpErrorResponse.notFoundError(getMessage(e, request));
         } catch (ActivationConflictException e) {
             return HttpErrorResponse.conflictWhenActivating(getMessage(e, request));
-        } catch (BadRequestException | IllegalArgumentException | IllegalStateException e) {
+        } catch (InvalidApplicationException e) {
+            return HttpErrorResponse.invalidApplicationPackage(getMessage(e, request));
+        } catch (IllegalArgumentException e) {
             return HttpErrorResponse.badRequest(getMessage(e, request));
         } catch (OutOfCapacityException e) {
             return HttpErrorResponse.outOfCapacity(getMessage(e, request));
-        } catch (InvalidApplicationException e) {
-            return HttpErrorResponse.invalidApplicationPackage(getMessage(e, request));
         } catch (InternalServerException e) {
             return HttpErrorResponse.internalServerError(getMessage(e, request));
         } catch (UnknownVespaVersionException e) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/InvalidApplicationException.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/InvalidApplicationException.java
index ae7557588c8..9ff81bde130 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/InvalidApplicationException.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/InvalidApplicationException.java
@@ -4,7 +4,7 @@ package com.yahoo.vespa.config.server.http;
 /**
  * @author hmusum
  */
-public class InvalidApplicationException extends RuntimeException {
+public class InvalidApplicationException extends IllegalArgumentException {
 
     public InvalidApplicationException(String message) {
         super(message);
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
index f878831b8c5..4e7afa7b3db 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
@@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableSet;
 import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.ApplicationPackage;
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ConfigDefinitionRepo;
 import com.yahoo.config.model.api.Model;
 import com.yahoo.config.model.api.ModelContext;
@@ -80,7 +81,8 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
         super(modelFactoryRegistry,
               configserverConfig,
               zone,
-              hostProvisionerProvider);
+              hostProvisionerProvider,
+              new SilentDeployLogger());
         this.tenant = tenant;
         this.applicationGeneration = applicationGeneration;
         this.zkClient = zkClient;
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
index 227e9c6a977..707a828bdd6 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
@@ -5,6 +5,7 @@ import com.google.common.util.concurrent.UncheckedTimeoutException;
 import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.ApplicationPackage;
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.HostProvisioner;
 import com.yahoo.config.model.api.ModelFactory;
 import com.yahoo.config.model.api.Provisioned;
@@ -18,10 +19,12 @@ import com.yahoo.config.provision.TransientException;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.lang.SettableOptional;
 import com.yahoo.vespa.config.server.http.InternalServerException;
+import com.yahoo.vespa.config.server.http.InvalidApplicationException;
 import com.yahoo.vespa.config.server.http.UnknownVespaVersionException;
 import com.yahoo.vespa.config.server.provision.HostProvisionerProvider;
 import com.yahoo.vespa.config.server.provision.ProvisionerAdapter;
 import com.yahoo.vespa.config.server.provision.StaticProvisioner;
+import com.yahoo.yolean.Exceptions;
 
 import java.time.Duration;
 import java.time.Instant;
@@ -56,18 +59,26 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
 
     private final HostProvisionerProvider hostProvisionerProvider;
 
-    ModelsBuilder(ModelFactoryRegistry modelFactoryRegistry, ConfigserverConfig configserverConfig,
-                  Zone zone, HostProvisionerProvider hostProvisionerProvider) {
+    private final DeployLogger deployLogger;
+
+    ModelsBuilder(ModelFactoryRegistry modelFactoryRegistry,
+                  ConfigserverConfig configserverConfig,
+                  Zone zone,
+                  HostProvisionerProvider hostProvisionerProvider,
+                  DeployLogger deployLogger) {
         this.modelFactoryRegistry = modelFactoryRegistry;
         this.configserverConfig = configserverConfig;
         this.hosted = configserverConfig.hostedVespa();
         this.zone = zone;
         this.hostProvisionerProvider = hostProvisionerProvider;
+        this.deployLogger = deployLogger;
     }
 
     /** Returns the zone this is running in */
     protected Zone zone() { return zone; }
 
+    protected DeployLogger  deployLogger() { return deployLogger; }
+
     /**
      * Builds all applicable model versions
      * 
@@ -122,23 +133,26 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
             catch (RuntimeException e) {
                 if (shouldSkipCreatingMajorVersionOnError(majorVersions, majorVersion)) {
                     log.log(Level.INFO, applicationId + ": Skipping major version " + majorVersion, e);
-                } else  {
-                    if (e instanceof NullPointerException || e instanceof NoSuchElementException | e instanceof UncheckedTimeoutException) {
-                        log.log(Level.WARNING, "Unexpected error when building model ", e);
-                        throw new InternalServerException(applicationId + ": Error loading model", e);
-                    } else {
-                        log.log(Level.WARNING, "Input error when building model ", e);
-                        throw new IllegalArgumentException(applicationId + ": Error loading model", e);
+                }
+                else {
+                    if (e instanceof IllegalArgumentException) {
+                        var wrapped = new InvalidApplicationException("Error loading " + applicationId, e);
+                        deployLogger.logApplicationPackage(Level.SEVERE, Exceptions.toMessageString(wrapped));
+                        throw wrapped;
+                    }
+                    else {
+                        log.log(Level.WARNING, "Unexpected error building " + applicationId, e);
+                        throw new InternalServerException("Unexpected error building " + applicationId, e);
                     }
                 }
             }
         }
         log.log(Level.FINE, () -> "Done building models for " + applicationId + ". Built models for versions " +
-                            allApplicationModels.stream()
-                                                .map(result -> result.getModel().version())
-                                                .map(Version::toFullString)
-                                                .collect(Collectors.toSet()) +
-                            " in " + Duration.between(start, Instant.now()));
+                                  allApplicationModels.stream()
+                                                      .map(result -> result.getModel().version())
+                                                      .map(Version::toFullString)
+                                                      .collect(Collectors.toSet()) +
+                                  " in " + Duration.between(start, Instant.now()));
         return allApplicationModels;
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
index 7a0a261c7ad..86eee9e1623 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
@@ -52,7 +52,6 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
     private final PermanentApplicationPackage permanentApplicationPackage;
     private final ConfigDefinitionRepo configDefinitionRepo;
     private final HostValidator<ApplicationId> hostValidator;
-    private final DeployLogger logger;
     private final PrepareParams params;
     private final FileDistributionProvider fileDistributionProvider;
     private final Optional<ApplicationSet> currentActiveApplicationSet;
@@ -66,18 +65,17 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
                                  HostProvisionerProvider hostProvisionerProvider,
                                  Curator curator,
                                  HostValidator<ApplicationId> hostValidator,
-                                 DeployLogger logger,
+                                 DeployLogger deployLogger,
                                  PrepareParams params,
                                  Optional<ApplicationSet> currentActiveApplicationSet,
                                  ModelContext.Properties properties,
                                  ConfigserverConfig configserverConfig) {
-        super(modelFactoryRegistry, configserverConfig, properties.zone(), hostProvisionerProvider);
+        super(modelFactoryRegistry, configserverConfig, properties.zone(), hostProvisionerProvider, deployLogger);
         this.permanentApplicationPackage = permanentApplicationPackage;
         this.configDefinitionRepo = configDefinitionRepo;
         this.fileDistributionProvider = fileDistributionProvider;
         this.hostValidator = hostValidator;
         this.curator = curator;
-        this.logger = logger;
         this.params = params;
         this.currentActiveApplicationSet = currentActiveApplicationSet;
         this.properties = properties;
@@ -99,7 +97,7 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
                 applicationPackage,
                 modelOf(modelVersion),
                 permanentApplicationPackage.applicationPackage(),
-                logger,
+                deployLogger(),
                 configDefinitionRepo,
                 fileDistributionProvider.getFileRegistry(),
                 new ApplicationCuratorDatabase(applicationId.tenant(), curator).readReindexingStatus(applicationId),
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
index f1404b30f30..7f0a49e6d32 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
@@ -242,7 +242,7 @@ public class HostedDeployTest {
      * Tests that we create the minimal set of models and that version 7.x is created
      * if creating version 8.x fails (to support upgrades to new major version for applications
      * that are still using features that do not work on version 8.x)
-     **/
+     */
     @Test
     public void testCreateLatestMajorOnPreviousMajorIfItFailsOnMajorVersion8() throws IOException {
         deployWithModelForLatestMajorVersionFailing(8);
@@ -252,7 +252,7 @@ public class HostedDeployTest {
      * Tests that we fail deployment for version 7.x if creating version 7.x fails (i.e. that we do not skip
      * building 7.x and only build version 6.x). Skipping creation of models for a major version is only supported
      * for major version >= 8 (see test above) or when major-version=6 is set in application package.
-     **/
+     */
     @Test(expected = InvalidApplicationException.class)
     public void testFailingToCreateModelVersion7FailsDeployment() throws IOException {
         deployWithModelForLatestMajorVersionFailing(7);
@@ -286,7 +286,7 @@ public class HostedDeployTest {
 
     /**
      * Tests that we fail deployment if a needed model version fails to be created
-     **/
+     */
     @Test(expected = InvalidApplicationException.class)
     public void testDeploymentFailsIfNeededModelVersionFails() throws IOException {
         List<Host> hosts = createHosts(7, "7.0.0");
@@ -302,7 +302,7 @@ public class HostedDeployTest {
      * Test that deploying an application works when there are no allocated hosts in the system
      * (the bootstrap a new zone case, so deploying the routing app since that is the first deployment
      * that will be done)
-     **/
+     */
     @Test
     public void testCreateOnlyNeededModelVersionsWhenNoHostsAllocated() throws IOException {
         CountingModelFactory factory700 = createHostedModelFactory(Version.fromString("7.0.0"));