From c8cebd9649e15e9a41d344bb5cea6e1ae1306d19 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 13 May 2022 15:45:20 +0200 Subject: rubyserver: Fix leaking Goroutine when shutting down workers When stopping the Ruby server, we need to first stop all of the workers. But because we first stop the worker's monitor before we actually stop the worker's process we're not consuming the event that is generated on shutdown of the worker anymore. This leads to a Goroutine leak because we're blocked waiting for the monitor to consume the event, but that's never going to happen. Fix this Goroutine leak by first stopping the process, and only stopping the monitor after that to ensure the event can still be read. This is in preparation for a set of tests we're going to add in the Ruby server which would otherwise trigger this leak. --- internal/gitaly/rubyserver/rubyserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go index 8db9c500d..125377514 100644 --- a/internal/gitaly/rubyserver/rubyserver.go +++ b/internal/gitaly/rubyserver/rubyserver.go @@ -117,8 +117,8 @@ func (s *Server) Stop() { } for _, w := range s.workers { - w.stopMonitor() w.Process.Stop() + w.stopMonitor() } } } -- cgit v1.2.3 From 25d75a7c4af07ee1daea8b57ed0de798cf75f7f7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 13 May 2022 15:31:51 +0200 Subject: rubyserver: Ensure we sync objects if there is no Rugged gitconfig The Gitaly config exposes a `rugged_git_config_search_path` config that allows the administrator to configure where Rugged should search for the gitconfig it is supposed to use. While this option rather feels like an edge case, it is in fact currently mandatory to set this configuration to a gitconfig that sets `core.fsyncObjectFiles=true`. If there is no such configuration, then the result is that Rugged will not flush newly written objects to disk, which can easily lead to repository corruption. Luckily, both CNG and Omnibus know to set up this config as expected. But this brings its own problem with it: we have `core.fsyncObjectFiles` set in a central location now by distributions, which also means that Git as spawned by our Git command factory picks it up. But because that option has been deprecated in Git v2.36.0 this means that all Git commands would now print a warning. So we're between a rock and a hard place now: we must make sure that the configuration is set in Rugged, but it must not be present in the Git configuration. Given that this is the only configuration that Rugged really needs, and given that it must always be present or we otherwise corrupt repos, we can fix this issue by simply writing our own Gitconfig file in case `rugged_git_config_search_path` is unset. Like this, we can stop setting the search path in distributions and eventually deprecate its use. --- internal/gitaly/rubyserver/rubyserver.go | 42 ++++++++++++++ internal/gitaly/rubyserver/rubyserver_test.go | 82 +++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go index 125377514..bc14bd2c1 100644 --- a/internal/gitaly/rubyserver/rubyserver.go +++ b/internal/gitaly/rubyserver/rubyserver.go @@ -100,6 +100,7 @@ type Server struct { workers []*worker clientConnMu sync.Mutex clientConn *grpc.ClientConn + gitconfigDir string } // New returns a new instance of the server. @@ -120,6 +121,10 @@ func (s *Server) Stop() { w.Process.Stop() w.stopMonitor() } + + if s.gitconfigDir != "" { + _ = os.RemoveAll(s.gitconfigDir) + } } } @@ -136,6 +141,43 @@ func (s *Server) start() error { } cfg := s.cfg + + // Both Omnibus and CNG set up the gitconfig in a non-default location. This means that they + // need to tell us where to find it, which is done via the Rugged config search path. In + // fact though, this configuration really only contains a single entry that is of importance + // to us in the context of Rugged, which is `core.fsyncObjectFiles`: if not set, then we may + // fail to persist objects correctly and thus corrupt the repository. We don't care about + // anything else nowadays anymore because most of the functionality was stripped out of the + // sidecar. + // + // Because we only care about a single option, and because that option is in fact mandatory + // or we may end up with corrupted data, we want to get rid of this configuration. Rugged + // doesn't give us any way to force-enable fsyncing though except if we write it to a file. + // Consequentially, we'll have to inject our own gitconfig into Rugged that enables this + // config. And that's exactly what the following block does: if we detect that the distro + // isn't telling us where to find the Rugged configuration, we write our own config. This is + // required so that we can phase out support of the gitconfig in these distributions. + // + // This is transitory until either the sidecar goes away or the upstream pull request is + // released (https://github.com/libgit2/rugged/pull/918). + if cfg.Ruby.RuggedGitConfigSearchPath == "" { + gitconfigDir := filepath.Join(cfg.RuntimeDir, "ruby-gitconfig") + if err := os.Mkdir(gitconfigDir, 0o777); err != nil { + return fmt.Errorf("creating gitconfig dir: %w", err) + } + + // This file must be called `gitconfig` given that we pretend it's the system-level + // Git configuration. Otherwise, Rugged wouldn't find it. + if err := os.WriteFile(filepath.Join(gitconfigDir, "gitconfig"), []byte( + "[core]\n\tfsyncObjectFiles = true\n", + ), 0o666); err != nil { + return fmt.Errorf("writing gitconfig: %w", err) + } + + cfg.Ruby.RuggedGitConfigSearchPath = gitconfigDir + s.gitconfigDir = gitconfigDir + } + env, err := setupEnv(cfg, s.gitCmdFactory) if err != nil { return fmt.Errorf("setting up sidecar environment: %w", err) diff --git a/internal/gitaly/rubyserver/rubyserver_test.go b/internal/gitaly/rubyserver/rubyserver_test.go index a98d0eeb4..9e24585b8 100644 --- a/internal/gitaly/rubyserver/rubyserver_test.go +++ b/internal/gitaly/rubyserver/rubyserver_test.go @@ -3,11 +3,14 @@ package rubyserver import ( "context" "fmt" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/internal/git" + "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/auth" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/log" @@ -141,3 +144,82 @@ func TestSetupEnv(t *testing.T) { "GIT_CONFIG_COUNT=1", }) } + +func TestServer_gitconfig(t *testing.T) { + for _, tc := range []struct { + desc string + setup func(t *testing.T) (config.Cfg, string) + expectedGitconfig string + }{ + { + desc: "writes a default gitconfig", + setup: func(t *testing.T) (config.Cfg, string) { + cfg := testcfg.Build(t) + expectedPath := filepath.Join(cfg.RuntimeDir, "ruby-gitconfig", "gitconfig") + return cfg, expectedPath + }, + // If no search path is provided, we should create a placeholder file that + // contains all settings important to Rugged. + expectedGitconfig: "[core]\n\tfsyncObjectFiles = true\n", + }, + { + desc: "uses injected gitconfig", + setup: func(t *testing.T) (config.Cfg, string) { + gitconfigDir := testhelper.TempDir(t) + expectedPath := filepath.Join(gitconfigDir, "gitconfig") + require.NoError(t, os.WriteFile(expectedPath, []byte("garbage"), 0o666)) + + cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{ + Ruby: config.Ruby{ + RuggedGitConfigSearchPath: gitconfigDir, + }, + })) + + return cfg, expectedPath + }, + // The gitconfig shouldn't have been rewritten, so it should still contain + // garbage after the server starts. + expectedGitconfig: "garbage", + }, + } { + t.Run(tc.desc, func(t *testing.T) { + ctx := testhelper.Context(t) + cfg, expectedPath := tc.setup(t) + + gitCmdFactory := gittest.NewCommandFactory(t, cfg) + locator := config.NewLocator(cfg) + + rubyServer := New(cfg, gitCmdFactory) + require.NoError(t, rubyServer.Start()) + defer rubyServer.Stop() + + // Verify that the expected gitconfig exists and has expected contents. + gitconfigContents := testhelper.MustReadFile(t, expectedPath) + require.Equal(t, tc.expectedGitconfig, string(gitconfigContents)) + + // Write a gitconfig that is invalidly formatted. Like this we can assert + // whether the Ruby server tries to read it or not because it should in fact + // fail. + require.NoError(t, os.WriteFile(expectedPath, []byte( + `[`, + ), 0o666)) + + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) + + // We now do any random RPC request that hits the Ruby server... + client, err := rubyServer.WikiServiceClient(ctx) + require.NoError(t, err) + + ctx, err = SetHeaders(ctx, locator, repo) + require.NoError(t, err) + + stream, err := client.WikiListPages(ctx, &gitalypb.WikiListPagesRequest{Repository: repo}) + require.NoError(t, err) + + // ... and expect it to fail with an error parsing the configuration. This + // demonstrates the config was injected successfully. + _, err = stream.Recv() + testhelper.RequireGrpcError(t, fmt.Errorf("Rugged::ConfigError: failed to parse config file: missing ']' in section header (in %s:1)", expectedPath), err) + }) + } +} -- cgit v1.2.3