Merge branch 'jv-ruby-delay-start' into 'master'

main: start ruby server after opening network listeners

Closes #1850

See merge request gitlab-org/gitaly!1455

16 files changed, 53 insertions, 63 deletions

diff --git a/changelogs/unreleased/jv-ruby-delay-start.yml b/changelogs/unreleased/jv-ruby-delay-start.yml
new file mode 100644
index 000000000..1a1a8fb27
--- /dev/null
+++ b/changelogs/unreleased/jv-ruby-delay-start.yml
@@ -0,0 +1,5 @@
+---
+title: 'main: start ruby server after opening network listeners'
+merge_request: 1455
+author:
+type: fixed
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index f3f97413b..3a506d86e 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -109,10 +109,7 @@ func main() {
 // Inside here we can use deferred functions. This is needed because
 // log.Fatal bypasses deferred functions.
 func run(b *bootstrap.Bootstrap) error {
-	servers, err := bootstrap.NewServerFactory()
-	if err != nil {
-		return err
-	}
+	servers := bootstrap.NewServerFactory()
 	defer servers.Stop()
 
 	b.StopAction = servers.GracefulStop
@@ -154,7 +151,8 @@ func run(b *bootstrap.Bootstrap) error {
 	}
 
 	for _, shard := range config.Config.Storages {
-		if err = storage.WriteMetadataFile(shard); err != nil {
+		if err := storage.WriteMetadataFile(shard); err != nil {
+			// TODO should this be a return? https://gitlab.com/gitlab-org/gitaly/issues/1893
 			log.WithError(err).Error("Unable to write gitaly metadata file")
 		}
 	}
@@ -163,5 +161,9 @@ func run(b *bootstrap.Bootstrap) error {
 		return fmt.Errorf("unable to start the bootstrap: %v", err)
 	}
 
+	if err := servers.StartRuby(); err != nil {
+		return fmt.Errorf("initialize gitaly-ruby: %v", err)
+	}
+
 	return b.Wait()
 }
diff --git a/internal/bootstrap/server_factory.go b/internal/bootstrap/server_factory.go
index f1bfa7624..bb26a59d4 100644
--- a/internal/bootstrap/server_factory.go
+++ b/internal/bootstrap/server_factory.go
@@ -4,7 +4,6 @@ import (
 	"net"
 	"sync"
 
-	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/server"
 	"google.golang.org/grpc"
@@ -20,20 +19,16 @@ type GracefulStoppableServer interface {
 	GracefulStop()
 	Stop()
 	Serve(l net.Listener, secure bool) error
+	StartRuby() error
 }
 
 // NewServerFactory initializes a rubyserver and then lazily initializes both secure and insecure grpc.Server
-func NewServerFactory() (GracefulStoppableServer, error) {
-	ruby, err := rubyserver.Start()
-	if err != nil {
-		log.Error("start ruby server")
-
-		return nil, err
-	}
-
-	return &serverFactory{ruby: ruby}, nil
+func NewServerFactory() GracefulStoppableServer {
+	return &serverFactory{ruby: &rubyserver.Server{}}
 }
 
+func (s *serverFactory) StartRuby() error { return s.ruby.Start() }
+
 func (s *serverFactory) Stop() {
 	for _, srv := range s.all() {
 		srv.Stop()
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 45c580a54..b143061ac 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -102,7 +102,7 @@ func runFullGitalyServer(t *testing.T) (*grpc.Server, string) {
 	return server, "unix://" + serverSocketPath
 }
 
-var RubyServer *rubyserver.Server
+var RubyServer = &rubyserver.Server{}
 
 func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
@@ -121,8 +121,7 @@ func testMain(m *testing.M) int {
 
 	testhelper.ConfigureGitalySSH()
 
-	RubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer RubyServer.Stop()
diff --git a/internal/rubyserver/concurrency_test.go b/internal/rubyserver/concurrency_test.go
index 979c5e3d0..f9b50896d 100644
--- a/internal/rubyserver/concurrency_test.go
+++ b/internal/rubyserver/concurrency_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"google.golang.org/grpc/codes"
 	healthpb "google.golang.org/grpc/health/grpc_health_v1"
@@ -29,10 +30,8 @@ func waitPing(s *Server) error {
 func BenchmarkConcurrency(b *testing.B) {
 	config.Config.Ruby.NumWorkers = 2
 
-	s, err := Start()
-	if err != nil {
-		b.Fatal(err)
-	}
+	s := &Server{}
+	require.NoError(b, s.Start())
 	defer s.Stop()
 
 	// Warm-up: wait for gitaly-ruby to boot
diff --git a/internal/rubyserver/health_test.go b/internal/rubyserver/health_test.go
index f514d59f8..c074435e8 100644
--- a/internal/rubyserver/health_test.go
+++ b/internal/rubyserver/health_test.go
@@ -8,8 +8,8 @@ import (
 )
 
 func TestPingSuccess(t *testing.T) {
-	s, err := Start()
-	require.NoError(t, err)
+	s := &Server{}
+	require.NoError(t, s.Start())
 	defer s.Stop()
 
 	require.True(t, len(s.workers) > 0, "expected at least one worker in server")
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 4b031762a..92196f79b 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -69,6 +69,8 @@ func socketPath(id int) string {
 
 // Server represents a gitaly-ruby helper process.
 type Server struct {
+	startOnce    sync.Once
+	startErr     error
 	workers      []*worker
 	clientConnMu sync.Mutex
 	clientConn   *grpc.ClientConn
@@ -95,12 +97,17 @@ func (s *Server) Stop() {
 }
 
 // Start spawns the Ruby server.
-func Start() (*Server, error) {
+func (s *Server) Start() error {
+	s.startOnce.Do(func() { s.startErr = s.start() })
+	return s.startErr
+}
+
+func (s *Server) start() error {
 	lazyInit.Do(prepareSocketPath)
 
 	wd, err := os.Getwd()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	cfg := config.Config
@@ -129,7 +136,6 @@ func Start() (*Server, error) {
 	numWorkers := cfg.Ruby.NumWorkers
 	balancer.ConfigureBuilder(numWorkers, 0)
 
-	s := &Server{}
 	for i := 0; i < numWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
 		socketPath := socketPath(i)
@@ -143,13 +149,13 @@ func Start() (*Server, error) {
 		check := func() error { return ping(socketPath) }
 		p, err := supervisor.New(name, env, args, cfg.Ruby.Dir, cfg.Ruby.MaxRSS, events, check)
 		if err != nil {
-			return nil, err
+			return err
 		}
 
 		s.workers = append(s.workers, newWorker(p, socketPath, events, false))
 	}
 
-	return s, nil
+	return nil
 }
 
 // CommitServiceClient returns a CommitServiceClient instance that is
diff --git a/internal/service/blob/testhelper_test.go b/internal/service/blob/testhelper_test.go
index 9bb24a1ef..0b4e2dc40 100644
--- a/internal/service/blob/testhelper_test.go
+++ b/internal/service/blob/testhelper_test.go
@@ -13,7 +13,7 @@ import (
 	"google.golang.org/grpc/reflection"
 )
 
-var rubyServer *rubyserver.Server
+var rubyServer = &rubyserver.Server{}
 
 func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
@@ -22,10 +22,8 @@ func TestMain(m *testing.M) {
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	var err error
 	testhelper.ConfigureRuby()
-	rubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer rubyServer.Stop()
diff --git a/internal/service/commit/testhelper_test.go b/internal/service/commit/testhelper_test.go
index b60efbd05..4137e9d6c 100644
--- a/internal/service/commit/testhelper_test.go
+++ b/internal/service/commit/testhelper_test.go
@@ -20,14 +20,12 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var rubyServer *rubyserver.Server
+var rubyServer = &rubyserver.Server{}
 
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	var err error
-	rubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer rubyServer.Stop()
diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go
index bf86ac742..0c1af2cfd 100644
--- a/internal/service/conflicts/testhelper_test.go
+++ b/internal/service/conflicts/testhelper_test.go
@@ -18,16 +18,14 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var RubyServer *rubyserver.Server
+var RubyServer = &rubyserver.Server{}
 
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
 	hooks.Override = "/does/not/exist"
 
-	var err error
-	RubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer RubyServer.Stop()
diff --git a/internal/service/diff/testhelper_test.go b/internal/service/diff/testhelper_test.go
index 8ce35a803..67742e7e1 100644
--- a/internal/service/diff/testhelper_test.go
+++ b/internal/service/diff/testhelper_test.go
@@ -17,14 +17,12 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var rubyServer *rubyserver.Server
+var rubyServer = &rubyserver.Server{}
 
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	var err error
-	rubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer rubyServer.Stop()
diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go
index ae731da2d..da3e9209b 100644
--- a/internal/service/operations/testhelper_test.go
+++ b/internal/service/operations/testhelper_test.go
@@ -23,7 +23,7 @@ var (
 	gitlabPostHooks = []string{"post-receive"}
 	GitlabPreHooks  = gitlabPreHooks
 	GitlabHooks     []string
-	RubyServer      *rubyserver.Server
+	RubyServer      = &rubyserver.Server{}
 	user            = &gitalypb.User{
 		Name:       []byte("Jane Doe"),
 		Email:      []byte("janedoe@gitlab.com"),
@@ -54,8 +54,7 @@ func testMain(m *testing.M) int {
 
 	testhelper.ConfigureGitalySSH()
 
-	RubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer RubyServer.Stop()
diff --git a/internal/service/ref/testhelper_test.go b/internal/service/ref/testhelper_test.go
index d66d2c64b..0b9abf90c 100644
--- a/internal/service/ref/testhelper_test.go
+++ b/internal/service/ref/testhelper_test.go
@@ -77,14 +77,12 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var rubyServer *rubyserver.Server
+var rubyServer = &rubyserver.Server{}
 
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	var err error
-	rubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer rubyServer.Stop()
diff --git a/internal/service/remote/testhelper_test.go b/internal/service/remote/testhelper_test.go
index 111710a4e..b939bf3e1 100644
--- a/internal/service/remote/testhelper_test.go
+++ b/internal/service/remote/testhelper_test.go
@@ -13,7 +13,7 @@ import (
 	"google.golang.org/grpc/reflection"
 )
 
-var RubyServer *rubyserver.Server
+var RubyServer = &rubyserver.Server{}
 
 func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
@@ -24,9 +24,7 @@ func testMain(m *testing.M) int {
 
 	testhelper.ConfigureGitalySSH()
 
-	var err error
-	RubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer RubyServer.Stop()
diff --git a/internal/service/repository/testhelper_test.go b/internal/service/repository/testhelper_test.go
index 905d152ad..245310ca6 100644
--- a/internal/service/repository/testhelper_test.go
+++ b/internal/service/repository/testhelper_test.go
@@ -24,7 +24,7 @@ const testTimeString = "200601021504.05"
 
 var (
 	testTime   = time.Date(2006, 1, 2, 15, 4, 5, 0, time.UTC)
-	RubyServer *rubyserver.Server
+	RubyServer = &rubyserver.Server{}
 )
 
 func newRepositoryClient(t *testing.T, serverSocketPath string) (gitalypb.RepositoryServiceClient, *grpc.ClientConn) {
@@ -94,8 +94,7 @@ func testMain(m *testing.M) int {
 
 	testhelper.ConfigureGitalySSH()
 
-	RubyServer, err = rubyserver.Start()
-	if err != nil {
+	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer RubyServer.Stop()
diff --git a/internal/service/wiki/testhelper_test.go b/internal/service/wiki/testhelper_test.go
index 28196a0b6..3aa24a065 100644
--- a/internal/service/wiki/testhelper_test.go
+++ b/internal/service/wiki/testhelper_test.go
@@ -35,16 +35,14 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var rubyServer *rubyserver.Server
+var rubyServer = &rubyserver.Server{}
 
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
 	hooks.Override = "/does/not/exist"
 
-	var err error
-	rubyServer, err = rubyserver.Start() // Do not use := because rubyServer is a global variable.
-	if err != nil {
+	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
 	defer rubyServer.Stop()