Do not shell out for tar #19043
Do not shell out for tar #19043
Conversation
When making a snapshot archive, we used to shell out and call `tar -S` for sparse file support. The tar crate supports sparse files, so no need to do this anymore. Fixes solana-labs#10860
|
Sweet! I think maybe we can remove flushing the mmaps explicitly now as well since we are reading from in-process memory. |
@sakridge One thing I'm not sure is how to actually test this change sufficiently. Is passing CI good enough? It looks like the tar crate added sparse file support, but I'm not sure if it's something that's automatically detected when tar-ing... @t-nelson I know you posted the link in the original issue; do you happen to know off-hand? |
Like if you want to test that it doesn't kill the tar? You can send a signal SIGUSR1 to the validator while it's making a snapshot. |
For the sparse part, you can start a node and it will create snapshots by default, compare those snapshot sizes to a node which is producing snapshots through the normal path. I think if it's not sparse the size difference will be large. Can also take a new snapshot, extract it and see the on-disk size is comparable as before. |
|
Sparse files should be detectable by comparing a file's total and allocated sizes, however browsing that commit I only see the read side of support added. Maybe the write side is already there? I think a lighter test would be to take an existing snapshot, use ledger-tool from this PR to generate a new snapshot, then use ledger-tool from the source release to generate another snapshot from from the PR snapshot. If both snapshots are generated successfully and all three have similar sizes, we're probably good |
Codecov Report
@@ Coverage Diff @@
## master #19043 +/- ##
=======================================
Coverage 82.8% 82.8%
=======================================
Files 449 449
Lines 127997 127988 -9
=======================================
+ Hits 105993 106007 +14
+ Misses 22004 21981 -23 |
|
I ran this PRThere are two snapshot, and I've unzstd'ed them both. So you'll see the output from And here's the result from master (cde1461):Similarly, I have two snapshots for the comparison with and I think this means it's working? What do you think @sakridge @t-nelson? Is generating snapshots via |
This is definitely a good sign! I'd like to see master load a PR-generated snapshot and vice versa so we can be confident in a mixed cluster during the SW upgrade that introduces this change. |
@t-nelson Gotcha! I ran bootstrap-validator 4 times, each for ~1000 slots so a few snapshots archives would be made. I did my PR, then master, then my PR again, and master once more. All runs successfully used the existing snapshot from the previous run (and therefore the opposite branch). |
There was a problem hiding this comment.
@t-nelson Gotcha! I ran bootstrap-validator 4 times, each for ~1000 slots so a few snapshots archives would be made. I did my PR, then master, then my PR again, and master once more. All runs successfully used the existing snapshot from the previous run (and therefore the opposite branch).
Awesome! Let's do it!
|
This fixes this problem as well, where logrotate's SIGUSR causes solana to leave snapshot droppings |
|
@brooksprumo any reason not to backport this one? |
|
@t-nelson I think it should be fine. I don't know what the criteria is to backport commits, so as long as the Rust tar package version is new enough in the previous branch(es) then we should be good. |
Could be. Motivation here is #19766, which I think is more costly than we really want to promote. I'd say v1.7 for sure, then look into v1.6 based on resource leak debug in v1.7 and prospects of an MB upgrade |
|
My vote is v1.7 only, cause it should be easy. The second the resource leak on v1.7 is mitigated it's MB upgrade time! |
|
I'mma do it |
|
Presumably v1.8 too? |
Yeah, but by hand to avoid resolving the conflicts twice |
When making a snapshot archive, we used to shell out and call `tar -S` for sparse file support. The tar crate supports sparse files, so no need to do this anymore. Fixes solana-labs#10860 (cherry picked from commit 68cc714) # Conflicts: # runtime/src/snapshot_utils.rs
When making a snapshot archive, we used to shell out and call
tar -Sfor sparse file support. The tar crate supports sparse files, so no
need to do this anymore.
Fixes #10860