Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes #16021

kkewwei · 2024-09-20T14:11:52Z

Description

The case is as follows:

When the node_t1 is excluded from the vote config, and the cluster starts a new leader election, but the the node node_t2 hasn't been elected as the new leader.
At the moment, we send request to get the ClusterManager, we first get ClusterManager name, and leads to the NullPointerException.

internalCluster().nonClusterManagerClient()-> ......->getClusterManagerName()

OpenSearch/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java

Line 2171 in f0ea056

    
           return client.admin().cluster().prepareState().get().getState().nodes().getClusterManagerNode().getName();

Related Issues

Resolves #15944 #16015

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-09-20T15:10:46Z

❕ Gradle check result for 1b3920b: UNSTABLE

TEST FAILURES:

      1 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

msfroh · 2024-09-20T22:59:22Z

Checking the history of this test, has this been flaky for more than a year?

The only code related to cluster manager election that I've been able to find changed more recently than ~5 years ago (besides renaming) is the introduction of DecommisionService in 2023.

kkewwei · 2024-09-21T05:45:10Z

Checking the history of this test, has this been flaky for more than a year?

The only code related to cluster manager election that I've been able to find changed more recently than ~5 years ago (besides renaming) is the introduction of DecommisionService in 2023.

@msfroh I'm not clearly why it wasn’t reported before, maybe the frequency of occurrence is very low and we ignored it? This issue #15944 collects some of the recent case.

Logically speaking, when the cluster has no cluster manager, It's right to return null from getClusterManagerNode, which will hit the case.

github-actions · 2024-09-21T07:05:20Z

✅ Gradle check result for 4d812e9: SUCCESS

codecov · 2024-09-21T07:05:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.26%. Comparing base (f9c239d) to head (40548ec).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #16021      +/-   ##
============================================
+ Coverage     72.11%   72.26%   +0.14%     
- Complexity    65260    65320      +60     
============================================
  Files          5301     5301              
  Lines        303801   303801              
  Branches      44029    44029              
============================================
+ Hits         219098   219539     +441     
+ Misses        66686    66223     -463     
- Partials      18017    18039      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kkewwei · 2024-09-24T09:39:59Z

@msfroh @gaobinlong please help confirm it when you are free.

opensearch-trigger-bot · 2024-11-06T15:22:18Z

This PR is stalled because it has been open for 30 days with no activity.

jed326 · 2024-11-06T22:02:26Z

server/src/internalClusterTest/java/org/opensearch/cluster/SpecificClusterManagerNodesIT.java

+                    return null;
+                }
+            };
+            assertThat(clusterManagerName.get(), equalTo(nextClusterManagerEligableNodeName));


So if we swallow the NPE with the try/catch above, won't this test still fail on the assertion here since clusterManagerName.get() would be null and nextClusterManagerEligableNodeName shouldn't be null?

@jed326 assertBusy allows the assert failed in 10s, so it seems ok.

We can add an additional assert before this to evaluate it is not null.

assertBusy can retry in 10s, it seems that we shouldn't add a new assertBusy before it.

rajiv-kv

Thanks @kkewwei for raising a fix

server/src/internalClusterTest/java/org/opensearch/cluster/SpecificClusterManagerNodesIT.java

rajiv-kv · 2025-01-08T12:10:11Z

server/src/internalClusterTest/java/org/opensearch/cluster/SpecificClusterManagerNodesIT.java

+                        .nodes()
+                        .getClusterManagerNode()
+                        .getName();
+                } catch (Exception e) {


Instead of catching a generic exception, could we catch a specific exception related to ClusterManager Not found.

rajiv-kv · 2025-01-08T12:10:52Z

server/src/internalClusterTest/java/org/opensearch/cluster/SpecificClusterManagerNodesIT.java

+                    return null;
+                }
+            };
+            assertThat(clusterManagerName.get(), equalTo(nextClusterManagerEligableNodeName));


We can add an additional assert before this to evaluate it is not null.

github-actions · 2025-01-12T06:52:39Z

❕ Gradle check result for 4e3feaa: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2025-01-12T12:16:22Z

✅ Gradle check result for b1ba756: SUCCESS

kkewwei · 2025-01-15T01:21:06Z

@rajiv-kv, please have a look when you are free.

rajiv-kv · 2025-01-15T04:04:29Z

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

@@ -2171,7 +2172,7 @@ public String getClusterManagerName(@Nullable String viaNode) {
            return client.admin().cluster().prepareState().get().getState().nodes().getClusterManagerNode().getName();
        } catch (Exception e) {


Can we have a seperate catch block for ClusterManagerNotDiscoveredException and not change the generic exception handler ?

Alternatively we can avoid this change and check for e.getCause at the caller as the exception is wrapped.

@rajiv-kv I think when null is detected, we should take the initiative to handle it, how about this fix?

…terManagerNodes Signed-off-by: kkewwei <[email protected]> Signed-off-by: kkewwei <[email protected]>

github-actions · 2025-01-15T10:52:21Z

❕ Gradle check result for 40548ec: UNSTABLE

TEST FAILURES:

      1 org.opensearch.test.rest.ClientYamlTestSuiteIT.test {p0=search.aggregation/20_terms/string profiler via global ordinals}

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions bot added bug Something isn't working Cluster Manager labels Sep 20, 2024

gaobinlong added backport 2.x Backport to 2.x branch skip-changelog labels Sep 20, 2024

opensearch-ci-bot mentioned this pull request Sep 20, 2024

[AUTOCUT] Gradle Check Flaky Test Report for MinimumClusterManagerNodesIT #14289

Open

kkewwei force-pushed the fix_16015 branch from 1b3920b to 3ffc620 Compare September 21, 2024 05:03

kkewwei force-pushed the fix_16015 branch 2 times, most recently from 1338c91 to 3823b1c Compare September 21, 2024 05:30

kkewwei changed the title ~~Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClus…~~ Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes Sep 21, 2024

kkewwei force-pushed the fix_16015 branch from 3823b1c to 4d812e9 Compare September 21, 2024 06:11

kkewwei mentioned this pull request Oct 10, 2024

[BUG] org.opensearch.cluster.SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes is flaky #16015

Closed

opensearch-trigger-bot bot added the stalled Issues that have stalled label Nov 6, 2024

jed326 reviewed Nov 6, 2024

View reviewed changes

opensearch-trigger-bot bot removed the stalled Issues that have stalled label Nov 7, 2024

github-actions bot added >test-failure Test failure from CI, local build, etc. autocut flaky-test Random test failure that succeeds on second run labels Dec 13, 2024

rajiv-kv reviewed Jan 8, 2025

View reviewed changes

kkewwei force-pushed the fix_16015 branch 2 times, most recently from d2c4d60 to 4e3feaa Compare January 12, 2025 05:51

kkewwei force-pushed the fix_16015 branch from 4e3feaa to b1ba756 Compare January 12, 2025 11:19

opensearch-ci-bot mentioned this pull request Jan 12, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreIT #16145

Open

rajiv-kv reviewed Jan 15, 2025

View reviewed changes

Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClus…

40548ec

…terManagerNodes Signed-off-by: kkewwei <[email protected]> Signed-off-by: kkewwei <[email protected]>

kkewwei force-pushed the fix_16015 branch from b1ba756 to 40548ec Compare January 15, 2025 09:55

rajiv-kv approved these changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes #16021

Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes #16021

kkewwei commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024

msfroh commented Sep 20, 2024

kkewwei commented Sep 21, 2024 •

edited

Loading

github-actions bot commented Sep 21, 2024

codecov bot commented Sep 21, 2024 •

edited

Loading

kkewwei commented Sep 24, 2024 •

edited

Loading

opensearch-trigger-bot bot commented Nov 6, 2024

jed326 Nov 6, 2024

kkewwei Nov 12, 2024 •

edited

Loading

rajiv-kv Jan 8, 2025

kkewwei Jan 12, 2025

rajiv-kv left a comment

rajiv-kv Jan 8, 2025

rajiv-kv Jan 8, 2025

github-actions bot commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

kkewwei commented Jan 15, 2025

rajiv-kv Jan 15, 2025 •

edited

Loading

kkewwei Jan 15, 2025

github-actions bot commented Jan 15, 2025

		@@ -2171,7 +2172,7 @@ public String getClusterManagerName(@Nullable String viaNode) {
		return client.admin().cluster().prepareState().get().getState().nodes().getClusterManagerNode().getName();
		} catch (Exception e) {

Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes #16021

Are you sure you want to change the base?

Fix Flaky Test SpecificClusterManagerNodesIT.testElectOnlyBetweenClusterManagerNodes #16021

Conversation

kkewwei commented Sep 20, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Sep 20, 2024

msfroh commented Sep 20, 2024

kkewwei commented Sep 21, 2024 • edited Loading

github-actions bot commented Sep 21, 2024

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

kkewwei commented Sep 24, 2024 • edited Loading

opensearch-trigger-bot bot commented Nov 6, 2024

jed326 Nov 6, 2024

Choose a reason for hiding this comment

kkewwei Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

rajiv-kv Jan 8, 2025

Choose a reason for hiding this comment

kkewwei Jan 12, 2025

Choose a reason for hiding this comment

rajiv-kv left a comment

Choose a reason for hiding this comment

rajiv-kv Jan 8, 2025

Choose a reason for hiding this comment

rajiv-kv Jan 8, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

kkewwei commented Jan 15, 2025

rajiv-kv Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

kkewwei Jan 15, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 15, 2025

kkewwei commented Sep 20, 2024 •

edited

Loading

kkewwei commented Sep 21, 2024 •

edited

Loading

codecov bot commented Sep 21, 2024 •

edited

Loading

kkewwei commented Sep 24, 2024 •

edited

Loading

kkewwei Nov 12, 2024 •

edited

Loading

rajiv-kv Jan 15, 2025 •

edited

Loading