Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test/add runtime manager tests #2175

Merged
merged 28 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
65f6bd3
add tests for runtime manager
miledxz Jul 7, 2024
b98942e
adding required tests update
miledxz Jul 7, 2024
266fdc0
resolving conflicts
miledxz Jul 7, 2024
676548c
adding resolving conflicts
miledxz Jul 7, 2024
88f656f
adding additional updates with Reload fnction tests
miledxz Feb 26, 2024
93157b9
adding required updates
miledxz Mar 12, 2024
df5ccbe
adding fixes
miledxz May 6, 2024
3dc727a
adding processHandler struct update
miledxz May 20, 2024
a0c0228
adding cleanup fix update
miledxz May 20, 2024
596efd3
adding gofumpt fix update
miledxz May 20, 2024
54e1efe
adding additional fix updates
miledxz May 20, 2024
39cc3d7
adding tests for runtime manager
miledxz Jun 27, 2024
7f6d1bf
resolving partionally comments in pr
miledxz Jun 29, 2024
9777319
Merge branch 'nginxinc:main' into test/add-runtime-manager-tests
miledxz Jul 7, 2024
0ecf5d1
adding first part of resolving final comments
miledxz Jul 7, 2024
ecc39c5
experimental commit for NewProcessHandlerImpl
miledxz Jul 7, 2024
948b9be
Merge branch 'nginxinc:main' into test/add-runtime-manager-tests
miledxz Jul 13, 2024
f146435
updating test desc
miledxz Jul 13, 2024
b7884d0
adding required updates regarding readFile
miledxz Jul 13, 2024
04f6e30
adding additional experimental ReadFile update
miledxz Jul 13, 2024
6963fd4
adding general fix for manager
miledxz Jul 14, 2024
547e903
adding small fix update
miledxz Aug 10, 2024
27fed56
adding manager fix update
miledxz Aug 20, 2024
8b94a05
quick upstream test update
miledxz Aug 20, 2024
eda39b2
Merge branch 'main' into test/add-runtime-manager-tests
kate-osborn Aug 20, 2024
7be17ee
experimental fix
miledxz Aug 21, 2024
1a56441
adding lint fix update
miledxz Aug 29, 2024
8913e20
Merge branch 'main' into test/add-runtime-manager-tests
sjberman Aug 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion internal/mode/static/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"os"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -144,8 +145,10 @@ func StartManager(cfg config.Config) error {
return fmt.Errorf("cannot clear NGINX configuration folders: %w", err)
}

processHandler := ngxruntime.NewProcessHandlerImpl(os.ReadFile, os.Stat)

// Ensure NGINX is running before registering metrics & starting the manager.
if err := ngxruntime.EnsureNginxRunning(ctx); err != nil {
if _, err := processHandler.FindMainProcess(ctx, ngxruntime.PidFileTimeout); err != nil {
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("NGINX is not running: %w", err)
}

Expand All @@ -156,6 +159,7 @@ func StartManager(cfg config.Config) error {

var ngxPlusClient *ngxclient.NginxClient
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
var usageSecret *usage.Secret

if cfg.Plus {
ngxPlusClient, err = ngxruntime.CreatePlusClient()
if err != nil {
Expand Down Expand Up @@ -221,6 +225,8 @@ func StartManager(cfg config.Config) error {
ngxPlusClient,
ngxruntimeCollector,
cfg.Logger.WithName("nginxRuntimeManager"),
processHandler,
ngxruntime.NewVerifyClient(ngxruntime.NginxReloadTimeout),
),
statusUpdater: groupStatusUpdater,
eventRecorder: recorder,
Expand Down
91 changes: 68 additions & 23 deletions internal/mode/static/nginx/runtime/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,52 @@ import (
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate

const (
pidFile = "/var/run/nginx/nginx.pid"
pidFileTimeout = 10000 * time.Millisecond
nginxReloadTimeout = 60000 * time.Millisecond
// PidFile specifies the location of the PID file for the Nginx process
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
PidFile = "/var/run/nginx/nginx.pid"
// PidFileTimeout defines the timeout duration for accessing the PID file
PidFileTimeout = 10000 * time.Millisecond
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
// NginxReloadTimeout sets the timeout duration for reloading the Nginx configuration
NginxReloadTimeout = 60000 * time.Millisecond
)

type (
readFileFunc func(string) ([]byte, error)
checkFileFunc func(string) (fs.FileInfo, error)
ReadFileFunc func(string) ([]byte, error)
CheckFileFunc func(string) (fs.FileInfo, error)
)

var childProcPathFmt = "/proc/%[1]v/task/%[1]v/children"

//counterfeiter:generate . nginxPlusClient
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved

type nginxPlusClient interface {
UpdateHTTPServers(
upstream string,
servers []ngxclient.UpstreamServer,
) (
added []ngxclient.UpstreamServer,
deleted []ngxclient.UpstreamServer,
updated []ngxclient.UpstreamServer,
err error,
)
GetUpstreams() (*ngxclient.Upstreams, error)
}

//counterfeiter:generate . ProcessHandler
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved

type ProcessHandler interface {
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
FindMainProcess(
ctx context.Context,
timeout time.Duration,
) (int, error)
ReadFile(file string) ([]byte, error)
Kill(pid int) error
}

type ProcessHandlerImpl struct {
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
readFile ReadFileFunc
checkFile CheckFileFunc
}

//counterfeiter:generate . Manager

// Manager manages the runtime of NGINX.
Expand All @@ -48,6 +82,8 @@ type Manager interface {
}

// MetricsCollector is an interface for the metrics of the NGINX runtime manager.
//
//counterfeiter:generate . MetricsCollector
type MetricsCollector interface {
IncReloadCount()
IncReloadErrors()
Expand All @@ -56,21 +92,25 @@ type MetricsCollector interface {

// ManagerImpl implements Manager.
type ManagerImpl struct {
verifyClient *verifyClient
processHandler ProcessHandler
metricsCollector MetricsCollector
ngxPlusClient *ngxclient.NginxClient
verifyClient nginxConfigVerifier
ngxPlusClient nginxPlusClient
logger logr.Logger
}

// NewManagerImpl creates a new ManagerImpl.
func NewManagerImpl(
ngxPlusClient *ngxclient.NginxClient,
ngxPlusClient nginxPlusClient,
collector MetricsCollector,
logger logr.Logger,
processHandler ProcessHandler,
verifyClient nginxConfigVerifier,
) *ManagerImpl {
return &ManagerImpl{
verifyClient: newVerifyClient(nginxReloadTimeout),
processHandler: processHandler,
metricsCollector: collector,
verifyClient: verifyClient,
ngxPlusClient: ngxPlusClient,
logger: logger,
}
Expand All @@ -84,25 +124,25 @@ func (m *ManagerImpl) IsPlus() bool {
func (m *ManagerImpl) Reload(ctx context.Context, configVersion int) error {
start := time.Now()
// We find the main NGINX PID on every reload because it will change if the NGINX container is restarted.
pid, err := findMainProcess(ctx, os.Stat, os.ReadFile, pidFileTimeout)
pid, err := m.processHandler.FindMainProcess(ctx, PidFileTimeout)
if err != nil {
return fmt.Errorf("failed to find NGINX main process: %w", err)
}

childProcFile := fmt.Sprintf(childProcPathFmt, pid)
previousChildProcesses, err := os.ReadFile(childProcFile)
previousChildProcesses, err := m.processHandler.ReadFile(childProcFile)
if err != nil {
return err
}

// send HUP signal to the NGINX main process reload configuration
// See https://nginx.org/en/docs/control.html
if err := syscall.Kill(pid, syscall.SIGHUP); err != nil {
if err := m.processHandler.Kill(pid); err != nil {
m.metricsCollector.IncReloadErrors()
return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", err)
}

if err = m.verifyClient.waitForCorrectVersion(
if err = m.verifyClient.WaitForCorrectVersion(
ctx,
configVersion,
childProcFile,
Expand Down Expand Up @@ -153,18 +193,15 @@ func (m *ManagerImpl) GetUpstreams() (ngxclient.Upstreams, error) {
return *upstreams, nil
}

// EnsureNginxRunning ensures NGINX is running by locating the main process.
func EnsureNginxRunning(ctx context.Context) error {
if _, err := findMainProcess(ctx, os.Stat, os.ReadFile, pidFileTimeout); err != nil {
return fmt.Errorf("failed to find NGINX main process: %w", err)
func NewProcessHandlerImpl(readFile ReadFileFunc, checkFile CheckFileFunc) *ProcessHandlerImpl {
return &ProcessHandlerImpl{
readFile: readFile,
checkFile: checkFile,
}
return nil
}

func findMainProcess(
func (p *ProcessHandlerImpl) FindMainProcess(
ctx context.Context,
checkFile checkFileFunc,
readFile readFileFunc,
timeout time.Duration,
) (int, error) {
ctx, cancel := context.WithTimeout(ctx, timeout)
Expand All @@ -175,7 +212,7 @@ func findMainProcess(
500*time.Millisecond,
true, /* poll immediately */
func(_ context.Context) (bool, error) {
_, err := checkFile(pidFile)
_, err := p.checkFile(PidFile)
if err == nil {
return true, nil
}
Expand All @@ -188,7 +225,7 @@ func findMainProcess(
return 0, err
}

content, err := readFile(pidFile)
content, err := p.readFile(PidFile)
if err != nil {
return 0, err
}
Expand All @@ -200,3 +237,11 @@ func findMainProcess(

return pid, nil
}

func (p *ProcessHandlerImpl) ReadFile(file string) ([]byte, error) {
return p.ReadFile(file)
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
}

func (p *ProcessHandlerImpl) Kill(pid int) error {
return syscall.Kill(pid, syscall.SIGHUP)
}
Loading