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

feat: cosmovisor batch upgrades #21790

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
4 changes: 4 additions & 0 deletions tools/cosmovisor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

### Features

* [#21790](https://github.com/cosmos/cosmos-sdk/pull/21790) Add `add-batch-upgrade` command.

### Improvements

* [#21462](https://github.com/cosmos/cosmos-sdk/pull/21462) Pass `stdin` to binary.
Expand Down
17 changes: 13 additions & 4 deletions tools/cosmovisor/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const (
EnvTimeFormatLogs = "COSMOVISOR_TIMEFORMAT_LOGS"
EnvCustomPreupgrade = "COSMOVISOR_CUSTOM_PREUPGRADE"
EnvDisableRecase = "COSMOVISOR_DISABLE_RECASE"
EnvCometBftRpcEndpoint = "COMETBFT_RPC_ENDPOINT"
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't make cosmovisor cometbft specific

)

const (
Expand Down Expand Up @@ -68,6 +69,7 @@ type Config struct {
TimeFormatLogs string `toml:"cosmovisor_timeformat_logs" mapstructure:"cosmovisor_timeformat_logs" default:"kitchen"`
CustomPreUpgrade string `toml:"cosmovisor_custom_preupgrade" mapstructure:"cosmovisor_custom_preupgrade" default:""`
DisableRecase bool `toml:"cosmovisor_disable_recase" mapstructure:"cosmovisor_disable_recase" default:"false"`
CometBftRpcEndpoint string `toml:"combetbft_rpc_endpoint" mapstructure:"combetbft_rpc_endpoint" default:"http://localhost:26657"`

// currently running upgrade
currentUpgrade upgradetypes.Plan
Expand Down Expand Up @@ -109,6 +111,11 @@ func (cfg *Config) UpgradeInfoFilePath() string {
return filepath.Join(cfg.Home, "data", upgradetypes.UpgradeInfoFilename)
}

// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix.
func (cfg *Config) UpgradeInfoBatchFilePath() string {
return cfg.UpgradeInfoFilePath() + ".batch"
}

// SymLinkToGenesis creates a symbolic link from "./current" to the genesis directory.
func (cfg *Config) SymLinkToGenesis() (string, error) {
genesis := filepath.Join(cfg.Root(), genesisDir)
Expand Down Expand Up @@ -207,10 +214,11 @@ func GetConfigFromFile(filePath string) (*Config, error) {
func GetConfigFromEnv(skipValidate bool) (*Config, error) {
var errs []error
cfg := &Config{
Home: os.Getenv(EnvHome),
Name: os.Getenv(EnvName),
DataBackupPath: os.Getenv(EnvDataBackupPath),
CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade),
Home: os.Getenv(EnvHome),
Name: os.Getenv(EnvName),
DataBackupPath: os.Getenv(EnvDataBackupPath),
CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade),
CometBftRpcEndpoint: os.Getenv(EnvCometBftRpcEndpoint),
}

if cfg.DataBackupPath == "" {
Expand Down Expand Up @@ -548,6 +556,7 @@ func (cfg Config) DetailString() string {
{EnvTimeFormatLogs, cfg.TimeFormatLogs},
{EnvCustomPreupgrade, cfg.CustomPreUpgrade},
{EnvDisableRecase, fmt.Sprintf("%t", cfg.DisableRecase)},
{EnvCometBftRpcEndpoint, cfg.CometBftRpcEndpoint},
}

derivedEntries := []struct{ name, value string }{
Expand Down
67 changes: 42 additions & 25 deletions tools/cosmovisor/cmd/cosmovisor/add_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func NewAddUpgradeCmd() *cobra.Command {
Short: "Add APP upgrade binary to cosmovisor",
SilenceUsage: true,
Args: cobra.ExactArgs(2),
RunE: AddUpgrade,
RunE: AddUpgradeCmd,
}

addUpgrade.Flags().Bool(cosmovisor.FlagForce, false, "overwrite existing upgrade binary / upgrade-info.json file")
Expand All @@ -29,25 +29,13 @@ func NewAddUpgradeCmd() *cobra.Command {
}

// AddUpgrade adds upgrade info to manifest
func AddUpgrade(cmd *cobra.Command, args []string) error {
configPath, err := cmd.Flags().GetString(cosmovisor.FlagCosmovisorConfig)
if err != nil {
return fmt.Errorf("failed to get config flag: %w", err)
}

cfg, err := cosmovisor.GetConfigFromFile(configPath)
if err != nil {
return err
}

func AddUpgrade(cfg *cosmovisor.Config, force bool, upgradeHeight int64, upgradeName, executablePath, upgradeInfoPath string) error {
logger := cfg.Logger(os.Stdout)

upgradeName := args[0]
if !cfg.DisableRecase {
upgradeName = strings.ToLower(args[0])
upgradeName = strings.ToLower(upgradeName)
}

executablePath := args[1]
if _, err := os.Stat(executablePath); err != nil {
if os.IsNotExist(err) {
return fmt.Errorf("invalid executable path: %w", err)
Expand All @@ -68,21 +56,14 @@ func AddUpgrade(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to read binary: %w", err)
}

force, err := cmd.Flags().GetBool(cosmovisor.FlagForce)
if err != nil {
return fmt.Errorf("failed to get force flag: %w", err)
}

if err := saveOrAbort(cfg.UpgradeBin(upgradeName), executableData, force); err != nil {
return err
}

logger.Info(fmt.Sprintf("Using %s for %s upgrade", executablePath, upgradeName))
logger.Info(fmt.Sprintf("Upgrade binary located at %s", cfg.UpgradeBin(upgradeName)))

if upgradeHeight, err := cmd.Flags().GetInt64(cosmovisor.FlagUpgradeHeight); err != nil {
return fmt.Errorf("failed to get upgrade-height flag: %w", err)
} else if upgradeHeight > 0 {
if upgradeHeight > 0 {
plan := upgradetypes.Plan{Name: upgradeName, Height: upgradeHeight}
if err := plan.ValidateBasic(); err != nil {
panic(fmt.Errorf("something is wrong with cosmovisor: %w", err))
Expand All @@ -94,16 +75,52 @@ func AddUpgrade(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to marshal upgrade plan: %w", err)
}

if err := saveOrAbort(cfg.UpgradeInfoFilePath(), planData, force); err != nil {
if err := saveOrAbort(upgradeInfoPath, planData, force); err != nil {
psiphi5 marked this conversation as resolved.
Show resolved Hide resolved
return err
}

logger.Info(fmt.Sprintf("%s created, %s upgrade binary will switch at height %d", cfg.UpgradeInfoFilePath(), upgradeName, upgradeHeight))
logger.Info(fmt.Sprintf("%s created, %s upgrade binary will switch at height %d", upgradeInfoPath, upgradeName, upgradeHeight))
}

return nil
}

// GetConfig returns a Config using passed-in flag
func GetConfig(cmd *cobra.Command) (*cosmovisor.Config, error) {
configPath, err := cmd.Flags().GetString(cosmovisor.FlagCosmovisorConfig)
if err != nil {
return nil, fmt.Errorf("failed to get config flag: %w", err)
}

cfg, err := cosmovisor.GetConfigFromFile(configPath)
if err != nil {
return nil, err
}
return cfg, nil
}

// AddUpgradeCmd parses input flags and adds upgrade info to manifest
func AddUpgradeCmd(cmd *cobra.Command, args []string) error {
cfg, err := GetConfig(cmd)
if err != nil {
return err
}

upgradeName, executablePath := args[0], args[1]

force, err := cmd.Flags().GetBool(cosmovisor.FlagForce)
if err != nil {
return fmt.Errorf("failed to get force flag: %w", err)
}

upgradeHeight, err := cmd.Flags().GetInt64(cosmovisor.FlagUpgradeHeight)
if err != nil {
return fmt.Errorf("failed to get upgrade-height flag: %w", err)
}

return AddUpgrade(cfg, force, upgradeHeight, upgradeName, executablePath, cfg.UpgradeInfoFilePath())
}

// saveOrAbort saves data to path or aborts if file exists and force is false
func saveOrAbort(path string, data []byte, force bool) error {
if _, err := os.Stat(path); err == nil {
Expand Down
80 changes: 80 additions & 0 deletions tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package main

import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"strconv"
"strings"

"github.com/spf13/cobra"
)

func NewBatchAddUpgradeCmd() *cobra.Command {
return &cobra.Command{
Use: "add-batch-upgrade <upgrade1-name>:<path-to-exec1>:<upgrade1-height> .. <upgradeN-name>:<path-to-execN>:<upgradeN-height>",
Short: "Add APP upgrades binary to cosmovisor",
SilenceUsage: true,
Args: cobra.MinimumNArgs(1),
RunE: AddBatchUpgrade,
}
}

// AddBatchUpgrade takes in multiple specified upgrades and creates a single
// batch upgrade file out of them
func AddBatchUpgrade(cmd *cobra.Command, args []string) error {
cfg, err := GetConfig(cmd)
if err != nil {
return err
}
upgradeInfoPaths := []string{}
for i, as := range args {
a := strings.Split(as, ":")
if len(a) != 3 {
return fmt.Errorf("argument at position %d (%s) is invalid", i, as)
}
upgradeName := filepath.Base(a[0])
upgradePath := a[1]
upgradeHeight, err := strconv.ParseInt(a[2], 10, 64)
if err != nil {
return fmt.Errorf("upgrade height at position %d (%s) is invalid", i, a[2])
}
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
}
Comment on lines +50 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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.go in the tools/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

1 change: 1 addition & 0 deletions tools/cosmovisor/cmd/cosmovisor/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func NewRootCmd() *cobra.Command {
configCmd,
NewVersionCmd(),
NewAddUpgradeCmd(),
NewBatchAddUpgradeCmd(),
)

rootCmd.PersistentFlags().StringP(cosmovisor.FlagCosmovisorConfig, "c", "", "path to cosmovisor config file")
Expand Down
4 changes: 2 additions & 2 deletions tools/cosmovisor/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ go 1.23
require (
cosmossdk.io/log v1.4.1
cosmossdk.io/x/upgrade v0.1.4
github.com/cometbft/cometbft v0.38.9
github.com/fsnotify/fsnotify v1.7.0
github.com/otiai10/copy v1.14.0
github.com/pelletier/go-toml/v2 v2.2.3
github.com/spf13/cobra v1.8.1
Expand Down Expand Up @@ -45,7 +47,6 @@ require (
github.com/cockroachdb/pebble v1.1.0 // indirect
github.com/cockroachdb/redact v1.1.5 // indirect
github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06 // indirect
github.com/cometbft/cometbft v0.38.9 // indirect
github.com/cometbft/cometbft-db v0.12.0 // indirect
github.com/cosmos/btcutil v1.0.5 // indirect
github.com/cosmos/cosmos-db v1.0.2 // indirect
Expand All @@ -68,7 +69,6 @@ require (
github.com/emicklei/dot v1.6.2 // indirect
github.com/fatih/color v1.17.0 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/getsentry/sentry-go v0.28.0 // indirect
github.com/go-kit/kit v0.13.0 // indirect
github.com/go-kit/log v0.2.1 // indirect
Expand Down
Loading
Loading