Make COSMOVISOR_ENABLED and START_CMD work as intended

[orenl-lava]

Teach the run.sh to use START_CMD if set, even when SNAPSHOT_PATH is unused (previously, it would exec whatever command line passed to the docker, or do nothing if none).

This also fixes the case of COSMOVISOR_ENABLED=1, which sets START_CMD to start cosmovisor but then was not actually run before.

[tombeynon]

This is great @orenl-lava, but I think we need to tweak it slightly. The way it's setup now, START_CMD will always be set due to lines 27-28 in run.sh, meaning the docker command will then be ignored.

We should remove the else on lines 27-28, and move line 28 above line 320 since snapshot.sh requires START_CMD to be set.

I can take a look at this if it's easier, let me know.

[orenl-lava]

Yes, that's a good point. I wonder if docker command args should take priority over START_CMD variable? If so, then "$@" -if set- should replace START_CMD altogether - also for COSMOVISOR_ENABLED and SNAPSHOT_PATH cases.

Thus, it should check: "if args exists then use them, else if START_CMD exists then use it, else use "start"; And also check if cosmovisor (and?) or snapshot enabled, then prefix something to the former. Maybe something like:

  if [ "$COSMOVISOR_ENABLED" == "1" ]; then
    PREFIX_CMD="cosmovisor run"
  elif [ -n "$SNAPSHOT_PATH"  ]; then                  <-- maybe without "else"?
    PREFIX_CMD="snapshot.sh"
  else
    PREFIX_CMD=
  fi

and then

  if [ "$#" -ne 0 ]; then
    exec $PREFIX_CMD "$@"
  elif [ -n "$START_CMD" ]; then
    exec $PREFIX_CMD $START_CMD
  else
    exec $PREFIX_CMD start
  fi

[tombeynon]

There are some intricacies here - the reason I added START_CMD is because the docker command is prefixed with /bin/bash -c {command} (if I recall correctly), which doesn't work with the snapshot script. Likely it would also cause problems with Cosmovisor. We could simply remove this prefix but I'm unsure if it's always /bin/bash -c or not.

Might require a bit of testing to make sure all the cases are covered. Would be nice to clean this up a bit though. Best case would be to get rid of START_CMD and just use the native command, if the prefix issue can be resolved.

[orenl-lava]

because the docker command is prefixed with /bin/bash -c {command} (if I recall correctly),

Unsure I follow what you mean by this.
The Dockerfile sets ENTRYPOINT to be run.sh, so docker simply execve() the script (docker doesn't know it's a shell script, let alone treat shell scripts specially). Any command line args for docker run ... (not consumed by docker) are passed as is.

I think that users would want to:

1. Run the blockchain binary either as-is, -or- with snapshot.sh, -or- with cosmovisor. These are controlled SNAPSHOT_PATH, and COSMOVISOR_ENABLED.
2. Be able to set the blockchain binary command (for any of the above) to something else than the default "start". This can be done with START_CMD, so users can amend/override that command via the environment without changes to deployment logic - e.g. docker-compose yml (unlike with command line args to "docker run ...").

Note that START_CMD is currently used by bostrom/build.yml here (and perhaps by elsewhere). Thus, I favor keeping START_CMD with the suggested logic: "use rule 1 above for prefix, then if args exists then use them, else if START_CMD exists then use it, else use start". I amended the PR accordingly.

[orenl-lava]

@tombeynon - you are right, Docker adds /bin/sh -c: because the CMD directives comes in shell form. However, I wonder why this is needed: if I understand correctly, START_CMD lets us indicate a command other than "start" for the chain, optionally to be used by snapshot/cosmovisor. Thus, it need not be engraved in the image build via CMD - it is processed by run.sh anyhow, so it suffices to pass it in env.

And then, we will have the following cases:

1. "docker run ..." with args: run.sh will use the args
2. (else) START_CMD set (at build or at "docker run ..."): run.sh will use $START_CMD
3. (else) START_CMD not set: run.sh will use the default "start" command

In all cases, if snapshot/cosmovisor are selected, then run.sh will prefix the relevant command. (To run the the image with arbitrary command, do not set snapshot/cosmovisor, or even modify the entrypoint at "docker run ..." time).

I pushed another patch to implement this - please review. I need this fixed to be able to use cosmovisor. Note the CMD [] directive, whose purpose is to un-inherit any CMD settings from parent images.

[tombeynon]

Direction is great, generally will be a cleaner implementation. But we need to be really careful not to break any of the existing ways this image is used.

I think the main test case you're missing is if no arguments are passed at all, i.e. no cmd override, no START_CMD set, and without snapshot.sh or cosmovisor.

[orenl-lava]

Thanks for the feedback - I think it's ready now.
I can squash, rebase, and force-push to tidy up - or do you prefer to take as is?

[tombeynon]

@orenl-lava sorry for the delay - this is great as-is, I prefer the history but the repo is set to squash commits anyway.

[PikachuEXE]

Issue:
cosmovisor run $PROJECT_BIN start = $PROJECT_BIN $PROJECT_BIN start

[orenl-lava]

@PikachuEXE - I uploaded a fix in PR #572, can you please try it?