Purges all bank snapshots after fastboot

[brooksprumo]

Problem

Given the following scenario:

• a (bank) snapshot interval of 100
• a node starts up and loads from a bank snapshot at slot 200 (i.e. fastboot)
• the node crashes shortly after load; before the next bank snapshot is taken (before slot 300)
• and shrink has run (e.g. shrink proceeds up to slot 275)
• the node restarts, still with fastboot enabled

Then the result is:

• due to shrink, the account storage files on disk will (may) have changed, and reflect the account state as of slot 275
• the bank snapshot at 200 has hard links to the account storage files, and those account storage files may have been shrunk
• the bank snapshot contains information about the accounts as of slot 200, but the actual storages reflect the accounts as of slot 275
• at startup, the bank snapshot at slot 200 will be selected for fastboot again, and the node will crash, saying that the/an account storage file does not hold the correct number of accounts

If the node has a script to auto-restart, it will enter a boot-crash loop indefinitely.

Summary of Changes

To break out of the boot-crash loop, purge all bank snapshots after loading from one. In the above scenario, this would cause the node to load from a snapshot archive instead, which is safe. If the node crashes after creating another bank snapshot, then fastboot will work properly again in that situation.

Fixes #35190

More Info

This problem has been hit on v1.17, so I want a solution that can be backported to v1.17. The implemented solution is the least invasive—and safest—one I am aware of. A different solution to allow successfully reusing the bank snapshot from slot 200 will land in master and not be backported.

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.7%. Comparing base (8ad125d) to head (35546ca).
Report is 21 commits behind head on master.

❗ Current head 35546ca differs from pull request most recent head 3e42432. Consider uploading reports for the commit 3e42432 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #35350     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         834      834             
  Lines      224235   224236      +1     
=========================================
- Hits       183390   183367     -23     
- Misses      40845    40869     +24     

[brooksprumo]

@apfitzge Requesting your review since the PR is fastboot-related, and you've reviewed all the fastboot code
@jeffwashington Requesting your review since it touches anything storage related
@steviez Requesting your review since you have a good eye for the system as a whole and if there may be unintended consequences with the impl

Also note I intend to backport this PR, so also consider if there's anything that should be done differently now that'll make the backporting better.

[jeffwashington]

@brooksprumo can you please link the pr that adds purge_all_bank_snapshots that I think also must be backported?

[brooksprumo]

@brooksprumo can you please link the pr that adds purge_all_bank_snapshots that I think also must be backported?

I wasn't originally intending to backport #35291. Instead I was planning on using the previous way to purge all bank snapshots:

snapshot_utils::purge_old_bank_snapshots(&snapshot_config.bank_snapshots_dir, 0, None);

Would backporting #35291 be preferred?

My though process was the backport the least amount of code possible.

[jeffwashington]

@brooksprumo can you please link the pr that adds purge_all_bank_snapshots that I think also must be backported?

I wasn't originally intending to backport #35291. Instead I was planning on using the previous way to purge all bank snapshots:

snapshot_utils::purge_old_bank_snapshots(&snapshot_config.bank_snapshots_dir, 0, None);

Would backporting #35291 be preferred?

My though process was the backport the least amount of code possible.

so the backport pr from this pr into 1.18 would be manually adjusted so it compiles in 1.18?

[brooksprumo]

so the backport pr from this pr into 1.18 would be manually adjusted so it compiles in 1.18?

Yes, that's what I was thinking.

[jeffwashington]

@brooksprumo can you please link the pr that adds purge_all_bank_snapshots that I think also must be backported?

I wasn't originally intending to backport #35291. Instead I was planning on using the previous way to purge all bank snapshots:

snapshot_utils::purge_old_bank_snapshots(&snapshot_config.bank_snapshots_dir, 0, None);

Would backporting #35291 be preferred?
My though process was the backport the least amount of code possible.

so the backport pr from this pr into 1.18 would be manually adjusted so it compiles in 1.18?

ugh. another approach is to submit what will compile in 1.18 in THIS pr and then backport that, then in a follow on master pr, change this code to use purge_old_bank_snapshots That leaves the commit in 1.18 matching a commit in master exactly.

Another alternative is don't backport anything, at least not to 1.17.

any other opinions? Am I being too pedantic?

[brooksprumo]

ugh. another approach is to submit what will compile in 1.18 in THIS pr and then backport that, then in a follow on master pr, change this code to use purge_old_bank_snapshots That leaves the commit in 1.18 matching a commit in master exactly.

Yeah, I considered this too. My thought was that the actual code change is a single line, so it will be very simple to inspect the backports for correctness; that they indeed are purging all bank snapshots.

I can adopt this approach if it's preferred though.

Another alternative is don't backport anything, at least not to 1.17.

Yes, that's true. However it feels dangerous to leave a known issue like this. One that requires manual intervention from a node operator in order for the node to startup.

[brooksprumo]

ugh. another approach is to submit what will compile in 1.18 in THIS pr and then backport that, then in a follow on master pr, change this code to use purge_old_bank_snapshots That leaves the commit in 1.18 matching a commit in master exactly.

Done in 3e42432. I've changed this PR to use purge_old_bank_snapshots() to allow for a clean backport.

Another alternative is don't backport anything, at least not to 1.17.

Per offline discussions, we will not backport to v1.17; only v1.18.

[jeffwashington]

lgtm. ty.

[apfitzge]

lgtm - easy backport.

[steviez]

Oops, thought I gave this one a ship-it; glad you didn't wait but no concerns from my end either + LGTM

[brooksprumo]

Oops, thought I gave this one a ship-it; glad you didn't wait but no concerns from my end either + LGTM

Thanks! For the merge to master I felt comfortable with two approvals. For the backport I'll wait for everyones 😸