-
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
overlaygen: cap max sparse file size #1294
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. |
7495b2d
to
3bbd3e8
Compare
Makefile
Outdated
$(_PYTHON_VENV) -m pytest $(REPORT_ARG) $(TEST_PATHS) $(LIBRARY_PATH) $(PYTEST_ARGS) | ||
$(_PYTHON_VENV) -m pytest $(REPORT_ARG) $(LIBRARY_PATH) $(PYTEST_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.
good catch!
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.
Actually, I committed this by accident - if you do make test_no_lint
you want to also test actors. I have removed the $(TEST_PATHS)
because there was no way (at least obvious to me) to test just common libraries.
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 am actually puzzled now :-D because I checked the Makefile as the change looked weird to me originally but going through it, it started to make sense to me as a good step. Reading your explanation and the Makefile again, I am now trying to find out why I thought that :D
😶🌫️ + 🌴 --> 🤦
Even Homer nods 🤷
On systems with large disks (e.g. 16TB) with lots of free space, leapp might attemt to create files larger than the max file size of the underlying FS. Attempting to create such large files causes leapp to crash. This patch caps the max image size to 1TB, based on empirical evidence that more free space is not needed for the upgrade RPM transaction. Jira-ref: RHEL-57064
3bbd3e8
to
71c6248
Compare
Tested. works as expected.
# leapp-inspector actors --actor target_userspace_creator --terminal-like | grep -e "- \['/bin/dd'," | grep big_disk
- ['/bin/dd', 'if=/dev/zero', 'of=/var/lib/leapp/scratch/diskimages/root_mnt_big_disk', 'bs=1M', 'count=0', 'seek=10260080']
# leapp-inspector actors --actor target_userspace_creator --terminal-like | grep -e "- \['/bin/dd'," | grep big_disk
- ['/bin/dd', 'if=/dev/zero', 'of=/var/lib/leapp/scratch/diskimages/root_mnt_big_disk', 'bs=1M', 'count=0', 'seek=1048576'] |
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 and works as expected (see comment above)
If the source system has a very large disk attached, leapp would attempt to create similarly large sparse file, possibly hitting the max file size limits of the filesystem where these images are created. This patch caps the maximum size of any of the created sparse files to 1TB, preventing such issues.
Jira ref: RHEL-57064