From 100d3549f7d8b09c49e50342645de2a1305f19e0 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 15 Dec 2021 10:15:18 -0500 Subject: [PATCH 1/2] git: Allow adding command to cgroup to fail Cgroups offer protection at the OS level from certain Git commands taking up too many system resources. However, there are situations where cgroups can suddenly become unavailable, such as on a restart of a server. In these situations, Gitaly should not be prevented from starting up, since Cgroups is not critical to the correct functioning of Gitaly. Instead, we should enter into a degraded state whereby Gitaly can still function, just not with the protection Cgroups offers. This change allows Gitaly to start up without Cgroups, as well as execute commands even if it fails getting added to a cgroup. Changelog: changed --- cmd/gitaly/main.go | 2 +- internal/git/command_factory.go | 3 +- internal/git/command_factory_cgroup_test.go | 57 +++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go index 1571c1d73f4..85e2a823f35 100644 --- a/cmd/gitaly/main.go +++ b/cmd/gitaly/main.go @@ -99,7 +99,7 @@ func configure(configPath string) (config.Cfg, error) { glog.Configure(glog.Loggers, cfg.Logging.Format, cfg.Logging.Level) if err := cgroups.NewManager(cfg.Cgroups).Setup(); err != nil { - return config.Cfg{}, fmt.Errorf("failed setting up cgroups: %w", err) + log.WithError(err).Error("failed setting up cgroups") } if err := verifyGitVersion(cfg); err != nil { diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index c3be0eb6072..84afc108dbc 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -6,6 +6,7 @@ import ( "fmt" "os/exec" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitaly/v14/internal/cgroups" "gitlab.com/gitlab-org/gitaly/v14/internal/command" @@ -142,7 +143,7 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi if featureflag.RunCommandsInCGroup.IsEnabled(ctx) { if err := cf.cgroupsManager.AddCommand(command); err != nil { - return nil, err + ctxlogrus.Extract(ctx).WithError(err).Error("could not add command to cgroup") } } diff --git a/internal/git/command_factory_cgroup_test.go b/internal/git/command_factory_cgroup_test.go index d2b0ab2f9a8..81dc56fab9d 100644 --- a/internal/git/command_factory_cgroup_test.go +++ b/internal/git/command_factory_cgroup_test.go @@ -1,11 +1,13 @@ package git import ( + "errors" "os" "path/filepath" "testing" "github.com/prometheus/client_golang/prometheus" + "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/internal/command" @@ -15,6 +17,14 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" ) +type mockErrorCgroupsManager struct { + mockCgroupsManager +} + +func (m *mockErrorCgroupsManager) AddCommand(_ *command.Command) error { + return errors.New("could not add command") +} + type mockCgroupsManager struct { commands []*command.Command } @@ -99,3 +109,50 @@ func TestNewCommandAddsToCgroup(t *testing.T) { }) } } + +func TestCgroupError(t *testing.T) { + root := testhelper.TempDir(t) + + cfg := config.Cfg{ + SocketPath: "/path/to/socket", + Cgroups: cgroups.Config{ + Count: 1, + }, + Storages: []config.Storage{{ + Name: "storage-1", + Path: root, + }}, + BinDir: filepath.Join(root, "bin.d"), + } + + require.NoError(t, os.MkdirAll(cfg.BinDir, 0o644)) + require.NoError(t, cfg.SetGitPath()) + require.NotEmpty(t, cfg.Git.BinPath) + + gitCmdFactory := NewExecCommandFactory(cfg) + + var manager mockErrorCgroupsManager + gitCmdFactory.cgroupsManager = &manager + + logger := testhelper.NewDiscardingLogEntry(t) + logHook := test.NewLocal(logger.Logger) + ctx, cancel := testhelper.Context(testhelper.ContextWithLogger(logger)) + defer cancel() + + cmd := SubCmd{ + Name: "hash-object", + } + + dir := testhelper.TempDir(t) + ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true) + _, err := gitCmdFactory.NewWithDir(ctx, dir, &cmd) + require.NoError(t, err) + + var found bool + for _, entry := range logHook.AllEntries() { + if entry.Message == "could not add command to cgroup" { + found = true + } + } + assert.True(t, found) +} -- GitLab From 2543b8f5543c5ded5829c6a201584b25fb0f19e7 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 15 Dec 2021 10:32:11 -0500 Subject: [PATCH 2/2] gitaly/main: Add background goroutine to setup cgroups In the previous commit we allow Gitaly startup to continue as normal even if cgroups fail to setup. Most of the time this will be because on a restart, the cgroup root hasn't been setup yet. This commit adds a background task that continues to try to setup the cgroups on an interval. This would allow the cgroup root to be setup after Gitaly starts up, instead of requiring another restart for cgroups to come into use. Changelog: changed --- cmd/gitaly/main.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go index 85e2a823f35..d62f9fc9c3e 100644 --- a/cmd/gitaly/main.go +++ b/cmd/gitaly/main.go @@ -100,6 +100,21 @@ func configure(configPath string) (config.Cfg, error) { if err := cgroups.NewManager(cfg.Cgroups).Setup(); err != nil { log.WithError(err).Error("failed setting up cgroups") + go func() { + ticker := time.NewTicker(5 * time.Minute) + defer ticker.Stop() + + for { + <-ticker.C + log.Info("attempting to setup cgroups") + + if err := cgroups.NewManager(cfg.Cgroups).Setup(); err != nil { + log.WithError(err).Error("failed setting up cgroups") + } else { + break + } + } + }() } if err := verifyGitVersion(cfg); err != nil { -- GitLab