Merge pull request #2934 from vespa-engine/hmusum/use-409-response-code-for-failing-activation

Return 409 if there is a conflict when activating

5 files changed, 44 insertions, 15 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java
new file mode 100644
index 00000000000..48023293cc4
--- /dev/null
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java
@@ -0,0 +1,15 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.config.server;
+
+/**
+ * Exception used when activation cannot be done because activation is for
+ * an older session than the one that is active now or because current active
+ * session has changed since the session to be activated was created
+ *
+ * @author hmusum
+ */
+public class ActivationConflictException extends RuntimeException {
+    public ActivationConflictException(String s) {
+        super(s);
+    }
+}
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
index 51995eb98cf..14c130ab463 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
@@ -9,6 +9,7 @@ import com.yahoo.log.LogLevel;
 import com.yahoo.path.Path;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.transaction.Transaction;
+import com.yahoo.vespa.config.server.ActivationConflictException;
 import com.yahoo.vespa.config.server.tenant.ActivateLock;
 import com.yahoo.vespa.config.server.ApplicationRepository;
 import com.yahoo.vespa.config.server.TimeoutBudget;
@@ -202,25 +203,25 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
                                 ", current active session=" + currentActiveSessionSessionId);
         if (currentActiveSession.isNewerThan(activeSessionAtCreate) &&
                 currentActiveSessionSessionId != sessionId) {
-            String errMsg = currentActiveSession.logPre()+"Cannot activate session " +
+            String errMsg = currentActiveSession.logPre() + "Cannot activate session " +
                             sessionId + " because the currently active session (" +
                             currentActiveSessionSessionId + ") has changed since session " + sessionId +
                             " was created (was " + activeSessionAtCreate + " at creation time)";
             if (ignoreStaleSessionFailure) {
                 log.warning(errMsg + " (Continuing because of force.)");
             } else {
-                throw new IllegalStateException(errMsg);
+                throw new ActivationConflictException(errMsg);
             }
         }
     }
 
-    // As of now, config generation is based on session id, and config generation must be an monotonically
+    // As of now, config generation is based on session id, and config generation must be a monotonically
     // increasing number
     private void checkIfActiveIsNewerThanSessionToBeActivated(long sessionId, long currentActiveSessionId) {
         if (sessionId < currentActiveSessionId) {
-            throw new IllegalArgumentException("It is not possible to activate session " + sessionId +
-                                               ", because it is older than current active session (" + 
-                                               currentActiveSessionId + ")");
+            throw new ActivationConflictException("It is not possible to activate session " + sessionId +
+                                          ", because it is older than current active session (" +
+                                          currentActiveSessionId + ")");
         }
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java
index 7e0c57f2e5f..7a86357cd95 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java
@@ -35,6 +35,7 @@ public class HttpErrorResponse extends HttpResponse {
     public enum errorCodes {
         APPLICATION_LOCK_FAILURE,
         BAD_REQUEST,
+        ACTIVATION_FAILED,
         INTERNAL_SERVER_ERROR,
         INVALID_APPLICATION_PACKAGE,
         METHOD_NOT_ALLOWED,
@@ -64,6 +65,10 @@ public class HttpErrorResponse extends HttpResponse {
         return new HttpErrorResponse(BAD_REQUEST, errorCodes.BAD_REQUEST.name(), msg);
     }
 
+    public static HttpErrorResponse conflictWhenActivating(String msg) {
+        return new HttpErrorResponse(CONFLICT, errorCodes.ACTIVATION_FAILED.name(), msg);
+    }
+
     public static HttpErrorResponse methodNotAllowed(String msg) {
         return new HttpErrorResponse(METHOD_NOT_ALLOWED, errorCodes.METHOD_NOT_ALLOWED.name(), msg);
     }
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 53bfbf5135f..c13d1b3fcfa 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
@@ -8,6 +8,7 @@ import com.yahoo.container.jdisc.LoggingRequestHandler;
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.log.LogLevel;
 import com.yahoo.config.provision.OutOfCapacityException;
+import com.yahoo.vespa.config.server.ActivationConflictException;
 import com.yahoo.yolean.Exceptions;
 
 import java.io.PrintWriter;
@@ -47,6 +48,8 @@ public class HttpHandler extends LoggingRequestHandler {
             }
         } catch (NotFoundException | com.yahoo.vespa.config.server.NotFoundException e) {
             return HttpErrorResponse.notFoundError(getMessage(e, request));
+        } catch (ActivationConflictException e) {
+            return HttpErrorResponse.conflictWhenActivating(getMessage(e, request));
         } catch (BadRequestException | IllegalArgumentException | IllegalStateException e) {
             return HttpErrorResponse.badRequest(getMessage(e, request));
         } catch (OutOfCapacityException e) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
index 7fca78b087b..f6108611f73 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
@@ -67,8 +67,12 @@ import java.time.Clock;
 import java.util.Collections;
 import java.util.Optional;
 
-import static com.yahoo.jdisc.Response.Status.*;
+import static com.yahoo.jdisc.Response.Status.BAD_REQUEST;
+import static com.yahoo.jdisc.Response.Status.CONFLICT;
+import static com.yahoo.jdisc.Response.Status.INTERNAL_SERVER_ERROR;
 import static com.yahoo.jdisc.Response.Status.METHOD_NOT_ALLOWED;
+import static com.yahoo.jdisc.Response.Status.NOT_FOUND;
+import static com.yahoo.jdisc.Response.Status.OK;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
@@ -154,7 +158,7 @@ public class SessionActiveHandlerTest extends SessionHandlerTest {
         ActivateRequest activateRequest = new ActivateRequest(sessionId, 1, "", Clock.systemUTC()).invoke();
         HttpResponse actResponse = activateRequest.getActResponse();
         String message = getRenderedString(actResponse);
-        assertThat(message, actResponse.getStatus(), Is.is(BAD_REQUEST));
+        assertThat(message, actResponse.getStatus(), Is.is(CONFLICT));
         assertThat(message,
                    containsString("Cannot activate session 3 because the currently active session (2) has changed since session 3 was created (was 1 at creation time)"));
     }
@@ -178,8 +182,8 @@ public class SessionActiveHandlerTest extends SessionHandlerTest {
         Clock clock = Clock.systemUTC();
         activateAndAssertOK(90l, 0l, clock);
         activateAndAssertError(92l, 89l, clock,
-                HttpErrorResponse.errorCodes.BAD_REQUEST,
-                "tenant:"+tenant+" app:default:default Cannot activate session 92 because the currently active session (90) has changed since session 92 was created (was 89 at creation time)");
+                               Response.Status.CONFLICT, HttpErrorResponse.errorCodes.ACTIVATION_FAILED,
+                               "tenant:" + tenant + " app:default:default Cannot activate session 92 because the currently active session (90) has changed since session 92 was created (was 89 at creation time)");
     }
 
     @Test
@@ -209,7 +213,7 @@ public class SessionActiveHandlerTest extends SessionHandlerTest {
     public void require_that_handler_gives_error_when_provisioner_activated_fails() throws Exception {
         hostProvisioner = new FailingMockProvisioner();
         hostProvisioner.activated = false;
-        activateAndAssertError(1, 0, Clock.systemUTC(), HttpErrorResponse.errorCodes.BAD_REQUEST, "Cannot activate application");
+        activateAndAssertError(1, 0, Clock.systemUTC(), BAD_REQUEST, HttpErrorResponse.errorCodes.BAD_REQUEST, "Cannot activate application");
         assertFalse(hostProvisioner.activated);
     }
 
@@ -249,12 +253,13 @@ public class SessionActiveHandlerTest extends SessionHandlerTest {
         return activateRequest;
     }
 
-    private ActivateRequest activateAndAssertErrorPut(long sessionId, long previousSessionId, Clock clock, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception {
+    private ActivateRequest activateAndAssertErrorPut(long sessionId, long previousSessionId, Clock clock,
+                                                      int statusCode, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception {
         ActivateRequest activateRequest = new ActivateRequest(sessionId, previousSessionId, "", clock);
         activateRequest.invoke();
         HttpResponse actResponse = activateRequest.getActResponse();
         RemoteSession session = activateRequest.getSession();
-        assertThat(actResponse.getStatus(), Is.is(BAD_REQUEST));
+        assertThat(actResponse.getStatus(), Is.is(statusCode));
         String message = getRenderedString(actResponse);
         assertThat(message, Is.is("{\"error-code\":\"" + errorCode.name() + "\",\"message\":\"" + expectedError + "\"}"));
         assertThat(session.getStatus(), Is.is(Session.Status.PREPARE));
@@ -353,9 +358,9 @@ public class SessionActiveHandlerTest extends SessionHandlerTest {
         assertThat(hostProvisioner.lastHosts.size(), is(1));
     }
 
-    private void activateAndAssertError(long sessionId, long previousSessionId, Clock clock, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception {
+    private void activateAndAssertError(long sessionId, long previousSessionId, Clock clock, int statusCode, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception {
         hostProvisioner.activated = false;
-        activateAndAssertErrorPut(sessionId, previousSessionId, clock, errorCode, expectedError);
+        activateAndAssertErrorPut(sessionId, previousSessionId, clock, statusCode, errorCode, expectedError);
         assertFalse(hostProvisioner.activated);
     }