diff options
author | John Cai <jcai@gitlab.com> | 2022-09-22 22:26:22 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2022-10-04 22:44:04 +0300 |
commit | 1244bec2e64e59558b59c9390cbd4fbc83ad12da (patch) | |
tree | 01f5db18b988b5dfaa7545741aac7a422ed69b11 | |
parent | fd5cbdc3389c9439b6eccb7812af905ee0a257a0 (diff) |
command: Add option to spawn a command with a pgid
Processes can be spawned with pgid [1], which can then be used to kill a
process and all child processes that were spawned with that pid as a pgid.
This allows us to, in a later commit, tie all Git commands to the
main Gitaly process. When a Gitaly process exits, a sys admin, a script,
or Gitlab Omnibus can do a `kill -9 -<old gitaly pid>` to kill any process
that has <old gitaly pid> as its pgid.
Add an option to allow commands to be instantiated with a parent pid.
1. https://pkg.go.dev/syscall#SysProcAttr
-rw-r--r-- | internal/command/command.go | 13 | ||||
-rw-r--r-- | internal/command/command_test.go | 64 | ||||
-rw-r--r-- | internal/command/option.go | 9 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_run_cmds_in_pgroup.go | 10 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 1 |
5 files changed, 94 insertions, 3 deletions
diff --git a/internal/command/command.go b/internal/command/command.go index 2881cf86f..8862fe3eb 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -201,6 +201,16 @@ func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, e cmd := exec.Command(nameAndArgs[0], nameAndArgs[1:]...) + if featureflag.RunCmdsInProcessGroup.IsEnabled(ctx) { + cmd.SysProcAttr = &syscall.SysProcAttr{ + Setpgid: true, + Pgid: cfg.parentPid, + } + } else { + // Start the command in its own process group (nice for signalling) + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + } + command := &Command{ cmd: cmd, startTime: time.Now(), @@ -222,9 +232,6 @@ func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, e // And finally inject environment variables required for tracing into the command. cmd.Env = envInjector(ctx, cmd.Env) - // Start the command in its own process group (nice for signalling) - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - // Three possible values for stdin: // * nil - Go implicitly uses /dev/null // * stdinSentinel - configure with cmd.StdinPipe(), allowing Write() to work diff --git a/internal/command/command_test.go b/internal/command/command_test.go index e10dadd4e..784b52e85 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -7,9 +7,11 @@ import ( "context" "fmt" "io" + "os" "path/filepath" "runtime" "strings" + "syscall" "testing" "time" @@ -21,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" ) @@ -512,3 +515,64 @@ func TestCommand_withFinalizer(t *testing.T) { } }) } + +func TestCommand_withPgid(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + r, w := io.Pipe() + parentCmd, err := New(ctx, []string{"cat", "/dev/stdin"}, WithStdin(r)) + require.NoError(t, err) + + errCh := make(chan error) + go func() { + errCh <- parentCmd.Wait() + }() + + parentPid := parentCmd.cmd.Process.Pid + + childCmd, err := New( + ctx, + []string{"cat", "/dev/stdin"}, + WithStdin(r), + WithParent(parentPid), + ) + require.NoError(t, err) + go func() { + errCh <- childCmd.Wait() + }() + + childPid := childCmd.cmd.Process.Pid + + // check that both processes are running + for _, pid := range []int{parentPid, childPid} { + process, err := os.FindProcess(pid) + require.NoError(t, err) + require.NoError(t, process.Signal(syscall.Signal(0))) + } + + // check that killing the pgid kills the childpid as well + if featureflag.RunCmdsInProcessGroup.IsEnabled(ctx) { + require.NoError(t, syscall.Kill(-parentPid, syscall.SIGKILL)) + } else { + require.NoError(t, syscall.Kill(parentPid, syscall.SIGKILL)) + require.NoError(t, syscall.Kill(childPid, syscall.SIGKILL)) + } + + w.Close() + + err = <-errCh + require.Error(t, err) + require.Contains(t, err.Error(), "signal: killed") + + err = <-errCh + require.Error(t, err) + require.Contains(t, err.Error(), "signal: killed") + + // check that both processes are not running + for _, pid := range []int{parentPid, childPid} { + process, err := os.FindProcess(pid) + require.NoError(t, err) + require.Error(t, process.Signal(syscall.Signal(0))) + } +} diff --git a/internal/command/option.go b/internal/command/option.go index cca8e62f1..b91ff8dbd 100644 --- a/internal/command/option.go +++ b/internal/command/option.go @@ -21,6 +21,8 @@ type config struct { cgroupsManager CgroupsManager cgroupsRepo repository.GitRepo + + parentPid int } // Option is an option that can be passed to `New()` for controlling how the command is being @@ -108,3 +110,10 @@ func WithFinalizer(finalizer func(*Command)) Option { cfg.finalizer = finalizer } } + +// WithParent sets a parent pid under which the command will be spawned +func WithParent(pid int) Option { + return func(cfg *config) { + cfg.parentPid = pid + } +} diff --git a/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go b/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go new file mode 100644 index 000000000..055ec6798 --- /dev/null +++ b/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go @@ -0,0 +1,10 @@ +package featureflag + +// RunCmdsInProcessGroup will run all git commands under the Gitaly pid as the +// pgid. +var RunCmdsInProcessGroup = NewFeatureFlag( + "run_cmds_in_process_group", + "v15.5.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4494", + false, +) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 02440e7f0..63ef64cad 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -196,6 +196,7 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { // deep in the call stack, so almost every test function would have to inject it into its // context. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true) + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCmdsInProcessGroup, rnd.Int()%2 == 0) // PraefectGeneratedReplicaPaths affects many tests as it changes the repository creation logic. // Randomly enable the flag to exercise both paths to some extent. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectGeneratedReplicaPaths, rnd.Int()%2 == 0) |