Merge branch 'ps-supervisor-config' into 'master'

Make gitaly supervisor configuration local

Closes #3651

See merge request gitlab-org/gitaly!3571

3 files changed, 29 insertions, 16 deletions

diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 659f47cf2..86738c4df 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -116,6 +116,11 @@ func (s *Server) start() error {
 	numWorkers := cfg.Ruby.NumWorkers
 	balancer.ConfigureBuilder(numWorkers, 0, time.Now)
 
+	svConfig, err := supervisor.NewConfigFromEnv()
+	if err != nil {
+		return fmt.Errorf("get supervisor configuration: %w", err)
+	}
+
 	for i := 0; i < numWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
 		socketPath := filepath.Join(cfg.InternalSocketDir, fmt.Sprintf("ruby.%d", i))
@@ -127,7 +132,7 @@ func (s *Server) start() error {
 
 		events := make(chan supervisor.Event)
 		check := func() error { return ping(socketPath) }
-		p, err := supervisor.New(name, env, args, cfg.Ruby.Dir, cfg.Ruby.MaxRSS, events, check)
+		p, err := supervisor.New(svConfig, name, env, args, cfg.Ruby.Dir, cfg.Ruby.MaxRSS, events, check)
 		if err != nil {
 			return err
 		}
diff --git a/internal/supervisor/supervisor.go b/internal/supervisor/supervisor.go
index c13e75948..f3a3dc0ee 100644
--- a/internal/supervisor/supervisor.go
+++ b/internal/supervisor/supervisor.go
@@ -34,18 +34,23 @@ var (
 		[]string{"name"},
 	)
 
-	config      Config
 	envInjector = tracing.NewEnvInjector()
 )
 
-func init() {
-	envconfig.MustProcess("gitaly_supervisor", &config)
+// NewConfigFromEnv returns Config initialised from environment variables or an error.
+func NewConfigFromEnv() (Config, error) {
+	var config Config
+	if err := envconfig.Process("gitaly_supervisor", &config); err != nil {
+		return Config{}, err
+	}
+	return config, nil
 }
 
 // Process represents a running process.
 type Process struct {
 	Name string
 
+	config          Config
 	memoryThreshold int
 	events          chan<- Event
 	healthCheck     func() error
@@ -62,13 +67,14 @@ type Process struct {
 }
 
 // New creates a new process instance.
-func New(name string, env []string, args []string, dir string, memoryThreshold int, events chan<- Event, healthCheck func() error) (*Process, error) {
+func New(config Config, name string, env []string, args []string, dir string, memoryThreshold int, events chan<- Event, healthCheck func() error) (*Process, error) {
 	if len(args) < 1 {
 		return nil, fmt.Errorf("need at least one argument")
 	}
 
 	p := &Process{
 		Name:            name,
+		config:          config,
 		memoryThreshold: memoryThreshold,
 		events:          events,
 		healthCheck:     healthCheck,
@@ -110,7 +116,7 @@ func watch(p *Process) {
 
 	// Use a buffered channel because we don't want to block the respawn loop
 	// on the monitor goroutine.
-	monitorChan := make(chan monitorProcess, config.CrashThreshold)
+	monitorChan := make(chan monitorProcess, p.config.CrashThreshold)
 	monitorDone := make(chan struct{})
 	go monitorRss(monitorChan, monitorDone, p.events, p.Name, p.memoryThreshold)
 
@@ -121,12 +127,12 @@ func watch(p *Process) {
 
 spawnLoop:
 	for {
-		if crashes >= config.CrashThreshold {
+		if crashes >= p.config.CrashThreshold {
 			logger.Warn("opening circuit breaker")
 			select {
 			case <-p.shutdown:
 				break spawnLoop
-			case <-time.After(config.CrashWaitTime):
+			case <-time.After(p.config.CrashWaitTime):
 				logger.Warn("closing circuit breaker")
 				crashes = 0
 			}
@@ -167,7 +173,7 @@ spawnLoop:
 				// We repeat this idempotent notification because its delivery is not
 				// guaranteed.
 				go p.notifyUp(pid)
-			case <-time.After(config.CrashResetTime):
+			case <-time.After(p.config.CrashResetTime):
 				crashes = 0
 			case <-waitCh:
 				crashes++
diff --git a/internal/supervisor/supervisor_test.go b/internal/supervisor/supervisor_test.go
index f38f21a6d..50e34440e 100644
--- a/internal/supervisor/supervisor_test.go
+++ b/internal/supervisor/supervisor_test.go
@@ -62,7 +62,9 @@ func testMain(m *testing.M) int {
 }
 
 func TestRespawnAfterCrashWithoutCircuitBreaker(t *testing.T) {
-	process, err := New(t.Name(), nil, []string{testExe}, testDir, 0, nil, nil)
+	config, err := NewConfigFromEnv()
+	require.NoError(t, err)
+	process, err := New(config, t.Name(), nil, []string{testExe}, testDir, 0, nil, nil)
 	require.NoError(t, err)
 	defer process.Stop()
 
@@ -83,7 +85,9 @@ func TestRespawnAfterCrashWithoutCircuitBreaker(t *testing.T) {
 }
 
 func TestTooManyCrashes(t *testing.T) {
-	process, err := New(t.Name(), nil, []string{testExe}, testDir, 0, nil, nil)
+	config, err := NewConfigFromEnv()
+	require.NoError(t, err)
+	process, err := New(config, t.Name(), nil, []string{testExe}, testDir, 0, nil, nil)
 	require.NoError(t, err)
 	defer process.Stop()
 
@@ -97,17 +101,15 @@ func TestTooManyCrashes(t *testing.T) {
 }
 
 func TestSpawnFailure(t *testing.T) {
-	defer func(waitTime time.Duration) {
-		config.CrashWaitTime = waitTime
-	}(config.CrashWaitTime)
-
+	config, err := NewConfigFromEnv()
+	require.NoError(t, err)
 	config.CrashWaitTime = 2 * time.Second
 
 	notFoundExe := filepath.Join(testDir, "not-found")
 	require.NoError(t, os.RemoveAll(notFoundExe))
 	defer os.Remove(notFoundExe)
 
-	process, err := New(t.Name(), nil, []string{notFoundExe}, testDir, 0, nil, nil)
+	process, err := New(config, t.Name(), nil, []string{notFoundExe}, testDir, 0, nil, nil)
 	require.NoError(t, err)
 	defer process.Stop()