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

Fix START_CMD implementation #576

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

tombeynon
Copy link
Collaborator

Adjusts the START_CMD implementation so that:

  • Docker command takes precedence if set
  • Next is START_CMD arg/env variable if set
  • Otherwise cosmovisor run start if COSMOVISOR_ENABLED
  • Otherwise $PROJECT_BIN start
  • snapshot.sh script is always used if SNAPSHOT_PATH is set, with the command it runs set by the above logic

Also documents Cosmovisor support in the README, and adds a bit more logging so it's clear what's happening.

I considered adding a SNAPSHOT_ENABLED env instead of implicitly enabling with SNAPSHOT_PATH, but decided not to change that yet.

@PikachuEXE
Copy link
Contributor

Tested on a testnet node with snapshot enabled
Seems working
image

@tombeynon tombeynon merged commit f64daaa into akash-network:master Jul 12, 2023
60 checks passed
@tombeynon tombeynon deleted the fix-start-cmd branch July 12, 2023 16:35
@orenl-lava
Copy link
Contributor

orenl-lava commented Jul 12, 2023

  • Docker command takes precedence if set
  • Next is START_CMD arg/env variable if set
  • Otherwise cosmovisor run start if COSMOVISOR_ENABLED
  • Otherwise $PROJECT_BIN start
    snapshot.sh script is always used if SNAPSHOT_PATH is set, with the command it runs set by the above logic

I'd prefer to treat cosmovisor like snapshot, i.e.
"cosmovisor is always used if COSMOVISOR_ENABLED is set, with the command set by the above logic."

Also, the separation between args ($@ case) and $START_CMD case is intentional, because the latter breaks when one of the args has includes a space.

exec $PREFIX_CMD "$@"
elif [ -n "$START_CMD" ]; then
exec $PREFIX_CMD $START_CMD
export START_CMD="$@"
Copy link
Contributor

@orenl-lava orenl-lava Jul 12, 2023

Choose a reason for hiding this comment

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

This would break if docker command has an arg that includes a (white)space, e.g. docker run .... arg1 arg2 "third arg" ... - the exec statements at lines 364/367 will not preserve the quotes. That s why the previous version kept the two cases - docker args and START_CMD - separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might need tweaking a bit further then, but should be an easy enough change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can take a look over the next few days unless you want to submit a PR, but could you raise an issue to explain it if not?

@tombeynon
Copy link
Collaborator Author

I'd prefer to treat cosmovisor like snapshot

The problem with that is cosmovisor can only run commands directed to the node binary, so you'd need to disable cosmovisor support if you want to run any other command, e.g. a simple ls. Snapshot can technically run any command just wrapped by the snapshot script. It feels too restrictive, especially since if you want to use cosmovisor with a custom command you can just pass the cosmovisor run prefix yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants