From d01a9e441da25801e39cb60fb94abccb18646ce1 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Sun, 16 Feb 2025 10:48:20 +0200 Subject: [PATCH] Invoke TestCluster#assertAfterTest before closing the cluster (#122639) In test-scoped internal ITs the `cluster().assertAfterTest()` method was invoked *after* the cluster nodes were closed. Consequently, the assertions that iterated over the internal nodes (and asserted some state on nodes after the test) were all effectively noops. This PR reverses that order, so that after-test assertions are effective again. --- .../cluster/PrevalidateNodeRemovalIT.java | 43 +++++++++++-------- .../elasticsearch/test/ESIntegTestCase.java | 2 +- .../UnregisteredSettingsIntegTests.java | 20 ++++++--- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/cluster/PrevalidateNodeRemovalIT.java b/server/src/internalClusterTest/java/org/elasticsearch/cluster/PrevalidateNodeRemovalIT.java index 90e3196d76378..56ac96d592f49 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/cluster/PrevalidateNodeRemovalIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/cluster/PrevalidateNodeRemovalIT.java @@ -179,28 +179,35 @@ public void testNodeRemovalFromRedClusterWithTimeout() throws Exception { // make it red! internalCluster().stopNode(node1); ensureRed(indexName); + CountDownLatch stallPrevalidateShardPathActionLatch = new CountDownLatch(1); MockTransportService.getInstance(node2) .addRequestHandlingBehavior(TransportPrevalidateShardPathAction.ACTION_NAME + "[n]", (handler, request, channel, task) -> { logger.info("drop the check shards request"); + safeAwait(stallPrevalidateShardPathActionLatch); + handler.messageReceived(request, channel, task); }); - PrevalidateNodeRemovalRequest req = PrevalidateNodeRemovalRequest.builder() - .setNames(node2) - .build(TEST_REQUEST_TIMEOUT) - .masterNodeTimeout(TimeValue.timeValueSeconds(1)) - .timeout(TimeValue.timeValueSeconds(1)); - PrevalidateNodeRemovalResponse resp = client().execute(PrevalidateNodeRemovalAction.INSTANCE, req).get(); - assertFalse("prevalidation result should return false", resp.getPrevalidation().isSafe()); - String node2Id = getNodeId(node2); - assertThat( - resp.getPrevalidation().message(), - equalTo("cannot prevalidate removal of nodes with the following IDs: [" + node2Id + "]") - ); - assertThat(resp.getPrevalidation().nodes().size(), equalTo(1)); - NodesRemovalPrevalidation.NodeResult nodeResult = resp.getPrevalidation().nodes().get(0); - assertThat(nodeResult.name(), equalTo(node2)); - assertFalse(nodeResult.result().isSafe()); - assertThat(nodeResult.result().message(), startsWith("failed contacting the node")); - assertThat(nodeResult.result().reason(), equalTo(NodesRemovalPrevalidation.Reason.UNABLE_TO_VERIFY)); + try { + PrevalidateNodeRemovalRequest req = PrevalidateNodeRemovalRequest.builder() + .setNames(node2) + .build(TEST_REQUEST_TIMEOUT) + .masterNodeTimeout(TimeValue.timeValueSeconds(1)) + .timeout(TimeValue.timeValueSeconds(1)); + PrevalidateNodeRemovalResponse resp = client().execute(PrevalidateNodeRemovalAction.INSTANCE, req).get(); + assertFalse("prevalidation result should return false", resp.getPrevalidation().isSafe()); + String node2Id = getNodeId(node2); + assertThat( + resp.getPrevalidation().message(), + equalTo("cannot prevalidate removal of nodes with the following IDs: [" + node2Id + "]") + ); + assertThat(resp.getPrevalidation().nodes().size(), equalTo(1)); + NodesRemovalPrevalidation.NodeResult nodeResult = resp.getPrevalidation().nodes().get(0); + assertThat(nodeResult.name(), equalTo(node2)); + assertFalse(nodeResult.result().isSafe()); + assertThat(nodeResult.result().message(), startsWith("failed contacting the node")); + assertThat(nodeResult.result().reason(), equalTo(NodesRemovalPrevalidation.Reason.UNABLE_TO_VERIFY)); + } finally { + stallPrevalidateShardPathActionLatch.countDown(); + } } private void ensureRed(String indexName) throws Exception { diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index a92706e83ba99..bb88f9d094514 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -585,10 +585,10 @@ private void afterInternal(boolean afterClass) throws Exception { ensureClusterInfoServiceRunning(); beforeIndexDeletion(); cluster().wipe(excludeTemplates()); // wipe after to make sure we fail in the test that didn't ack the delete + cluster().assertAfterTest(); if (afterClass || currentClusterScope == Scope.TEST) { cluster().close(); } - cluster().assertAfterTest(); } } finally { if (currentClusterScope == Scope.TEST) { diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/UnregisteredSettingsIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/UnregisteredSettingsIntegTests.java index c714aa352fd41..5a76b81a9f3fc 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/UnregisteredSettingsIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/UnregisteredSettingsIntegTests.java @@ -11,6 +11,8 @@ import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.SecurityIntegTestCase; +import java.io.IOException; + import static org.elasticsearch.test.NodeRoles.dataOnlyNode; import static org.elasticsearch.test.NodeRoles.masterNode; import static org.hamcrest.Matchers.containsString; @@ -18,15 +20,19 @@ @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, autoManageMasterNodes = false) public class UnregisteredSettingsIntegTests extends SecurityIntegTestCase { - public void testIncludeReservedRolesSettingNotRegistered() { + public void testIncludeReservedRolesSettingNotRegistered() throws IOException { internalCluster().setBootstrapMasterNodeIndex(0); final Settings.Builder builder = Settings.builder() .put(randomBoolean() ? masterNode() : dataOnlyNode()) .putList("xpack.security.reserved_roles.include", "superuser"); - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(builder)); - assertThat(e.getMessage(), containsString("unknown setting [xpack.security.reserved_roles.include]")); + try { + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(builder)); + assertThat(e.getMessage(), containsString("unknown setting [xpack.security.reserved_roles.include]")); + } finally { + internalCluster().close(); + } } public void testSamlExcludeRolesSettingNotRegistered() throws Exception { @@ -36,7 +42,11 @@ public void testSamlExcludeRolesSettingNotRegistered() throws Exception { .put(randomBoolean() ? masterNode() : dataOnlyNode()) .putList("xpack.security.authc.realms.saml.saml1.exclude_roles", "superuser"); - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(builder)); - assertThat(e.getMessage(), containsString("unknown setting [xpack.security.authc.realms.saml.saml1.exclude_roles]")); + try { + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(builder)); + assertThat(e.getMessage(), containsString("unknown setting [xpack.security.authc.realms.saml.saml1.exclude_roles]")); + } finally { + internalCluster().close(); + } } }