[Test] Wait for clsuter to form before assertions #130973

ywangd · 2025-07-10T03:17:09Z

The original cluster should be properly formed before the tests kick off. This PR ensures that.

Relates: #129118
Resolves: #130883
Resolves: #130979

Nodes that are not publish quorum may not have applied the latest update when awaitMasterNode returns. This PR fixes it by waiting for the desired number of nodes. Relates: elastic#129118 Resolves: elastic#130883

elasticsearchmachine · 2025-07-10T03:17:34Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner · 2025-07-10T06:29:22Z

...erTest/java/org/elasticsearch/cluster/coordination/votingonly/VotingOnlyNodePluginTests.java

        internalCluster().stopCurrentMasterNode();
        awaitMasterNode();
        assertNotEquals(originalMaster, internalCluster().getMasterName());
+        ensureStableCluster(2 + numDataNodes); // wait for all nodes to join


This weakens the test slightly more than I'd like. The trouble here is in fact that these tests run with autoManageMasterNodes = false so they skip over the call to validateClusterFormed() when starting nodes here:

elasticsearch/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

Lines 2252 to 2254 in ecbc360

if (autoManageMasterNodes) {

validateClusterFormed();

}

That makes sense in general because if we're not auto-bootstrapping the test cluster we cannot expect it to be fully-formed after each node starts. But in these tests we can expect the cluster to be fully formed after we've finished starting all the nodes, so we should call that method explicitly ourselves there. Importantly, I think we should do that before stopping the original master node, since the new master node should not spuriously drop any of the other nodes from the cluster when it takes over.

I think I'd like us to rework these tests slightly so that rather than an assertBusy() on the current state having the right size of configuration we use ESIntegTestCase#awaitClusterState(Predicate<ClusterState>) to wait for a state which has the right voting configuration and the right number of nodes.

Updated as suggested in ce1452c
Please let me know if it looks right to you. Thanks!

The update also includes fix for a new but similar nature failure #130979

DaveCTurner · 2025-07-10T06:34:34Z

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java


    /**
-     * Waits for all nodes in the cluster to have a consistent view of which node is currently the master.
+     * Waits for all nodes forming the publish quorum in the cluster to have a consistent view of which node is currently the master.


This isn't correct. A publish quorum is some subset of the master-eligible nodes in the cluster (e.g. 2 of the 3 masters) which is much weaker than what this does. Here we're waiting for all the nodes in the cluster (i.e. in ClusterState#nodes).

Thanks! I meant the nodes could still be in a different cluster state version after this method is returned, is this true? But you are right that the nodes should all have the same master. The trouble here is that there could still be nodes trying to join the cluster when this returns. In that case, it does not guarantee the "all nodes", which is more than ClusterState#nodes, to see the same master. Technically it is the current behaviour, but kindas defeats its purpose in such situation?

The setWaitForEvents(Priority.LANGUID) means that there was a point in time during the execution of this method where there was a committed cluster state and all the nodes in that cluster state were on the same cluster state version (or stuff was timing out, but that doesn't happen in these tests). It can't guarantee that another publication hasn't started before the method returns ofc.

You're right that there may also be nodes still trying to join the cluster at this point, but they're not in the cluster (they haven't joined it yet). In practice, it's up to the caller to account for this.

I reverted changes to the comment in 5445ef6. We can update it separately (if at all). Thanks!

DaveCTurner · 2025-07-10T08:34:47Z

...erTest/java/org/elasticsearch/cluster/coordination/votingonly/VotingOnlyNodePluginTests.java


        internalCluster().stopCurrentMasterNode();
-        awaitMasterNode();
+        internalCluster().validateClusterFormed();


I'd rather this remained as just an awaitMasterNode(). That should be sufficient here...

I think it is theoretically possible that the data node fails to join the new master so that awaitMasterNode() returns without seeing the data node. If we later ask the data node (via a randomed client) about its master and it could fail with NPE. That said, it is not a practical concern for this test. So I reverted back to use awaitMasterNode.

theoretically possible that the data node fails to join the new master

No, that shouldn't be possible (i.e. I'd want the test to fail if that happens). The new master will start from the state that the old master last committed, which will include the data node, so the data node will automatically be part of the new master's cluster. The new master would only remove the data node from its cluster if something positively fails (e.g. network disconnect).

DaveCTurner · 2025-07-10T08:36:21Z

...erTest/java/org/elasticsearch/cluster/coordination/votingonly/VotingOnlyNodePluginTests.java

-            equalTo(false)
-        );
+        final ClusterState state = clusterAdmin().prepareState(TEST_REQUEST_TIMEOUT).get().getState();
+        assertThat(state.nodes().size(), equalTo(2 + numDataNodes));


... except for this new assertion, which I think we should revert. This requires us to wait for the old master to be removed, and that shouldn't be necessary to make the more important assertion that the elected master is not voting-only.

Good point. I removed this.

DaveCTurner · 2025-07-10T08:38:13Z

...erTest/java/org/elasticsearch/cluster/coordination/votingonly/VotingOnlyNodePluginTests.java


        // start a fresh full master node, which will be brought into the cluster as master by the voting-only nodes
        final String newMaster = internalCluster().startNode();
+        internalCluster().validateClusterFormed();


I think we should revert this too, it's stronger than needed. InternalTestCluster#getMasterName already waits for the new master to be elected, and that's enough.

I think my above reply applies to here. Thanks!

DaveCTurner

LGTM

ywangd · 2025-07-10T09:37:59Z

This has been a good case for me to dig a bit deeper into the cluster coordination area. Thanks a lot for the review and discussion!

The original cluster should be properly formed before the tests kick off. This PR ensures that. Relates: elastic#129118 Resolves: elastic#130883 Resolves: elastic#130979

[Test] Wait for stable cluster before assertion

8018a91

Nodes that are not publish quorum may not have applied the latest update when awaitMasterNode returns. This PR fixes it by waiting for the desired number of nodes. Relates: elastic#129118 Resolves: elastic#130883

ywangd requested review from DaveCTurner and pxsalehi July 10, 2025 03:17

ywangd added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v9.2.0 labels Jul 10, 2025

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jul 10, 2025

DaveCTurner requested changes Jul 10, 2025

View reviewed changes

ywangd added 3 commits July 10, 2025 18:19

revert changes to comment

5445ef6

feedback

ce1452c

Merge remote-tracking branch 'origin/main' into es-130883-fix

c84e96a

ywangd requested a review from DaveCTurner July 10, 2025 08:34

DaveCTurner reviewed Jul 10, 2025

View reviewed changes

feedback

c039174

DaveCTurner approved these changes Jul 10, 2025

View reviewed changes

elasticsearchmachine and others added 2 commits July 10, 2025 09:38

[CI] Auto commit changes from spotless

5ded2bd

Merge remote-tracking branch 'origin/main' into es-130883-fix

84945a8

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 10, 2025

ywangd changed the title ~~[Test] Wait for stable cluster before assertion~~ [Test] Wait for clsuter to form before assertions Jul 10, 2025

elasticsearchmachine merged commit e2ec28d into elastic:main Jul 10, 2025
33 checks passed

ywangd deleted the es-130883-fix branch July 10, 2025 11:23

[Test] Wait for clsuter to form before assertions #130973

[Test] Wait for clsuter to form before assertions #130973

Uh oh!

Conversation

ywangd commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

ywangd commented Jul 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ywangd commented Jul 10, 2025 •

edited

Loading