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

Properly utilize exec in helper scripts #822

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Properly utilize exec in helper scripts #822

merged 1 commit into from
Oct 19, 2023

Conversation

felddy
Copy link
Owner

@felddy felddy commented Oct 19, 2023

🗣 Description

This PR modifies the way that launcher.sh and the ultimate node processes are created.

💭 Motivation and context

The enterypoint.sh script will now call exec su-exec instead of just su-exec when
starting the launcher.sh. I had wrongly assumed that su-exec was also execing.

Similarly launcher.sh will now use exec env... instead of env to start node.

The motivation is to get the node process as PID 1 in the container, thus simplifying signal handling.

I went this route instead of implementing traps or using an init process as it was my original design goal to have a simple, single-process container.

Closes #813

🧪 Testing

Tested locally on my development box. I was able to verify that the process list is collapsed as designed.

Before:

➤ docker compose exec -it foundry ps
PID   USER     TIME  COMMAND
    1 root      0:00 {entrypoint.sh} /bin/sh ./entrypoint.sh resources/app/main.mjs --port=30000 --headless --noupdate --dataPath=/data
  112 501       0:00 {launcher.sh} /bin/sh ./launcher.sh resources/app/main.mjs --port=30000 --headless --noupdate --dataPath=/data
  142 501       0:01 node resources/app/main.mjs --port=30000 --headless --noupdate --dataPath=/data
  163 root      0:00 ps

After:

➤ docker compose exec -it foundry ps
PID   USER     TIME  COMMAND
    1 501       0:01 node resources/app/main.mjs --port=30000 --headless --noupdate --dataPath=/data
  329 root      0:00 ps

Also tested in CI.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • All new and existing tests pass.

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

Successfully merging this pull request may close these issues.

Remaining config.json.lock folder after stopping the container
1 participant