From 3b751d3116d21ed1a8739bfa2855a8ff23dea3b5 Mon Sep 17 00:00:00 2001
From: "Jacob Vosmaer (GitLab)" <jacob@gitlab.com>
Date: Mon, 5 Mar 2018 18:33:46 +0000
Subject: Use only 1 gitaly-ruby process in test

---
 CHANGELOG.md                      |  2 ++
 doc/configuration/README.md       |  1 +
 internal/config/config_test.go    | 25 +++++++++++++++++++++++++
 internal/config/ruby.go           |  6 ++++++
 internal/rubyserver/rubyserver.go |  2 +-
 internal/rubyserver/worker.go     |  7 -------
 internal/testhelper/testhelper.go |  6 ++++++
 7 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f367e2e28..160d5c73b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 UNRELEASED
 
+- Use only 1 gitaly-ruby process in test
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/615
 - Bump github-linguist to 5.3.3
   https://gitlab.com/gitlab-org/gitaly/merge_requests/613
 
diff --git a/doc/configuration/README.md b/doc/configuration/README.md
index a5e9b5265..8dae81773 100644
--- a/doc/configuration/README.md
+++ b/doc/configuration/README.md
@@ -110,6 +110,7 @@ max\_rss limit.
 |max_rss|integer|no|Resident set size limit that triggers a gitaly-ruby restart, in bytes. Default 300MB.|
 |graceful_restart_timeout|string|no|Grace period to allow a gitaly-ruby process to finish ongoing requests. Default 10 minutes ("10m").|
 |restart_delay|string|no|Time memory must be high before a restart is triggered, in seconds. Default 5 minutes ("5m").|
+|num_workers|integer|no|Number of gitaly-ruby worker processes. Default 2, minimum 2.|
 
 ## Environment variables
 
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 3e50a8e64..cdd8f3561 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -2,6 +2,7 @@ package config
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
@@ -381,6 +382,30 @@ func TestConfigureRuby(t *testing.T) {
 	}
 }
 
+func TestConfigureRubyNumWorkers(t *testing.T) {
+	defer func(oldRuby Ruby) {
+		Config.Ruby = oldRuby
+	}(Config.Ruby)
+
+	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) {
+			Config.Ruby = Ruby{Dir: "/", NumWorkers: tc.in}
+			require.NoError(t, ConfigureRuby())
+			require.Equal(t, tc.out, Config.Ruby.NumWorkers)
+		})
+	}
+}
+
 func TestValidateListeners(t *testing.T) {
 	defer func(cfg config) {
 		Config = cfg
diff --git a/internal/config/ruby.go b/internal/config/ruby.go
index 4d65939b2..462dd93a6 100644
--- a/internal/config/ruby.go
+++ b/internal/config/ruby.go
@@ -13,6 +13,7 @@ type Ruby struct {
 	GracefulRestartTimeoutToml duration `toml:"graceful_restart_timeout"`
 	RestartDelay               time.Duration
 	RestartDelayToml           duration `toml:"restart_delay"`
+	NumWorkers                 int      `toml:"num_workers"`
 }
 
 // This type is a trick to let our TOML library parse durations from strings.
@@ -46,5 +47,10 @@ func ConfigureRuby() error {
 		return fmt.Errorf("gitaly-ruby.dir is not set")
 	}
 
+	minWorkers := 2
+	if Config.Ruby.NumWorkers < minWorkers {
+		Config.Ruby.NumWorkers = minWorkers
+	}
+
 	return validateIsDirectory(Config.Ruby.Dir, "gitaly-ruby.dir")
 }
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 35dcdcca0..a333432ae 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -114,7 +114,7 @@ func Start() (*Server, error) {
 	gitalyRuby := path.Join(cfg.Ruby.Dir, "bin/gitaly-ruby")
 
 	s := &Server{}
-	for i := 0; i < numWorkers; i++ {
+	for i := 0; i < cfg.Ruby.NumWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
 		socketPath := socketPath(i)
 
diff --git a/internal/rubyserver/worker.go b/internal/rubyserver/worker.go
index 41de27a6d..a06e09618 100644
--- a/internal/rubyserver/worker.go
+++ b/internal/rubyserver/worker.go
@@ -13,13 +13,6 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-const (
-	// We might want numWorkers to be configurable in the future. But at the
-	// moment we only support 'pick first' balancing so there is no point in
-	// having more than 2 workers yet.
-	numWorkers = 2
-)
-
 var (
 	terminationCounter = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 0b1388566..5b0c4957b 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -260,6 +260,12 @@ func ConfigureRuby() {
 	if err := config.ConfigureRuby(); err != nil {
 		log.Fatal("validate ruby config: %v", err)
 	}
+
+	// This speeds up test boot-up. The reason we use more than 1 worker in
+	// production is to handle memory leaks, but the tests don't run for long
+	// enough to hit those memory leaks anyway. Most tests also run faster
+	// than the minimum delay before a ruby worker failover can even happen.
+	config.Config.Ruby.NumWorkers = 1
 }
 
 // NewTestGrpcServer creates a GRPC Server for testing purposes
-- 
cgit v1.2.3