diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go index 54fcc64ba5..4c4cddda49 100644 --- a/internal/nginx/runtime/manager.go +++ b/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 diff --git a/internal/nginx/runtime/manager_test.go b/internal/nginx/runtime/manager_test.go index 8d716f8d9d..c5ff89b041 100644 --- a/internal/nginx/runtime/manager_test.go +++ b/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)