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

Figure out why Python containers don't respond to SIGTERM #66

Open
lance opened this issue Mar 29, 2021 · 20 comments
Open

Figure out why Python containers don't respond to SIGTERM #66

lance opened this issue Mar 29, 2021 · 20 comments

Comments

@lance
Copy link
Member

lance commented Mar 29, 2021

When run in a container, the Python applications built with these buildpacks don't respond to SIGTERM. When the same commands are run in a local execution environment, the applications respond correctly.

#60 (comment)

@matejvasek
Copy link
Contributor

Seems that for some reason python is ignoring most of the signals when running with pid 1.

@matejvasek
Copy link
Contributor

Well, at least default handlers are not working when pid == 1.

This could be fixed by installing own handlers:

def receive_signal(signal_number, frame):
    sys.stderr.write('Received: ' + str(signal_number) + "\n")
    sys.exit(1)
    return


signal.signal(signal.SIGTERM, receive_signal)
signal.signal(signal.SIGINT, receive_signal)

@matejvasek
Copy link
Contributor

Note that code above terminates python quite ungracefully by sys.exit(), maybe it could be done better.

@matejvasek
Copy link
Contributor

@lance I looked into it a bit ^^^

@matejvasek
Copy link
Contributor

this probably should be fixed in parliament project

@lance
Copy link
Member Author

lance commented Mar 30, 2021

@lance I looked into it a bit ^^^

Thank you! This is very helpful.

this probably should be fixed in parliament project

Agree

@lance
Copy link
Member Author

lance commented Mar 30, 2021

@matejvasek
Copy link
Contributor

matejvasek commented Mar 30, 2021

@matejvasek this is some good and relevant reading https://hackernoon.com/my-process-became-pid-1-and-now-signals-behave-strangely-b05c52cc551c

That checks out. The other two runtimes: golang and Quarkus are explicitly handling and they work as expected.

@lance
Copy link
Member Author

lance commented Mar 30, 2021

@matejvasek see #70

this probably should be fixed in parliament project

After reading more about the problem, I don't think the solution belongs with parliament, since that would require that parliament actually implement a lot of stuff that is unrelated to what it does - e.g. reaping zombie and orphaned processes. Other tools like dumb-init are explicitly designed for this.

@matejvasek
Copy link
Contributor

matejvasek commented Mar 30, 2021

After reading more about the problem, I don't think the solution belongs with parliament, since that would require that parliament actually implement a lot of stuff that is unrelated to what it does - e.g. reaping zombie and orphaned processes.

@lance I think it should be fixed in both:
#68
boson-project/parliament#2

@matejvasek
Copy link
Contributor

implement a lot of stuff that is unrelated to what it does - e.g. reaping zombie and orphaned processes.

why?

@lance
Copy link
Member Author

lance commented Mar 30, 2021

why?

Well, that's what an init process is supposed to do. But of course, you are right to question this since parliament was previously PID 1. So, fair point. :)

@matejvasek
Copy link
Contributor

But of course, you are right to question this since parliament was previously PID 1. So, fair point. :)

Actually it wasn't hence #68.

@matejvasek
Copy link
Contributor

It didn't work for two reason:

  1. It's forked from shell, therefore signal is recieved by shell (pid 1), not by the python process. fix: use exec in entrypoint #68
  2. When you fix that by exec then here is the problem with default signal handlers, fixed fix: explicit signal handling parliament#2.

@matejvasek
Copy link
Contributor

Well, that's what an init process is supposed to do.

I know this is true for standard OS, but I have no idea if that's needed in OCI where the process is the only one.

@matejvasek
Copy link
Contributor

withoud exec:

docker top quirky_solomon 
UID                 PID                 PPID                C                   STIME               TTY                 TIME                CMD
t1000               170975              170953              0                   15:59               pts/0               00:00:00            /bin/sh
t1000               171064              170975              10                  16:00               pts/0               00:00:00            python -m parliament 

with exec:

docker top nifty_swanson                             SIGINT 
UID                 PID                 PPID                C                   STIME               TTY                 TIME                CMD
t1000               174492              174471              22                  16:06               pts/0               00:00:00            python3 -m parliament .

@matejvasek
Copy link
Contributor

I know this is true for standard OS, but I have no idea if that's needed in OCI where the process is the only one.

Although noting prevents user from calling fork, no idea what happens then.

@matejvasek
Copy link
Contributor

@lance I am not against dumb-init, but if it's the right solution we should also use it in another runtimes, right?

@lance
Copy link
Member Author

lance commented Mar 30, 2021

@lance I am not against dumb-init, but if it's the right solution we should also use it in another runtimes, right?

Probably... I don't know enough about what the other runtimes are doing to avoid this to say with any confidence though.

@lance
Copy link
Member Author

lance commented Mar 30, 2021

Rather than making that big change today (1.14 cutoff day) for all runtimes, I think we stick with your small changes like exec and the changes to parliament.

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 a pull request may close this issue.

2 participants