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): load cosmovisor configuration from toml file #19995

Merged
merged 22 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
71b692a
feat: load cosmovisor configuration from toml file
akaladarshi Apr 10, 2024
af65025
fix: linter issues
akaladarshi Apr 10, 2024
2cfc0c3
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 11, 2024
7335e53
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 11, 2024
3351c7b
test: add test cases
akaladarshi Apr 11, 2024
2d2da1b
fix: linter errors
akaladarshi Apr 11, 2024
240402e
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 11, 2024
7e84917
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 12, 2024
44a724e
docs: update changelog
akaladarshi Apr 12, 2024
74c8b24
fix: changelog indentation
akaladarshi Apr 12, 2024
13faea8
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 13, 2024
fa973a5
address comments
akaladarshi Apr 14, 2024
03aa6d4
nit: fix typo
akaladarshi Apr 14, 2024
b2f5594
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 19, 2024
75f7c2e
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 20, 2024
7307c76
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 21, 2024
bc91f5a
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 23, 2024
9a9a729
address comments
akaladarshi Apr 23, 2024
5356e55
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 23, 2024
5a7eecc
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 23, 2024
3ea4396
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi Apr 29, 2024
6724f0c
Merge branch 'main' into akaladarshi/load-config-cosmovisor
akaladarshi May 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
119 changes: 97 additions & 22 deletions tools/cosmovisor/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
"cosmossdk.io/log"
"cosmossdk.io/x/upgrade/plan"
upgradetypes "cosmossdk.io/x/upgrade/types"
"github.com/pelletier/go-toml/v2"
"github.com/spf13/viper"
)

// environment variable names
Expand Down Expand Up @@ -42,26 +44,33 @@
genesisDir = "genesis"
upgradesDir = "upgrades"
currentLink = "current"

cfgFileName = "config"
cfgExtension = "toml"
)

var (
ErrEmptyConfigENV = errors.New("config env variable not set or empty")
)

// Config is the information passed in to control the daemon
type Config struct {
Home string
Name string
AllowDownloadBinaries bool
DownloadMustHaveChecksum bool
RestartAfterUpgrade bool
RestartDelay time.Duration
ShutdownGrace time.Duration
PollInterval time.Duration
UnsafeSkipBackup bool
DataBackupPath string
PreupgradeMaxRetries int
DisableLogs bool
ColorLogs bool
TimeFormatLogs string
CustomPreupgrade string
DisableRecase bool
Home string `toml:"DAEMON_HOME" mapstructure:"DAEMON_HOME"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we have the toml keys lower-case and kebab-case? This looks odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Name string `toml:"DAEMON_NAME" mapstructure:"DAEMON_NAME"`
AllowDownloadBinaries bool `toml:"DAEMON_ALLOW_DOWNLOAD_BINARIES" mapstructure:"DAEMON_ALLOW_DOWNLOAD_BINARIES" default:"false"`
DownloadMustHaveChecksum bool `toml:"DAEMON_DOWNLOAD_MUST_HAVE_CHECKSUM" mapstructure:"DAEMON_DOWNLOAD_MUST_HAVE_CHECKSUM" default:"false"`
RestartAfterUpgrade bool `toml:"DAEMON_RESTART_AFTER_UPGRADE" mapstructure:"DAEMON_RESTART_AFTER_UPGRADE" default:"true"`
RestartDelay time.Duration `toml:"DAEMON_RESTART_DELAY" mapstructure:"DAEMON_RESTART_DELAY"`
ShutdownGrace time.Duration `toml:"DAEMON_SHUTDOWN_GRACE" mapstructure:"DAEMON_SHUTDOWN_GRACE"`
PollInterval time.Duration `toml:"DAEMON_POLL_INTERVAL" mapstructure:"DAEMON_POLL_INTERVAL" default:"300ms"`
UnsafeSkipBackup bool `toml:"UNSAFE_SKIP_BACKUP" mapstructure:"UNSAFE_SKIP_BACKUP" default:"false"`
DataBackupPath string `toml:"DAEMON_DATA_BACKUP_DIR" mapstructure:"DAEMON_DATA_BACKUP_DIR"`
PreUpgradeMaxRetries int `toml:"DAEMON_PREUPGRADE_MAX_RETRIES" mapstructure:"DAEMON_PREUPGRADE_MAX_RETRIES" default:"0"`
DisableLogs bool `toml:"COSMOVISOR_DISABLE_LOGS" mapstructure:"COSMOVISOR_DISABLE_LOGS" default:"false"`
ColorLogs bool `toml:"COSMOVISOR_COLOR_LOGS" mapstructure:"COSMOVISOR_COLOR_LOGS" default:"true"`
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"`

// currently running upgrade
currentUpgrade upgradetypes.Plan
Expand Down Expand Up @@ -145,6 +154,42 @@
return binpath, nil
}

// GetConfigFromFile will read the configuration from the file at the given path.
// It will return an error if the file does not exist or if the configuration is invalid.
// If ENV variables are set, they will override the values in the file.
func GetConfigFromFile(filePath string) (*Config, error) {
if filePath == "" {
return nil, ErrEmptyConfigENV
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should return an error, that will be a behavior change for existing users. Maybe we should use a temporary empty file? Quite hacky, but at least it will preserve the current behavior for users that won't use the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

Updated. Now if the filePath is not provided instead of returning an error it will try to load config from env variables

}

// ensure the file exist
if _, err := os.Stat(filePath); err != nil {
return nil, fmt.Errorf("config file does not exist: at %s : %w", filePath, err)
}

// read the configuration from the file
viper.SetConfigFile(filePath)
// load the env variables
// if the env variable is set, it will override the value provided by the config
viper.AutomaticEnv()

if err := viper.ReadInConfig(); err != nil {
return nil, fmt.Errorf("failed to read config file: %w", err)
}

cfg := &Config{}
if err := viper.Unmarshal(cfg); err != nil {
return nil, fmt.Errorf("failed to unmarshal configuration: %w", err)
}

errs := cfg.validate()
if len(errs) > 0 {
return nil, errors.Join(errs...)
}

return cfg, nil
}

// GetConfigFromEnv will read the environmental variables into a config
// and then validate it is reasonable
func GetConfigFromEnv() (*Config, error) {
Expand All @@ -153,7 +198,7 @@
Home: os.Getenv(EnvHome),
Name: os.Getenv(EnvName),
DataBackupPath: os.Getenv(EnvDataBackupPath),
CustomPreupgrade: os.Getenv(EnvCustomPreupgrade),
CustomPreUpgrade: os.Getenv(EnvCustomPreupgrade),
}

if cfg.DataBackupPath == "" {
Expand Down Expand Up @@ -220,8 +265,8 @@
}
}

envPreupgradeMaxRetriesVal := os.Getenv(EnvPreupgradeMaxRetries)
if cfg.PreupgradeMaxRetries, err = strconv.Atoi(envPreupgradeMaxRetriesVal); err != nil && envPreupgradeMaxRetriesVal != "" {
envPreUpgradeMaxRetriesVal := os.Getenv(EnvPreupgradeMaxRetries)
if cfg.PreUpgradeMaxRetries, err = strconv.Atoi(envPreUpgradeMaxRetriesVal); err != nil && envPreUpgradeMaxRetriesVal != "" {
errs = append(errs, fmt.Errorf("%s could not be parsed to int: %w", EnvPreupgradeMaxRetries, err))
}

Expand Down Expand Up @@ -395,12 +440,13 @@
return false, fmt.Errorf("env variable %q must have a boolean value (\"true\" or \"false\"), got %q", name, p)
}

// checks and validates env option
// TimeFormatOptionFromEnv checks and validates the time format option
func TimeFormatOptionFromEnv(env, defaultVal string) (string, error) {
val, set := os.LookupEnv(env)
if !set {
return defaultVal, nil
}

switch val {
case "layout":
return time.Layout, nil
Expand Down Expand Up @@ -445,11 +491,11 @@
{EnvInterval, cfg.PollInterval.String()},
{EnvSkipBackup, fmt.Sprintf("%t", cfg.UnsafeSkipBackup)},
{EnvDataBackupPath, cfg.DataBackupPath},
{EnvPreupgradeMaxRetries, fmt.Sprintf("%d", cfg.PreupgradeMaxRetries)},
{EnvPreupgradeMaxRetries, fmt.Sprintf("%d", cfg.PreUpgradeMaxRetries)},
{EnvDisableLogs, fmt.Sprintf("%t", cfg.DisableLogs)},
{EnvColorLogs, fmt.Sprintf("%t", cfg.ColorLogs)},
{EnvTimeFormatLogs, cfg.TimeFormatLogs},
{EnvCustomPreupgrade, cfg.CustomPreupgrade},
{EnvCustomPreupgrade, cfg.CustomPreUpgrade},
{EnvDisableRecase, fmt.Sprintf("%t", cfg.DisableRecase)},
}

Expand Down Expand Up @@ -479,3 +525,32 @@
}
return sb.String()
}

// Export exports the configuration to a file at the given path.
func (cfg Config) Export(path string) (string, error) {
// if path is empty, use the default path
if path == "" {
path = filepath.Join(cfg.Root(), filepath.Join(cfgFileName+"."+cfgExtension))
}

// ensure the path has proper extension
if !strings.HasSuffix(path, cfgExtension) {
return "", fmt.Errorf("provided config file must have %s extension", cfgExtension)
}

// create the file
file, err := os.Create(path)
Fixed Show fixed Hide fixed
if err != nil {
return "", fmt.Errorf("failed to create configuration file: %w", err)
}

defer file.Close()

// write the configuration to the file
err = toml.NewEncoder(file).Encode(cfg)
if err != nil {
return "", fmt.Errorf("failed to encode configuration: %w", err)
}

return path, nil
}
8 changes: 4 additions & 4 deletions tools/cosmovisor/args_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ func (s *argsTestSuite) TestDetailString() {
PollInterval: pollInterval,
UnsafeSkipBackup: unsafeSkipBackup,
DataBackupPath: dataBackupPath,
PreupgradeMaxRetries: preupgradeMaxRetries,
PreUpgradeMaxRetries: preupgradeMaxRetries,
}

expectedPieces := []string{
Expand Down Expand Up @@ -477,11 +477,11 @@ func (s *argsTestSuite) TestGetConfigFromEnv() {
PollInterval: time.Millisecond * time.Duration(interval),
UnsafeSkipBackup: skipBackup,
DataBackupPath: dataBackupPath,
PreupgradeMaxRetries: preupgradeMaxRetries,
PreUpgradeMaxRetries: preupgradeMaxRetries,
DisableLogs: disableLogs,
ColorLogs: colorLogs,
TimeFormatLogs: timeFormatLogs,
CustomPreupgrade: customPreUpgrade,
CustomPreUpgrade: customPreUpgrade,
DisableRecase: disableRecase,
ShutdownGrace: time.Duration(shutdownGrace),
}
Expand Down Expand Up @@ -791,7 +791,7 @@ func BenchmarkDetailString(b *testing.B) {
AllowDownloadBinaries: true,
UnsafeSkipBackup: true,
PollInterval: 450 * time.Second,
PreupgradeMaxRetries: 1e7,
PreUpgradeMaxRetries: 1e7,
}

b.ReportAllocs()
Expand Down
8 changes: 7 additions & 1 deletion tools/cosmovisor/cmd/cosmovisor/add_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,19 @@ func NewAddUpgradeCmd() *cobra.Command {

addUpgrade.Flags().Bool(cosmovisor.FlagForce, false, "overwrite existing upgrade binary / upgrade-info.json file")
addUpgrade.Flags().Int64(cosmovisor.FlagUpgradeHeight, 0, "define a height at which to upgrade the binary automatically (without governance proposal)")
addUpgrade.Flags().String(cosmovisor.FlagConfig, "", "path to cosmovisor config file")

return addUpgrade
}

// AddUpgrade adds upgrade info to manifest
func AddUpgrade(cmd *cobra.Command, args []string) error {
cfg, err := cosmovisor.GetConfigFromEnv()
configPath, err := cmd.Flags().GetString(cosmovisor.FlagConfig)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we read the config in the root command and pass it down to sub commands?
this way we can have a persistent flag on the root command and avoid adding a flag to each command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used two flags:

  • config for taking config file as input used by (add-upgrade)
  • export-config for exporting configuration to toml file used by (init).

Using config flag in root command will create confusion for user as they will see both flags export-config and config for init command.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don't think we should allow to export during init. Imho init should always create the file (maybe ask for a confirmation if the file already exists).

Additionally for people upgrading having an export command that converts flags to a config flag would be handy.

Additionally this way you don't have clashing flag names as you just pointed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.
So basically init command should only write to default path(leaving file already exist case aside).

additionally export command can read the config from the file(override if env is set) and print it.

Copy link
Member

Choose a reason for hiding this comment

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

yes, sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if err != nil {
return fmt.Errorf("failed to get config flag: %w", err)
}

cfg, err := cosmovisor.GetConfigFromFile(configPath)
if err != nil {
return err
}
Expand Down
41 changes: 33 additions & 8 deletions tools/cosmovisor/cmd/cosmovisor/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,30 @@ import (
"path/filepath"
"time"

"github.com/spf13/viper"

"github.com/spf13/cobra"

"cosmossdk.io/log"
"cosmossdk.io/tools/cosmovisor"
"cosmossdk.io/x/upgrade/plan"
)

var initCmd = &cobra.Command{
Use: "init <path to executable>",
Short: "Initialize a cosmovisor daemon home directory.",
Args: cobra.ExactArgs(1),
SilenceUsage: true,
RunE: func(cmd *cobra.Command, args []string) error {
return InitializeCosmovisor(nil, args)
},
func NewIntCmd() *cobra.Command {
var initCmd = &cobra.Command{
Use: "init <path to executable>",
Short: "Initialize a cosmovisor daemon home directory.",
Args: cobra.ExactArgs(1),
SilenceUsage: true,
RunE: func(cmd *cobra.Command, args []string) error {
mustNoError(viper.BindPFlags(cmd.Flags()))
return InitializeCosmovisor(nil, args)
},
}

initCmd.Flags().String(cosmovisor.FlagExportConfig, "", "path to export the configuration file to (default is <-home_path->/cosmovisor/config.toml")
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where the default is set.
IMHO it shouldn't be current dir but within the node directory (DAEMON_HOME)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is being set here.

I agree with you that's why I didn't set default path directly in flag because it depends on DEAMON_HOME so it has to be loaded at run time.

The default path is: DEAMON_HOME/COSMOVISOR/config.toml, you can check here.

Copy link
Member

Choose a reason for hiding this comment

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

I see it, thanks!


return initCmd
}

// InitializeCosmovisor initializes the cosmovisor directories, current link, and initial executable.
Expand Down Expand Up @@ -88,12 +97,20 @@ func InitializeCosmovisor(logger log.Logger, args []string) error {
}
logger.Info(fmt.Sprintf("the current symlink points to: %q", cur))

filePath, err := cfg.Export(viper.GetString(cosmovisor.FlagExportConfig))
if err != nil {
return fmt.Errorf("failed to export configuration: %w", err)
}

logger.Info(fmt.Sprintf("exported configuration to: %s", filePath))

return nil
}

// getConfigForInitCmd gets just the configuration elements needed to initialize cosmovisor.
func getConfigForInitCmd() (*cosmovisor.Config, error) {
var errs []error

// Note: Not using GetConfigFromEnv here because that checks that the directories already exist.
// We also don't care about the rest of the configuration stuff in here.
cfg := &cosmovisor.Config{
Expand All @@ -105,19 +122,27 @@ func getConfigForInitCmd() (*cosmovisor.Config, error) {
if cfg.ColorLogs, err = cosmovisor.BooleanOption(cosmovisor.EnvColorLogs, true); err != nil {
errs = append(errs, err)
}

if cfg.TimeFormatLogs, err = cosmovisor.TimeFormatOptionFromEnv(cosmovisor.EnvTimeFormatLogs, time.Kitchen); err != nil {
errs = append(errs, err)
}

// if backup is not set, use the home directory
if cfg.DataBackupPath == "" {
cfg.DataBackupPath = cfg.Home
}

if len(cfg.Name) == 0 {
errs = append(errs, fmt.Errorf("%s is not set", cosmovisor.EnvName))
}

switch {
case len(cfg.Home) == 0:
errs = append(errs, fmt.Errorf("%s is not set", cosmovisor.EnvHome))
case !filepath.IsAbs(cfg.Home):
errs = append(errs, fmt.Errorf("%s must be an absolute path", cosmovisor.EnvHome))
}

if len(errs) > 0 {
return cfg, errors.Join(errs...)
}
Expand Down