Make sure not to overwrite application data if data already exists

3 files changed, 82 insertions, 17 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
index 38be13894e2..a4bab8302cb 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
@@ -77,13 +77,13 @@ public class ApplicationCuratorDatabase {
     public void createApplication(ApplicationId id, boolean writeAsJson) {
         if ( ! id.tenant().equals(tenant))
             throw new IllegalArgumentException("Cannot write application id '" + id + "' for tenant '" + tenant + "'");
+        if (curator.exists(applicationPath(id))) return;
 
         try (Lock lock = lock(id)) {
             if (writeAsJson) {
                 var applicationData = new ApplicationData(id, OptionalLong.empty(), OptionalLong.empty());
                 curator.set(applicationPath(id), applicationData.toJson());
             } else {
-                if (curator.exists(applicationPath(id))) return;
                 curator.create(applicationPath(id));
             }
             modifyReindexing(id, ApplicationReindexing.empty(), UnaryOperator.identity());
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
index 2a79e2c03aa..a704599c6d4 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
@@ -31,7 +31,9 @@ import com.yahoo.vespa.config.PayloadChecksums;
 import com.yahoo.vespa.config.protocol.ConfigResponse;
 import com.yahoo.vespa.config.protocol.DefContent;
 import com.yahoo.vespa.config.protocol.VespaVersion;
+import com.yahoo.vespa.config.server.application.ApplicationData;
 import com.yahoo.vespa.config.server.application.OrchestratorMock;
+import com.yahoo.vespa.config.server.application.TenantApplications;
 import com.yahoo.vespa.config.server.deploy.DeployTester;
 import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs;
 import com.yahoo.vespa.config.server.filedistribution.FileDirectory;
@@ -49,6 +51,7 @@ import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.config.server.tenant.TestTenantRepository;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.flags.PermanentFlags;
 import com.yahoo.vespa.model.VespaModelFactory;
@@ -57,6 +60,7 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
+
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
@@ -219,6 +223,27 @@ public class ApplicationRepositoryTest {
     }
 
     @Test
+    public void applicationData() {
+        flagSource = flagSource.withBooleanFlag(Flags.WRITE_APPLICATION_DATA_AS_JSON.id(), true);
+        long firstSessionId = deployApp(testApp).sessionId();
+        assertApplicationData(firstSessionId, firstSessionId);
+        assertEquals(firstSessionId, applicationRepository.getActiveSession(applicationId()).get().getSessionId());
+
+        // Create new session, no changes to application data
+        long secondSessionId = createSession(applicationId(), timeoutBudget, testApp);
+        assertNotEquals(firstSessionId, secondSessionId);
+        assertApplicationData(firstSessionId, firstSessionId);
+
+        // Prepare, last deployed session id should be the new one
+        prepare(testApp, secondSessionId);
+        assertApplicationData(firstSessionId, secondSessionId);
+
+        // Activate, active session id should be the new one
+        activate(applicationId(), secondSessionId, timeoutBudget);
+        assertApplicationData(secondSessionId, secondSessionId);
+    }
+
+    @Test
     public void createFromActiveSession() {
         long originalSessionId = deployApp(testApp).sessionId();
 
@@ -680,6 +705,11 @@ public class ApplicationRepositoryTest {
         return applicationRepository.deploy(application, prepareParams());
     }
 
+    private long prepare(File application, long sessionId) {
+        applicationRepository.prepare(sessionId, prepareParams());
+        return sessionId;
+    }
+
     private PrepareResult deployApp(File applicationPackage) {
         return deployApp(applicationPackage, prepareParams());
     }
@@ -808,4 +838,12 @@ public class ApplicationRepositoryTest {
         return applicationRepository.createSessionFromExisting(applicationId, false, timeoutBudget, new BaseDeployLogger());
     }
 
+    private void assertApplicationData(long expectedActiveSesionId, long expectedLastDeployedSessionId) {
+        TenantApplications applications = tenantRepository.getTenant(applicationId().tenant()).getApplicationRepo();
+        ApplicationData applicationData = applications.applicationData(applicationId()).get();
+        assertEquals(applicationId(), applicationData.applicationId());
+        assertEquals(expectedActiveSesionId, applicationData.activeSession().get().longValue());
+        assertEquals(expectedLastDeployedSessionId, applicationData.lastDeployedSession().get().longValue());
+    }
+
 }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
index 80f705dcbad..2f0ac236170 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
@@ -13,17 +13,18 @@ import java.util.OptionalLong;
 import static org.junit.Assert.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @author jonmv
  */
 public class ApplicationCuratorDatabaseTest {
 
+    private final MockCurator curator = new MockCurator();
+
     @Test
     public void testReindexingStatusSerialization() {
         ApplicationId id = ApplicationId.defaultId();
-        ApplicationCuratorDatabase db = new ApplicationCuratorDatabase(id.tenant(), new MockCurator());
+        ApplicationCuratorDatabase db = new ApplicationCuratorDatabase(id.tenant(), curator);
 
         assertEquals(Optional.empty(), db.readReindexingStatus(id));
 
@@ -42,13 +43,13 @@ public class ApplicationCuratorDatabaseTest {
     @Test
     public void testReadingAndWritingApplicationData() {
         ApplicationId id = ApplicationId.defaultId();
-        MockCurator curator = new MockCurator();
         ApplicationCuratorDatabase db = new ApplicationCuratorDatabase(id.tenant(), curator);
 
         assertEquals(Optional.empty(), db.applicationData(id));
 
         db.createApplication(id, false);
         assertEquals(Optional.empty(), db.applicationData(id)); // still empty, as no data has been written to node
+        deleteApplication(db, id);
 
         db.createApplication(id, true);
         // Can be read as json, but no active session or last deployed session
@@ -59,13 +60,9 @@ public class ApplicationCuratorDatabaseTest {
         assertFalse(applicationData.get().lastDeployedSession().isPresent());
 
         // Prepare session 2, no active session
-        try (var t = db.createWritePrepareTransaction(new CuratorTransaction(curator), id, 2, OptionalLong.empty(), false)) {
-            t.commit();
-        }
+        prepareSession(db, id, 2, OptionalLong.empty(), false);
         // Activate session 2, last deployed session not present (not writing json)
-        try (var t = db.createWriteActiveTransaction(new CuratorTransaction(curator), id, 2, false)) {
-            t.commit();
-        }
+        activateSession(db, id, 2, false);
         // Can be read as session id only
         applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
@@ -82,21 +79,28 @@ public class ApplicationCuratorDatabaseTest {
         assertFalse(applicationData.get().lastDeployedSession().isPresent());
 
         // Prepare session 3, last deployed session is still 2
-        try (var t = db.createWritePrepareTransaction(new CuratorTransaction(curator), id, 3, OptionalLong.of(2), true)) {
-            t.commit();
-        }
+        prepareSession(db, id, 3, OptionalLong.of(2), true);
         // Can be read as json, active session is still 2 and last deployed session is 3
         applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
         assertEquals(id, applicationData.get().applicationId());
-        assertTrue(applicationData.get().activeSession().isPresent());
+        assertTrue(applicationData.get().activeSession().isPresent(), applicationData.get().toString());
         assertEquals(2L, applicationData.get().activeSession().get().longValue());
         assertTrue(applicationData.get().lastDeployedSession().isPresent());
         assertEquals(3, applicationData.get().lastDeployedSession().get().longValue());
 
-        try (var t = db.createWriteActiveTransaction(new CuratorTransaction(curator), id, 3, true)) {
-            t.commit();
-        }
+        activateSession(db, id, 3, true);
+        // Can be read as json, active session and last deployed session present
+        applicationData = db.applicationData(id);
+        assertTrue(applicationData.isPresent());
+        assertEquals(id, applicationData.get().applicationId());
+        assertTrue(applicationData.get().activeSession().isPresent());
+        assertEquals(3, applicationData.get().activeSession().get().longValue());
+        assertTrue(applicationData.get().lastDeployedSession().isPresent());
+        assertEquals(3, applicationData.get().lastDeployedSession().get().longValue());
+
+        // createApplication should not overwrite the node if it already exists
+        db.createApplication(id, true);
         // Can be read as json, active session and last deployed session present
         applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
@@ -107,4 +111,27 @@ public class ApplicationCuratorDatabaseTest {
         assertEquals(3, applicationData.get().lastDeployedSession().get().longValue());
     }
 
+
+    private void deleteApplication(ApplicationCuratorDatabase db, ApplicationId applicationId) {
+        try (var t = db.createDeleteTransaction(applicationId)) {
+            t.commit();
+        }
+    }
+
+    private void prepareSession(ApplicationCuratorDatabase db, ApplicationId applicationId, long sessionId, OptionalLong activesSessionId, boolean writeAsJson) {
+        try (var t = db.createWritePrepareTransaction(new CuratorTransaction(curator),
+                                                      applicationId,
+                                                      sessionId,
+                                                      activesSessionId,
+                                                      writeAsJson)) {
+            t.commit();
+        }
+    }
+
+    private void activateSession(ApplicationCuratorDatabase db, ApplicationId applicationId, long sessionId, boolean writeAsJson) {
+        try (var t = db.createWriteActiveTransaction(new CuratorTransaction(curator), applicationId, sessionId, writeAsJson)) {
+            t.commit();
+        }
+    }
+
 }