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

ilab wrapper: exec for better signal handling / termination #697

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

omertuc
Copy link
Contributor

@omertuc omertuc commented Jul 25, 2024

If the wrapper script is killed, the container will be left running. Instead of just running the command, use exec to replace the wrapper script with the command, so that the command will receive the same signals as the wrapper script and the container will be terminated as expected.

If the wrapper script is killed, the container will be left running.
Instead of just running the command, use `exec` to replace the
wrapper script with the command, so that the command will receive
the same signals as the wrapper script and the container will be
terminated as expected.

Signed-off-by: Omer Tuchfeld <[email protected]>
Copy link
Contributor

@eranco74 eranco74 left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks!!

@eranco74
Copy link
Contributor

I just checked and it works as expected.
I execute ilab model serve from systemd service, prior to this change if I execute systemctl stop ilab.service the ilab container would keep running and the service journal log would say:

Jul 25 12:46:40 ip-172-31-6-97.ec2.internal systemd[1672]: ilab.service: State 'final-sigterm' timed out. Killing.
Jul 25 12:46:40 ip-172-31-6-97.ec2.internal systemd[1672]: ilab.service: Killing process 75939 (conmon) with signal SIGKILL.
Jul 25 12:46:40 ip-172-31-6-97.ec2.internal systemd[1672]: ilab.service: Failed with result 'timeout'.
Jul 25 12:46:40 ip-172-31-6-97.ec2.internal systemd[1672]: Stopped ilab model serve service.

With this change the systemd service behaves properly.

@rhatdan
Copy link
Member

rhatdan commented Jul 25, 2024

LGTM

@rhatdan rhatdan merged commit 7df349f into containers:main Jul 25, 2024
1 check passed
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