Close orchestrator locks

3 files changed, 99 insertions, 60 deletions

diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
index ee33a92367b..9a013689224 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
@@ -20,9 +20,9 @@ public class ApplicationApiImplTest {
 
     @Test
     public void testApplicationId() {
-        ApplicationApi applicationApi =
-                modelUtils.createApplicationApiImpl(modelUtils.createApplicationInstance(new ArrayList<>()));
-        assertEquals("tenant:application-name:default", applicationApi.applicationId().serializedForm());
+        try (var api = modelUtils.createScopedApplicationApi(modelUtils.createApplicationInstance(new ArrayList<>()))) {
+            assertEquals("tenant:application-name:default", api.applicationApi().applicationId().serializedForm());
+        }
     }
 
     @Test
@@ -60,23 +60,25 @@ public class ApplicationApiImplTest {
                         )
                 ));
 
-        verifyClustersInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName1), 1, 2, 3);
-        verifyClustersInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName2), 2, 3);
-        verifyClustersInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName3), 1);
-        verifyClustersInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName4));
+        verifyClustersInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName1), 1, 2, 3);
+        verifyClustersInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName2), 2, 3);
+        verifyClustersInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName3), 1);
+        verifyClustersInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName4));
     }
 
-    private void verifyClustersInOrder(ApplicationApi applicationApi,
+    private void verifyClustersInOrder(ScopedApplicationApi scopedApi,
                                        Integer... expectedClusterNumbers) {
-        // Note: we require the clusters to be in order.
-        List<ClusterApi> clusterApis = applicationApi.getClusters();
-        String clusterInfos = clusterApis.stream().map(clusterApi -> clusterApi.clusterInfo()).collect(Collectors.joining(","));
+        try (scopedApi) {
+            // Note: we require the clusters to be in order.
+            List<ClusterApi> clusterApis = scopedApi.applicationApi().getClusters();
+            String clusterInfos = clusterApis.stream().map(clusterApi -> clusterApi.clusterInfo()).collect(Collectors.joining(","));
 
-        String expectedClusterInfos = Arrays.stream(expectedClusterNumbers)
-                .map(number -> "{ clusterId=cluster-" + number + ", serviceType=service-type-" + number + " }")
-                .collect(Collectors.joining(","));
+            String expectedClusterInfos = Arrays.stream(expectedClusterNumbers)
+                    .map(number -> "{ clusterId=cluster-" + number + ", serviceType=service-type-" + number + " }")
+                    .collect(Collectors.joining(","));
 
-        assertEquals(expectedClusterInfos, clusterInfos);
+            assertEquals(expectedClusterInfos, clusterInfos);
+        }
     }
 
     @Test
@@ -127,33 +129,35 @@ public class ApplicationApiImplTest {
                         )
                 ));
 
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName1), hostName1);
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName2), hostName2);
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName3)); // host3 is DOWN
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName4), hostName4);
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName5)); // not a storage cluster
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName1), hostName1);
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName2), hostName2);
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName3)); // host3 is DOWN
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName4), hostName4);
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName5)); // not a storage cluster
 
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName1, hostName3), hostName1);
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName1, hostName3), hostName1);
 
         // For the node group (host1, host4), they both have an up storage node (service instance)
         // with clusters (cluster-3, cluster-1) respectively, and so the order of the hosts are reversed
         // (host4, host1) when sorted by the clusters.
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(applicationInstance, hostName1, hostName4), hostName4, hostName1);
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(applicationInstance, hostName1, hostName4), hostName4, hostName1);
 
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(
                 applicationInstance, hostName1, hostName4, hostName5), hostName4, hostName1);
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(
                 applicationInstance, hostName1, hostName4, hostName5, hostName6), hostName4, hostName1);
-        verifyUpStorageNodesInOrder(modelUtils.createApplicationApiImpl(
+        verifyUpStorageNodesInOrder(modelUtils.createScopedApplicationApi(
                 applicationInstance, hostName1, hostName4, hostName5, hostName7), hostName4, hostName7, hostName1);
     }
 
-    private void verifyUpStorageNodesInOrder(ApplicationApi applicationApi,
+    private void verifyUpStorageNodesInOrder(ScopedApplicationApi scopedApi,
                                              HostName... expectedHostNames) {
-        List<HostName> upStorageNodes = applicationApi.getUpStorageNodesInGroupInClusterOrder().stream()
-                .map(storageNode -> storageNode.hostName())
-                .collect(Collectors.toList());
-        assertEquals(Arrays.asList(expectedHostNames), upStorageNodes);
+        try (scopedApi) {
+            List<HostName> upStorageNodes = scopedApi.applicationApi().getUpStorageNodesInGroupInClusterOrder().stream()
+                    .map(storageNode -> storageNode.hostName())
+                    .collect(Collectors.toList());
+            assertEquals(Arrays.asList(expectedHostNames), upStorageNodes);
+        }
     }
 
     @Test
@@ -179,13 +183,16 @@ public class ApplicationApiImplTest {
 
         modelUtils.createNode("host1", hostStatus);
 
-        ApplicationApi applicationApi = modelUtils.createApplicationApiImpl(applicationInstance, hostName1);
-        List<HostName> upStorageNodes = expectUp ? Arrays.asList(hostName1) : new ArrayList<>();
+        try (var scopedApi = modelUtils.createScopedApplicationApi(applicationInstance, hostName1)) {
+            List<HostName> upStorageNodes = expectUp ? Arrays.asList(hostName1) : new ArrayList<>();
 
-        List<HostName> actualStorageNodes = applicationApi.getUpStorageNodesInGroupInClusterOrder().stream()
-                .map(storageNode -> storageNode.hostName())
-                .collect(Collectors.toList());
-        assertEquals(upStorageNodes, actualStorageNodes);
+            List<HostName> actualStorageNodes = scopedApi.applicationApi()
+                    .getUpStorageNodesInGroupInClusterOrder()
+                    .stream()
+                    .map(storageNode -> storageNode.hostName())
+                    .collect(Collectors.toList());
+            assertEquals(upStorageNodes, actualStorageNodes);
+        }
     }
 
     @Test
@@ -219,30 +226,33 @@ public class ApplicationApiImplTest {
         modelUtils.createNode(hostName3, HostStatus.ALLOWED_TO_BE_DOWN);
 
         verifyNodesInGroupWithoutRemarks(
-                modelUtils.createApplicationApiImpl(applicationInstance, hostName1),
+                modelUtils.createScopedApplicationApi(applicationInstance, hostName1),
                 Arrays.asList(hostName1),
                 Arrays.asList());
         verifyNodesInGroupWithoutRemarks(
-                modelUtils.createApplicationApiImpl(applicationInstance, hostName1, hostName2),
+                modelUtils.createScopedApplicationApi(applicationInstance, hostName1, hostName2),
                 Arrays.asList(hostName1, hostName2),
                 Arrays.asList());
         verifyNodesInGroupWithoutRemarks(
-                modelUtils.createApplicationApiImpl(applicationInstance, hostName1, hostName2, hostName3),
+                modelUtils.createScopedApplicationApi(applicationInstance, hostName1, hostName2, hostName3),
                 Arrays.asList(hostName1, hostName2),
                 Arrays.asList(hostName3));
         verifyNodesInGroupWithoutRemarks(
-                modelUtils.createApplicationApiImpl(applicationInstance, hostName3),
+                modelUtils.createScopedApplicationApi(applicationInstance, hostName3),
                 Arrays.asList(),
                 Arrays.asList(hostName3));
     }
 
-    private void verifyNodesInGroupWithoutRemarks(ApplicationApi applicationApi,
+    private void verifyNodesInGroupWithoutRemarks(ScopedApplicationApi scopedApi,
                                                   List<HostName> noRemarksHostNames,
                                                   List<HostName> allowedToBeDownHostNames) {
-        List<HostName> actualNoRemarksHosts = applicationApi.getNodesInGroupWithStatus(HostStatus.NO_REMARKS);
-        assertEquals(noRemarksHostNames, actualNoRemarksHosts);
-        List<HostName> actualAllowedToBeDownHosts = applicationApi.getNodesInGroupWithStatus(HostStatus.ALLOWED_TO_BE_DOWN);
-        assertEquals(allowedToBeDownHostNames, actualAllowedToBeDownHosts);
+        try (scopedApi) {
+            List<HostName> actualNoRemarksHosts = scopedApi.applicationApi().getNodesInGroupWithStatus(HostStatus.NO_REMARKS);
+            assertEquals(noRemarksHostNames, actualNoRemarksHosts);
+            List<HostName> actualAllowedToBeDownHosts = scopedApi.applicationApi()
+                    .getNodesInGroupWithStatus(HostStatus.ALLOWED_TO_BE_DOWN);
+            assertEquals(allowedToBeDownHostNames, actualAllowedToBeDownHosts);
+        }
     }
 
     @Test
@@ -300,36 +310,40 @@ public class ApplicationApiImplTest {
         modelUtils.createNode(allowedToBeDownHost7, HostStatus.ALLOWED_TO_BE_DOWN);
 
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost1), allowedToBeDownHost1);
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost1), allowedToBeDownHost1);
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, noRemarksHost2));
+                modelUtils.createScopedApplicationApi(applicationInstance, noRemarksHost2));
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost3));
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost3));
 
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost1, noRemarksHost6), allowedToBeDownHost1);
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost1, noRemarksHost6), allowedToBeDownHost1);
 
         // allowedToBeDownHost4 is in cluster-3, while allowedToBeDownHost1 is in cluster-4, so allowedToBeDownHost4 should be ordered
         // before allowedToBeDownHost1.
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost1, noRemarksHost6, allowedToBeDownHost4),
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost1, noRemarksHost6, allowedToBeDownHost4),
                 allowedToBeDownHost4, allowedToBeDownHost1);
 
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost1, allowedToBeDownHost4, allowedToBeDownHost7),
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost1, allowedToBeDownHost4, allowedToBeDownHost7),
                 allowedToBeDownHost7, allowedToBeDownHost4, allowedToBeDownHost1);
 
         verifyStorageNodesAllowedToBeDown(
-                modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost4, allowedToBeDownHost1, allowedToBeDownHost7),
+                modelUtils.createScopedApplicationApi(applicationInstance, allowedToBeDownHost4, allowedToBeDownHost1, allowedToBeDownHost7),
                 allowedToBeDownHost7, allowedToBeDownHost4, allowedToBeDownHost1);
     }
 
     private void verifyStorageNodesAllowedToBeDown(
-            ApplicationApi applicationApi, HostName... hostNames) {
-        List<HostName> actualStorageNodes =
-                applicationApi.getSuspendedStorageNodesInGroupInReverseClusterOrder().stream()
-                .map(storageNode -> storageNode.hostName())
-                .collect(Collectors.toList());
-        assertEquals(Arrays.asList(hostNames), actualStorageNodes);
+            ScopedApplicationApi scopedApi, HostName... hostNames) {
+        try (scopedApi) {
+            List<HostName> actualStorageNodes = scopedApi
+                    .applicationApi()
+                    .getSuspendedStorageNodesInGroupInReverseClusterOrder()
+                    .stream()
+                    .map(storageNode -> storageNode.hostName())
+                    .collect(Collectors.toList());
+            assertEquals(Arrays.asList(hostNames), actualStorageNodes);
+        }
     }
 }
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
index c4087daeb94..3f79aa3a098 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
@@ -114,14 +114,16 @@ class ModelTestUtils {
         return hostName;
     }
 
-    ApplicationApi createApplicationApiImpl(
+    ScopedApplicationApi createScopedApplicationApi(
             ApplicationInstance applicationInstance,
             HostName... hostnames) {
         NodeGroup nodeGroup = new NodeGroup(applicationInstance, hostnames);
         ApplicationLock lock = statusService.lockApplication(
                 OrchestratorContext.createContextForSingleAppOp(Clock.systemUTC()),
                 applicationInstance.reference());
-        return applicationApiFactory().create(nodeGroup, lock, clusterControllerClientFactory);
+        return new ScopedApplicationApi(
+                applicationApiFactory().create(nodeGroup, lock, clusterControllerClientFactory),
+                lock);
     }
 
     ApplicationInstance createApplicationInstance(
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ScopedApplicationApi.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ScopedApplicationApi.java
new file mode 100644
index 00000000000..6f47e862ac3
--- /dev/null
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ScopedApplicationApi.java
@@ -0,0 +1,23 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.orchestrator.model;
+
+import com.yahoo.vespa.orchestrator.status.ApplicationLock;
+
+class ScopedApplicationApi implements AutoCloseable {
+    private final ApplicationApi applicationApi;
+    private final ApplicationLock lock;
+
+    ScopedApplicationApi(ApplicationApi applicationApi, ApplicationLock lock) {
+        this.applicationApi = applicationApi;
+        this.lock = lock;
+    }
+
+    ApplicationApi applicationApi() {
+        return applicationApi;
+    }
+
+    @Override
+    public void close() {
+        lock.close();
+    }
+}