Praefect graceful stop

In order to reduce usage of global configuration
Bootstrap struct changed to accept duration as
parameter.
That was a cause of adding a new configuration setting
to praefect configuration file, because previously it
used default value of 1m configured for gitaly.

2 files changed, 15 insertions, 24 deletions

diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go
index 839906003..2c8c35d9a 100644
--- a/internal/bootstrap/bootstrap.go
+++ b/internal/bootstrap/bootstrap.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/cloudflare/tableflip"
 	log "github.com/sirupsen/logrus"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"golang.org/x/sys/unix"
 )
 
@@ -147,7 +146,7 @@ func (b *Bootstrap) Start() error {
 // Wait will signal process readiness to the parent and than wait for an exit condition
 // SIGTERM, SIGINT and a runtime error will trigger an immediate shutdown
 // in case of an upgrade there will be a grace period to complete the ongoing requests
-func (b *Bootstrap) Wait() error {
+func (b *Bootstrap) Wait(gracefulTimeout time.Duration) error {
 	signals := []os.Signal{syscall.SIGTERM, syscall.SIGINT}
 	immediateShutdown := make(chan os.Signal, len(signals))
 	signal.Notify(immediateShutdown, signals...)
@@ -163,7 +162,7 @@ func (b *Bootstrap) Wait() error {
 		// the new process signaled its readiness and we started a graceful stop
 		// however no further upgrades can be started until this process is running
 		// we set a grace period and then we force a termination.
-		waitError := b.waitGracePeriod(immediateShutdown)
+		waitError := b.waitGracePeriod(gracefulTimeout, immediateShutdown)
 
 		err = fmt.Errorf("graceful upgrade: %v", waitError)
 	case s := <-immediateShutdown:
@@ -174,8 +173,8 @@ func (b *Bootstrap) Wait() error {
 	return err
 }
 
-func (b *Bootstrap) waitGracePeriod(kill <-chan os.Signal) error {
-	log.WithField("graceful_restart_timeout", config.Config.GracefulRestartTimeout).Warn("starting grace period")
+func (b *Bootstrap) waitGracePeriod(gracefulTimeout time.Duration, kill <-chan os.Signal) error {
+	log.WithField("graceful_timeout", gracefulTimeout).Warn("starting grace period")
 
 	allServersDone := make(chan struct{})
 	go func() {
@@ -186,7 +185,7 @@ func (b *Bootstrap) waitGracePeriod(kill <-chan os.Signal) error {
 	}()
 
 	select {
-	case <-time.After(config.Config.GracefulRestartTimeout):
+	case <-time.After(gracefulTimeout):
 		return fmt.Errorf("grace period expired")
 	case <-kill:
 		return fmt.Errorf("force shutdown")
diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go
index 16e85fe65..23c49209b 100644
--- a/internal/bootstrap/bootstrap_test.go
+++ b/internal/bootstrap/bootstrap_test.go
@@ -14,11 +14,8 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
-var testConfigGracefulRestartTimeout = 2 * time.Second
-
 type mockUpgrader struct {
 	exit      chan struct{}
 	hasParent bool
@@ -107,7 +104,7 @@ func TestImmediateTerminationOnSocketError(t *testing.T) {
 	b, server := makeBootstrap(t)
 
 	waitCh := make(chan error)
-	go func() { waitCh <- b.Wait() }()
+	go func() { waitCh <- b.Wait(2 * time.Second) }()
 
 	require.NoError(t, server.listeners["tcp"].Close(), "Closing first listener")
 
@@ -124,7 +121,7 @@ func TestImmediateTerminationOnSignal(t *testing.T) {
 			done := server.slowRequest(3 * time.Minute)
 
 			waitCh := make(chan error)
-			go func() { waitCh <- b.Wait() }()
+			go func() { waitCh <- b.Wait(2 * time.Second) }()
 
 			// make sure we are inside b.Wait() or we'll kill the test suite
 			time.Sleep(100 * time.Millisecond)
@@ -148,7 +145,7 @@ func TestImmediateTerminationOnSignal(t *testing.T) {
 func TestGracefulTerminationStuck(t *testing.T) {
 	b, server := makeBootstrap(t)
 
-	err := testGracefulUpdate(t, server, b, testConfigGracefulRestartTimeout+(1*time.Second), nil)
+	err := testGracefulUpdate(t, server, b, 3*time.Second, 2*time.Second, nil)
 	require.Contains(t, err.Error(), "grace period expired")
 }
 
@@ -160,7 +157,7 @@ func TestGracefulTerminationWithSignals(t *testing.T) {
 		t.Run(sig.String(), func(t *testing.T) {
 			b, server := makeBootstrap(t)
 
-			err := testGracefulUpdate(t, server, b, 1*time.Second, func() {
+			err := testGracefulUpdate(t, server, b, 1*time.Second, 2*time.Second, func() {
 				require.NoError(t, self.Signal(sig))
 			})
 			require.Contains(t, err.Error(), "force shutdown")
@@ -178,14 +175,14 @@ func TestGracefulTerminationServerErrors(t *testing.T) {
 		require.NoError(t, server.listeners["unix"].Close())
 
 		// we start a new TCP request that if faster than the grace period
-		req := server.slowRequest(config.Config.GracefulRestartTimeout / 2)
+		req := server.slowRequest(time.Second)
 		done <- <-req
 		close(done)
 
 		server.server.Shutdown(context.Background())
 	}
 
-	err := testGracefulUpdate(t, server, b, testConfigGracefulRestartTimeout+(1*time.Second), nil)
+	err := testGracefulUpdate(t, server, b, 3*time.Second, 2*time.Second, nil)
 	require.Contains(t, err.Error(), "grace period expired")
 
 	require.NoError(t, <-done)
@@ -197,7 +194,7 @@ func TestGracefulTermination(t *testing.T) {
 	// Using server.Close we bypass the graceful shutdown faking a completed shutdown
 	b.StopAction = func() { server.server.Close() }
 
-	err := testGracefulUpdate(t, server, b, 1*time.Second, nil)
+	err := testGracefulUpdate(t, server, b, 1*time.Second, 2*time.Second, nil)
 	require.Contains(t, err.Error(), "completed")
 }
 
@@ -219,17 +216,12 @@ func TestPortReuse(t *testing.T) {
 	require.NoError(t, l.Close())
 }
 
-func testGracefulUpdate(t *testing.T, server *testServer, b *Bootstrap, waitTimeout time.Duration, duringGracePeriodCallback func()) error {
-	defer func(oldVal time.Duration) {
-		config.Config.GracefulRestartTimeout = oldVal
-	}(config.Config.GracefulRestartTimeout)
-	config.Config.GracefulRestartTimeout = testConfigGracefulRestartTimeout
-
+func testGracefulUpdate(t *testing.T, server *testServer, b *Bootstrap, waitTimeout, gracefulWait time.Duration, duringGracePeriodCallback func()) error {
 	waitCh := make(chan error)
-	go func() { waitCh <- b.Wait() }()
+	go func() { waitCh <- b.Wait(gracefulWait) }()
 
 	// Start a slow request to keep the old server from shutting down immediately.
-	req := server.slowRequest(2 * config.Config.GracefulRestartTimeout)
+	req := server.slowRequest(2 * gracefulWait)
 
 	// make sure slow request is being handled
 	time.Sleep(100 * time.Millisecond)