Gitaly: Refactor ConfigureRuby()

The 'ConfigureRuby()' method now returns 'ValidationErrors'.
The check were re-arrange a bit to group them together.
Also, the type of the 'MaxRSS' and 'NumWorkers' changed
to accept only unsigned values as negative values are not
allowed for these settings. And the checks for the duration
were added for 'GracefulRestartTimeout' and 'RestartDelay'.

4 files changed, 165 insertions, 97 deletions

diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 74a1010f3..b3ec65061 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -572,80 +572,6 @@ func TestValidateShellPath(t *testing.T) {
 	}
 }
 
-func TestConfigureRuby(t *testing.T) {
-	tmpDir := testhelper.TempDir(t)
-
-	tmpFile := filepath.Join(tmpDir, "file")
-	require.NoError(t, os.WriteFile(tmpFile, nil, perm.SharedFile))
-
-	testCases := []struct {
-		desc      string
-		dir       string
-		expErrMsg string
-	}{
-		{
-			desc: "relative path",
-			dir:  ".",
-		},
-		{
-			desc: "ok",
-			dir:  tmpDir,
-		},
-		{
-			desc:      "empty",
-			dir:       "",
-			expErrMsg: "gitaly-ruby.dir: is not set",
-		},
-		{
-			desc:      "does not exist",
-			dir:       "/does/not/exist",
-			expErrMsg: `'/does/not/exist' dir doesn't exist`,
-		},
-		{
-			desc:      "exists but is not a directory",
-			dir:       tmpFile,
-			expErrMsg: fmt.Sprintf(`'%s' is not a dir`, tmpFile),
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			cfg := Cfg{Ruby: Ruby{Dir: tc.dir}}
-
-			err := cfg.ConfigureRuby()
-			if tc.expErrMsg != "" {
-				require.EqualError(t, err, tc.expErrMsg)
-				return
-			}
-
-			require.NoError(t, err)
-
-			dir := cfg.Ruby.Dir
-			require.True(t, filepath.IsAbs(dir), "expected %q to be absolute path", dir)
-		})
-	}
-}
-
-func TestConfigureRubyNumWorkers(t *testing.T) {
-	testCases := []struct {
-		in, out int
-	}{
-		{in: -1, out: 2},
-		{in: 0, out: 2},
-		{in: 1, out: 2},
-		{in: 2, out: 2},
-		{in: 3, out: 3},
-	}
-
-	for _, tc := range testCases {
-		t.Run(fmt.Sprintf("%+v", tc), func(t *testing.T) {
-			cfg := Cfg{Ruby: Ruby{Dir: "/", NumWorkers: tc.in}}
-			require.NoError(t, cfg.ConfigureRuby())
-			require.Equal(t, tc.out, cfg.Ruby.NumWorkers)
-		})
-	}
-}
-
 func TestValidateListeners(t *testing.T) {
 	testCases := []struct {
 		desc string
diff --git a/internal/gitaly/config/ruby.go b/internal/gitaly/config/ruby.go
index 0cd1b530f..b53b13bdc 100644
--- a/internal/gitaly/config/ruby.go
+++ b/internal/gitaly/config/ruby.go
@@ -3,6 +3,7 @@ package config
 import (
 	"fmt"
 	"path/filepath"
+	"strings"
 	"time"
 
 	log "github.com/sirupsen/logrus"
@@ -12,53 +13,81 @@ import (
 // Ruby contains setting for Ruby worker processes
 type Ruby struct {
 	Dir                       string            `toml:"dir"`
-	MaxRSS                    int               `toml:"max_rss"`
+	MaxRSS                    uint              `toml:"max_rss"`
 	GracefulRestartTimeout    duration.Duration `toml:"graceful_restart_timeout"`
 	RestartDelay              duration.Duration `toml:"restart_delay"`
-	NumWorkers                int               `toml:"num_workers"`
+	NumWorkers                uint              `toml:"num_workers"`
 	RuggedGitConfigSearchPath string            `toml:"rugged_git_config_search_path"`
 }
 
 // ConfigureRuby validates the gitaly-ruby configuration and sets default values.
 func (cfg *Cfg) ConfigureRuby() error {
-	if cfg.Ruby.GracefulRestartTimeout.Duration() == 0 {
+	var errs ValidationErrors
+
+	if timeout := cfg.Ruby.GracefulRestartTimeout.Duration(); timeout == 0 {
 		cfg.Ruby.GracefulRestartTimeout = duration.Duration(10 * time.Minute)
+	} else if timeout < 0 {
+		errs = append(errs, ValidationError{
+			Key:     []string{"gitaly-ruby", "graceful_restart_timeout"},
+			Message: "cannot be negative",
+		})
 	}
 
-	if cfg.Ruby.MaxRSS == 0 {
+	if maxRSS := cfg.Ruby.MaxRSS; maxRSS == 0 {
 		cfg.Ruby.MaxRSS = 200 * 1024 * 1024
 	}
 
-	if cfg.Ruby.RestartDelay.Duration() == 0 {
+	if delay := cfg.Ruby.RestartDelay.Duration(); delay == 0 {
 		cfg.Ruby.RestartDelay = duration.Duration(5 * time.Minute)
+	} else if delay < 0 {
+		errs = append(errs, ValidationError{
+			Key:     []string{"gitaly-ruby", "restart_delay"},
+			Message: "cannot be negative",
+		})
 	}
 
-	if len(cfg.Ruby.Dir) == 0 {
-		return fmt.Errorf("gitaly-ruby.dir: is not set")
-	}
-
-	minWorkers := 2
-	if cfg.Ruby.NumWorkers < minWorkers {
+	if minWorkers := uint(2); cfg.Ruby.NumWorkers < minWorkers {
 		cfg.Ruby.NumWorkers = minWorkers
 	}
 
-	var err error
-	cfg.Ruby.Dir, err = filepath.Abs(cfg.Ruby.Dir)
-	if err != nil {
-		return err
+	if len(cfg.Ruby.Dir) == 0 {
+		errs = append(errs, ValidationError{
+			Key:     []string{"gitaly-ruby", "dir"},
+			Message: "is not set",
+		})
+	} else {
+		rubyDir, err := filepath.Abs(cfg.Ruby.Dir)
+		if err != nil {
+			errs = append(errs, ValidationError{
+				Key:     []string{"gitaly-ruby", "dir"},
+				Message: fmt.Sprintf("'%s' %s", cfg.Ruby.Dir, err),
+			})
+		} else if err := validateIsDirectory(rubyDir); err != nil {
+			errs = append(errs, ValidationError{
+				Key:     []string{"gitaly-ruby", "dir"},
+				Message: err.Error(),
+			})
+		} else {
+			cfg.Ruby.Dir = rubyDir
+			log.WithField("dir", cfg.Ruby.Dir).Debug("gitaly-ruby.dir set")
+		}
 	}
 
-	if len(cfg.Ruby.RuggedGitConfigSearchPath) != 0 {
-		cfg.Ruby.RuggedGitConfigSearchPath, err = filepath.Abs(cfg.Ruby.RuggedGitConfigSearchPath)
+	if path := strings.TrimSpace(cfg.Ruby.RuggedGitConfigSearchPath); path != "" {
+		ruggedPath, err := filepath.Abs(path)
 		if err != nil {
-			return err
+			errs = append(errs, ValidationError{
+				Key:     []string{"gitaly-ruby", "rugged_git_config_search_path"},
+				Message: fmt.Sprintf("'%s' %s", path, err),
+			})
+		} else {
+			cfg.Ruby.RuggedGitConfigSearchPath = ruggedPath
 		}
 	}
 
-	if err := validateIsDirectory(cfg.Ruby.Dir); err != nil {
-		return err
+	if len(errs) != 0 {
+		return errs
 	}
 
-	log.WithField("dir", cfg.Ruby.Dir).Debug("gitaly-ruby.dir set")
 	return nil
 }
diff --git a/internal/gitaly/config/ruby_test.go b/internal/gitaly/config/ruby_test.go
new file mode 100644
index 000000000..f5d773449
--- /dev/null
+++ b/internal/gitaly/config/ruby_test.go
@@ -0,0 +1,113 @@
+package config
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/duration"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+)
+
+func TestCfg_ConfigureRuby(t *testing.T) {
+	t.Parallel()
+	tmpDir := testhelper.TempDir(t)
+
+	tmpFile := filepath.Join(tmpDir, "file")
+	require.NoError(t, os.WriteFile(tmpFile, nil, perm.SharedFile))
+
+	testCases := []struct {
+		desc        string
+		rubyCfg     Ruby
+		expectedErr error
+	}{
+		{
+			desc:    "relative path",
+			rubyCfg: Ruby{Dir: "."},
+		},
+		{
+			desc:    "ok",
+			rubyCfg: Ruby{Dir: tmpDir},
+		},
+		{
+			desc:    "empty",
+			rubyCfg: Ruby{},
+			expectedErr: ValidationErrors{
+				{Key: []string{"gitaly-ruby", "dir"}, Message: "is not set"},
+			},
+		},
+		{
+			desc: "invalid",
+			rubyCfg: Ruby{
+				Dir:                    "/does/not/exist",
+				GracefulRestartTimeout: duration.Duration(-1),
+				RestartDelay:           duration.Duration(-1),
+				// Even though the path doesn't exist it doesn't produce an error
+				// as there is no check for the directory existence.
+				RuggedGitConfigSearchPath: "/does/not/exist/",
+			},
+			expectedErr: ValidationErrors{
+				{
+					Key:     []string{"gitaly-ruby", "graceful_restart_timeout"},
+					Message: "cannot be negative",
+				},
+				{
+					Key:     []string{"gitaly-ruby", "restart_delay"},
+					Message: "cannot be negative",
+				},
+				{
+					Key:     []string{"gitaly-ruby", "dir"},
+					Message: `'/does/not/exist' dir doesn't exist`,
+				},
+			},
+		},
+		{
+			desc:    "exists but is not a directory",
+			rubyCfg: Ruby{Dir: tmpFile},
+			expectedErr: ValidationErrors{
+				{
+					Key:     []string{"gitaly-ruby", "dir"},
+					Message: fmt.Sprintf("'%s' is not a dir", tmpFile),
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			cfg := Cfg{Ruby: tc.rubyCfg}
+
+			errs := cfg.ConfigureRuby()
+			require.Equal(t, tc.expectedErr, errs)
+			if tc.expectedErr != nil {
+				return
+			}
+
+			dir := cfg.Ruby.Dir
+			require.True(t, filepath.IsAbs(dir), "expected %q to be absolute path", dir)
+		})
+	}
+}
+
+func TestCfg_ConfigureRuby_numWorkers(t *testing.T) {
+	t.Parallel()
+	testCases := []struct {
+		in, out uint
+	}{
+		{in: 0, out: 2},
+		{in: 1, out: 2},
+		{in: 2, out: 2},
+		{in: 3, out: 3},
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("%+v", tc), func(t *testing.T) {
+			cfg := Cfg{Ruby: Ruby{Dir: "/", NumWorkers: tc.in}}
+			require.Empty(t, cfg.ConfigureRuby())
+			require.Equal(t, tc.out, cfg.Ruby.NumWorkers)
+		})
+	}
+}
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index d047f8707..49815a62d 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -177,7 +177,7 @@ func (s *Server) start() error {
 
 	gitalyRuby := filepath.Join(cfg.Ruby.Dir, "bin", "gitaly-ruby")
 
-	numWorkers := cfg.Ruby.NumWorkers
+	numWorkers := int(cfg.Ruby.NumWorkers)
 	balancer.ConfigureBuilder(numWorkers, 0, time.Now)
 
 	svConfig, err := supervisor.NewConfigFromEnv()
@@ -196,7 +196,7 @@ func (s *Server) start() error {
 
 		events := make(chan supervisor.Event)
 		check := func() error { return ping(socketPath) }
-		p, err := supervisor.New(svConfig, name, env, args, cfg.Ruby.Dir, cfg.Ruby.MaxRSS, events, check)
+		p, err := supervisor.New(svConfig, name, env, args, cfg.Ruby.Dir, int(cfg.Ruby.MaxRSS), events, check)
 		if err != nil {
 			return err
 		}