diff options
5 files changed, 166 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(); + } +} diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java index 0c51cfc685d..e9cb37ae02c 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.curator.stats; import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.PriorityQueue; import java.util.concurrent.ConcurrentHashMap; @@ -119,4 +120,68 @@ public class LockStats { } } + + private static volatile String CURRENT_TEST_ID = null; + + public void invokingAcquire(Thread thread) { + // HACK: Try to see if this is the first invocation on a new test method. If so verify + // the #acquires = #releases, or otherwise some intervening tests broke them + getTestId(thread).ifPresent(testId -> { + if (Objects.equals(testId, CURRENT_TEST_ID)) { + return; + } + + // New test, verify balance between acquired and released. + metricsByLockPath.forEach((lockPath, lockMetrics) -> { + int numAcquired = lockMetrics.getCumulativeAcquireSucceededCount(); + int numReleased = lockMetrics.getCumulativeReleaseCount(); + if (numAcquired != numReleased) { + // ensure next test does not fail + clearForTesting(); + + throw new IllegalStateException("Detected in " + testId + ": lock path " + + lockPath + " has been acquired " + numAcquired + " times and released " + + numReleased + " times. The problem is the test " + CURRENT_TEST_ID + + ", or later tests leading up to this"); + } + }); + + CURRENT_TEST_ID = testId; + }); + } + + private static Optional<String> getTestId(Thread thread) { + // The stack trace is of the following form: + // + // ... + // com.yahoo.vespa.curator.stats.LockTest.nestedLocks(LockTest.java:191) + // ... + // org.junit.runner.JUnitCore.run(JUnitCore.java:xyz) + // ... + // + // And we'd like to return the test ID "com.yahoo.vespa.curator.stats.LockTest.nestedLocks". + + StackTraceElement[] elements = thread.getStackTrace(); + for (int index = 0; index < elements.length; ++index) { + if (stackFrameMethod(elements[index]).equals("org.junit.runner.JUnitCore.run")) { + while (index --> 0) { + String qualifiedMethod = stackFrameMethod(elements[index]); + if (qualifiedMethod.startsWith("com.yahoo.vespa.")) { + return Optional.of(qualifiedMethod); + } + } + + // Failed to find jdk.internal.reflect.NativeMethodAccessorImpl.invoke0 + throw new IllegalStateException("Bad stack trace!?"); + } + } + + // Failed to find org.junit.runners.model.FrameworkMethod$1.runReflectiveCall, which may happen + // in a spawned thread. + return Optional.empty(); + } + + private static String stackFrameMethod(StackTraceElement element) { + return element.getClassName() + "." + element.getMethodName(); + } } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java index 70442e4833d..087c133a44c 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java @@ -68,6 +68,8 @@ public class ThreadLockStats { /** Mutable method (see class doc) */ public void invokingAcquire(String lockPath, Duration timeout) { + LockStats.getGlobal().invokingAcquire(thread); + boolean reentry = lockAttemptsStack.stream().anyMatch(lockAttempt -> lockAttempt.getLockPath().equals(lockPath)); if (!reentry) { |