Skip to content

Test/add runtime manager tests #2175

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

Merged
merged 28 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
11 changes: 8 additions & 3 deletions internal/mode/static/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
"context"
"errors"
"fmt"
"os"
"time"

"github.com/go-logr/logr"
ngxclient "github.com/nginxinc/nginx-plus-go-client/client"
tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry"
"github.com/prometheus/client_golang/prometheus"
"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc"
Expand Down Expand Up @@ -146,8 +146,10 @@
return fmt.Errorf("cannot clear NGINX configuration folders: %w", err)
}

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

Check warning on line 149 in internal/mode/static/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/manager.go#L149

Added line #L149 was not covered by tests

// 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 {

Check warning on line 152 in internal/mode/static/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/manager.go#L152

Added line #L152 was not covered by tests
return fmt.Errorf("NGINX is not running: %w", err)
}

Expand All @@ -156,8 +158,9 @@
handlerCollector handlerMetricsCollector = collectors.NewControllerNoopCollector()
)

var ngxPlusClient *ngxclient.NginxClient
var ngxPlusClient ngxruntime.NginxPlusClient

Check warning on line 161 in internal/mode/static/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/manager.go#L161

Added line #L161 was not covered by tests
var usageSecret *usage.Secret

if cfg.Plus {
ngxPlusClient, err = ngxruntime.CreatePlusClient()
if err != nil {
Expand Down Expand Up @@ -223,6 +226,8 @@
ngxPlusClient,
ngxruntimeCollector,
cfg.Logger.WithName("nginxRuntimeManager"),
processHandler,
ngxruntime.NewVerifyClient(ngxruntime.NginxReloadTimeout),

Check warning on line 230 in internal/mode/static/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/manager.go#L229-L230

Added lines #L229 - L230 were not covered by tests
),
statusUpdater: groupStatusUpdater,
eventRecorder: recorder,
Expand Down
10 changes: 8 additions & 2 deletions internal/mode/static/metrics/collectors/nginx.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package collectors

import (
"fmt"

"github.com/go-kit/log"
"github.com/nginxinc/nginx-plus-go-client/client"
prometheusClient "github.com/nginxinc/nginx-prometheus-exporter/client"
Expand All @@ -26,12 +28,16 @@ func NewNginxMetricsCollector(constLabels map[string]string, logger log.Logger)

// NewNginxPlusMetricsCollector creates an NginxCollector which fetches stats from NGINX Plus API over a unix socket.
func NewNginxPlusMetricsCollector(
plusClient *client.NginxClient,
plusClient runtime.NginxPlusClient,
constLabels map[string]string,
logger log.Logger,
) (prometheus.Collector, error) {
nc, ok := plusClient.(*client.NginxClient)
if !ok {
panic(fmt.Sprintf("expected *client.NginxClient, got %T", plusClient))
}
collector := nginxCollector.NewNginxPlusCollector(
plusClient,
nc,
metrics.Namespace,
nginxCollector.VariableLabelNames{},
constLabels,
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,36 @@
//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.
PidFile = "/var/run/nginx/nginx.pid"
// PidFileTimeout defines the timeout duration for accessing the PID file.
PidFileTimeout = 10000 * time.Millisecond
// 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

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 . Manager

// Manager manages the runtime of NGINX.
Expand All @@ -48,6 +66,8 @@
}

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

// 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 +108,25 @@
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 +177,31 @@
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)
//counterfeiter:generate . ProcessHandler

type ProcessHandler interface {
FindMainProcess(
ctx context.Context,
timeout time.Duration,
) (int, error)
ReadFile(file string) ([]byte, error)
Kill(pid int) error
}

type ProcessHandlerImpl struct {
readFile ReadFileFunc
checkFile CheckFileFunc
}

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 @@
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 @@
return 0, err
}

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

return pid, nil
}

func (p *ProcessHandlerImpl) ReadFile(file string) ([]byte, error) {
return p.readFile(file)

Check warning on line 242 in internal/mode/static/nginx/runtime/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/nginx/runtime/manager.go#L241-L242

Added lines #L241 - L242 were not covered by tests
}

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

Check warning on line 246 in internal/mode/static/nginx/runtime/manager.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/nginx/runtime/manager.go#L245-L246

Added lines #L245 - L246 were not covered by tests
}
Loading
Loading