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

Add Signal Forwarding to Entrypoint Runner #2426

Merged
merged 1 commit into from
May 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions cmd/entrypoint/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package main
import (
"os"
"os/exec"
"os/signal"
"syscall"

"github.com/tektoncd/pipeline/pkg/entrypoint"
)
Expand All @@ -11,22 +13,52 @@ import (
// stdout/stderr are collected -- needs e2e tests.

// realRunner actually runs commands.
type realRunner struct{}
type realRunner struct {
signals chan os.Signal
}

var _ entrypoint.Runner = (*realRunner)(nil)

func (*realRunner) Run(args ...string) error {
func (rr *realRunner) Run(args ...string) error {
if len(args) == 0 {
return nil
}
name, args := args[0], args[1:]

// Receive system signals on "rr.signals"
if rr.signals == nil {
rr.signals = make(chan os.Signal, 1)
}
defer close(rr.signals)
signal.Notify(rr.signals)
defer signal.Reset()

cmd := exec.Command(name, args...)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
// dedicated PID group used to forward signals to
// main process and all children
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

// Start defined command
if err := cmd.Start(); err != nil {
return err
}

if err := cmd.Run(); err != nil {
// Goroutine for signals forwarding
go func() {
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))
Copy link

Choose a reason for hiding this comment

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

Curious - why do we negate cmd.Process.Pid here?

Copy link
Member Author

@waveywaves waveywaves May 6, 2020

Choose a reason for hiding this comment

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

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.

}
}
}()

// Wait for command to exit
if err := cmd.Wait(); err != nil {
return err
}

return nil
}
22 changes: 22 additions & 0 deletions cmd/entrypoint/runner_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package main

import (
"os"
"syscall"
"testing"
)

// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for adding the docstring!

Copy link
Member Author

Choose a reason for hiding this comment

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

:3

func TestRealRunnerSignalForwarding(t *testing.T) {
rr := realRunner{}
rr.signals = make(chan os.Signal, 1)
rr.signals <- syscall.SIGINT
if err := rr.Run("sleep", "3600"); err.Error() == "signal: interrupt" {
waveywaves marked this conversation as resolved.
Show resolved Hide resolved
t.Logf("SIGINT forwarded to Entrypoint")
} else {
t.Fatalf("Unexpected error received: %v", err)
}
}
waveywaves marked this conversation as resolved.
Show resolved Hide resolved