diff options
author | Toon Claes <toon@gitlab.com> | 2022-10-05 17:57:19 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-10-05 17:57:19 +0300 |
commit | 847af5af1cb914da74f3f9c5f3e774237ab10e14 (patch) | |
tree | 2b27dc740712284b050c6c43c67cb6e7846eaa7e | |
parent | f955266a86bb86637389583c591ae79ed7ad460e (diff) | |
parent | 8a83b0785f364a634758d26290eff001de6eb12b (diff) |
Merge branch 'revert-f955266a' into 'master'
Revert "Merge branch 'jc-spawn-git-in-pgid' into 'master'"
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4908
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
-rw-r--r-- | cmd/gitaly/main.go | 5 | ||||
-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/git/command_factory.go | 13 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_run_cmds_in_pgroup.go | 10 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 1 |
7 files changed, 4 insertions, 111 deletions
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go index 62428a511..805ac336a 100644 --- a/cmd/gitaly/main.go +++ b/cmd/gitaly/main.go @@ -195,10 +195,7 @@ func run(cfg config.Cfg) error { } skipHooks, _ := env.GetBool("GITALY_TESTING_NO_GIT_HOOKS", false) - commandFactoryOpts := []git.ExecCommandFactoryOption{ - git.WithGitalyPid(os.Getpid()), - } - + var commandFactoryOpts []git.ExecCommandFactoryOption if skipHooks { commandFactoryOpts = append(commandFactoryOpts, git.WithSkipHooks()) } diff --git a/internal/command/command.go b/internal/command/command.go index 8862fe3eb..2881cf86f 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -201,16 +201,6 @@ 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(), @@ -232,6 +222,9 @@ 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 784b52e85..e10dadd4e 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -7,11 +7,9 @@ import ( "context" "fmt" "io" - "os" "path/filepath" "runtime" "strings" - "syscall" "testing" "time" @@ -23,7 +21,6 @@ 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" ) @@ -515,64 +512,3 @@ 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 b91ff8dbd..cca8e62f1 100644 --- a/internal/command/option.go +++ b/internal/command/option.go @@ -21,8 +21,6 @@ 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 @@ -110,10 +108,3 @@ 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 53444b43b..dae719935 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -43,7 +43,6 @@ type CommandFactory interface { type execCommandFactoryConfig struct { hooksPath string gitBinaryPath string - gitalyPid int } // ExecCommandFactoryOption is an option that can be passed to NewExecCommandFactory. @@ -70,14 +69,6 @@ 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 } @@ -98,8 +89,6 @@ type ExecCommandFactory struct { cachedGitVersionLock sync.RWMutex cachedGitVersionByBinary map[string]cachedGitVersion - - gitalyPid int } // NewExecCommandFactory returns a new instance of initialized ExecCommandFactory. The returned @@ -149,7 +138,6 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_ ), hookDirs: hookDirectories, cachedGitVersionByBinary: make(map[string]cachedGitVersion), - gitalyPid: factoryCfg.gitalyPid, } return gitCmdFactory, runCleanups, nil @@ -425,7 +413,6 @@ 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 deleted file mode 100644 index 055ec6798..000000000 --- a/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go +++ /dev/null @@ -1,10 +0,0 @@ -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 63ef64cad..02440e7f0 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -196,7 +196,6 @@ 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) |