diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-06-17 09:53:17 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-06-23 09:13:31 +0300 |
commit | c25f5094e1a0916b133358f078f1496985c28f00 (patch) | |
tree | a40af0c0c6834b2f039d73257855186944a58a66 | |
parent | 1bb94d32c13c5804ec337dde26540b2bc964f960 (diff) |
command: Fix race with setting command names and context cancellation
When spawning commands, callers can afterwards call `SetMetricsCmd()`
and `SetMetricsSubCmd()` to have the command be properly represented in
Prometheus metrics via its command and subcommand. But because we spawn
a Goroutine that logs these metrics as soon as the context is done, this
creates a race between setting those names and context cancellation.
Fix this race by converting these functions into options that can be
passed to `New()` so that they're set up correctly at construction time.
Changelog: fixed
-rw-r--r-- | internal/command/command.go | 20 | ||||
-rw-r--r-- | internal/command/option.go | 11 | ||||
-rw-r--r-- | internal/git/command_factory.go | 8 | ||||
-rw-r--r-- | internal/git2go/executor.go | 3 | ||||
-rw-r--r-- | internal/gitaly/hook/custom.go | 3 | ||||
-rw-r--r-- | internal/gitaly/linguist/linguist.go | 8 |
6 files changed, 28 insertions, 25 deletions
diff --git a/internal/command/command.go b/internal/command/command.go index 5782e75dc..e1f3f0458 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -185,10 +185,12 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) { }() command := &Command{ - cmd: cmd, - startTime: time.Now(), - context: ctx, - span: span, + cmd: cmd, + startTime: time.Now(), + context: ctx, + span: span, + metricsCmd: cfg.commandName, + metricsSubCmd: cfg.subcommandName, } // Export allowed environment variables as set in the Gitaly process. @@ -297,16 +299,6 @@ func (c *Command) SetCgroupPath(path string) { c.cgroupPath = path } -// SetMetricsCmd overrides the "cmd" label used in metrics -func (c *Command) SetMetricsCmd(metricsCmd string) { - c.metricsCmd = metricsCmd -} - -// SetMetricsSubCmd sets the "subcmd" label used in metrics -func (c *Command) SetMetricsSubCmd(metricsSubCmd string) { - c.metricsSubCmd = metricsSubCmd -} - // This function should never be called directly, use Wait(). func (c *Command) wait() { if c.writer != nil { diff --git a/internal/command/option.go b/internal/command/option.go index 7e4c0c64f..ade36dbd5 100644 --- a/internal/command/option.go +++ b/internal/command/option.go @@ -7,6 +7,9 @@ type config struct { stdout io.Writer stderr io.Writer environment []string + + commandName string + subcommandName string } // Option is an option that can be passed to `New()` for controlling how the command is being @@ -49,3 +52,11 @@ func WithEnvironment(environment []string) Option { cfg.environment = environment } } + +// WithCommandName overrides the "cmd" and "subcmd" label used in metrics. +func WithCommandName(commandName, subcommandName string) Option { + return func(cfg *config) { + cfg.commandName = commandName + cfg.subcommandName = subcommandName + } +} diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index a18c9fd63..93242d1f3 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -397,13 +397,15 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi execCommand := exec.Command(execEnv.BinaryPath, args...) execCommand.Dir = dir - command, err := command.New(ctx, execCommand, append(config.commandOpts, command.WithEnvironment(env))...) + command, err := command.New(ctx, execCommand, append( + config.commandOpts, + command.WithEnvironment(env), + command.WithCommandName("git", sc.Subcommand()), + )...) if err != nil { return nil, err } - command.SetMetricsSubCmd(sc.Subcommand()) - if featureflag.RunCommandsInCGroup.IsEnabled(ctx) { if err := cf.cgroupsManager.AddCommand(command, repo); err != nil { return nil, err diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go index d5319304f..c1e220f43 100644 --- a/internal/git2go/executor.go +++ b/internal/git2go/executor.go @@ -93,13 +93,12 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re command.WithStdout(&stdout), command.WithStderr(log), command.WithEnvironment(env), + command.WithCommandName("gitaly-git2go", subcmd), ) if err != nil { return nil, err } - cmd.SetMetricsSubCmd(subcmd) - if err := cmd.Wait(); err != nil { return nil, err } diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go index 490409773..2c819dd3e 100644 --- a/internal/gitaly/hook/custom.go +++ b/internal/gitaly/hook/custom.go @@ -73,13 +73,12 @@ func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, ho command.WithStdout(stdout), command.WithStderr(stderr), command.WithEnvironment(env), + command.WithCommandName("gitaly-hooks", hookName), ) if err != nil { return err } - c.SetMetricsSubCmd(hookName) - if err = c.Wait(); err != nil { // Custom hook errors need to be handled specially when we update // refs via updateref.UpdaterWithHooks: their stdout and stderr must diff --git a/internal/gitaly/linguist/linguist.go b/internal/gitaly/linguist/linguist.go index 4bc9dbce1..da085b58a 100644 --- a/internal/gitaly/linguist/linguist.go +++ b/internal/gitaly/linguist/linguist.go @@ -95,14 +95,14 @@ func (inst *Instance) startGitLinguist(ctx context.Context, repoPath string, com cmd := exec.Command(bundle, "exec", "bin/gitaly-linguist", "--repository="+repoPath, "--commit="+commitID) cmd.Dir = inst.cfg.Ruby.Dir - internalCmd, err := command.New(ctx, cmd, command.WithEnvironment(env.AllowedRubyEnvironment(os.Environ()))) + internalCmd, err := command.New(ctx, cmd, + command.WithEnvironment(env.AllowedRubyEnvironment(os.Environ())), + command.WithCommandName("git-linguist", "stats"), + ) if err != nil { return nil, fmt.Errorf("creating command: %w", err) } - internalCmd.SetMetricsCmd("git-linguist") - internalCmd.SetMetricsSubCmd("stats") - return internalCmd, nil } |