Skip to content
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

Revert "SnapshotPackagerService purges old bank snapshots (#31511)" #31524

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented May 6, 2023

Problem

PR #31511 introduced intermittent test failures like this:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread 'test_epoch_accounts_hash_basic::with_snapshots' panicked at 'send epoch accounts hash request: "SendError(..)"', runtime/src/bank_forks.rs:326:18
stack backtrace:
   0: rust_begin_unwind
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:579:5
   1: core::panicking::panic_fmt
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:64:14
   2: core::result::unwrap_failed
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/result.rs:1750:5
   3: core::result::Result<T,E>::expect
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/result.rs:1047:23
   4: solana_runtime::bank_forks::BankForks::do_set_root_return_metrics
             at /solana/runtime/src/bank_forks.rs:319:13
   5: solana_runtime::bank_forks::BankForks::set_root
             at /solana/runtime/src/bank_forks.rs:427:49
   6: epoch_accounts_hash::test_epoch_accounts_hash_basic
             at ./tests/epoch_accounts_hash.rs:299:13
   7: epoch_accounts_hash::test_epoch_accounts_hash_basic::with_snapshots
             at ./tests/epoch_accounts_hash.rs:257:1
   8: epoch_accounts_hash::test_epoch_accounts_hash_basic::with_snapshots::{{closure}}
             at ./tests/epoch_accounts_hash.rs:257:1
   9: core::ops::function::FnOnce::call_once
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/ops/function.rs:250:5
  10: core::ops::function::FnOnce::call_once
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
 
failures:
    test_epoch_accounts_hash_basic::with_snapshots
 
test result: FAILED. 4 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 30.31s
 
error: test failed, to rerun pass `-p solana-core --test epoch_accounts_hash`
🚨 Error: The command exited with status 101
Running local post-command hook

I haven't investigated the tests yet, but I believe they rely on checking the file system for progress. The PR changed how the bank snapshots were cleaned up, so the tests likely need to be updated.

Summary of Changes

Revert PR #31511

@brooksprumo brooksprumo self-assigned this May 6, 2023
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #31524 (e42504b) into master (f42955c) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #31524   +/-   ##
=======================================
  Coverage    81.3%    81.3%           
=======================================
  Files         731      731           
  Lines      205115   205088   -27     
=======================================
- Hits       166944   166929   -15     
+ Misses      38171    38159   -12     

@ryoqun ryoqun self-requested a review May 6, 2023 13:02
@brooksprumo brooksprumo marked this pull request as ready for review May 6, 2023 13:06
Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ for the act of reverting itself.

sanity-checked as follows:

$ git show --oneline --no-patch a6c39ded8ee48bc0fa50de4c85c01925c0afb6d9
a6c39ded8ee SnapshotPackagerService purges old bank snapshots (#31511)
$ git revert a6c39ded8ee48bc0fa50de4c85c01925c0afb6d9
$ git range-diff HEAD...brooksprumo3/snap/revert-sps-purges-bank-snapshots
1:  13e0d56a6e9 ! 1:  e42504b3393 Revert "SnapshotPackagerService purges old bank snapshots (#31511)"
    @@
      ## Metadata ##
    -Author: Ryo Onodera <[email protected]>
    +Author: brooks <[email protected]>
     
      ## Commit message ##
         Revert "SnapshotPackagerService purges old bank snapshots (#31511)"

@brooksprumo brooksprumo merged commit 775639c into solana-labs:master May 6, 2023
@brooksprumo brooksprumo deleted the snap/revert-sps-purges-bank-snapshots branch May 6, 2023 13:18
brooksprumo added a commit to brooksprumo/solana that referenced this pull request May 9, 2023
brooksprumo added a commit to brooksprumo/solana that referenced this pull request May 9, 2023
brooksprumo added a commit to brooksprumo/solana that referenced this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants