Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fix race in ci/run-sanity.sh#10796

Merged
mergify[bot] merged 1 commit intosolana-labs:masterfrom
ryoqun:run-sh-race
Jun 25, 2020
Merged

Fix race in ci/run-sanity.sh#10796
mergify[bot] merged 1 commit intosolana-labs:masterfrom
ryoqun:run-sh-race

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Jun 25, 2020

Problem

CI is a bit unstable on master:

https://buildkite.com/solana-labs/solana/builds/26540#3a9158a5-2de3-4b61-b25a-25e63f4400ac/2724-2898
Even if this

 while [[ $($solana_cli --url http://localhost:8899 slot --commitment recent) -eq 0 ]]; do

evaluates to:

+ [[ 1 -eq 0 ]]

, it seems that we can't create snapshot for the slot 1:

     Running `target/debug/solana-ledger-tool create-snapshot --ledger config/ledger 1 config/snapshot-ledger`
...
Error: Slot 1 is not available

Summary of Changes

Wait more.

context

frustrated while developing at #10718

@ryoqun ryoqun added automerge Merge this Pull Request automatically once CI passes v1.1 labels Jun 25, 2020
@t-nelson
Copy link
Copy Markdown
Contributor

Is the problem not that we're querying the slot with recent commitment and ledger-tool wants a root?

I ran into similar when plumbing --warp-slot through for rolling upgrade tests and we run wallet sanity before the deposit transaction's slot gets rooted. Can-o-worms these scripts 🙂

@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Jun 25, 2020

@t-nelson Well, ledger-tool create-snapshot should work even with non-root slots... run-sanity.sh doesn't definitely wait for the first root (slot = 32) here. My guess is that it just don't fsync for every slot. We rather forcibly shutdown a validator with the validatorexit rpc endpoint.

Let's see how this pr stabilizes. :)

@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Jun 25, 2020

@t-nelson I was lying...:

**slot <= snapshot_slot && accounts_index.is_root(**slot)

non root doesn't work. And it's quite dangerous..... I'll add rooted check in ledger-tool create-snapshot...

@mergify mergify Bot merged commit 4dc9f37 into solana-labs:master Jun 25, 2020
mergify Bot pushed a commit that referenced this pull request Jun 25, 2020
(cherry picked from commit 4dc9f37)
mergify Bot pushed a commit that referenced this pull request Jun 25, 2020
(cherry picked from commit 4dc9f37)
mergify Bot added a commit that referenced this pull request Jun 25, 2020
(cherry picked from commit 4dc9f37)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
mergify Bot added a commit that referenced this pull request Jun 25, 2020
(cherry picked from commit 4dc9f37)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Jun 30, 2020

Is the problem not that we're querying the slot with recent commitment and ledger-tool wants a root?

I ran into similar when plumbing --warp-slot through for rolling upgrade tests and we run wallet sanity before the deposit transaction's slot gets rooted. Can-o-worms these scripts slightly_smiling_face

I'll add rooted check in ledger-tool create-snapshot...

@t-nelson I was about to create a new PR for this, but it turns out ledger-tool create-snapshot does it correct:

bank.squash();

So, my quoted code does work correctly:

**slot <= snapshot_slot && accounts_index.is_root(**slot)

So, I think there is no need to improve this further.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants