-
Notifications
You must be signed in to change notification settings - Fork 40
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
Allow to disable ZIP compression & fix bug in overwriting info files #114
Conversation
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.
Looks good for CI
Thanks for this PR. That said I'd like to investigate it further if possible. Also the merge is blocked as |
uncompressed. Also rewrite coredump piping code to use io::copy which prevents core dump corruption in large core dumps. Signed-off-by: André Stein <andre@deepl.com>
Thanks for the comment! I just ran Your reaction is completely right. I just though I wouldn't want to disable it for everybody because for some sizes it probably works - otherwise I would have suspected much more bug reports. In my case it is a 8GB core dump on an Ubuntu 22.04 machine. |
Thanks for the update. |
Hi there, are there any updates on getting this reproduced on your end? Let me know if you need more information! |
Haven't had the bandwidth to look at it yet. Too much happening at this time of year. |
I followed your idea and ran the tests locally and I can produce a failed unit test with a random core file of size 512M:
Test output:
A core file of 256M was working fine BTW. As a side note: for the tests I always needed to clear the folder |
Excellent thanks for looking into this and providing reproduction steps. |
Also: if I change the tests to use the new |
Found the problem - The default time out for the thread is 120 seconds and the zip for 512M is taking longer than that. Given the impact of compression I think having the option to disable it is a good feature to have so I'm happy to land this PR. @timbuchwaldt any additional thoughts on the timeout setting change? |
containers. Signed-off-by: André Stein <andre@deepl.com>
Signed-off-by: André Stein <andre@deepl.com>
Okay perfect, thanks for your help! I just pushed a commit that changes the default for the timeout parameter. |
Thanks for the change. Can you apply the signoff for DCO please I'll look to merge this over the weekend. |
Yes, sorry, done! Thank you a lot for taking care! |
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.
Discussed questions commit and this LGTM
One question: is there a plan when this going to be released in a new minor release? Thanks. |
Hey @stonemaster |
This MR fixes two issues seen in a production environment:
io::copy
instead of the manual read/write logic. It's unclear to me why because I am not a Rust guru. Another problem with big core dumps was that they turned out to be corrupted when compression is enabled. The-D
flag works well for that and just stores data as-is in the resulting ZIP file.counter
variable to gather information on containers was never incremented thus overwriting previous container information.