-
Notifications
You must be signed in to change notification settings - Fork 148
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
Fix IPU being blocked by resource limitations #1256
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
Note that first time contributors cannot run tests automatically - they need to be started by a reviewer. It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
ae14f2e
to
1bfaa9b
Compare
/packit copr-build |
/packit test |
I don't really like the redundancy of code here, but I'm not sure where should the common logic be placed. |
I think |
/packit retest-failed |
87f52e4
to
e4afdea
Compare
Thanks, I put it there |
/packit copr-build |
all standard tests passed. executing extended tests |
/packit test |
ah.. I thought it's different PR :D mama mia! |
67a4af0
to
1b9ecbe
Compare
/packit retest-failed |
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.
Tested for both maximum file size and maximum open fd's, works as expected.
Tested by setting a limit low enough that leapp fails without this patch and confirming it then works with it.
However unit tests are missing.
First resource limit is maximum number of open file descriptors limit, second one being limit for maximum writable file size. Plus add unit tests. Resolves: RHEL-26459 and RHEL-16881
2ee1137
to
c217010
Compare
Latest push squashes commits. |
Unit tests were added. |
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.
LGTM
NOTE: If this breaks local tests in containers for you, just rebuild them (make clean_containers
and then it will be rebuilt the next time you run make test_container
)
First resource limit is maximum number of open file descriptors limit, second one being limit for maximum writable file size.
With resource lib we set the limit for the current process (tested with my testing scripts) and its children processes. By testing pre-upgrade and upgrade I found a threshold of max. open file descriptors to be around 512, (the default value for ulimit -n is 1024) so a small multiplier of the default value shouldn't be a problem to set. With writing size there shouldn't be a problem to set it to "infinity".
Both problems from the tickets were solved, but there could be other side effects.
Resolves: RHEL-26459 and RHEL-16881