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.

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

Limit max workers for windows gradle check. #112

Closed
wants to merge 1 commit into from

Conversation

mch2
Copy link
Member

@mch2 mch2 commented Jan 19, 2023

Signed-off-by: Marc Handalian [email protected]

Description

Windows gradle checks are running into oom issues - see https://build.ci.opensearch.org/job/gradle-check/9515/consoleFull.
I believe this is because the hosts used are limited to 32GB of RAM while spawning threads up to the amount of available cores. Each worker is allowed max 3GB of ram, so we can easily run oom if too many threads with over 10 workers spawned running expensive tests. This PR limits the max to 8 workers to avoid this scenario.

Issues Resolved

closes #113

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.

@mch2 mch2 requested a review from a team as a code owner January 19, 2023 01:58
Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Looks like tests are failing! Not sure if you ran ./gradlew test -info -Ppipeline.stack.write=true so that txt file gets update accordingly.


if (uname -s | grep -i NT); then
echo "Start gradlecheck on windows"
./gradlew clean && ./gradlew check --max-workers 8 -Dtests.coverage=true --no-daemon --no-scan || GRADLE_CHECK_STATUS=1
Copy link
Member

Choose a reason for hiding this comment

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

Can the max-workers change in future? Asking if it can be something that should be passed as an argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes that would be ideal to pass the entire command from https://github.com/opensearch-project/OpenSearch/blob/main/.github/workflows/gradle-check.yml. This change is against windows CI only that runs nightly & that config is not in the other repo, so we'll also want to move that before being able to pass as a param bc we don't want to impact non windows checks.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe time to redo gradle check workflow as well? :) (cc: @bbarani)

Looks like it is distributed in around 3 repos today which is
Build repo:
https://github.com/opensearch-project/opensearch-build/blob/main/scripts/gradle/gradle-check.sh
https://github.com/opensearch-project/opensearch-build/blob/main/jenkins/gradle/gradle-check.jenkinsfile

This repo and OpenSearch repo.

Adding @peterzhuamazon to this conversation as he is the original contributor.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @gaiksaya I will work with @mch2 on this.

@codecov-commenter
Copy link

Codecov Report

Merging #112 (af62225) into main (ea7a7cc) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #112   +/-   ##
=========================================
  Coverage     83.41%   83.41%           
  Complexity       24       24           
=========================================
  Files            68       68           
  Lines           193      193           
  Branches         11       11           
=========================================
  Hits            161      161           
  Misses           25       25           
  Partials          7        7           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@peterzhuamazon
Copy link
Member

Another way is to add another profile just for gradle check AMI in opensearch-ci.

@mch2
Copy link
Member Author

mch2 commented Jan 23, 2023

Another way is to add another profile just for gradle check AMI in opensearch-ci.

Just cut this #114

@peterzhuamazon
Copy link
Member

Pausing this a bit as we are testing the bigger winrm memory setups here:

@mch2
Copy link
Member Author

mch2 commented Feb 19, 2023

Closing this as memory issues were resolved by opensearch-project/opensearch-ci#242

@mch2 mch2 closed this Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Windows gradle checks running OOM.
4 participants