Use single timeout for reload retries (#1128)

* Use single context for reload retries

* Review feedback

Author: ciarams87

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
-		},
-	)
-}

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())
-			}
-		})
-	}
-}

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,20 +71,29 @@ 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 := wait.PollUntilContextCancel(
+	if err := ensureNewNginxWorkers(
 		ctx,
-		25*time.Millisecond,
-		true, /* poll immediately */
-		func(ctx context.Context) (bool, error) {
-			version, err := c.getConfigVersion()
-			return version == expectedVersion, err
-		}); err != nil {
+		childProcFile,
+		previousChildProcesses,
+		readFile,
+	); err != nil {
+		return fmt.Errorf(noNewWorkersErrFmt, expectedVersion, err)
+	}
+
+	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",
@@ -91,3 +104,38 @@ func (c *verifyClient) waitForCorrectVersion(ctx context.Context, expectedVersio
 	}
 	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,
+	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
+		},
+	)
+}