Add the ability to run Rest integration tests with 1 allocated processor #89234

fcofdez · 2022-08-10T08:29:51Z

This commit adds a new system property tests.configure_test_clusters_with_one_processor
that configures Rest integration tests to run with 1 processor, this should help finding possible
deadlocks (if any) when the Elasticsearch nodes have node.processors set to 1.

With this change it should be possible to catch possible deadlocks that would surface when a node runs in a 1 CPU hosts or node.processors is configured to use just 1 processor when the ThreadPools are configured/

elasticsearchmachine · 2022-08-10T08:30:13Z

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

fcofdez · 2022-08-10T09:09:47Z

I haven't set it up for Rest tests as the nodes are configured in gradle and we don't have access to the test random seed there.

DaveCTurner · 2022-08-10T09:38:27Z

configured in gradle and we don't have access to the test random seed

Could you do something like this?

elasticsearch/x-pack/qa/multi-cluster-search-security/build.gradle

Line 25 in 546a2e2

    
           boolean proxyMode = (new Random(Long.parseUnsignedLong(BuildParams.testSeed.tokenize(':').get(0), 16))).nextBoolean()

DaveCTurner

LGTM

fcofdez · 2022-08-11T08:04:45Z

I discussed this with @mark-vieira offline and we decided to define a new CI job that would run all the tests using 1 processor a few times a day, the arguments in favour of that approach were:

Introducing this change based on randomness could make difficult to identify the root cause (is this test failure cased by a deadlock? or is it something else?) while if we get the same test failure in the specific CI job it would be easier to triage.
There were some concerns about the slowness of tests running with 1 processor configured, but for most tests that shouldn't be problematic.

As cons:

We would run the tests with 1 processor less often, meaning that possible problems would take longer to surface.

The idea is to change this PR and let the test code accept a new system property that would set node.processors to 1 and @mark-vieira will create the CI job to run the tests with that flag.

Are there any objections to this @henningandersen @DaveCTurner?

DaveCTurner · 2022-08-11T08:44:32Z

Are there any objections to this?

No, that also sounds good to me.

henningandersen · 2022-08-11T09:29:03Z

Are there any objections to this

I'd like to keep the randomness in InternalTestCluster (since we always had it), but do not mind for the REST tests (though it seems a bit overkill for something we expect not to result in more failures, nor a slowdown).

mark-vieira

One comment that I'd like @breskeby to provide feedback on. Otherwise LGTM.

mark-vieira · 2022-08-11T18:25:06Z

test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java

    }

    protected void indexRandomDocs(String index, int numdocs) throws InterruptedException {
+        indexRandomDocs(index, numdocs, true);


Are these simply test fixes related to this change?

Yes, that's correct

mark-vieira · 2022-08-11T18:25:35Z

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

        builder.put(
            EsExecutors.NODE_PROCESSORS_SETTING.getKey(),
-            1 + random.nextInt(Math.min(4, Runtime.getRuntime().availableProcessors()))
+            RandomNumbers.randomIntBetween(random, 1, Runtime.getRuntime().availableProcessors())


Ok, so looks like we were already doing this sort of thing for internal cluster tests.

mark-vieira · 2022-08-11T18:26:51Z

...ols-internal/src/main/java/org/elasticsearch/gradle/internal/InternalTestClustersPlugin.java

    }

+    private boolean shouldConfigureTestClustersWithOneProcessor() {
+        return Boolean.parseBoolean(System.getProperty("tests.configure_test_clusters_with_one_processor", "false"));


@breskeby Can you confirm it's no longer necessary to use provider factory to get system properties to ensure things are config cache compliant?

@mark-vieira yes its not required anymore with newer gradle versions

fcofdez · 2022-08-17T13:00:01Z

@elasticmachine update branch

fcofdez · 2022-08-17T15:42:53Z

Thanks all!

…89562) Ass a follow up to #89234, we want to also include this system property in reproduction lines so that developers are actually enabling this setting when trying to reproduce these failures.

…lastic#89562) Ass a follow up to elastic#89234, we want to also include this system property in reproduction lines so that developers are actually enabling this setting when trying to reproduce these failures.

Set node.processors lower bound to 1 in InternalTestCluster

a91183d

With this change it should be possible to catch possible deadlocks that would surface when a node runs in a 1 CPU hosts or node.processors is configured to use just 1 processor when the ThreadPools are configured/

Set node.processors 1 rarely in rest tests

776ee39

fcofdez requested a review from DaveCTurner August 10, 2022 15:51

fcofdez changed the title ~~Set node.processors lower bound to 1 in InternalTestCluster#getRandomNodeSettings~~ Set node.processors lower bound to 1 in integration tests Aug 10, 2022

DaveCTurner approved these changes Aug 10, 2022

View reviewed changes

fcofdez added 3 commits August 11, 2022 15:13

Add system property to configure rest tests

1ea00c5

Merge remote-tracking branch 'origin/main' into random-node-processors

dafa82f

Fix AbortedRestoreIT

ae4ef04

mark-vieira approved these changes Aug 11, 2022

View reviewed changes

breskeby self-requested a review August 11, 2022 21:32

breskeby approved these changes Aug 11, 2022

View reviewed changes

Bound max processors used

a1327e9

Merge branch 'main' into random-node-processors

0e918db

fcofdez changed the title ~~Set node.processors lower bound to 1 in integration tests~~ Add the ability to run Rest integration tests with 1 allocated processor Aug 17, 2022

fcofdez merged commit cbea639 into elastic:main Aug 17, 2022

mark-vieira mentioned this pull request Aug 23, 2022

Add configure_test_clusters_with_one_processor to repro line printer #89562

Merged

kingherc mentioned this pull request Oct 6, 2022

[CI] SnapshotRepoTestKitClientYamlTestSuiteIT test {p0=/10_analyze/Timeout with large blobs} failing #90353

Closed

Add the ability to run Rest integration tests with 1 allocated processor #89234

Add the ability to run Rest integration tests with 1 allocated processor #89234

Uh oh!

Conversation

fcofdez commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 10, 2022

Uh oh!

fcofdez commented Aug 10, 2022

Uh oh!

DaveCTurner commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Aug 11, 2022

Uh oh!

DaveCTurner commented Aug 11, 2022

Uh oh!

henningandersen commented Aug 11, 2022

Uh oh!

mark-vieira left a comment

Choose a reason for hiding this comment

Uh oh!

mark-vieira Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

fcofdez Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

mark-vieira Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

mark-vieira Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

breskeby Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Aug 17, 2022

Uh oh!

fcofdez commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fcofdez commented Aug 10, 2022 •

edited

Loading

DaveCTurner commented Aug 10, 2022 •

edited

Loading