From 2e33fd1901b52ffbd09b462dc773e55049f8ca11 Mon Sep 17 00:00:00 2001 From: Richard Anderson Date: Tue, 14 Jul 2026 23:34:21 +0100 Subject: [PATCH 1/2] wip --- cmd/agent.go | 8 ++ cmd/agent_test.go | 105 ++++++++++++++++ pkg/config/config.go | 8 ++ pkg/config/config_test.go | 89 ++++++++++++++ pkg/services/services.go | 127 +++++++++++++++++++ pkg/services/services_test.go | 224 ++++++++++++++++++++++++++++++++++ 6 files changed, 561 insertions(+) create mode 100644 cmd/agent_test.go create mode 100644 pkg/config/config_test.go create mode 100644 pkg/services/services.go create mode 100644 pkg/services/services_test.go diff --git a/cmd/agent.go b/cmd/agent.go index 91b6e21..ee2542e 100644 --- a/cmd/agent.go +++ b/cmd/agent.go @@ -12,6 +12,7 @@ import ( "github.com/vitodeploy/agent/pkg/disk" "github.com/vitodeploy/agent/pkg/host" "github.com/vitodeploy/agent/pkg/memory" + "github.com/vitodeploy/agent/pkg/services" ) type Payload struct { @@ -36,6 +37,10 @@ type Payload struct { OOMKillCount int64 `json:"oom_kill_count"` UptimeSeconds float64 `json:"uptime_seconds"` RebootRequired bool `json:"reboot_required"` + + // Services is omitted entirely when no service statuses are available, which + // older Vito versions expect. + Services []services.ServiceStatus `json:"services,omitempty"` } func main() { @@ -45,6 +50,7 @@ func main() { diskInfo := disk.GetDiskInfo() memoryInfo := memory.GetMemoryInfo() hostInfo := host.GetHostInfo() + serviceStatuses := services.GetServiceStatuses(cfg.Services) payload := Payload{ Load: cpuInfo.Load, DiskTotal: diskInfo.Total, @@ -67,6 +73,8 @@ func main() { OOMKillCount: hostInfo.OOMKillCount, UptimeSeconds: hostInfo.UptimeSeconds, RebootRequired: hostInfo.RebootRequired, + + Services: serviceStatuses, } jsonPayload, err := json.Marshal(payload) if err != nil { diff --git a/cmd/agent_test.go b/cmd/agent_test.go new file mode 100644 index 0000000..6b0e106 --- /dev/null +++ b/cmd/agent_test.go @@ -0,0 +1,105 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/vitodeploy/agent/pkg/services" +) + +func samplePayload() Payload { + return Payload{ + Load: 1.2, + DiskTotal: "100M", + DiskFree: "40M", + DiskUsed: "60M", + MemoryTotal: "2000M", + MemoryFree: "500M", + MemoryUsed: "1500M", + + CPUCores: 4, + CPUPhysicalCores: 2, + CPUUsagePercent: 12.5, + CPUPerCoreUsage: []float64{1.5, 2.5}, + CPUStealPercent: 0.1, + MemoryUsedPercent: 75, + SwapTotal: "1000M", + SwapFree: "900M", + SwapUsed: "100M", + SwapUsedPercent: 10, + OOMKillCount: 3, + UptimeSeconds: 99999.5, + RebootRequired: true, + } +} + +// The marshalled payload of an agent without service statuses must stay +// byte-for-byte identical to the one previous agent versions sent. The expected +// value was captured from the agent before service reporting was added. +func TestMarshalPayloadWithoutServices(t *testing.T) { + expected := `{"load":1.2,"disk_total":"100M","disk_free":"40M","disk_used":"60M","memory_total":"2000M","memory_free":"500M","memory_used":"1500M","cpu_cores":4,"cpu_physical_cores":2,"cpu_usage_percent":12.5,"cpu_per_core_usage_percent":[1.5,2.5],"cpu_steal_percent":0.1,"memory_used_percent":75,"swap_total":"1000M","swap_free":"900M","swap_used":"100M","swap_used_percent":10,"oom_kill_count":3,"uptime_seconds":99999.5,"reboot_required":true}` + + payload, err := json.Marshal(samplePayload()) + if err != nil { + t.Fatalf("marshalling failed: %v", err) + } + + if string(payload) != expected { + t.Errorf("payload changed:\nexpected %s\ngot %s", expected, payload) + } +} + +func TestMarshalZeroPayloadWithoutServices(t *testing.T) { + expected := `{"load":0,"disk_total":"","disk_free":"","disk_used":"","memory_total":"","memory_free":"","memory_used":"","cpu_cores":0,"cpu_physical_cores":0,"cpu_usage_percent":0,"cpu_per_core_usage_percent":null,"cpu_steal_percent":0,"memory_used_percent":0,"swap_total":"","swap_free":"","swap_used":"","swap_used_percent":0,"oom_kill_count":0,"uptime_seconds":0,"reboot_required":false}` + + payload, err := json.Marshal(Payload{}) + if err != nil { + t.Fatalf("marshalling failed: %v", err) + } + + if string(payload) != expected { + t.Errorf("payload changed:\nexpected %s\ngot %s", expected, payload) + } +} + +// A nil or empty slice must omit the key entirely rather than send null or [], +// which the server reads as an agent that does not report services. +func TestMarshalPayloadOmitsServicesKey(t *testing.T) { + for name, statuses := range map[string][]services.ServiceStatus{ + "nil": nil, + "empty": {}, + } { + t.Run(name, func(t *testing.T) { + payload := samplePayload() + payload.Services = statuses + + marshalled, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshalling failed: %v", err) + } + + if strings.Contains(string(marshalled), "services") { + t.Errorf("expected no services key, got %s", marshalled) + } + }) + } +} + +func TestMarshalPayloadWithServices(t *testing.T) { + payload := samplePayload() + payload.Services = []services.ServiceStatus{ + {Id: 3, Status: "active"}, + {Id: 7, Status: "failed"}, + } + + marshalled, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshalling failed: %v", err) + } + + expected := `,"services":[{"id":3,"status":"active"},{"id":7,"status":"failed"}]}` + if !strings.HasSuffix(string(marshalled), expected) { + t.Errorf("expected payload to end with %s, got %s", expected, marshalled) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index c802d9e..712d961 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -9,9 +9,17 @@ import ( const configDir = "/etc/vito-agent" const configFile = "config.json" +type ServiceConfig struct { + Id int64 `json:"id"` + Unit string `json:"unit"` +} + type Config struct { Url string `json:"url"` Secret string `json:"secret"` + // Services is optional and is absent in configs written by older Vito versions. + // omitempty keeps the self-created default config file identical to previous versions. + Services []ServiceConfig `json:"services,omitempty"` } func GetConfig() *Config { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go new file mode 100644 index 0000000..c0c4434 --- /dev/null +++ b/pkg/config/config_test.go @@ -0,0 +1,89 @@ +package config + +import ( + "bytes" + "encoding/json" + "testing" +) + +func TestDecodeConfigWithoutServices(t *testing.T) { + config := &Config{} + input := `{"url":"https://vito.example/api/servers/1/agent/12","secret":"a-uuid"}` + + if err := json.Unmarshal([]byte(input), config); err != nil { + t.Fatalf("decoding failed: %v", err) + } + + if config.Url != "https://vito.example/api/servers/1/agent/12" { + t.Errorf("got url %q", config.Url) + } + if config.Secret != "a-uuid" { + t.Errorf("got secret %q", config.Secret) + } + if config.Services != nil { + t.Errorf("expected nil services, got %#v", config.Services) + } +} + +func TestDecodeConfigWithServices(t *testing.T) { + config := &Config{} + input := `{"url":"https://vito.example","secret":"a-uuid","services":[{"id":3,"unit":"nginx"},{"id":7,"unit":"php8.4-fpm"}]}` + + if err := json.Unmarshal([]byte(input), config); err != nil { + t.Fatalf("decoding failed: %v", err) + } + + expected := []ServiceConfig{{Id: 3, Unit: "nginx"}, {Id: 7, Unit: "php8.4-fpm"}} + if len(config.Services) != len(expected) { + t.Fatalf("expected %d services, got %d", len(expected), len(config.Services)) + } + for i, service := range expected { + if config.Services[i] != service { + t.Errorf("service %d: expected %#v, got %#v", i, service, config.Services[i]) + } + } +} + +func TestDecodeConfigWithNullServices(t *testing.T) { + config := &Config{} + input := `{"url":"https://vito.example","secret":"a-uuid","services":null}` + + if err := json.Unmarshal([]byte(input), config); err != nil { + t.Fatalf("decoding failed: %v", err) + } + + if config.Services != nil { + t.Errorf("expected nil services, got %#v", config.Services) + } +} + +// Configs written by newer Vito versions may contain keys this agent does not +// know about. +func TestDecodeConfigIgnoresUnknownFields(t *testing.T) { + config := &Config{} + input := `{"url":"https://vito.example","secret":"a-uuid","something_new":{"a":1}}` + + if err := json.Unmarshal([]byte(input), config); err != nil { + t.Fatalf("decoding failed: %v", err) + } + + if config.Url != "https://vito.example" { + t.Errorf("got url %q", config.Url) + } +} + +// The self-created default config file must stay identical to the one older +// agent versions wrote. +func TestEncodeDefaultConfig(t *testing.T) { + buffer := &bytes.Buffer{} + encoder := json.NewEncoder(buffer) + encoder.SetIndent("", " ") + if err := encoder.Encode(&Config{Url: "", Secret: ""}); err != nil { + t.Fatalf("encoding failed: %v", err) + } + + expected := "{\n \"url\": \"\",\n \"secret\": \"\"\n}\n" + if buffer.String() != expected { + t.Errorf("expected %q, got %q", expected, buffer.String()) + } +} diff --git a/pkg/services/services.go b/pkg/services/services.go new file mode 100644 index 0000000..ae4e57e --- /dev/null +++ b/pkg/services/services.go @@ -0,0 +1,127 @@ +package services + +import ( + "context" + "errors" + "fmt" + "os/exec" + "strings" + "time" + + "github.com/vitodeploy/agent/pkg/config" +) + +// The server rejects the whole request when more than maxServices entries are +// sent, or when a status is longer than maxStatusChars. +const maxServices = 100 +const maxStatusChars = 32 +const commandTimeout = 5 * time.Second + +// commandContext is a variable so tests can stub out systemctl. +var commandContext = exec.CommandContext + +type ServiceStatus struct { + Id int64 `json:"id"` + Status string `json:"status"` +} + +// GetServiceStatuses reports the raw `systemctl is-active` status of each +// configured service. It returns nil when there is nothing to report or when +// the statuses cannot be collected, so that metrics delivery is never blocked. +func GetServiceStatuses(services []config.ServiceConfig) []ServiceStatus { + entries := selectEntries(services) + if len(entries) == 0 { + return nil + } + + output, err := isActive(units(entries)) + if err != nil { + fmt.Println("Error running systemctl is-active:", err) + return nil + } + + return parseStatuses(entries, output) +} + +// selectEntries drops unusable config entries and caps the result at the number +// of services the server accepts. +func selectEntries(services []config.ServiceConfig) []config.ServiceConfig { + entries := make([]config.ServiceConfig, 0, len(services)) + for _, service := range services { + if service.Id == 0 || strings.TrimSpace(service.Unit) == "" { + continue + } + if len(entries) == maxServices { + break + } + entries = append(entries, service) + } + return entries +} + +func units(entries []config.ServiceConfig) []string { + units := make([]string, 0, len(entries)) + for _, entry := range entries { + units = append(units, entry.Unit) + } + return units +} + +// isActive returns the stdout of `systemctl is-active`, which prints one line +// per unit in argument order. A non-zero exit code only means that a unit is +// not active, so it is ignored. +func isActive(units []string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) + defer cancel() + + args := append([]string{"is-active"}, units...) + output, err := commandContext(ctx, "systemctl", args...).Output() + if ctx.Err() != nil { + return nil, ctx.Err() + } + var exitErr *exec.ExitError + if err != nil && !errors.As(err, &exitErr) { + return nil, err + } + + return output, nil +} + +// parseStatuses pairs each line of the is-active output with the entry it was +// requested for. The whole collection is discarded when the line count does not +// match the unit count, rather than risking a misaligned status. +func parseStatuses(entries []config.ServiceConfig, output []byte) []ServiceStatus { + trimmed := strings.TrimRight(string(output), "\n") + if trimmed == "" { + fmt.Println("Unexpected output from systemctl is-active: no output") + return nil + } + + lines := strings.Split(trimmed, "\n") + if len(lines) != len(entries) { + fmt.Println("Unexpected output from systemctl is-active: expected", len(entries), "lines, got", len(lines)) + return nil + } + + statuses := make([]ServiceStatus, 0, len(entries)) + for i, entry := range entries { + status := truncate(strings.TrimSpace(lines[i])) + if status == "" { + continue + } + statuses = append(statuses, ServiceStatus{Id: entry.Id, Status: status}) + } + if len(statuses) == 0 { + return nil + } + + return statuses +} + +func truncate(status string) string { + runes := []rune(status) + if len(runes) <= maxStatusChars { + return status + } + return string(runes[:maxStatusChars]) +} diff --git a/pkg/services/services_test.go b/pkg/services/services_test.go new file mode 100644 index 0000000..538184a --- /dev/null +++ b/pkg/services/services_test.go @@ -0,0 +1,224 @@ +package services + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/vitodeploy/agent/pkg/config" +) + +// stubCommand replaces systemctl with a shell script for the duration of a test. +func stubCommand(t *testing.T, script string) { + t.Helper() + original := commandContext + commandContext = func(ctx context.Context, name string, args ...string) *exec.Cmd { + return exec.CommandContext(ctx, "sh", "-c", script) + } + t.Cleanup(func() { commandContext = original }) +} + +func TestGetServiceStatuses(t *testing.T) { + stubCommand(t, "echo active; echo failed") + + statuses := GetServiceStatuses([]config.ServiceConfig{ + {Id: 3, Unit: "nginx"}, + {Id: 7, Unit: "php8.4-fpm"}, + }) + + expected := []ServiceStatus{{Id: 3, Status: "active"}, {Id: 7, Status: "failed"}} + if len(statuses) != len(expected) { + t.Fatalf("expected %d statuses, got %#v", len(expected), statuses) + } + for i, status := range expected { + if statuses[i] != status { + t.Errorf("status %d: expected %#v, got %#v", i, status, statuses[i]) + } + } +} + +// `systemctl is-active` exits 3 when a unit is not active. The exit code carries +// no information the output does not, so it must be ignored. +func TestGetServiceStatusesIgnoresExitCode(t *testing.T) { + stubCommand(t, "echo inactive; exit 3") + + statuses := GetServiceStatuses([]config.ServiceConfig{{Id: 3, Unit: "nginx"}}) + + if len(statuses) != 1 || statuses[0] != (ServiceStatus{Id: 3, Status: "inactive"}) { + t.Fatalf("expected inactive status, got %#v", statuses) + } +} + +func TestGetServiceStatusesMissingBinary(t *testing.T) { + original := commandContext + commandContext = func(ctx context.Context, name string, args ...string) *exec.Cmd { + return exec.CommandContext(ctx, "vito-agent-no-such-binary") + } + t.Cleanup(func() { commandContext = original }) + + if statuses := GetServiceStatuses([]config.ServiceConfig{{Id: 3, Unit: "nginx"}}); statuses != nil { + t.Fatalf("expected nil statuses, got %#v", statuses) + } +} + +func TestGetServiceStatusesCountMismatch(t *testing.T) { + stubCommand(t, "echo active") + + statuses := GetServiceStatuses([]config.ServiceConfig{ + {Id: 3, Unit: "nginx"}, + {Id: 7, Unit: "php8.4-fpm"}, + }) + + if statuses != nil { + t.Fatalf("expected nil statuses, got %#v", statuses) + } +} + +func TestGetServiceStatusesNoOutput(t *testing.T) { + stubCommand(t, "true") + + if statuses := GetServiceStatuses([]config.ServiceConfig{{Id: 3, Unit: "nginx"}}); statuses != nil { + t.Fatalf("expected nil statuses, got %#v", statuses) + } +} + +func TestGetServiceStatusesWithoutConfiguredServices(t *testing.T) { + stubCommand(t, "echo active") + + if statuses := GetServiceStatuses(nil); statuses != nil { + t.Fatalf("expected nil statuses for nil config, got %#v", statuses) + } + if statuses := GetServiceStatuses([]config.ServiceConfig{}); statuses != nil { + t.Fatalf("expected nil statuses for empty config, got %#v", statuses) + } +} + +// systemctl is never run when every configured entry is unusable. +func TestGetServiceStatusesOnlyInvalidEntries(t *testing.T) { + stubCommand(t, "echo active; echo active") + + statuses := GetServiceStatuses([]config.ServiceConfig{ + {Id: 3, Unit: ""}, + {Id: 0, Unit: "nginx"}, + }) + + if statuses != nil { + t.Fatalf("expected nil statuses, got %#v", statuses) + } +} + +// The other tests stub out the command entirely, so this one runs the real +// exec path against a systemctl shim to pin down how it is invoked. Units are +// passed as separate arguments and never reach a shell, so a unit name +// containing shell metacharacters is inert. +func TestGetServiceStatusesInvokesSystemctl(t *testing.T) { + dir := t.TempDir() + argsFile := filepath.Join(dir, "args.txt") + shim := "#!/bin/sh\nfor arg in \"$@\"; do echo \"$arg\" >> " + argsFile + "; done\necho active\necho active\n" + if err := os.WriteFile(filepath.Join(dir, "systemctl"), []byte(shim), 0755); err != nil { + t.Fatalf("writing shim failed: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + statuses := GetServiceStatuses([]config.ServiceConfig{ + {Id: 3, Unit: "nginx"}, + {Id: 7, Unit: "redis; rm -rf /"}, + }) + + if len(statuses) != 2 { + t.Fatalf("expected 2 statuses, got %#v", statuses) + } + recorded, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("reading recorded args failed: %v", err) + } + expected := "is-active\nnginx\nredis; rm -rf /\n" + if string(recorded) != expected { + t.Errorf("expected args %q, got %q", expected, recorded) + } +} + +func TestSelectEntries(t *testing.T) { + entries := selectEntries([]config.ServiceConfig{ + {Id: 3, Unit: "nginx"}, + {Id: 0, Unit: "redis"}, // no id + {Id: 7, Unit: ""}, // no unit + {Id: 9, Unit: " "}, // blank unit + {Id: 11, Unit: "mysql"}, + }) + + expected := []config.ServiceConfig{{Id: 3, Unit: "nginx"}, {Id: 11, Unit: "mysql"}} + if len(entries) != len(expected) { + t.Fatalf("expected %d entries, got %#v", len(expected), entries) + } + for i, entry := range expected { + if entries[i] != entry { + t.Errorf("entry %d: expected %#v, got %#v", i, entry, entries[i]) + } + } +} + +func TestSelectEntriesCapsAtMaxServices(t *testing.T) { + services := make([]config.ServiceConfig, 150) + for i := range services { + services[i] = config.ServiceConfig{Id: int64(i + 1), Unit: "nginx"} + } + + entries := selectEntries(services) + + if len(entries) != maxServices { + t.Fatalf("expected %d entries, got %d", maxServices, len(entries)) + } + if entries[0].Id != 1 || entries[maxServices-1].Id != maxServices { + t.Errorf("expected the first %d services, got ids %d..%d", maxServices, entries[0].Id, entries[maxServices-1].Id) + } +} + +func TestParseStatusesTruncatesLongStatus(t *testing.T) { + long := strings.Repeat("a", 40) + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} + + statuses := parseStatuses(entries, []byte(long+"\n")) + + if len(statuses) != 1 { + t.Fatalf("expected 1 status, got %#v", statuses) + } + if statuses[0].Status != strings.Repeat("a", maxStatusChars) { + t.Errorf("expected status truncated to %d chars, got %q", maxStatusChars, statuses[0].Status) + } +} + +func TestParseStatusesDropsEmptyStatus(t *testing.T) { + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}, {Id: 7, Unit: "php8.4-fpm"}} + + statuses := parseStatuses(entries, []byte(" \nactive\n")) + + if len(statuses) != 1 || statuses[0] != (ServiceStatus{Id: 7, Status: "active"}) { + t.Fatalf("expected only the php8.4-fpm status, got %#v", statuses) + } +} + +// Unknown statuses are round-tripped rather than normalized; the server ignores +// what it does not understand. +func TestParseStatusesKeepsRawStatus(t *testing.T) { + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} + + statuses := parseStatuses(entries, []byte("activating\n")) + + if len(statuses) != 1 || statuses[0].Status != "activating" { + t.Fatalf("expected raw activating status, got %#v", statuses) + } +} + +func TestParseStatusesWithoutTrailingNewline(t *testing.T) { + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} + + statuses := parseStatuses(entries, []byte("active")) + + if len(statuses) != 1 || statuses[0].Status != "active" { + t.Fatalf("expected active status, got %#v", statuses) + } +} From a2364109d12c424663c1e6916dff80396c03ba74 Mon Sep 17 00:00:00 2001 From: Richard Anderson Date: Tue, 14 Jul 2026 23:41:02 +0100 Subject: [PATCH 2/2] fixes + tests --- pkg/services/services.go | 13 +++- pkg/services/services_test.go | 116 +++++++++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/pkg/services/services.go b/pkg/services/services.go index ae4e57e..e6e1198 100644 --- a/pkg/services/services.go +++ b/pkg/services/services.go @@ -15,10 +15,11 @@ import ( // sent, or when a status is longer than maxStatusChars. const maxServices = 100 const maxStatusChars = 32 -const commandTimeout = 5 * time.Second -// commandContext is a variable so tests can stub out systemctl. +// These are variables so tests can stub out systemctl and shorten the waits. var commandContext = exec.CommandContext +var commandTimeout = 5 * time.Second +var waitDelay = time.Second type ServiceStatus struct { Id int64 `json:"id"` @@ -75,7 +76,13 @@ func isActive(units []string) ([]byte, error) { defer cancel() args := append([]string{"is-active"}, units...) - output, err := commandContext(ctx, "systemctl", args...).Output() + cmd := commandContext(ctx, "systemctl", args...) + // Killing systemctl on timeout only closes the pipe once every process + // holding it has exited, so a lingering child would otherwise block the + // metrics loop forever. WaitDelay bounds that wait. + cmd.WaitDelay = waitDelay + + output, err := cmd.Output() if ctx.Err() != nil { return nil, ctx.Err() } diff --git a/pkg/services/services_test.go b/pkg/services/services_test.go index 538184a..06c4365 100644 --- a/pkg/services/services_test.go +++ b/pkg/services/services_test.go @@ -7,18 +7,24 @@ import ( "path/filepath" "strings" "testing" + "time" + "unicode/utf8" "github.com/vitodeploy/agent/pkg/config" ) -// stubCommand replaces systemctl with a shell script for the duration of a test. -func stubCommand(t *testing.T, script string) { +// stubCommand replaces systemctl with a shell script for the duration of a +// test. It returns a counter of how often the command was run. +func stubCommand(t *testing.T, script string) *int { t.Helper() + runs := 0 original := commandContext commandContext = func(ctx context.Context, name string, args ...string) *exec.Cmd { + runs++ return exec.CommandContext(ctx, "sh", "-c", script) } t.Cleanup(func() { commandContext = original }) + return &runs } func TestGetServiceStatuses(t *testing.T) { @@ -64,6 +70,52 @@ func TestGetServiceStatusesMissingBinary(t *testing.T) { } } +// shortenTimeouts keeps the timeout tests fast. +func shortenTimeouts(t *testing.T) { + t.Helper() + originalTimeout, originalWaitDelay := commandTimeout, waitDelay + commandTimeout, waitDelay = 200*time.Millisecond, 50*time.Millisecond + t.Cleanup(func() { commandTimeout, waitDelay = originalTimeout, originalWaitDelay }) +} + +// A systemctl that prints a full set of statuses and then hangs is killed on the +// deadline, which surfaces as the same *exec.ExitError an inactive unit causes. +// Its output must be discarded rather than reported as a fresh reading. +func TestGetServiceStatusesDiscardsOutputOnTimeout(t *testing.T) { + shortenTimeouts(t) + // exec so that the shell is replaced and nothing outlives the kill. + stubCommand(t, "echo active; exec sleep 30") + + statuses := GetServiceStatuses([]config.ServiceConfig{{Id: 3, Unit: "nginx"}}) + + if statuses != nil { + t.Fatalf("expected nil statuses on timeout, got %#v", statuses) + } +} + +// Killing systemctl does not close the output pipe while a child it left behind +// still holds it, so without a WaitDelay the collection would block for as long +// as that child lives and stall the metrics loop with it. +func TestGetServiceStatusesReturnsWhenSystemctlLeavesAChildBehind(t *testing.T) { + shortenTimeouts(t) + // No exec, so the shell forks and `sleep` inherits the output pipe. + stubCommand(t, "echo active; sleep 30") + + done := make(chan []ServiceStatus, 1) + go func() { + done <- GetServiceStatuses([]config.ServiceConfig{{Id: 3, Unit: "nginx"}}) + }() + + select { + case statuses := <-done: + if statuses != nil { + t.Errorf("expected nil statuses, got %#v", statuses) + } + case <-time.After(10 * time.Second): + t.Fatal("GetServiceStatuses blocked on a lingering child instead of giving up") + } +} + func TestGetServiceStatusesCountMismatch(t *testing.T) { stubCommand(t, "echo active") @@ -86,7 +138,7 @@ func TestGetServiceStatusesNoOutput(t *testing.T) { } func TestGetServiceStatusesWithoutConfiguredServices(t *testing.T) { - stubCommand(t, "echo active") + runs := stubCommand(t, "echo active") if statuses := GetServiceStatuses(nil); statuses != nil { t.Fatalf("expected nil statuses for nil config, got %#v", statuses) @@ -94,11 +146,13 @@ func TestGetServiceStatusesWithoutConfiguredServices(t *testing.T) { if statuses := GetServiceStatuses([]config.ServiceConfig{}); statuses != nil { t.Fatalf("expected nil statuses for empty config, got %#v", statuses) } + if *runs != 0 { + t.Errorf("expected systemctl not to be run, got %d runs", *runs) + } } -// systemctl is never run when every configured entry is unusable. func TestGetServiceStatusesOnlyInvalidEntries(t *testing.T) { - stubCommand(t, "echo active; echo active") + runs := stubCommand(t, "echo active; echo active") statuses := GetServiceStatuses([]config.ServiceConfig{ {Id: 3, Unit: ""}, @@ -108,6 +162,9 @@ func TestGetServiceStatusesOnlyInvalidEntries(t *testing.T) { if statuses != nil { t.Fatalf("expected nil statuses, got %#v", statuses) } + if *runs != 0 { + t.Errorf("expected systemctl not to be run, got %d runs", *runs) + } } // The other tests stub out the command entirely, so this one runs the real @@ -161,7 +218,9 @@ func TestSelectEntries(t *testing.T) { } } -func TestSelectEntriesCapsAtMaxServices(t *testing.T) { +// The server validates max:100, so this asserts the literal limit rather than +// maxServices, which would make a wrong constant assert itself correct. +func TestSelectEntriesCapsAt100(t *testing.T) { services := make([]config.ServiceConfig, 150) for i := range services { services[i] = config.ServiceConfig{Id: int64(i + 1), Unit: "nginx"} @@ -169,25 +228,43 @@ func TestSelectEntriesCapsAtMaxServices(t *testing.T) { entries := selectEntries(services) - if len(entries) != maxServices { - t.Fatalf("expected %d entries, got %d", maxServices, len(entries)) + if len(entries) != 100 { + t.Fatalf("expected 100 entries, got %d", len(entries)) } - if entries[0].Id != 1 || entries[maxServices-1].Id != maxServices { - t.Errorf("expected the first %d services, got ids %d..%d", maxServices, entries[0].Id, entries[maxServices-1].Id) + if entries[0].Id != 1 || entries[99].Id != 100 { + t.Errorf("expected the first 100 services, got ids %d..%d", entries[0].Id, entries[99].Id) } } -func TestParseStatusesTruncatesLongStatus(t *testing.T) { - long := strings.Repeat("a", 40) +// The server validates max:32 characters, so this asserts the literal limit +// rather than maxStatusChars, which would make a wrong constant assert itself +// correct. Laravel counts characters, not bytes, hence the multi-byte case. +func TestParseStatusesTruncatesLongStatusTo32(t *testing.T) { entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} - statuses := parseStatuses(entries, []byte(long+"\n")) + statuses := parseStatuses(entries, []byte(strings.Repeat("a", 40)+"\n")) if len(statuses) != 1 { t.Fatalf("expected 1 status, got %#v", statuses) } - if statuses[0].Status != strings.Repeat("a", maxStatusChars) { - t.Errorf("expected status truncated to %d chars, got %q", maxStatusChars, statuses[0].Status) + if statuses[0].Status != strings.Repeat("a", 32) { + t.Errorf("expected status truncated to 32 chars, got %q", statuses[0].Status) + } +} + +func TestParseStatusesTruncatesMultiByteStatusTo32Chars(t *testing.T) { + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} + + statuses := parseStatuses(entries, []byte(strings.Repeat("é", 40)+"\n")) + + if len(statuses) != 1 { + t.Fatalf("expected 1 status, got %#v", statuses) + } + if statuses[0].Status != strings.Repeat("é", 32) { + t.Errorf("expected status truncated to 32 chars, got %q", statuses[0].Status) + } + if count := utf8.RuneCountInString(statuses[0].Status); count != 32 { + t.Errorf("expected 32 characters, got %d", count) } } @@ -201,6 +278,15 @@ func TestParseStatusesDropsEmptyStatus(t *testing.T) { } } +// nil rather than an empty slice, so that the payload omits the key outright. +func TestParseStatusesReturnsNilWhenEveryStatusIsEmpty(t *testing.T) { + entries := []config.ServiceConfig{{Id: 3, Unit: "nginx"}} + + if statuses := parseStatuses(entries, []byte(" \n")); statuses != nil { + t.Fatalf("expected nil statuses, got %#v", statuses) + } +} + // Unknown statuses are round-tripped rather than normalized; the server ignores // what it does not understand. func TestParseStatusesKeepsRawStatus(t *testing.T) {