Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use unique ports per test worker #43983

Merged
merged 4 commits into from
Jul 5, 2019
Merged

Use unique ports per test worker #43983

merged 4 commits into from
Jul 5, 2019

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jul 4, 2019

A recent PR to replace the randomized testing task by Gradle's test task broke a property used by our tests to distinguish between different parallel test processes and allocate separate port ranges for those parallel JVMs so that the tests would not communicate with each other. This has caused failures like #41067 where tests from different JVMs are now interacting with each other.

As tests are now executed with the regular Gradle runner, this PR now switches to the logic that recognizes different test processes with the help of the org.gradle.test.worker system property, which is unique for each process, see https://docs.gradle.org/current/userguide/java_testing.html

@ywelsch ywelsch requested a review from henningandersen July 4, 2019 13:47
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@ywelsch ywelsch requested a review from mark-vieira July 4, 2019 13:48
@ywelsch ywelsch mentioned this pull request Jul 4, 2019
Closed
henningandersen
henningandersen approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rjernst
rjernst reviewed Jul 4, 2019
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find! I wonder if we could fail if we can't find this property? If gradle changes something, we should fail fast instead of silently going on like we have for these months since moving the runner from randomized testing.

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 4, 2019

I wonder if we could fail if we can't find this property?

I've added testing for this now. As I wasn't exactly sure where to put what, please have a look and let me know what you think.

@rjernst
Copy link
Member

rjernst commented Jul 4, 2019

I don't think we should care about the tests.gradle sysprops. Iirc we set that ourselves. Why couldn't we error if the sysprop you are checking for the gradle worker doesn't exist?

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 4, 2019

I don't think we should care about the tests.gradle sysprops. Iirc we set that ourselves.

alright, I missed that.

Why couldn't we error if the sysprop you are checking for the gradle worker doesn't exist?

We still want to be able to run tests from the IDE. If we always require org.gradle.test.worker, that's going to be a pain.

alpar-t
alpar-t approved these changes Jul 5, 2019
View reviewed changes
Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Great find !

@ywelsch ywelsch merged commit 3166f7b into elastic:master Jul 5, 2019
@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 5, 2019

I forgot to say here that the credits go to @henningandersen. He pointed me to this while looking at #43970.

@ywelsch ywelsch mentioned this pull request Jul 5, 2019
Closed
ywelsch added a commit that referenced this pull request Jul 5, 2019
@ywelsch
Use unique ports per test worker (#43983)
d090fa5 
* Use unique ports per test worker

* Add test for system property

* check presence of tests.gradle

* Revert "check presence of tests.gradle"

This reverts commit 2fee751.
ywelsch added a commit that referenced this pull request Jul 5, 2019
@ywelsch
Use unique ports per test worker (#43983)
0a91464 
* Use unique ports per test worker

* Add test for system property

* check presence of tests.gradle

* Revert "check presence of tests.gradle"

This reverts commit 2fee751.
@mark-vieira
Copy link
Contributor

As tests are now executed with the regular Gradle runner, this PR now switches to the logic that recognizes different test processes with the help of the org.gradle.test.worker system property, which is unique for each process, see https://docs.gradle.org/current/userguide/java_testing.html

This is awesome. Great find indeed. I had no idea that Gradle injected that system property into tests.

@original-brownbear original-brownbear mentioned this pull request Jul 9, 2019
Merged
alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Jul 11, 2019
@alpar-t
Fix port range allocation with large worker IDs
fe7aba7 
Relates to elastic#43983

The IDs gradle uses are incremented for the lifetime of the daemon which
can result in port ranges that are outside the valid range.
This change implements a modulo based formula to wrap the port ranges
when the IDs get too large.

Adresses elastic#44134 but elastic#44157 is also required to be able to close it.
@alpar-t alpar-t mentioned this pull request Jul 11, 2019
Merged
alpar-t added a commit that referenced this pull request Jul 12, 2019
@alpar-t
Fix port range allocation with large worker IDs (#44213)
433b345 
* Fix port range allocation with large worker IDs

Relates to #43983

The IDs gradle uses are incremented for the lifetime of the daemon which
can result in port ranges that are outside the valid range.
This change implements a modulo based formula to wrap the port ranges
when the IDs get too large.

Adresses #44134 but #44157 is also required to be able to close it.
alpar-t added a commit that referenced this pull request Jul 12, 2019
@alpar-t
Fix port range allocation with large worker IDs (#44213)
8d35583 
* Fix port range allocation with large worker IDs

Relates to #43983

The IDs gradle uses are incremented for the lifetime of the daemon which
can result in port ranges that are outside the valid range.
This change implements a modulo based formula to wrap the port ranges
when the IDs get too large.

Adresses #44134 but #44157 is also required to be able to close it.
alpar-t added a commit that referenced this pull request Jul 12, 2019
@alpar-t
Fix port range allocation with large worker IDs (#44213)
ba01bbf 
* Fix port range allocation with large worker IDs

Relates to #43983

The IDs gradle uses are incremented for the lifetime of the daemon which
can result in port ranges that are outside the valid range.
This change implements a modulo based formula to wrap the port ranges
when the IDs get too large.

Adresses #44134 but #44157 is also required to be able to close it.
@henningandersen henningandersen mentioned this pull request Aug 21, 2019
Closed
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst left review comments

@alpar-t alpar-t alpar-t approved these changes

@henningandersen henningandersen henningandersen approved these changes

@mark-vieira mark-vieira Awaiting requested review from mark-vieira

Assignees
No one assigned
Labels
>bug :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.3.0 v7.4.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ywelsch @elasticmachine @rjernst @mark-vieira @alpar-t @henningandersen @jakelandis