diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2023-10-12 19:47:50 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2023-10-23 19:47:49 +0300 |
commit | 0ca8e2a71beea49bd725447545a914568760de0c (patch) | |
tree | 9b4731d64bfdd010f0c5f6f30f5274febff9a718 | |
parent | d347233e976adfe590a696b56424ad5d473f733d (diff) |
Start commands directly in correct cgroups
Gitaly is currently first launching commands, and then adding them
to the correct cgroup. This has the downside that the commands get
to execute without the appropriate limits being applied for some
time before Gitaly manages to configure the Cgroup. This commit
changes Gitaly to launch commands directly in the correct cgroup.
This gets rid of the brief window with incorrect limits, and should
also be more efficient than moving processes between cgroups.
The fix only applies when using cgroups version 2 with Linux 5.7
or later.
Changelog: fixed
-rw-r--r-- | internal/command/command.go | 17 | ||||
-rw-r--r-- | internal/featureflag/ff_exec_command_directly_in_cgroup.go | 9 | ||||
-rw-r--r-- | internal/git/command_factory_cgroup_test.go | 10 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 2 |
4 files changed, 37 insertions, 1 deletions
diff --git a/internal/command/command.go b/internal/command/command.go index a97574680..0067f9836 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -256,6 +256,21 @@ func New(ctx context.Context, logger log.Logger, nameAndArgs []string, opts ...O // Start the command in its own process group (nice for signalling) cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + useCloneIntoCgroup := cfg.cgroupsManager != nil && cfg.cgroupsManager.SupportsCloneIntoCgroup() && featureflag.ExecCommandDirectlyInCgroup.IsEnabled(ctx) + if useCloneIntoCgroup { + // Configure the command to be executed in the correct cgroup. + cgroupPath, fd, err := cfg.cgroupsManager.CloneIntoCgroup(cmd, cfg.cgroupsAddCommandOpts...) + if err != nil { + return nil, fmt.Errorf("clone into cgroup: %w", err) + } + defer func() { + if err := fd.Close(); err != nil { + logger.WithError(err).ErrorContext(ctx, "failed to close cgroup file descriptor") + } + }() + command.cgroupPath = cgroupPath + } + // If requested, we will set up the command such that `Write()` can be called on it directly. Otherwise, // we simply pass as stdin whatever the user has asked us to set up. If no `stdin` was set up, the command // will implicitly read from `/dev/null`. @@ -337,7 +352,7 @@ func New(ctx context.Context, logger log.Logger, nameAndArgs []string, opts ...O }() }() - if cfg.cgroupsManager != nil { + if cfg.cgroupsManager != nil && !useCloneIntoCgroup { cgroupPath, err := cfg.cgroupsManager.AddCommand(command.cmd, cfg.cgroupsAddCommandOpts...) if err != nil { return nil, err diff --git a/internal/featureflag/ff_exec_command_directly_in_cgroup.go b/internal/featureflag/ff_exec_command_directly_in_cgroup.go new file mode 100644 index 000000000..a47b0339f --- /dev/null +++ b/internal/featureflag/ff_exec_command_directly_in_cgroup.go @@ -0,0 +1,9 @@ +package featureflag + +// ExecCommandDirectlyInCgroup enables directly spawning commands in the correct Cgroup. +var ExecCommandDirectlyInCgroup = NewFeatureFlag( + "exec_command_directly_in_cgroup", + "v16.5.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/5639", + false, +) diff --git a/internal/git/command_factory_cgroup_test.go b/internal/git/command_factory_cgroup_test.go index 6b92bf4b4..561a84bf0 100644 --- a/internal/git/command_factory_cgroup_test.go +++ b/internal/git/command_factory_cgroup_test.go @@ -1,6 +1,7 @@ package git_test import ( + "io" "os/exec" "testing" @@ -22,6 +23,15 @@ func (m *mockCgroupsManager) AddCommand(c *exec.Cmd, _ ...cgroups.AddCommandOpti return "", nil } +func (m *mockCgroupsManager) SupportsCloneIntoCgroup() bool { + return true +} + +func (m *mockCgroupsManager) CloneIntoCgroup(c *exec.Cmd, _ ...cgroups.AddCommandOption) (string, io.Closer, error) { + m.commands = append(m.commands, c) + return "", io.NopCloser(nil), nil +} + func TestNewCommandAddsToCgroup(t *testing.T) { t.Parallel() diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index e1a335e07..6de813fb6 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -257,6 +257,8 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.MailmapOptions, rand.Int()%2 == 0) // Randomly enable limiter.resizableSemaphore ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.UseResizableSemaphoreInConcurrencyLimiter, rand.Int()%2 == 0) + // Randomly enable ExecCommandDirectlyInCgroup. + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.ExecCommandDirectlyInCgroup, rand.Int()%2 == 0) for _, opt := range opts { ctx = opt(ctx) |