Improve usability of snapshots saved by snapshotting node #749
Conversation
WalkthroughThe recent changes in the snapshot.sh script involve modifying the pruning configuration in the app.toml file and updating the SNAP_NAME variable. The pruning configuration is now set to "nothing", and the SNAP_NAME variable has been modified to include the date and time in the filename. Changes
TipsChat with CodeRabbit Bot (
|
roy-dydx
force-pushed
the
roy/snap
branch
2 times, most recently
from
c697ec1
to
7257a8e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/snapshotting/snapshot.sh (2 hunks)
Additional comments: 2
protocol/testing/snapshotting/snapshot.sh (2)
87-91: The change to the pruning configuration is a significant one. It's important to ensure that this change doesn't negatively impact the performance of the system or lead to unexpected behavior. Please verify that the system can handle retaining only the last two states and that this doesn't cause any issues with data integrity or system performance.
105-108: The change to include the date and time in the snapshot filename is a good one for usability. However, it's important to ensure that this change doesn't cause any issues with existing systems that might be expecting the old filename format. Please verify that all systems that interact with these snapshots can handle the new filename format.
|
Also update the script here? Probably should just pull the snapshot script from this repo in the setup script in the other repos that push up docker images. |
| @@ -99,7 +102,7 @@ while true; do | |||
| kill -TERM $(pidof cosmovisor) | |||
|
|
|||
| log_this "Creating new snapshot" | |||
| SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%M-%H').tar.gz") | |||
| dydxprotocold init --chain-id=${CHAIN_ID} --home /dydxprotocol/chain/local_node local_node | ||
| curl -X GET ${genesis_file_rpc_address}/genesis | jq '.result.genesis' > /dydxprotocol/chain/local_node/config/genesis.json | ||
|
|
||
| # Set pruning to prune all but the last two states. Prevents snapshots from getting too big. | ||
| sed -i 's/pruning = "default"/pruning = "everything"/' /dydxprotocol/chain/local_node/config/app.toml |
There was a problem hiding this comment.
The comment on line 90 and the code on line 91 seem to contradict each other. The comment says that all but the last two states are pruned, but the code sets pruning to "everything". Please verify this.
- # Set pruning to prune all but the last two states. Prevents snapshots from getting too big.
- sed -i 's/pruning = "default"/pruning = "everything"/' /dydxprotocol/chain/local_node/config/app.toml
+ # Set pruning to prune everything. Prevents snapshots from getting too big.
+ sed -i 's/pruning = "default"/pruning = "everything"/' /dydxprotocol/chain/local_node/config/app.tomlCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| dydxprotocold init --chain-id=${CHAIN_ID} --home /dydxprotocol/chain/local_node local_node | |
| curl -X GET ${genesis_file_rpc_address}/genesis | jq '.result.genesis' > /dydxprotocol/chain/local_node/config/genesis.json | |
| # Set pruning to prune all but the last two states. Prevents snapshots from getting too big. | |
| sed -i 's/pruning = "default"/pruning = "everything"/' /dydxprotocol/chain/local_node/config/app.toml | |
| # Set pruning to prune everything. Prevents snapshots from getting too big. | |
| sed -i 's/pruning = "default"/pruning = "everything"/' /dydxprotocol/chain/local_node/config/app.toml |
| kill -TERM $(pidof cosmovisor) | ||
|
|
||
| log_this "Creating new snapshot" | ||
| SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%M-%H').tar.gz") | ||
| SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M').tar.gz") | ||
| tar cvzf ${SNAP_PATH}/${SNAP_NAME} ${DATA_PATH} | ||
| aws s3 cp ${SNAP_PATH}/${SNAP_NAME} s3://${s3_snapshot_bucket}/ --region ap-northeast-1 --debug || true | ||
| rm -rf ${SNAP_PATH}/${SNAP_NAME} |
There was a problem hiding this comment.
The new format for SNAP_NAME is a good improvement for better sorting and access of snapshots. However, consider using a more precise timestamp format to avoid potential conflicts if two snapshots are created within the same minute.
- SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M').tar.gz")
+ SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M-%S').tar.gz")Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| kill -TERM $(pidof cosmovisor) | |
| log_this "Creating new snapshot" | |
| SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%M-%H').tar.gz") | |
| SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M').tar.gz") | |
| tar cvzf ${SNAP_PATH}/${SNAP_NAME} ${DATA_PATH} | |
| aws s3 cp ${SNAP_PATH}/${SNAP_NAME} s3://${s3_snapshot_bucket}/ --region ap-northeast-1 --debug || true | |
| rm -rf ${SNAP_PATH}/${SNAP_NAME} | |
| kill -TERM $(pidof cosmovisor) | |
| log_this "Creating new snapshot" | |
| - SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M').tar.gz") | |
| + SNAP_NAME=$(echo "${CHAIN_ID}_$(date '+%Y-%m-%d-%H-%M-%S').tar.gz") | |
| tar cvzf ${SNAP_PATH}/${SNAP_NAME} ${DATA_PATH} | |
| aws s3 cp ${SNAP_PATH}/${SNAP_NAME} s3://${s3_snapshot_bucket}/ --region ap-northeast-1 --debug || true | |
| rm -rf ${SNAP_PATH}/${SNAP_NAME} |
Changelist
Test Plan
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.