diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-11-08 14:48:13 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-11-08 15:49:15 +0300 |
commit | 6b32a64bbd4567122643505bdb93a748f2417da4 (patch) | |
tree | 1d0a56ea39042bf4b53e9dede05950ab01e6252a | |
parent | 3cd2794f55fd6e425aa0ab38d2d043da35e961f7 (diff) |
global: Fix use of different cgroups when serving repos
There are several Git commands required for serving repositories to
clients that typically don't get spawned in the repository itself, but
which get the repository path as argument. These are git-upload-pack(1),
git-upload-archive(1) and git-receive-pack(1), all of which we use in
our `smarthttp` and `ssh` services. And because they don't require us to
set up the repository via the `--git-dir` option, we're currently using
`NewWithoutRepo()` to spawn them.
This has a major downside in the context of cgroups though. In order to
decide the cgroup bucket into which we put spawned commands, we either
hash the repository's information or, if not present, the command line.
As we're not passing a repo when serving them, it means that we always
use the command line for them. This is different from most of the other
RPCs though, where we use the repository information instead. Ultimately
this means that every repository has two different cgroups it frequently
gets assigned to, and that's of course not great.
Fix this bug by switching to `New()` instead of `NewWithoutRepo()`.
While not really necessary, it also shouldn't harm us either to make the
Git directory known to git-upload-pack(1) et al. And given that we now
have a repository context available, it means that we will put serving
commands into the same cgroup as for all the other commands.
Changelog: fixed
-rw-r--r-- | internal/gitaly/service/smarthttp/inforefs.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/monitor_stdin_command.go | 13 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/receive_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_archive.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack.go | 2 |
7 files changed, 17 insertions, 8 deletions
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go index 8799f517f..431429eb0 100644 --- a/internal/gitaly/service/smarthttp/inforefs.go +++ b/internal/gitaly/service/smarthttp/inforefs.go @@ -70,7 +70,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r } cmdOpts = append(cmdOpts, git.WithConfig(config...)) - cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{ + cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{ Name: service, Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}, git.Flag{Name: "--advertise-refs"}}, Args: []string{repoPath}, diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go index f7c1e123e..5fed6f6b0 100644 --- a/internal/gitaly/service/smarthttp/receive_pack.go +++ b/internal/gitaly/service/smarthttp/receive_pack.go @@ -50,7 +50,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac return err } - cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, + cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{ Name: "receive-pack", Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}}, diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go index 2fd4956dc..83076ee47 100644 --- a/internal/gitaly/service/smarthttp/upload_pack.go +++ b/internal/gitaly/service/smarthttp/upload_pack.go @@ -142,7 +142,7 @@ func (s *server) runUploadPack(ctx context.Context, req basicPostUploadPackReque git.WithPackObjectsHookEnv(req.GetRepository(), "http"), } - cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{ + cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{ Name: "upload-pack", Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}}, Args: []string{repoPath}, diff --git a/internal/gitaly/service/ssh/monitor_stdin_command.go b/internal/gitaly/service/ssh/monitor_stdin_command.go index dbed02dc3..f3bf43a70 100644 --- a/internal/gitaly/service/ssh/monitor_stdin_command.go +++ b/internal/gitaly/service/ssh/monitor_stdin_command.go @@ -8,15 +8,24 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/command" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline" + "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) -func monitorStdinCommand(ctx context.Context, gitCmdFactory git.CommandFactory, stdin io.Reader, stdout, stderr io.Writer, sc git.SubCmd, opts ...git.CmdOpt) (*command.Command, *pktline.ReadMonitor, error) { +func monitorStdinCommand( + ctx context.Context, + gitCmdFactory git.CommandFactory, + repo *gitalypb.Repository, + stdin io.Reader, + stdout, stderr io.Writer, + sc git.SubCmd, + opts ...git.CmdOpt, +) (*command.Command, *pktline.ReadMonitor, error) { stdinPipe, monitor, cleanup, err := pktline.NewReadMonitor(ctx, stdin) if err != nil { return nil, nil, fmt.Errorf("create monitor: %w", err) } - cmd, err := gitCmdFactory.NewWithoutRepo(ctx, sc, append([]git.CmdOpt{ + cmd, err := gitCmdFactory.New(ctx, repo, sc, append([]git.CmdOpt{ git.WithStdin(stdinPipe), git.WithStdout(stdout), git.WithStderr(stderr), diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go index c8d0657ba..4f2a84988 100644 --- a/internal/gitaly/service/ssh/receive_pack.go +++ b/internal/gitaly/service/ssh/receive_pack.go @@ -76,7 +76,7 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, return err } - cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, + cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{ Name: "receive-pack", Args: []string{repoPath}, diff --git a/internal/gitaly/service/ssh/upload_archive.go b/internal/gitaly/service/ssh/upload_archive.go index b473a10b9..7ed1c0ac5 100644 --- a/internal/gitaly/service/ssh/upload_archive.go +++ b/internal/gitaly/service/ssh/upload_archive.go @@ -53,7 +53,7 @@ func (s *server) sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer return stream.Send(&gitalypb.SSHUploadArchiveResponse{Stderr: p}) }) - cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, stdin, stdout, stderr, git.SubCmd{ + cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, req.GetRepository(), stdin, stdout, stderr, git.SubCmd{ Name: "upload-archive", Args: []string{repoPath}, }) diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go index 82318968c..072b8ec9c 100644 --- a/internal/gitaly/service/ssh/upload_pack.go +++ b/internal/gitaly/service/ssh/upload_pack.go @@ -128,7 +128,7 @@ func (s *server) sshUploadPack(rpcContext context.Context, req sshUploadPackRequ var stderrBuilder strings.Builder stderr = io.MultiWriter(stderr, &stderrBuilder) - cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, stdin, stdout, stderr, git.SubCmd{ + cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, repo, stdin, stdout, stderr, git.SubCmd{ Name: "upload-pack", Args: []string{repoPath}, }, commandOpts...) |