Skip to content

Commit

Permalink
feat: use nginxprocess
Browse files Browse the repository at this point in the history
Swap internal packages to use the shared `nginxprocess` package for
details about NGINX processes.

This is mostly switching `model.Process` for `nginxprocess.Process`,
updating tests, and using features of `nginxprocess. There is one
non-trivial change: `model.Process.Running` has been dropped, there is
no equivalent in `nginxprocess`.

`Running` was almost always true, because it was calculated immediately after
a successful call to `NewProcessWithContext`. In order for it to be
false, a process would have to end after `NewProcessWithContext`, but
before `IsRunningWithContext`, which is an incredibly tiny time
window. There is also no guarantee that the process is _still_ running
by the time callers check `Running`, so the field was a little
misleading. The system now relies solely on errors from
`NewProcessWithContext` to identify terminated processes. Worst case
we catch a dead process on the next tick of `HealthWatcherService`
  • Loading branch information
ryepup committed Jan 17, 2025
1 parent 766e251 commit fada7ab
Show file tree
Hide file tree
Showing 10 changed files with 104 additions and 178 deletions.
16 changes: 0 additions & 16 deletions internal/model/os.go

This file was deleted.

9 changes: 3 additions & 6 deletions internal/watcher/health/nginx_health_watcher_operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,12 @@ package health

import (
"context"
"errors"
"fmt"
"strings"

"github.com/shirou/gopsutil/v4/process"

mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1"
"github.com/nginx/agent/v3/internal/datasource/host/exec"
processwatcher "github.com/nginx/agent/v3/internal/watcher/process"
"github.com/nginx/agent/v3/pkg/nginxprocess"
)

type NginxHealthWatcher struct {
Expand All @@ -39,7 +36,7 @@ func (nhw *NginxHealthWatcher) Health(ctx context.Context, instance *mpi.Instanc
}

proc, err := nhw.processOperator.Process(ctx, instance.GetInstanceRuntime().GetProcessId())
if errors.Is(err, process.ErrorProcessNotRunning) {
if nginxprocess.IsNotRunningErr(err) {
health.Description = fmt.Sprintf("PID: %d is not running", instance.GetInstanceRuntime().GetProcessId())
health.InstanceHealthStatus = mpi.InstanceHealth_INSTANCE_HEALTH_STATUS_UNHEALTHY

Expand All @@ -48,7 +45,7 @@ func (nhw *NginxHealthWatcher) Health(ctx context.Context, instance *mpi.Instanc
return nil, err
}

if strings.Contains(proc.Status, "zombie") || !proc.Running {
if !proc.IsHealthy() {
health.Description = fmt.Sprintf("PID: %d is unhealthy, status: %s", proc.PID, proc.Status)
health.InstanceHealthStatus = mpi.InstanceHealth_INSTANCE_HEALTH_STATUS_UNHEALTHY
}
Expand Down
61 changes: 29 additions & 32 deletions internal/watcher/health/nginx_health_watcher_operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ import (
"fmt"
"testing"

"github.com/shirou/gopsutil/v4/process"

mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1"
"github.com/nginx/agent/v3/internal/model"
"github.com/nginx/agent/v3/internal/watcher/process/processfakes"
"github.com/nginx/agent/v3/pkg/nginxprocess"
"github.com/nginx/agent/v3/test/protos"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -30,20 +32,19 @@ func TestNginxHealthWatcherOperator_Health(t *testing.T) {
tests := []struct {
expected *mpi.InstanceHealth
instance *mpi.Instance
process *model.Process
process *nginxprocess.Process
err error
name string
}{
{
name: "Test 1: Healthy Instance",
process: &model.Process{
PID: 123,
PPID: 456,
Name: "nginx",
Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;",
Exe: "/usr/local/Cellar/nginx/1.25.3/bin/nginx",
Status: "running",
Running: true,
process: &nginxprocess.Process{
PID: instance.GetInstanceRuntime().GetProcessId(),
PPID: 456,
Name: "nginx",
Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;",
Exe: "/usr/local/Cellar/nginx/1.25.3/bin/nginx",
Status: "running",
},
expected: &mpi.InstanceHealth{
InstanceId: instance.GetInstanceMeta().GetInstanceId(),
Expand All @@ -54,38 +55,34 @@ func TestNginxHealthWatcherOperator_Health(t *testing.T) {
},
{
name: "Test 2: Unhealthy Instance, Not Running",
process: &model.Process{
PID: 123,
PPID: 456,
Name: "nginx",
Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;",
Exe: "/usr/local/Cellar/nginx/1.25.3/bin/nginx",
Status: "sleep",
Running: false,
},
expected: &mpi.InstanceHealth{
InstanceId: instance.GetInstanceMeta().GetInstanceId(),
Description: fmt.Sprintf("PID: %d is unhealthy, status: %s", 123, "sleep"),
InstanceId: instance.GetInstanceMeta().GetInstanceId(),
Description: fmt.Sprintf(
"PID: %d is not running",
instance.GetInstanceRuntime().GetProcessId(),
),
InstanceHealthStatus: mpi.InstanceHealth_INSTANCE_HEALTH_STATUS_UNHEALTHY,
},
err: nil,
err: process.ErrorProcessNotRunning,
instance: instance,
},
{
name: "Test 3: Degraded Instance, Not Children",
process: &model.Process{
PID: 123,
PPID: 456,
Name: "nginx",
Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;",
Exe: "/usr/local/Cellar/nginx/1.25.3/bin/nginx",
Status: "sleep",
Running: false,
process: &nginxprocess.Process{
PID: instance.GetInstanceRuntime().GetProcessId(),
PPID: 456,
Name: "nginx",
Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;",
Exe: "/usr/local/Cellar/nginx/1.25.3/bin/nginx",
Status: "zombie",
},
expected: &mpi.InstanceHealth{
InstanceId: instance.GetInstanceMeta().GetInstanceId(),
Description: fmt.Sprintf("PID: %d is unhealthy, status: %s, instance "+
"does not have enough children", 123, "sleep"),
Description: fmt.Sprintf(
"PID: %d is unhealthy, status: %s, instance does not have enough children",
instance.GetInstanceRuntime().GetProcessId(),
"zombie",
),
InstanceHealthStatus: mpi.InstanceHealth_INSTANCE_HEALTH_STATUS_DEGRADED,
},
err: nil,
Expand Down
3 changes: 2 additions & 1 deletion internal/watcher/instance/instance_watcher_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/nginx/agent/v3/internal/logger"
"github.com/nginx/agent/v3/internal/model"
"github.com/nginx/agent/v3/internal/watcher/process"
"github.com/nginx/agent/v3/pkg/nginxprocess"
)

const defaultAgentPath = "/run/nginx-agent"
Expand All @@ -31,7 +32,7 @@ const defaultAgentPath = "/run/nginx-agent"

type (
processParser interface {
Parse(ctx context.Context, processes []*model.Process) map[string]*mpi.Instance
Parse(ctx context.Context, processes []*nginxprocess.Process) map[string]*mpi.Instance
}

nginxConfigParser interface {
Expand Down
4 changes: 2 additions & 2 deletions internal/watcher/instance/instance_watcher_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestInstanceWatcherService_checkForUpdates(t *testing.T) {
nginxConfigContext := testModel.GetConfigContext()

fakeProcessWatcher := &processfakes.FakeProcessOperatorInterface{}
fakeProcessWatcher.ProcessesReturns([]*model.Process{}, nil)
fakeProcessWatcher.ProcessesReturns(nil, nil)

fakeProcessParser := &instancefakes.FakeProcessParser{}
fakeProcessParser.ParseReturns(map[string]*mpi.Instance{
Expand Down Expand Up @@ -131,7 +131,7 @@ func TestInstanceWatcherService_instanceUpdates(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
fakeProcessWatcher := &processfakes.FakeProcessOperatorInterface{}
fakeProcessWatcher.ProcessesReturns([]*model.Process{}, nil)
fakeProcessWatcher.ProcessesReturns(nil, nil)

fakeProcessParser := &instancefakes.FakeProcessParser{}
fakeProcessParser.ParseReturns(test.parsedInstances)
Expand Down
18 changes: 9 additions & 9 deletions internal/watcher/instance/instancefakes/fake_process_parser.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 8 additions & 14 deletions internal/watcher/instance/nginx_process_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (

mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1"
"github.com/nginx/agent/v3/internal/datasource/host/exec"
"github.com/nginx/agent/v3/internal/model"
"github.com/nginx/agent/v3/pkg/nginxprocess"
"github.com/nginx/agent/v3/pkg/uuid"
)

Expand Down Expand Up @@ -62,15 +62,15 @@ func NewNginxProcessParser() *NginxProcessParser {
// cognitive complexity of 16 because of the if statements in the for loop
// don't think can be avoided due to the need for continue
// nolint: revive
func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*model.Process) map[string]*mpi.Instance {
func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*nginxprocess.Process) map[string]*mpi.Instance {
instanceMap := make(map[string]*mpi.Instance) // key is instanceID
workers := make(map[int32][]*mpi.InstanceChild) // key is ppid of process

nginxProcesses := filterNginxProcesses(processes)
nginxProcesses := convertToMap(processes)

for _, nginxProcess := range nginxProcesses {
// Here we are determining if the nginxProcess is a worker process
if strings.Contains(nginxProcess.Cmd, "worker") {
if nginxProcess.IsWorker() {
// Here we are determining if the worker process has a master
if masterProcess, ok := nginxProcesses[nginxProcess.PPID]; ok {
workers[masterProcess.PID] = append(workers[masterProcess.PID],
Expand Down Expand Up @@ -125,7 +125,7 @@ func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*model.Pro
return instanceMap
}

func (npp *NginxProcessParser) getInfo(ctx context.Context, nginxProcess *model.Process) (*Info, error) {
func (npp *NginxProcessParser) getInfo(ctx context.Context, nginxProcess *nginxprocess.Process) (*Info, error) {
exePath := nginxProcess.Exe

if exePath == "" {
Expand Down Expand Up @@ -382,18 +382,12 @@ func readDirectory(dir, extension string) (files []string, err error) {
return files, err
}

func filterNginxProcesses(processes []*model.Process) map[int32]*model.Process {
nginxProcesses := make(map[int32]*model.Process)
func convertToMap(processes []*nginxprocess.Process) map[int32]*nginxprocess.Process {
nginxProcesses := make(map[int32]*nginxprocess.Process)

for _, p := range processes {
if isNginxProcess(p.Name, p.Cmd) {
nginxProcesses[p.PID] = p
}
nginxProcesses[p.PID] = p
}

return nginxProcesses
}

func isNginxProcess(name, cmd string) bool {
return name == "nginx" && !strings.Contains(cmd, "upgrade") && strings.HasPrefix(cmd, "nginx:")
}
Loading

0 comments on commit fada7ab

Please sign in to comment.