Fix move_and_async_delete_path #32020

apfitzge · 2023-06-07T20:56:01Z

Problem

purge_old_bank_snapshots_at_startup async deletes account snapshot paths.
The rename breaks symlinks, and the _to_be_deleted paths are now considered orphaned.
Then clean_orphaned_account_snapshot_dirs tries to delete them again, via rename & delete.
This rugs the original threads which were doing the delete.

Summary of Changes

Use a static set to track which paths are currently being deleted by move_and_async_delete_path (asynchronously or synchronously).
This prevents double calls, as well as trying to delete an in-progress *_to_be_deleted.

Fixes #

apfitzge · 2023-06-07T20:57:35Z

runtime/src/snapshot_utils.rs

@@ -478,37 +479,47 @@ pub fn move_and_async_delete_path_contents(path: impl AsRef<Path>) {
 }

 /// Delete directories/files asynchronously to avoid blocking on it.
-/// First, in sync context, rename the original path to *_deleted,
-/// then spawn a thread to delete the renamed path.
-/// If the process is killed and the deleting process is not done,


Deleted this part of the comment. There's nothing about this function itself which would do it the next time the process life.

apfitzge · 2023-06-07T21:02:42Z

runtime/src/snapshot_utils.rs

+        return;
+    }
+
+    path_delete.push("_to_be_deleted");
    if let Err(err) = std::fs::rename(&path, &path_delete) {


if we can't rename it - there's a few reasons:

permission issues (can't do anything about that 😦 )

path_delete exists

path does not exist (already confirmed it does, while holding the lock)

if path_delete exists, then we have both path and path_exists, potentially because of a crash mid-delete on the last validator run.

another case is that we have move_and_async_delete_path_contents which re-creates the original directory!

apfitzge · 2023-06-07T21:04:54Z

runtime/src/snapshot_utils.rs

    if let Err(err) = std::fs::rename(&path, &path_delete) {
        warn!(
            "Path renaming failed: {}.  Falling back to rm_dir in sync mode",
            err.to_string()
        );
+        // Although the delete here is synchronous, we want to prevent another thread


issues were not explicitly from multiple threads, but w/ the mutex lock it's easy enough to make this thread-safe (I think?)

runtime/src/snapshot_utils.rs

codecov · 2023-06-07T22:25:22Z

Codecov Report

Merging #32020 (97ce0c2) into master (8596e00) will decrease coverage by 0.1%.
The diff coverage is 66.6%.

@@            Coverage Diff            @@
##           master   #32020     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         763      763             
  Lines      207671   207676      +5     
=========================================
- Hits       170073   170068      -5     
- Misses      37598    37608     +10

brooksprumo

Looks good to me, I'll test this version out.

Also, any reason this PR is still in draft?

brooksprumo · 2023-06-08T14:09:08Z

I'll test this version out.

Ran this branch on a validator that I tried to put into the same error cases (i.e. orphaned accounts/snapshot/ dirs that were renamed but not deleted yet), and it worked/didn't crash. Yay!

apfitzge · 2023-06-08T17:19:33Z

Also, any reason this PR is still in draft?

I had left for the day before CI finished 😴

apfitzge · 2023-06-08T17:33:45Z

From logs:

[2023-06-08T14:00:12.550304957Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/snapshot/198280062_to_be_deleted...
[2023-06-08T14:00:12.550347197Z INFO  solana_runtime::snapshot_utils] Purged incomplete snapshot dir: /home/sol/ledger/snapshot/198280062
[2023-06-08T14:00:12.550540425Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/snapshot/198279943_to_be_deleted...
[2023-06-08T14:00:12.633155292Z DEBUG solana_runtime::snapshot_utils] Retained bank snapshot for slot 198279838, and purged the rest.
[2023-06-08T14:00:12.633181301Z INFO  solana_core::validator] Cleaning orphaned account snapshot directories..
[2023-06-08T14:00:12.633229062Z INFO  solana_runtime::snapshot_utils] Removing orphaned account snapshot hardlink directory: /home/sol/ledger/accounts/snapshot/198280062_to_be_deleted
[2023-06-08T14:00:12.633238470Z INFO  solana_runtime::snapshot_utils] Removing orphaned account snapshot hardlink directory: /home/sol/ledger/accounts/snapshot/198279943_to_be_deleted
...
[2023-06-08T14:00:15.075610449Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/run_to_be_deleted... Done, and took 2.5s
[2023-06-08T14:00:15.139898521Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/snapshot/198279943_to_be_deleted... Done, and took 2.6s
[2023-06-08T14:00:15.292164501Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/snapshot/198280062_to_be_deleted... Done, and took 2.7s
[2023-06-08T14:00:16.985215590Z INFO  solana_ledger::bank_forks_utils] Initializing bank snapshot path: /home/sol/ledger/snapshot
[2023-06-08T14:00:16.985682749Z TRACE solana_runtime::snapshot_utils] background deleting /home/sol/ledger/accounts/snapshot/198279838_to_be_deleted...

We begin background deleting 198279943 (now 198279943_to_be_deleted), then we see the orphan clean up also try to clean it up at 14:00:12.633238470Z. This would have previously renamed the dir to 198279943_to_be_deleted_to_be_deleted and rugged the original background delete task.

The check on line 501 prevented us from renaming, and the call to move_and_async_delete_path effectively does nothing.

brooksprumo

lgtm

t-nelson · 2023-06-08T18:00:41Z

sry apparently i queued this up as a review message instead of sending a single as intended...

wouldn't it be cleaner to make the delete thread long living and pipe the paths to be deleted over to it with a channel. delete thread could inspect the directory on startup to snag any remnants from previous, interrupted work before beginning to consume the channel

brooksprumo · 2023-06-08T18:23:22Z

Heh, I suggested this offline with Andrew as well. We can impl the channel approach in master/v1.17. We need a fix for v1.16, so probably a smaller change is safer. Wdyt?

t-nelson · 2023-06-08T18:38:52Z

heh, i'm not sure this is actually "smaller" in terms of complexity (or complete, for that matter due to how branchy that function is). also seems like the number bg delete threads is effectively unbounded, which probably isn't great for io

brooksprumo · 2023-06-08T19:11:34Z

I'd like to add the v1.16 backport label so that we'll have it ready in case we need it for a release. Don't need to merge it unless needed. Is that ok?

also seems like the number bg delete threads is effectively unbounded, which probably isn't great for io

Yeah there can be multiple delete threads, but we know all the spots its called. This is mostly solving an issue at startup, and I think it's ok if there's an io spike then. We save a decent amount of time with this async-delete code.

heh, i'm not sure this is actually "smaller" in terms of complexity (or complete, for that matter due to how branchy that function is).

Yeah, totally agree. Mostly not sure how high to prioritize an alternative.

apfitzge · 2023-06-08T21:58:22Z

wouldn't it be cleaner to make the delete thread long living and pipe the paths to be deleted over to it with a channel. delete thread could inspect the directory on startup to snag any remnants from previous, interrupted work before beginning to consume the channel

We could do it either way. I tried to keep things effectively the same but with protection.
I didn't want to add an extra thread waiting around the entire app lifetime, even if it is effectively sleeping. also can delete all account paths in parallel instead of sequentially in a "delete thread".

Not too strong of an opinion on it, just tried to keep things as simple as possible for the fix.

(cherry picked from commit 3ba05d9)

Fix move_and_async_delete_path (#32020) (cherry picked from commit 3ba05d9) Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>

static set to track in-progress async deletes

04e234b

apfitzge changed the title ~~static set to track in-progress async deletes~~ Fix move_and_async_delete_path Jun 7, 2023

apfitzge requested a review from brooksprumo June 7, 2023 20:56

apfitzge commented Jun 7, 2023

View reviewed changes

brooksprumo reviewed Jun 7, 2023

View reviewed changes

runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved

revert set_file_name change

330f772

brooksprumo reviewed Jun 7, 2023

View reviewed changes

runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved

use path for contains checks

97ce0c2

brooksprumo reviewed Jun 8, 2023

View reviewed changes

apfitzge marked this pull request as ready for review June 8, 2023 17:19

brooksprumo approved these changes Jun 8, 2023

View reviewed changes

apfitzge merged commit 3ba05d9 into solana-labs:master Jun 8, 2023

apfitzge deleted the async_delete_tracking branch June 8, 2023 17:55

brooksprumo added the v1.16 PRs that should be backported to v1.16 label Jun 9, 2023

mergify bot mentioned this pull request Jun 9, 2023

v1.16: Fix move_and_async_delete_path (backport of #32020) #32043

Merged

mergify bot pushed a commit that referenced this pull request Jun 9, 2023

Fix move_and_async_delete_path (#32020)

24cb011

(cherry picked from commit 3ba05d9)

mergify bot added a commit that referenced this pull request Jun 9, 2023

v1.16: Fix move_and_async_delete_path (backport of #32020) (#32043)

fa97441

Fix move_and_async_delete_path (#32020) (cherry picked from commit 3ba05d9) Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix move_and_async_delete_path #32020

Fix move_and_async_delete_path #32020

apfitzge commented Jun 7, 2023

apfitzge Jun 7, 2023

apfitzge Jun 7, 2023

apfitzge Jun 7, 2023

apfitzge Jun 7, 2023

codecov bot commented Jun 7, 2023

brooksprumo left a comment

brooksprumo commented Jun 8, 2023

apfitzge commented Jun 8, 2023

apfitzge commented Jun 8, 2023

brooksprumo left a comment

t-nelson commented Jun 8, 2023

brooksprumo commented Jun 8, 2023

t-nelson commented Jun 8, 2023

brooksprumo commented Jun 8, 2023

apfitzge commented Jun 8, 2023

Fix move_and_async_delete_path #32020

Fix move_and_async_delete_path #32020

Conversation

apfitzge commented Jun 7, 2023

Problem

Summary of Changes

apfitzge Jun 7, 2023

Choose a reason for hiding this comment

apfitzge Jun 7, 2023

Choose a reason for hiding this comment

apfitzge Jun 7, 2023

Choose a reason for hiding this comment

apfitzge Jun 7, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2023

Codecov Report

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo commented Jun 8, 2023

apfitzge commented Jun 8, 2023

apfitzge commented Jun 8, 2023

brooksprumo left a comment

Choose a reason for hiding this comment

t-nelson commented Jun 8, 2023

brooksprumo commented Jun 8, 2023

t-nelson commented Jun 8, 2023

brooksprumo commented Jun 8, 2023

apfitzge commented Jun 8, 2023