feat(server/v2): Add snapshots commands #21065
Conversation
WalkthroughWalkthroughThe recent changes enhance the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
C:Store
Type: Build
C:server/v2
simapp/v2/simdv2/cmd/commands.go
Outdated
| ); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Add RestoreSnapshotCmd separately cause need appCreator |
There was a problem hiding this comment.
imho we still need appCreator here for storeKeys since removing ss & sc database for restore cmd
There was a problem hiding this comment.
hum, cannot store.New take the appCreator?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| func processChunk(tarWriter *tar.Writer, path, tarName string) error { | ||
| file, err := os.Open(path) |
Check failure
Code scanning / gosec
Potential file inclusion via variable Error
| } | ||
|
|
||
| path := args[0] | ||
| fp, err := os.Open(path) |
Check failure
Code scanning / gosec
Potential file inclusion via variable Error
| return err | ||
| } | ||
|
|
||
| fp, err := os.Create(output) |
Check failure
Code scanning / gosec
Potential file inclusion via variable Error
There was a problem hiding this comment.
Can this be a system test instead? cc @alpe
simapp/v2/simdv2/cmd/commands.go
Outdated
| ); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Add RestoreSnapshotCmd separately cause need appCreator |
There was a problem hiding this comment.
hum, cannot store.New take the appCreator?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (10)
server/v2/store/flags.go (1)
5-5: Incomplete renaming of constant.The constant
FlagPruningKeepRecenthas not been fully renamed toFlagKeepRecentacross the codebase. Please ensure that all occurrences ofFlagPruningKeepRecentare updated toFlagKeepRecent.
server/pruning.goserver/pruning_test.goserver/start.goclient/pruning/main.goAnalysis chain
Verify the usage of the renamed constant.
Ensure that all occurrences of
FlagPruningKeepRecenthave been updated toFlagKeepRecentacross the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of the renamed constant `FlagKeepRecent`. # Test: Search for the old constant name. Expect: No occurrences of the old name. rg --type go 'FlagPruningKeepRecent' # Test: Search for the new constant name. Expect: Occurrences of the new name. rg --type go 'FlagKeepRecent'Length of output: 1212
server/v2/store/snapshot.go (8)
28-75: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Starting snapshot export") ... + cmd.Println("Snapshot export completed")
79-113: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Starting snapshot restoration") ... + cmd.Println("Snapshot restoration completed")
116-139: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Listing snapshots") ... + cmd.Println("Snapshots listing completed")
142-167: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Starting snapshot deletion") ... + cmd.Println("Snapshot deletion completed")
170-259: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Starting snapshot dump") ... + cmd.Println("Snapshot dump completed")
263-349: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ cmd.Println("Starting snapshot load") ... + cmd.Println("Snapshot load completed")
353-376: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ logger.Info("Creating snapshot manager") ... + logger.Info("Snapshot manager created")
383-407: Ensure proper error handling and logging.The function handles errors correctly and logs important steps. However, consider logging the start and end of the process for better traceability.
+ logger.Info("Processing chunk", "path", path) ... + logger.Info("Chunk processed", "path", path)simapp/v2/simdv2/cmd/testnet.go (1)
347-347: Ensure proper error handling and logging.The grpcServer initialization is reintroduced. Ensure proper error handling and logging for this initialization.
+ logger.Info("Initializing grpcServer") ... + logger.Info("grpcServer initialized")
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- scripts/test_snapshots_export_restore.sh (1 hunks)
- server/v2/store/commands.go (3 hunks)
- server/v2/store/flags.go (1 hunks)
- server/v2/store/server.go (1 hunks)
- server/v2/store/snapshot.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/testnet.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
Additional context used
Path-based instructions (7)
server/v2/store/flags.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/snapshot.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Shellcheck
scripts/test_snapshots_export_restore.sh
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
Additional comments not posted (11)
server/v2/store/flags.go (1)
6-6: Verify the usage of the new constant.Ensure that the new constant
FlagIntervalis used correctly in the codebase.Verification successful
Verify the usage of the new constant
FlagInterval.The constant
FlagIntervalis used correctly in the following locations:
server/v2/store/snapshot.go: if cmd.Flags().Changed(FlagInterval) {server/v2/store/snapshot.go: interval, err = cmd.Flags().GetUint64(FlagInterval)server/v2/store/snapshot.go: cmd.Flags().Uint64(FlagInterval, 0, "Interval defines at which heights the snapshot is taken")These usages are consistent with handling command-line flags in Go.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new constant `FlagInterval`. # Test: Search for the new constant name. Expect: Occurrences of the new name. rg --type go 'FlagInterval'Length of output: 357
server/v2/store/server.go (2)
52-56: Verify the removal of methods.Ensure that all references to the removed methods
GetCommands,GetTxs, andGetQuerieshave been updated or removed across the codebase.
52-56: LGTM! But verify the new command methods.The code changes are approved.
However, ensure that the new command methods
ExportSnapshotCmd,DeleteSnapshotCmd,ListSnapshotsCmd,DumpArchiveCmd, andLoadArchiveCmdare implemented and used correctly.scripts/test_snapshots_export_restore.sh (1)
1-50: LGTM! But verify the script functionality.The code changes are approved.
However, ensure that the script functions correctly and achieves the intended purpose.
Tools
Shellcheck
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
store/v2/root/factory.go (1)
58-62: Verify the impact of increased pruning intervals.The
Intervalvalue forSCPruningOptionandSSPruningOptionhas been increased from 1 to 100. Ensure that this change does not negatively impact performance and memory management.server/v2/store/commands.go (3)
48-48: LGTM! Simplified function signature.The
createRootStorefunction now directly accesses thehomedirectory using theviperconfiguration, simplifying the function signature and streamlining the command's execution logic.
77-77: LGTM! Renamed flag for clarity.The flag for keeping recent heights has been renamed from
FlagPruningKeepRecenttoFlagKeepRecent, enhancing clarity and maintainability.
Line range hint
82-102:
LGTM! Improved configuration handling.The
homedirectory is now accessed using theviperconfiguration, and the flag handling logic has been updated, simplifying the function signature and improving the handling of configuration parameters.simapp/v2/simdv2/cmd/commands.go (3)
71-71: LGTM! Enhanced modularity.The direct instantiation of the
storecomponent has been replaced with a new variablestoreComponent, enhancing clarity and modularity.
79-79: LGTM! Proper initialization of storeComponent.The
storeComponentis now passed as an argument to theAddCommandsfunction, ensuring proper initialization.
84-90: LGTM! Explicit association of snapshot restoration.The loop adds a
RestoreSnapshotCmdspecifically tied to thestoreComponent, ensuring that the snapshot restoration functionality is explicitly associated with the newly created store component.
|
|
||
| COSMOS_BUILD_OPTIONS=v2 make build | ||
|
|
||
| if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi |
There was a problem hiding this comment.
Quote the command substitution to prevent word splitting.
The command substitution should be quoted to prevent word splitting and potential issues.
- if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi
+ if [ -d "$("$SIMD" config home)" ]; then rm -rv "$("$SIMD" config home)"; fiCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi | |
| if [ -d "$("$SIMD" config home)" ]; then rm -rv "$("$SIMD" config home)"; fi |
Tools
Shellcheck
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/store/server.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/v2/store/server.go
- store/v2/root/factory.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- server/v2/store/server.go (2 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/testnet.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- server/v2/store/server.go
- simapp/v2/simdv2/cmd/commands.go
- simapp/v2/simdv2/cmd/testnet.go
scripts/test_snapshots_v2.sh
Outdated
|
|
||
| kill -9 "$SIMD_PID" | ||
|
|
||
| $SIMD store export --height 5 |
There was a problem hiding this comment.
Update full flow test.
cc @julienrbrt @alpe
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- scripts/test_snapshots_v2.sh (1 hunks)
Additional context used
Shellcheck
scripts/test_snapshots_v2.sh
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
Additional comments not posted (11)
scripts/test_snapshots_v2.sh (11)
1-6: Good practice: Error handling and debugging options.The use of
set -o errexit,set -o nounset, andset -xis a good practice for error handling and debugging.
7-11: Define variables with clear context.The variables
ROOT,SIMD, andCONFIGare defined with clear context. Ensure that the$ROOT/build/simdv2path is correct and theCONFIGvariable is set appropriately.
12-12: Ensure build options are correct.The build command
COSMOS_BUILD_OPTIONS=v2 make buildshould be verified to ensure it includes all necessary build options forsimapp/v2.
16-16: Initialization command is appropriate.The command
$SIMD init simapp-v2-node --chain-id simapp-v2-chaininitializes the blockchain node with the specified chain ID.
20-24: Genesis file modifications are clear.The genesis file modifications using
jqare clear and correctly update the voting period and inflation parameters.
26-27: Client configuration and key addition are correct.The client configuration and key addition commands are correct. Ensure that the
--indiscreetflag forkeys addis intentional and secure.
28-33: Genesis account and transaction setup are correct.The commands for adding a genesis account, generating a transaction, and collecting genesis transactions are correct. Ensure that the keyring backend is set appropriately.
34-41: Review sleep duration and process termination.The sleep duration of 10 seconds and the use of
kill -9should be reviewed to ensure they are appropriate for the test scenario. Consider using a more graceful shutdown method if possible.
42-51: Snapshot operations are clear and specific.The snapshot operations (export, dump, delete, load, list) are clear and specific to the functionality being tested. Ensure that the paths and parameters are correct and valid.
52-55: Data clearing commands are appropriate.The commands for clearing the data directories are appropriate for ensuring a clean state before restoration.
58-63: Review restoration and node restart process.The restoration command and node restart process are appropriate. Review the sleep duration and consider a more graceful shutdown method if possible.
scripts/test_snapshots_v2.sh
Outdated
|
|
||
| COSMOS_BUILD_OPTIONS=v2 make build | ||
|
|
||
| if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi |
There was a problem hiding this comment.
Quote the command substitution to prevent word splitting.
To prevent potential word splitting issues, quote the command substitution.
- if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi
+ if [ -d "$($SIMD config home)" ]; then rm -rv "$($SIMD config home)"; fiCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -d "$($SIMD config home)" ]; then rm -rv $($SIMD config home); fi | |
| if [ -d "$($SIMD config home)" ]; then rm -rv "$($SIMD config home)"; fi |
Tools
Shellcheck
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/store/server.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/store/server.go
simapp/v2/simdv2/cmd/commands.go
Outdated
| // wire server commands | ||
| if err = serverv2.AddCommands( | ||
| rootCmd, | ||
| newApp, | ||
| logger, | ||
| cometbft.New(&genericTxDecoder[T]{txConfig}, cometbft.DefaultServerOptions[T]()), | ||
| grpc.New[T](), | ||
| store.New[T](), | ||
| storeComponent, |
scripts/test_snapshots_v2.sh
Outdated
| @@ -0,0 +1,63 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I would still prefer this as a system test, as this never run anywhere. Can we maybe remove this and create an issue for it?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/commands.go
(cherry picked from commit 8fb47b3) # Conflicts: # server/v2/store/commands.go # server/v2/store/flags.go # server/v2/store/server.go # store/v2/root/factory.go
Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #20512
Add snapshots commands for server/v2:
Tests:
Blocked on:
chain-idfrom viper #21116StateChangeschannel indoRestoreSnapshot#21106Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Bug Fixes