Merge branch 'jc-spawn-git-in-pgid' into 'master'

command: Add option to spawn a command with a pgid

Closes #4493

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4889

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>

7 files changed, 111 insertions, 4 deletions

diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 805ac336a..62428a511 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -195,7 +195,10 @@ func run(cfg config.Cfg) error {
 	}
 
 	skipHooks, _ := env.GetBool("GITALY_TESTING_NO_GIT_HOOKS", false)
-	var commandFactoryOpts []git.ExecCommandFactoryOption
+	commandFactoryOpts := []git.ExecCommandFactoryOption{
+		git.WithGitalyPid(os.Getpid()),
+	}
+
 	if skipHooks {
 		commandFactoryOpts = append(commandFactoryOpts, git.WithSkipHooks())
 	}
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/git/command_factory.go b/internal/git/command_factory.go
index dae719935..53444b43b 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -43,6 +43,7 @@ type CommandFactory interface {
 type execCommandFactoryConfig struct {
 	hooksPath     string
 	gitBinaryPath string
+	gitalyPid     int
 }
 
 // ExecCommandFactoryOption is an option that can be passed to NewExecCommandFactory.
@@ -69,6 +70,14 @@ func WithGitBinaryPath(path string) ExecCommandFactoryOption {
 	}
 }
 
+// WithGitalyPid will cause new commands to be started under the gitaly pid as
+// the pgid
+func WithGitalyPid(gitalyPid int) ExecCommandFactoryOption {
+	return func(cfg *execCommandFactoryConfig) {
+		cfg.gitalyPid = gitalyPid
+	}
+}
+
 type hookDirectories struct {
 	tempHooksPath string
 }
@@ -89,6 +98,8 @@ type ExecCommandFactory struct {
 
 	cachedGitVersionLock     sync.RWMutex
 	cachedGitVersionByBinary map[string]cachedGitVersion
+
+	gitalyPid int
 }
 
 // NewExecCommandFactory returns a new instance of initialized ExecCommandFactory. The returned
@@ -138,6 +149,7 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_
 		),
 		hookDirs:                 hookDirectories,
 		cachedGitVersionByBinary: make(map[string]cachedGitVersion),
+		gitalyPid:                factoryCfg.gitalyPid,
 	}
 
 	return gitCmdFactory, runCleanups, nil
@@ -413,6 +425,7 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 		command.WithCommandName("git", sc.Subcommand()),
 		command.WithCgroup(cf.cgroupsManager, repo),
 		command.WithCommandGitVersion(cmdGitVersion.String()),
+		command.WithParent(cf.gitalyPid),
 	)...)
 	if err != nil {
 		return nil, err
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)