Add timeout and retry logic for finding NGINX PID file (#676)

* Add timeout and retry logic for the NGINX pid file

* Add timeout and retry logic for the NGINX pid file

Author: ciarams87

internal/nginx/runtime/manager.go

@@ -2,17 +2,27 @@ package runtime
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"strconv"
 	"strings"
 	"syscall"
 	"time"
+
+	"k8s.io/apimachinery/pkg/util/wait"
 )
 
-const pidFile = "/etc/nginx/nginx.pid"
+const (
+	pidFile        = "/etc/nginx/nginx.pid"
+	pidFileTimeout = 5 * time.Second
+)
 
-type readFileFunc func(string) ([]byte, error)
+type (
+	readFileFunc  func(string) ([]byte, error)
+	checkFileFunc func(string) (fs.FileInfo, error)
+)
 
 //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager
 
@@ -31,13 +41,8 @@ func NewManagerImpl() *ManagerImpl {
 }
 
 func (m *ManagerImpl) Reload(ctx context.Context) error {
-	// FIXME(pleshakov): Before reload attempt, make sure NGINX is running.
-	// If the gateway container starts before NGINX container (which is possible),
-	// then it is possible that a reload can be attempted when NGINX is not running yet.
-	// Make sure to prevent this case, so we don't get an error.
-
 	// We find the main NGINX PID on every reload because it will change if the NGINX container is restarted.
-	pid, err := findMainProcess(os.ReadFile)
+	pid, err := findMainProcess(ctx, os.Stat, os.ReadFile, pidFileTimeout)
 	if err != nil {
 		return fmt.Errorf("failed to find NGINX main process: %w", err)
 	}
@@ -65,7 +70,33 @@ func (m *ManagerImpl) Reload(ctx context.Context) error {
 	return nil
 }
 
-func findMainProcess(readFile readFileFunc) (int, error) {
+func findMainProcess(
+	ctx context.Context,
+	checkFile checkFileFunc,
+	readFile readFileFunc,
+	timeout time.Duration,
+) (int, error) {
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
+	err := wait.PollUntilContextCancel(
+		ctx,
+		1*time.Second,
+		true, /* poll immediately */
+		func(ctx context.Context) (bool, error) {
+			_, err := checkFile(pidFile)
+			if err == nil {
+				return true, nil
+			}
+			if !errors.Is(err, fs.ErrNotExist) {
+				return false, err
+			}
+			return false, nil
+		})
+	if err != nil {
+		return 0, err
+	}
+
 	content, err := readFile(pidFile)
 	if err != nil {
 		return 0, err

internal/nginx/runtime/manager_test.go

@@ -1,8 +1,11 @@
 package runtime
 
 import (
+	"context"
 	"errors"
+	"io/fs"
 	"testing"
+	"time"
 )
 
 func TestFindMainProcess(t *testing.T) {
@@ -18,40 +21,82 @@ func TestFindMainProcess(t *testing.T) {
 		return nil, errors.New("error")
 	}
 
+	checkFileFuncGen := func(content fs.FileInfo) checkFileFunc {
+		return func(name string) (fs.FileInfo, error) {
+			if name != pidFile {
+				return nil, errors.New("error")
+			}
+			return content, nil
+		}
+	}
+	checkFileError := func(string) (fs.FileInfo, error) {
+		return nil, errors.New("error")
+	}
+	var testFileInfo fs.FileInfo
+	ctx := context.Background()
+	cancellingCtx, cancel := context.WithCancel(ctx)
+	time.AfterFunc(1*time.Millisecond, cancel)
+
 	tests := []struct {
+		ctx         context.Context
 		readFile    readFileFunc
+		checkFile   checkFileFunc
 		msg         string
 		expected    int
 		expectError bool
 	}{
 		{
+			ctx:         ctx,
 			readFile:    readFileFuncGen([]byte("1\n")),
+			checkFile:   checkFileFuncGen(testFileInfo),
 			expected:    1,
 			expectError: false,
 			msg:         "normal case",
 		},
 		{
+			ctx:         ctx,
 			readFile:    readFileFuncGen([]byte("")),
+			checkFile:   checkFileFuncGen(testFileInfo),
 			expected:    0,
 			expectError: true,
 			msg:         "empty file content",
 		},
 		{
+			ctx:         ctx,
 			readFile:    readFileFuncGen([]byte("not a number")),
+			checkFile:   checkFileFuncGen(testFileInfo),
 			expected:    0,
 			expectError: true,
 			msg:         "bad file content",
 		},
 		{
+			ctx:         ctx,
 			readFile:    readFileError,
+			checkFile:   checkFileFuncGen(testFileInfo),
 			expected:    0,
 			expectError: true,
 			msg:         "cannot read file",
 		},
+		{
+			ctx:         ctx,
+			readFile:    readFileFuncGen([]byte("1\n")),
+			checkFile:   checkFileError,
+			expected:    0,
+			expectError: true,
+			msg:         "cannot find pid file",
+		},
+		{
+			ctx:         cancellingCtx,
+			readFile:    readFileFuncGen([]byte("1\n")),
+			checkFile:   checkFileError,
+			expected:    0,
+			expectError: true,
+			msg:         "context canceled",
+		},
 	}
 
 	for _, test := range tests {
-		result, err := findMainProcess(test.readFile)
+		result, err := findMainProcess(test.ctx, test.checkFile, test.readFile, 2*time.Millisecond)
 
 		if result != test.expected {
 			t.Errorf("findMainProcess() returned %d but expected %d for case %q", result, test.expected, test.msg)