diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-01-11 13:39:48 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-01-14 17:25:03 +0300 |
commit | 55ce2628b6795bf5f80b1fad4916568b049c6533 (patch) | |
tree | e09ea91e6b89ce37fd8e6465588462ca02d699bf /cmd/gitaly-wrapper | |
parent | 44f4a68d69d59b98bf5d94c8f9fe1c62f370529e (diff) |
cmd/gitaly-wrapper: Add tests for finding processes by PID file
Now that `readPIDFile()` doesn't depend on global state anymore, it's
also a lot easier to test `findProcess()` via a PID file given that it
also doesn't depend on environment variables anymore, but instead
accepts a parameter. Add a bunch of tests to verify it works as
expected.
While at it, rename the function to `findProcess()` instead of
`findGitaly()`. The executable is used for both Praefect and Gitaly, so
the found process may not in fact be a Gitaly instance.
Diffstat (limited to 'cmd/gitaly-wrapper')
-rw-r--r-- | cmd/gitaly-wrapper/main.go | 30 | ||||
-rw-r--r-- | cmd/gitaly-wrapper/main_test.go | 65 |
2 files changed, 79 insertions, 16 deletions
diff --git a/cmd/gitaly-wrapper/main.go b/cmd/gitaly-wrapper/main.go index a70855bf5..7f3a93899 100644 --- a/cmd/gitaly-wrapper/main.go +++ b/cmd/gitaly-wrapper/main.go @@ -44,16 +44,16 @@ func main() { if pidFilePath == "" { logger.Fatalf("missing pid file ENV variable %q", bootstrap.EnvPidFile) } - logger.WithField("pid_file", pidFilePath).Info("finding gitaly") + logger.WithField("pid_file", pidFilePath).Info("finding process") - gitaly, err := findGitaly(pidFilePath) + process, err := findProcess(pidFilePath) if err != nil && !isRecoverable(err) { - logger.WithError(err).Fatal("find gitaly") + logger.WithError(err).Fatal("find process") } else if err != nil { - logger.WithError(err).Error("find gitaly") + logger.WithError(err).Error("find process") } - if gitaly != nil && isGitaly(gitaly, gitalyBin) { + if process != nil && isGitaly(process, gitalyBin) { logger.Info("adopting a process") } else { logger.Info("spawning a process") @@ -63,20 +63,20 @@ func main() { logger.WithError(err).Fatal("spawn gitaly") } - gitaly = proc + process = proc } - logger = logger.WithField("gitaly", gitaly.Pid) - logger.Info("monitoring gitaly") + logger = logger.WithField("process", process.Pid) + logger.Info("monitoring process") - forwardSignals(gitaly, logger) + forwardSignals(process, logger) // wait - for isAlive(gitaly) { + for isAlive(process) { time.Sleep(1 * time.Second) } - logger.Error("wrapper for gitaly shutting down") + logger.Error("wrapper for process shutting down") } func isRecoverable(err error) bool { @@ -84,20 +84,20 @@ func isRecoverable(err error) bool { return os.IsNotExist(err) || errors.As(err, &numError) } -func findGitaly(pidFilePath string) (*os.Process, error) { +func findProcess(pidFilePath string) (*os.Process, error) { pid, err := readPIDFile(pidFilePath) if err != nil { return nil, err } // os.FindProcess on unix do not return an error if the process does not exist - gitaly, err := os.FindProcess(pid) + process, err := os.FindProcess(pid) if err != nil { return nil, err } - if isAlive(gitaly) { - return gitaly, nil + if isAlive(process) { + return process, nil } return nil, nil diff --git a/cmd/gitaly-wrapper/main_test.go b/cmd/gitaly-wrapper/main_test.go index 8664c0bcb..5895fd60d 100644 --- a/cmd/gitaly-wrapper/main_test.go +++ b/cmd/gitaly-wrapper/main_test.go @@ -33,7 +33,7 @@ func TestStolenPid(t *testing.T) { require.NoError(t, err) require.NoError(t, pidFile.Close()) - tail, err := findGitaly(pidFile.Name()) + tail, err := findProcess(pidFile.Name()) require.NoError(t, err) require.NotNil(t, tail) require.Equal(t, cmd.Process.Pid, tail.Pid) @@ -47,6 +47,69 @@ func TestStolenPid(t *testing.T) { }) } +func TestFindProcess(t *testing.T) { + t.Run("nonexistent PID file", func(t *testing.T) { + _, err := findProcess("does-not-exist") + require.True(t, os.IsNotExist(err)) + }) + + t.Run("PID file with garbage", func(t *testing.T) { + path := filepath.Join(testhelper.TempDir(t), "pid") + require.NoError(t, os.WriteFile(path, []byte("garbage"), 0o644)) + + _, err := findProcess(path) + _, expectedErr := strconv.Atoi("garbage") + require.Equal(t, expectedErr, err) + }) + + t.Run("nonexistent process", func(t *testing.T) { + // The below PID can exist, but chances are sufficiently low to hopefully not matter + // in practice. + path := filepath.Join(testhelper.TempDir(t), "pid") + require.NoError(t, os.WriteFile(path, []byte("7777777"), 0o644)) + + // The process isn't alive, so we expect neither an error nor a process to be + // returned. + process, err := findProcess(path) + require.Nil(t, process) + require.Nil(t, err) + }) + + t.Run("running process", func(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + // The process will exit immediately, but that's not much of a problem given that + // `findProcess()` also works with zombies. + executable := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "noop"), []byte( + `#!/usr/bin/env bash + echo ready + `)) + + cmd := exec.CommandContext(ctx, executable) + stdout, err := cmd.StdoutPipe() + require.NoError(t, err) + + require.NoError(t, cmd.Start()) + defer func() { + require.NoError(t, cmd.Wait()) + }() + + // Wait for the process to be ready such that we know it's started up successfully + // and is executing the bash shell. + _, err = stdout.Read(make([]byte, 10)) + require.NoError(t, err) + + path := filepath.Join(testhelper.TempDir(t), "pid") + require.NoError(t, os.WriteFile(path, []byte(strconv.FormatInt(int64(cmd.Process.Pid), 10)), 0o644)) + + process, err := findProcess(path) + require.NotNil(t, process) + require.Equal(t, cmd.Process.Pid, process.Pid) + require.Equal(t, nil, err) + }) +} + func TestIsRecoverable(t *testing.T) { _, numericError := strconv.Atoi("") |