From f810022dd96bba240923e7276f5d15f76d58ad36 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Tue, 20 Oct 2020 20:51:04 +0200 Subject: Close orchestrator locks --- .../orchestrator/model/ApplicationApiImplTest.java | 130 ++++++++++++--------- .../vespa/orchestrator/model/ModelTestUtils.java | 6 +- .../orchestrator/model/ScopedApplicationApi.java | 23 ++++ 3 files changed, 99 insertions(+), 60 deletions(-) create mode 100644 orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ScopedApplicationApi.java (limited to 'orchestrator') 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 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 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 upStorageNodes = applicationApi.getUpStorageNodesInGroupInClusterOrder().stream() - .map(storageNode -> storageNode.hostName()) - .collect(Collectors.toList()); - assertEquals(Arrays.asList(expectedHostNames), upStorageNodes); + try (scopedApi) { + List 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 upStorageNodes = expectUp ? Arrays.asList(hostName1) : new ArrayList<>(); + try (var scopedApi = modelUtils.createScopedApplicationApi(applicationInstance, hostName1)) { + List upStorageNodes = expectUp ? Arrays.asList(hostName1) : new ArrayList<>(); - List actualStorageNodes = applicationApi.getUpStorageNodesInGroupInClusterOrder().stream() - .map(storageNode -> storageNode.hostName()) - .collect(Collectors.toList()); - assertEquals(upStorageNodes, actualStorageNodes); + List 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 noRemarksHostNames, List allowedToBeDownHostNames) { - List actualNoRemarksHosts = applicationApi.getNodesInGroupWithStatus(HostStatus.NO_REMARKS); - assertEquals(noRemarksHostNames, actualNoRemarksHosts); - List actualAllowedToBeDownHosts = applicationApi.getNodesInGroupWithStatus(HostStatus.ALLOWED_TO_BE_DOWN); - assertEquals(allowedToBeDownHostNames, actualAllowedToBeDownHosts); + try (scopedApi) { + List actualNoRemarksHosts = scopedApi.applicationApi().getNodesInGroupWithStatus(HostStatus.NO_REMARKS); + assertEquals(noRemarksHostNames, actualNoRemarksHosts); + List 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 actualStorageNodes = - applicationApi.getSuspendedStorageNodesInGroupInReverseClusterOrder().stream() - .map(storageNode -> storageNode.hostName()) - .collect(Collectors.toList()); - assertEquals(Arrays.asList(hostNames), actualStorageNodes); + ScopedApplicationApi scopedApi, HostName... hostNames) { + try (scopedApi) { + List 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(); + } +} -- cgit v1.2.3