-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-24126 Up the container nproc uplimit from 10000 to 12500 #1450
Conversation
(!) A patch to the testing environment has been detected. |
2 similar comments
(!) A patch to the testing environment has been detected. |
(!) A patch to the testing environment has been detected. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Will merge this to master in morning unless objection. |
dev-support/hbase-personality.sh
Outdated
@@ -513,6 +516,7 @@ function hadoopcheck_parse_args | |||
## @stability evolving | |||
function hadoopcheck_docker_support | |||
{ | |||
DOCKER_EXTRAARGS=("${DOCKER_EXTRAARGS[@]}" "--ulimit" "nproc=12500") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be here. we want this proc limit change all the time, not just when the hadoopcheck test is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow. The new 12500 number is put everywhere, not just here conditional on when hadoopcheck runs. Thanks for taking a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reference you've added here is in the function that defines what is needed when we run docker specifically just when we're doing the hadoopcheck test.
if we need it all the time, then it should be elsewhere. It looks it should be coming in from the YETUS_ARGS that have been added to the wrapper. If that's the case, then removing it here should have no impact on what happens. If removing it here makes it so things don't do what you expect, then something else is broken because the inclusion here is only going to happen when the hadoopcheck test is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me push new patch w/ your change. Thanks @busbey
Address @busbey comment. |
(!) A patch to the testing environment has been detected. |
2 similar comments
(!) A patch to the testing environment has been detected. |
(!) A patch to the testing environment has been detected. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -43,7 +43,7 @@ pipeline { | |||
flaky_args=("${flaky_args[@]}" --urls "${JENKINS_URL}/job/HBase%20Nightly/job/${BRANCH_NAME}" --is-yetus True --max-builds 10) | |||
flaky_args=("${flaky_args[@]}" --urls "${JENKINS_URL}/job/HBase-Flaky-Tests/job/${BRANCH_NAME}" --is-yetus False --max-builds 30) | |||
docker build -t hbase-dev-support dev-support | |||
docker run -v "${WORKSPACE}":/hbase --workdir=/hbase hbase-dev-support python dev-support/flaky-tests/report-flakies.py --mvn -v "${flaky_args[@]}" | |||
docker run --ulimit nproc=12500 -v "${WORKSPACE}":/hbase --workdir=/hbase hbase-dev-support python dev-support/flaky-tests/report-flakies.py --mvn -v "${flaky_args[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes. This is just hiding over here in the corner. TODO: update flaky reporting to use yetus to run builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, one way to run all builds would be sweet.
@@ -159,4 +159,4 @@ done | |||
echo "Successfully built ${IMAGE_NAME}." | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, I didn't know this script existed. I probably broke it with the improvements that went in with HBASE-23945.
@@ -81,8 +81,11 @@ function personality_globals | |||
|
|||
# Yetus 0.7.0 enforces limits. Default proclimit is 1000. | |||
# Up it. See HBASE-19902 for how we arrived at this number. | |||
# NOTE: I don't think changing this has an effect. Set the | |||
# --proclimit passed to yetus. This seems to do what we | |||
# need changing proclimit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. In yetus, the env variable is PROC_LIMIT
. You found a bug :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try filing an issue.
@@ -66,7 +66,7 @@ YETUS_ARGS=("--branch=${BRANCH_NAME}" "${YETUS_ARGS[@]}") | |||
YETUS_ARGS=("--tests-filter=${TESTS_FILTER}" "${YETUS_ARGS[@]}") | |||
YETUS_ARGS=("--ignore-unknown-options=true" "${YETUS_ARGS[@]}") | |||
# Why are these not being picked up from hbase-personality? | |||
YETUS_ARGS=("--proclimit=10000" "${YETUS_ARGS[@]}") | |||
YETUS_ARGS=("--proclimit=12500" "${YETUS_ARGS[@]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the above change for PROC_LIMIT
works, this line can be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. For now they needed. Let me file an issue.
@@ -102,7 +102,7 @@ YETUS_ARGS+=("--reapermode=kill") | |||
# set relatively high limits for ASF machines | |||
# changing these to higher values may cause problems | |||
# with other jobs on systemd-enabled machines | |||
YETUS_ARGS+=("--proclimit=10000") | |||
YETUS_ARGS+=("--proclimit=12500") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise.
Signed-off-by: Reid Chan <[email protected]> Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
After push, I went to file issue against YETUS and noticed YETUS expects PROC_LIMIT, not PROCLIMIT as I have in our personality (an old error but relevant here). I reverted. Let me see if using PROC_LIMIT instead helps here. New PR coming. |
Oops. I meant to say that you found a bug in our personality, not in Yetus. That wasn't clear in my earlier comment. |
bq. Oops. I meant to say that you found a bug in our personality, not in Yetus. That wasn't clear in my earlier comment. Took me a while but now I get what you meant. |
apache#1450)" This reverts commit 14342b6.
apache#1450)" This reverts commit 14342b6.
…e#1450) Signed-off-by: Reid Chan <[email protected]> Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
apache#1450)" This reverts commit 14342b6.
…e#1450) Signed-off-by: Reid Chan <[email protected]> Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
apache#1450)" This reverts commit 14342b6.
…e#1450) Signed-off-by: Reid Chan <[email protected]> Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
apache#1450)" This reverts commit 14342b6.
…e#1450) Signed-off-by: Reid Chan <[email protected]> Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
apache#1450)" This reverts commit f5b3945.
No description provided.