From 990e5aa900d59b3e3f8c3a5d4ccff921643fe908 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Tue, 10 Oct 2023 13:57:38 +0100 Subject: [PATCH 1/2] Use single context for reload retries --- internal/mode/static/nginx/runtime/manager.go | 44 +------- .../mode/static/nginx/runtime/manager_test.go | 78 ------------- internal/mode/static/nginx/runtime/verify.go | 49 ++++++++- .../mode/static/nginx/runtime/verify_test.go | 104 +++++++++++++++++- 4 files changed, 152 insertions(+), 123 deletions(-) diff --git a/internal/mode/static/nginx/runtime/manager.go b/internal/mode/static/nginx/runtime/manager.go index 4e64fa56f1..e206ab504d 100644 --- a/internal/mode/static/nginx/runtime/manager.go +++ b/internal/mode/static/nginx/runtime/manager.go @@ -1,7 +1,6 @@ package runtime import ( - "bytes" "context" "errors" "fmt" @@ -18,7 +17,6 @@ import ( const ( pidFile = "/var/run/nginx/nginx.pid" pidFileTimeout = 10000 * time.Millisecond - childProcsTimeout = 1000 * time.Millisecond nginxReloadTimeout = 60000 * time.Millisecond ) @@ -27,11 +25,7 @@ type ( checkFileFunc func(string) (fs.FileInfo, error) ) -var ( - noNewWorkersErrFmt = "reload unsuccessful: no new NGINX worker processes started for config version %d." + - " Please check the NGINX container logs for possible configuration issues: %w" - childProcPathFmt = "/proc/%[1]v/task/%[1]v/children" -) +var childProcPathFmt = "/proc/%[1]v/task/%[1]v/children" //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager @@ -83,18 +77,13 @@ func (m *ManagerImpl) Reload(ctx context.Context, configVersion int) error { return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", err) } - if err := ensureNewNginxWorkers( + if err = m.verifyClient.waitForCorrectVersion( ctx, + configVersion, childProcFile, previousChildProcesses, os.ReadFile, - childProcsTimeout, ); err != nil { - m.managerCollector.IncReloadErrors() - return fmt.Errorf(noNewWorkersErrFmt, configVersion, err) - } - - if err = m.verifyClient.waitForCorrectVersion(ctx, configVersion); err != nil { m.managerCollector.IncReloadErrors() return err } @@ -152,30 +141,3 @@ func findMainProcess( return pid, nil } - -func ensureNewNginxWorkers( - ctx context.Context, - childProcFile string, - previousContents []byte, - readFile readFileFunc, - timeout time.Duration, -) error { - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - - return wait.PollUntilContextCancel( - ctx, - 25*time.Millisecond, - true, /* poll immediately */ - func(ctx context.Context) (bool, error) { - content, err := readFile(childProcFile) - if err != nil { - return false, err - } - if !bytes.Equal(previousContents, content) { - return true, nil - } - return false, nil - }, - ) -} diff --git a/internal/mode/static/nginx/runtime/manager_test.go b/internal/mode/static/nginx/runtime/manager_test.go index 1b31408580..3ce479a21a 100644 --- a/internal/mode/static/nginx/runtime/manager_test.go +++ b/internal/mode/static/nginx/runtime/manager_test.go @@ -112,81 +112,3 @@ func TestFindMainProcess(t *testing.T) { }) } } - -func TestEnsureNewNginxWorkers(t *testing.T) { - previousContents := []byte("1 2 3") - newContents := []byte("4 5 6") - - readFileError := func(string) ([]byte, error) { - return nil, errors.New("error") - } - - readFilePrevious := func(string) ([]byte, error) { - return previousContents, nil - } - - readFileNew := func(string) ([]byte, error) { - return newContents, nil - } - - ctx := context.Background() - cancellingCtx, cancel := context.WithCancel(ctx) - time.AfterFunc(1*time.Millisecond, cancel) - - tests := []struct { - ctx context.Context - readFile readFileFunc - name string - previousContents []byte - expectError bool - }{ - { - ctx: ctx, - readFile: readFileNew, - previousContents: previousContents, - expectError: false, - name: "normal case", - }, - { - ctx: ctx, - readFile: readFileError, - previousContents: previousContents, - expectError: true, - name: "cannot read file", - }, - { - ctx: ctx, - readFile: readFilePrevious, - previousContents: previousContents, - expectError: true, - name: "no new workers", - }, - { - ctx: cancellingCtx, - readFile: readFilePrevious, - previousContents: previousContents, - expectError: true, - name: "context canceled", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - - err := ensureNewNginxWorkers( - test.ctx, - "/childfile", - test.previousContents, - test.readFile, - 2*time.Millisecond, - ) - - if test.expectError { - g.Expect(err).To(HaveOccurred()) - } else { - g.Expect(err).ToNot(HaveOccurred()) - } - }) - } -} diff --git a/internal/mode/static/nginx/runtime/verify.go b/internal/mode/static/nginx/runtime/verify.go index 1b5dff93d1..a965b59f75 100644 --- a/internal/mode/static/nginx/runtime/verify.go +++ b/internal/mode/static/nginx/runtime/verify.go @@ -1,6 +1,7 @@ package runtime import ( + "bytes" "context" "errors" "fmt" @@ -15,6 +16,9 @@ import ( const configVersionURI = "/var/run/nginx/nginx-config-version.sock" +var noNewWorkersErrFmt = "reload unsuccessful: no new NGINX worker processes started for config version %d." + + " Please check the NGINX container logs for possible configuration issues: %w" + // verifyClient is a client for verifying the config version. type verifyClient struct { client *http.Client @@ -67,12 +71,28 @@ func (c *verifyClient) getConfigVersion() (int, error) { return v, nil } -// waitForCorrectVersion calls the config version endpoint until it gets the expectedVersion, -// which ensures that a new worker process has been started for that config version. -func (c *verifyClient) waitForCorrectVersion(ctx context.Context, expectedVersion int) error { +// waitForCorrectVersion first ensures any new worker processes have been started, and then calls the config version +// endpoint until it gets the expectedVersion, which ensures that a new worker process has been started for that config +// version. +func (c *verifyClient) waitForCorrectVersion( + ctx context.Context, + expectedVersion int, + childProcFile string, + previousChildProcesses []byte, + readFile readFileFunc, +) error { ctx, cancel := context.WithTimeout(ctx, c.timeout) defer cancel() + if err := ensureNewNginxWorkers( + ctx, + childProcFile, + previousChildProcesses, + readFile, + ); err != nil { + return fmt.Errorf(noNewWorkersErrFmt, expectedVersion, err) + } + if err := wait.PollUntilContextCancel( ctx, 25*time.Millisecond, @@ -91,3 +111,26 @@ func (c *verifyClient) waitForCorrectVersion(ctx context.Context, expectedVersio } return nil } + +func ensureNewNginxWorkers( + ctx context.Context, + childProcFile string, + previousContents []byte, + readFile readFileFunc, +) error { + return wait.PollUntilContextCancel( + ctx, + 25*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + content, err := readFile(childProcFile) + if err != nil { + return false, err + } + if !bytes.Equal(previousContents, content) { + return true, nil + } + return false, nil + }, + ) +} diff --git a/internal/mode/static/nginx/runtime/verify_test.go b/internal/mode/static/nginx/runtime/verify_test.go index 9cd35f0bca..92f50e35c4 100644 --- a/internal/mode/static/nginx/runtime/verify_test.go +++ b/internal/mode/static/nginx/runtime/verify_test.go @@ -3,6 +3,7 @@ package runtime import ( "bytes" "context" + "errors" "io" "net/http" "testing" @@ -38,8 +39,18 @@ func TestVerifyClient(t *testing.T) { cancellingCtx, cancel := context.WithCancel(ctx) time.AfterFunc(1*time.Millisecond, cancel) + newContents := []byte("4 5 6") + + readFileNew := func(string) ([]byte, error) { + return newContents, nil + } + readFileError := func(string) ([]byte, error) { + return nil, errors.New("error") + } + tests := []struct { ctx context.Context + readFile readFileFunc name string expectedVersion int expectError bool @@ -47,18 +58,28 @@ func TestVerifyClient(t *testing.T) { { ctx: ctx, expectedVersion: 42, + readFile: readFileNew, expectError: false, name: "normal case", }, { ctx: ctx, expectedVersion: 43, + readFile: readFileNew, expectError: true, name: "wrong version", }, + { + ctx: ctx, + expectedVersion: 0, + readFile: readFileError, + expectError: true, + name: "no new workers", + }, { ctx: cancellingCtx, expectedVersion: 0, + readFile: readFileNew, expectError: true, name: "context canceled", }, @@ -68,7 +89,88 @@ func TestVerifyClient(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - err := c.waitForCorrectVersion(test.ctx, test.expectedVersion) + err := c.waitForCorrectVersion(test.ctx, test.expectedVersion, "/childfile", []byte("1 2 3"), test.readFile) + + if test.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestEnsureNewNginxWorkers(t *testing.T) { + previousContents := []byte("1 2 3") + newContents := []byte("4 5 6") + + readFileError := func(string) ([]byte, error) { + return nil, errors.New("error") + } + + readFilePrevious := func(string) ([]byte, error) { + return previousContents, nil + } + + readFileNew := func(string) ([]byte, error) { + return newContents, nil + } + + ctx := context.Background() + + cancellingCtx, cancel := context.WithCancel(ctx) + time.AfterFunc(10000*time.Millisecond, cancel) + + cancellingCtx2, cancel2 := context.WithCancel(ctx) + time.AfterFunc(1*time.Millisecond, cancel2) + + tests := []struct { + ctx context.Context + readFile readFileFunc + name string + previousContents []byte + expectError bool + }{ + { + ctx: ctx, + readFile: readFileNew, + previousContents: previousContents, + expectError: false, + name: "normal case", + }, + { + ctx: ctx, + readFile: readFileError, + previousContents: previousContents, + expectError: true, + name: "cannot read file", + }, + { + ctx: cancellingCtx, + readFile: readFilePrevious, + previousContents: previousContents, + expectError: true, + name: "no new workers", + }, + { + ctx: cancellingCtx2, + readFile: readFilePrevious, + previousContents: previousContents, + expectError: true, + name: "context canceled", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + err := ensureNewNginxWorkers( + test.ctx, + "/childfile", + test.previousContents, + test.readFile, + ) if test.expectError { g.Expect(err).To(HaveOccurred()) From 7a95c9f1d34129c3da26e46595e59df57ab320e0 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Thu, 12 Oct 2023 14:42:22 +0100 Subject: [PATCH 2/2] Review feedback --- internal/mode/static/nginx/runtime/verify.go | 21 ++++++++++++------- .../mode/static/nginx/runtime/verify_test.go | 4 ++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/internal/mode/static/nginx/runtime/verify.go b/internal/mode/static/nginx/runtime/verify.go index a965b59f75..673b4e1ec5 100644 --- a/internal/mode/static/nginx/runtime/verify.go +++ b/internal/mode/static/nginx/runtime/verify.go @@ -93,14 +93,7 @@ func (c *verifyClient) waitForCorrectVersion( return fmt.Errorf(noNewWorkersErrFmt, expectedVersion, err) } - if err := wait.PollUntilContextCancel( - ctx, - 25*time.Millisecond, - true, /* poll immediately */ - func(ctx context.Context) (bool, error) { - version, err := c.getConfigVersion() - return version == expectedVersion, err - }); err != nil { + if err := c.ensureConfigVersion(ctx, expectedVersion); err != nil { if errors.Is(err, context.DeadlineExceeded) { err = fmt.Errorf( "config version check didn't return expected version %d within the deadline", @@ -112,6 +105,18 @@ func (c *verifyClient) waitForCorrectVersion( return nil } +func (c *verifyClient) ensureConfigVersion(ctx context.Context, expectedVersion int) error { + return wait.PollUntilContextCancel( + ctx, + 25*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + version, err := c.getConfigVersion() + return version == expectedVersion, err + }, + ) +} + func ensureNewNginxWorkers( ctx context.Context, childProcFile string, diff --git a/internal/mode/static/nginx/runtime/verify_test.go b/internal/mode/static/nginx/runtime/verify_test.go index 92f50e35c4..0a5431d59c 100644 --- a/internal/mode/static/nginx/runtime/verify_test.go +++ b/internal/mode/static/nginx/runtime/verify_test.go @@ -119,7 +119,7 @@ func TestEnsureNewNginxWorkers(t *testing.T) { ctx := context.Background() cancellingCtx, cancel := context.WithCancel(ctx) - time.AfterFunc(10000*time.Millisecond, cancel) + time.AfterFunc(100*time.Millisecond, cancel) cancellingCtx2, cancel2 := context.WithCancel(ctx) time.AfterFunc(1*time.Millisecond, cancel2) @@ -150,7 +150,7 @@ func TestEnsureNewNginxWorkers(t *testing.T) { readFile: readFilePrevious, previousContents: previousContents, expectError: true, - name: "no new workers", + name: "timed out waiting for new workers", }, { ctx: cancellingCtx2,