Skip to content

Commit

Permalink
Add timeout and retry logic for the NGINX pid file
Browse files Browse the repository at this point in the history
  • Loading branch information
ciarams87 committed Jun 12, 2023
1 parent 609beef commit ec53c9d
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 11 deletions.
36 changes: 26 additions & 10 deletions internal/nginx/runtime/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,23 @@ package runtime
import (
"context"
"fmt"
"io/fs"
"os"
"strconv"
"strings"
"syscall"
"time"
)

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

Expand All @@ -31,13 +38,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(os.Stat, os.ReadFile, pidFileTimeout)
if err != nil {
return fmt.Errorf("failed to find NGINX main process: %w", err)
}
Expand Down Expand Up @@ -65,8 +67,22 @@ func (m *ManagerImpl) Reload(ctx context.Context) error {
return nil
}

func findMainProcess(readFile readFileFunc) (int, error) {
content, err := readFile(pidFile)
func findMainProcess(checkFile checkFileFunc, readFile readFileFunc, timeout time.Duration) (int, error) {
startTime := time.Now()
deadline := startTime.Add(timeout)

fileCheck := func() (content []byte, err error) {
for time.Now().Before(deadline) {
_, err := checkFile(pidFile)
if err == nil {
return readFile(pidFile)
}
time.Sleep(1 * time.Second)
}
return nil, fmt.Errorf("timeout waiting for pid file to appear")
}

content, err := fileCheck()
if err != nil {
return 0, err
}
Expand Down
29 changes: 28 additions & 1 deletion internal/nginx/runtime/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package runtime

import (
"errors"
"io/fs"
"testing"
"time"
)

func TestFindMainProcess(t *testing.T) {
Expand All @@ -18,40 +20,65 @@ 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

tests := []struct {
readFile readFileFunc
checkFile checkFileFunc
msg string
expected int
expectError bool
}{
{
readFile: readFileFuncGen([]byte("1\n")),
checkFile: checkFileFuncGen(testFileInfo),
expected: 1,
expectError: false,
msg: "normal case",
},
{
readFile: readFileFuncGen([]byte("")),
checkFile: checkFileFuncGen(testFileInfo),
expected: 0,
expectError: true,
msg: "empty file content",
},
{
readFile: readFileFuncGen([]byte("not a number")),
checkFile: checkFileFuncGen(testFileInfo),
expected: 0,
expectError: true,
msg: "bad file content",
},
{
readFile: readFileError,
checkFile: checkFileFuncGen(testFileInfo),
expected: 0,
expectError: true,
msg: "cannot read file",
},
{
readFile: readFileFuncGen([]byte("1\n")),
checkFile: checkFileError,
expected: 0,
expectError: true,
msg: "cannot file pid file",
},
}

for _, test := range tests {
result, err := findMainProcess(test.readFile)
result, err := findMainProcess(test.checkFile, test.readFile, 1*time.Microsecond)

if result != test.expected {
t.Errorf("findMainProcess() returned %d but expected %d for case %q", result, test.expected, test.msg)
Expand Down

0 comments on commit ec53c9d

Please sign in to comment.