Updates to Docker worker image entrypoints #405
Conversation
There was a problem hiding this comment.
Overall this looks good, thanks @robertbartel! I left a few minor comments that should be trivial to address.
| else | ||
| MPI_HOSTS_FILE="$(su ${MPI_USER} -c 'echo "${HOME}"')/.mpi_hosts" | ||
| fi | ||
| RUN_SENTINEL="/home/${MPI_USER}/.run_sentinel" |
There was a problem hiding this comment.
Should we put this in /var instead?
There was a problem hiding this comment.
Hmm. To go that route, I'd want us to add a dedicated directory under /var as part of the image. It would hold job-execution-items like this and be chown-ed to mpiuser. Otherwise we eventually won't be able to create the sentinel file in the container (see #238).
We also aren't really writing anything to this file, just creating, watching, and removing it. We can discuss further, but I'm inclined to leave it as-is for now.
There was a problem hiding this comment.
Im totally okay with leave it how it is. The only reason I asked was in the situation that an MPI_USER did not have a /home directory.
| @@ -36,6 +14,7 @@ if [ "$(whoami)" = "${MPI_USER}" ]; then | |||
| else | |||
| MPI_HOSTS_FILE="$(su ${MPI_USER} -c 'echo "${HOME}"')/.mpi_hosts" | |||
| fi | |||
| RUN_SENTINEL="/home/${MPI_USER}/.run_sentinel" | |||
There was a problem hiding this comment.
Same as above comment. Just marking
Move ngen entrypoint args to use of explicit flags and to use of single composite config dataset arg/name/directory.
Updating functions for generating Docker CMD args to account for a flag-based approach rather than a positional one.
Updating entrypoint.sh to copy various job-related configs into output dataset for record keeping and reproducibility.
Fixing initial sanity checks that use tests and print error messages but were not actually exiting in error, and updating test for WORKER_INDEX to account for any non-integer values properly.
Adding sanity check for received MPI_NODE_COUNT value in ngen worker image entrypoint.sh that ensures both that a value was provided and that it is a positive integer.
- Moving to bash for access to trap command - Move logic for closing of remote workers to function - Add trap on any exit (though specific to when MPI user is active) for remote worker cleanup - Add trap on any exit (though specific to SSHD user is active) for closing sshd process
Copying config files to output datasets for record keeping, and making sure calibration routine uses (i.e., alters) the copy written to output rather than the original.
Updating to use bash shell and to use flag-based approach for args.
Account for ".yml" and ".yaml" extensions when trying to find calibration config file, as well as ignore the case. Co-authored-by: Austin Raney <[email protected]>
robertbartel
force-pushed
the
f/update_ngen_docker/update_entrypoint
branch
from
e9d3ccc
to
df67199
Compare
| else | ||
| MPI_HOSTS_FILE="$(su ${MPI_USER} -c 'echo "${HOME}"')/.mpi_hosts" | ||
| fi | ||
| RUN_SENTINEL="/home/${MPI_USER}/.run_sentinel" |
There was a problem hiding this comment.
Im totally okay with leave it how it is. The only reason I asked was in the situation that an MPI_USER did not have a /home directory.
|
Merge at will. I had a question above that does not hold up merging this. |
Relates to #168.
Closes #268.