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.

Sign up for GitHub

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

Jump to bottom

[Feature]: Take configuration from config file instead of env vars on Cosmovisor #19764

Open
freak12techno opened this issue Mar 15, 2024 · 6 comments · May be fixed by #19995
Open

[Feature]: Take configuration from config file instead of env vars on Cosmovisor #19764

freak12techno opened this issue Mar 15, 2024 · 6 comments · May be fixed by #19995
Assignees
@akaladarshi
Labels
C:Cosmovisor Issues and PR related to Cosmovisor T:feature-request

Comments

@freak12techno
Copy link
Contributor

Summary

Moved from #19755:

It'd be nice if Cosmovisor would take configuration from config file instead of env vars.

Problem Definition

If a person is running Cosmovisor as a systemd service in background, with one set of variables, and then runs it via shell, with a different set of variables, for example invoking a command to show Cosmovisor config, it would use different params than the running node.

Real life example: I have the following env variables in ~/.zshrc:

export DAEMON_NAME=gaiad
export DAEMON_HOME=/home/validator/.gaia
export PATH="$DAEMON_HOME/cosmovisor/current/bin:$PATH"

and cosmovisor config shows the following:

validator@cosmos-monitoring ~ ❯ cosmovisor config
Configurable Values:
  DAEMON_HOME: /home/validator/.gaia
  DAEMON_NAME: gaiad
  DAEMON_ALLOW_DOWNLOAD_BINARIES: false
  DAEMON_DOWNLOAD_MUST_HAVE_CHECKSUM: false
  DAEMON_RESTART_AFTER_UPGRADE: true
  DAEMON_RESTART_DELAY: 0s
  DAEMON_SHUTDOWN_GRACE: 0s
  DAEMON_POLL_INTERVAL: 300ms
  UNSAFE_SKIP_BACKUP: false
  DAEMON_DATA_BACKUP_DIR: /home/validator/.gaia
  DAEMON_PREUPGRADE_MAX_RETRIES: 0
  COSMOVISOR_DISABLE_LOGS: false
  COSMOVISOR_COLOR_LOGS: true
  COSMOVISOR_TIMEFORMAT_LOGS: 3:04PM
  COSMOVISOR_CUSTOM_PREUPGRADE:
  COSMOVISOR_DISABLE_RECASE: false
Derived Values:
         Root Dir: /home/validator/.gaia/cosmovisor
      Upgrade Dir: /home/validator/.gaia/cosmovisor/upgrades
      Genesis Bin: /home/validator/.gaia/cosmovisor/genesis/bin/gaiad
   Monitored File: /home/validator/.gaia/data/upgrade-info.json
  Data Backup Dir: /home/validator/.gaia

(for example, note the UNSAFE_SKIP_BACKUP: false line).
But in fact my node's running as a systemd service with the following configuration:

validator@cosmos-monitoring ~ ❯ cat /etc/systemd/system/cosmovisor.service
[Unit]
Description=Cosmovisor
After=network-online.target

[Service]
User=validator
ExecStart=/home/validator/go/bin/cosmovisor run start
LimitNOFILE=infinity

Environment="DAEMON_HOME=/home/validator/.gaia"
Environment="DAEMON_NAME=gaiad"
Environment="DAEMON_ALLOW_DOWNLOAD_BINARIES=false"
Environment="DAEMON_RESTART_AFTER_UPGRADE=true"
Environment="UNSAFE_SKIP_BACKUP=true"
Environment="DAEMON_SHUTDOWN_GRACE=5s"

[Install]
WantedBy=multi-user.target

(note the Environment="UNSAFE_SKIP_BACKUP=true" line).
In fact it would actually avoid doing an upgrade, so running cosmovisor config produces misleading result.

Proposed Feature

  1. Have a config file (.toml? as it's used elsewhere for configuration) that stores all Cosmovisor configuration.
  2. On cosmovisor xxxx commands, take the config not from env vars, but from this file.
  3. Have a nice default for this path if it's not provided, maybe something like ~/.cosmovisor/config.toml.
  4. In cosmovisor init create this file if it's not present

Maybe it also makes sense to allow overriding the config's params via env variables, but I am not sure yet about that.
@freak12techno
Copy link
Contributor Author

@julienrbrt need your thoughts here

@julienrbrt
Copy link
Member

Yes it makes total sense. Thank you for writing this up.

It should indeed be toml for consistency.
Additionally, I would keep supporting environment variables and let it overwrite a config value. This way we keep backward compatible behavior.

@julienrbrt julienrbrt added the C:Cosmovisor Issues and PR related to Cosmovisor label Mar 15, 2024
@akaladarshi
Copy link
Contributor

Hey @julienrbrt. Can I work on this issue?

@julienrbrt
Copy link
Member

Hi, yes! I'll be happy to review. Thank you!

@akaladarshi
Copy link
Contributor

Yes it makes total sense. Thank you for writing this up.

It should indeed be toml for consistency. Additionally, I would keep supporting environment variables and let it overwrite a config value. This way we keep backward compatible behavior.

Just to confirm, by default we'll load configurations from the config file first and then overwrite them with any environment variables, if provided, correct?

If this is the case then we don't need to initialise the config through cosmovisor init as it should be provided by binary's executor.

@julienrbrt
Copy link
Member

Yea, but you still need to create the config the first time you use cosmovisor. So we should create the config at cosmovisor init if it doesn't exists.

@akaladarshi akaladarshi linked a pull request Apr 10, 2024 that will close this issue
Open
12 tasks
@julienrbrt julienrbrt mentioned this issue Apr 17, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@akaladarshi akaladarshi

Labels
C:Cosmovisor Issues and PR related to Cosmovisor T:feature-request
Projects
Cosmos-SDK
Status: 📋 Backlog
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(cosmovisor): load cosmovisor configuration from toml file akaladarshi/cosmos-sdk
3 participants
@julienrbrt @akaladarshi @freak12techno