hashicorp / terraform-aws-consul

A Terraform Module for how to run Consul on AWS using Terraform and Packer
Apache License 2.0
401 stars 484 forks source link

Init script pointing to /usr/local/bin/supervisor* incorrectly #37

Closed jasonmcintosh closed 6 years ago

jasonmcintosh commented 6 years ago

the install-consul shell script has:

  # On Amazon Linux, /usr/local/bin is not in PATH for the root user, so we add symlinks to /usr/bin, which is in PATH
  if [[ ! -f "/usr/bin/supervisorctl" ]]; then
    sudo ln -s /usr/local/bin/supervisorctl /usr/bin/supervisorctl
  fi
  if [[ ! -f "/usr/bin/supervisord" ]]; then
    sudo ln -s /usr/local/bin/supervisord /usr/bin/supervisord
  fi

However, the init script is hard coded to a path in /usr/local/bin

supervisorctl=/usr/local/bin/supervisorctl
supervisord=${SUPERVISORD-/usr/local/bin/supervisord}

Init script should probably be hard coded to /usr/bin/supervisorctl since the installer script is creating a symlink in /usr/bin - hit this issue trying to install this on a CentOS 7 based AMI.

brikis98 commented 6 years ago

The init script is one that's used pretty widely (you can see from the comments at the top we didn't write it ourselves), so I'm a bit hesitant to change it. We add the symlink mostly for user convenience rather than the init script. On CentOS, does supervisord end up in /usr/bin and NOT /usr/local/bin?

jasonmcintosh commented 6 years ago

Something logic wise.... IF NOT EXIST /usr/local/bin/supervisorctl IF EXIST /usr/bin/supervisorctl -- Link /usr/bin/supervisorctl to /usr/local/bin/supervisorctl ELSE IF EXIST /usr/local/bin/supervisorctl -- Link /usr/local/bin/supervisorctl to /usr/bin/supervisorctl ELSE -- ERROR OUT - no supervisorctl found ENDIF ENDIF

NOT so sure we even need the /usr/bin/supervisorctl link - since the init script is hard coded to go to /usr/local/bin/supervisorctl. BUT doesn't hurt I'd guess and that way if something needs to start things, it can do so.

brikis98 commented 6 years ago

We probably added the symlink so this works, even when executed as root user from a User Data script: https://github.com/hashicorp/terraform-aws-consul/blob/ee980b4a0282204a242fe4862525d753e3329c77/modules/run-consul/run-consul#L234-L235

jasonmcintosh commented 6 years ago

Yeah IF supervisor installed in /usr/local/bin/ you'd NOT get that as root (which is many init scripts). Traditionally you don't want /usr/local/bin in root's path - separate discussion around the Linux OS :) In this case, since it's already installed in /usr/bin, it's a symlink the other way that'd be needed so that init scripts will work. We're saying that /usr/local/bin/supervisorctl == /usr/bin/supervisorctl.

Think I'll tweak the installer to use the logic I listed above, should have a pull request soon. Got some updated docs on other dependencies (e.g. awscli is required for the run-consul app, which isn't ALWAYS installed by default).

jasonmcintosh commented 6 years ago

Should be fixed by https://github.com/hashicorp/terraform-aws-consul/pull/40