Add a nginxprocess package for working with NGINX processes

.testcoverage.yaml

@@ -40,9 +40,10 @@ exclude:
     - ^test/.*$
     - app.go          # app.go and main.go should be tested by integration tests.
     - main.go
-# Intentionally ignoring as GetProcesses & KillProcess arent being tested as they are wrappers
+    # ignore wrappers around gopsutil
     - internal/datasource/host
     - internal/watcher/process
+    - pkg/nginxprocess
 
 # NOTES:
 # - symbol `/` in all path regexps will be replaced by

CONTRIBUTING.md

@@ -48,6 +48,8 @@ can be merged. Your agreement signature will be safely stored by F5 and no longe
 
 <!-- ### Go Guidelines Here Linter-->
 
+* when writing [table-driven tests](https://go.dev/wiki/TableDrivenTests), add a numbered prefix to each test case name (`Test 1:`, `Test 2:`, and so on) to make it easier to see which cases failed.
+
 ### Git Guidelines
 
 * Keep a clean, concise and meaningful git commit history on your branch (within reason), rebasing locally and squashing before submitting a PR.

internal/watcher/health/nginx_health_watcher_operator.go

@@ -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 {
@@ -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
 
@@ -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
 	}

internal/watcher/health/nginx_health_watcher_operator_test.go

@@ -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"
@@ -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(),
@@ -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,

internal/watcher/instance/instance_watcher_service.go

@@ -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"
@@ -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 {

internal/watcher/instance/instance_watcher_service_test.go

@@ -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{
@@ -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)

internal/watcher/instance/nginx_process_parser.go

@@ -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"
 )
 
@@ -62,25 +62,24 @@ 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)
+	processesByPID := convertToMap(processes)
 
-	for _, nginxProcess := range nginxProcesses {
-		// Here we are determining if the nginxProcess is a worker process
-		if strings.Contains(nginxProcess.Cmd, "worker") {
+	for _, proc := range processesByPID {
+		if proc.IsWorker() {
 			// Here we are determining if the worker process has a master
-			if masterProcess, ok := nginxProcesses[nginxProcess.PPID]; ok {
+			if masterProcess, ok := processesByPID[proc.PPID]; ok {
 				workers[masterProcess.PID] = append(workers[masterProcess.PID],
-					&mpi.InstanceChild{ProcessId: nginxProcess.PID})
+					&mpi.InstanceChild{ProcessId: proc.PID})
 
 				continue
 			}
-			nginxInfo, err := npp.getInfo(ctx, nginxProcess)
+			nginxInfo, err := npp.getInfo(ctx, proc)
 			if err != nil {
-				slog.DebugContext(ctx, "Unable to get NGINX info", "pid", nginxProcess.PID, "error", err)
+				slog.DebugContext(ctx, "Unable to get NGINX info", "pid", proc.PID, "error", err)
 
 				continue
 			}
@@ -91,23 +90,23 @@ func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*model.Pro
 
 			if foundInstance, ok := instanceMap[instance.GetInstanceMeta().GetInstanceId()]; ok {
 				foundInstance.GetInstanceRuntime().InstanceChildren = append(foundInstance.GetInstanceRuntime().
-					GetInstanceChildren(), &mpi.InstanceChild{ProcessId: nginxProcess.PID})
+					GetInstanceChildren(), &mpi.InstanceChild{ProcessId: proc.PID})
 
 				continue
 			}
 
 			instance.GetInstanceRuntime().InstanceChildren = append(instance.GetInstanceRuntime().
-				GetInstanceChildren(), &mpi.InstanceChild{ProcessId: nginxProcess.PID})
+				GetInstanceChildren(), &mpi.InstanceChild{ProcessId: proc.PID})
 
 			instanceMap[instance.GetInstanceMeta().GetInstanceId()] = instance
 
 			continue
 		}
 
-		// Here we are determining if the nginxProcess is a master process
-		nginxInfo, err := npp.getInfo(ctx, nginxProcess)
+		// proc is a master process
+		nginxInfo, err := npp.getInfo(ctx, proc)
 		if err != nil {
-			slog.DebugContext(ctx, "Unable to get NGINX info", "pid", nginxProcess.PID, "error", err)
+			slog.DebugContext(ctx, "Unable to get NGINX info", "pid", proc.PID, "error", err)
 
 			continue
 		}
@@ -125,13 +124,13 @@ func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*model.Pro
 	return instanceMap
 }
 
-func (npp *NginxProcessParser) getInfo(ctx context.Context, nginxProcess *model.Process) (*Info, error) {
-	exePath := nginxProcess.Exe
+func (npp *NginxProcessParser) getInfo(ctx context.Context, proc *nginxprocess.Process) (*Info, error) {
+	exePath := proc.Exe
 
 	if exePath == "" {
 		exePath = npp.getExe(ctx)
 		if exePath == "" {
-			return nil, fmt.Errorf("unable to find NGINX exe for process %d", nginxProcess.PID)
+			return nil, fmt.Errorf("unable to find NGINX exe for process %d", proc.PID)
 		}
 	}
 
@@ -145,7 +144,7 @@ func (npp *NginxProcessParser) getInfo(ctx context.Context, nginxProcess *model.
 	nginxInfo = parseNginxVersionCommandOutput(ctx, outputBuffer)
 
 	nginxInfo.ExePath = exePath
-	nginxInfo.ProcessID = nginxProcess.PID
+	nginxInfo.ProcessID = proc.PID
 
 	loadableModules := getLoadableModules(nginxInfo)
 	nginxInfo.LoadableModules = loadableModules
@@ -382,18 +381,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 {
+	processesByPID := make(map[int32]*nginxprocess.Process)
 
 	for _, p := range processes {
-		if isNginxProcess(p.Name, p.Cmd) {
-			nginxProcesses[p.PID] = p
-		}
+		processesByPID[p.PID] = p
 	}
 
-	return nginxProcesses
-}
-
-func isNginxProcess(name, cmd string) bool {
-	return name == "nginx" && !strings.Contains(cmd, "upgrade") && strings.HasPrefix(cmd, "nginx:")
+	return processesByPID
 }