Add Signal Forwarding to Entrypoint Runner #2426
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
waveywaves
force-pushed
the
update/entrypoint
branch
from
170d5be to
fcb1bea
Compare
|
Nice! Can you add to the release notes section of the PR since I think this is something we'll want to call out in the next release. |
waveywaves
force-pushed
the
update/entrypoint
branch
2 times, most recently
from
83842eb to
3f58719
Compare
waveywaves
force-pushed
the
update/entrypoint
branch
from
3f58719 to
430f729
Compare
waveywaves
force-pushed
the
update/entrypoint
branch
from
430f729 to
bd8299e
Compare
|
/test pull-tekton-pipeline-integration-tests |
|
|
waveywaves
force-pushed
the
update/entrypoint
branch
from
bd8299e to
49abf13
Compare
|
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
2 similar comments
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-integration-tests |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
| for s := range rr.signals { | ||
| // Forward signal to main process and all children | ||
| if s != syscall.SIGCHLD { | ||
| _ = syscall.Kill(-cmd.Process.Pid, s.(syscall.Signal)) |
There was a problem hiding this comment.
Curious - why do we negate cmd.Process.Pid here?
There was a problem hiding this comment.
If PID given is negative, then the signal is sent to the process (with the same PID) as well as all it's children. This ensures signal forwarding.
There was a problem hiding this comment.
@vdemeester @imjasonh this seems pretty much ready to go, any objections on your end?
|
Thanks for pinging @bobcatfish, the PR lgtm but I'll hold off in case others have questions. |
The Entrypoint process should be notified and signals received should be propogated as necessary. This signal forwarding mimics the one in https://github.com/pablo-ruth/go-init which is an golang implementation of https://github.com/Yelp/dumb-init . The cmd.Run() has also been replaced with a cmd.Start() and cmd.Wait() to systematically start the command and Wait for it's completion without prematurely exiting.
waveywaves
force-pushed
the
update/entrypoint
branch
from
49abf13 to
4ecb74c
Compare
|
The following is the coverage report on the affected files.
|
|
/retest |
| // TestRealRunnerSignalForwarding will artificially put an interrupt signal (SIGINT) in the rr.signals chan. | ||
| // The chan will not be reinitialized in the runner considering we have already initialized it here. | ||
| // Once the sleep process starts, if the signal is successfully received by the parent process, it | ||
| // will interrupt and stop the sleep command. |
There was a problem hiding this comment.
thanks for adding the docstring!
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/kind feature |
tekton-robot
added
the
kind/feature
The Entrypoint process should be notified and signals
received should be propogated as necessary. This signal
forwarding mimics the one in
https://github.com/pablo-ruth/go-init which is an golang
implementation of https://github.com/Yelp/dumb-init .
The cmd.Run() has also been replaced with a cmd.Start()
and cmd.Wait() to systematically start the command
and Wait for it's completion without prematurely exiting.
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes