Skip to content

Commit edf4040

Browse files
harp-intelharp-intel
authored
refactor: enhance formMasterScript to return error and add integration tests (#537)
Signed-off-by: Harper, Jason M <[email protected]>
1 parent e3f7566 commit edf4040

File tree

2 files changed

+268
-84
lines changed

2 files changed

+268
-84
lines changed

‎internal/script/script.go‎

Lines changed: 118 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"slices"
1515
"strconv"
1616
"strings"
17+
"text/template"
1718

1819
"perfspect/internal/cpus"
1920
"perfspect/internal/target"
@@ -93,7 +94,11 @@ func RunScripts(myTarget target.Target, scripts []ScriptDefinition, ignoreScript
9394
if len(parallelScripts) > 0 {
9495
// form one master script that calls all the parallel scripts in the background
9596
masterScriptName := "parallel_master.sh"
96-
masterScript, needsElevatedPrivileges := formMasterScript(myTarget.GetTempDirectory(), parallelScripts)
97+
masterScript, needsElevatedPrivileges, err := formMasterScript(myTarget.GetTempDirectory(), parallelScripts)
98+
if err != nil {
99+
err = fmt.Errorf("error forming master script: %v", err)
100+
return nil, err
101+
}
97102
// write master script to local file
98103
masterScriptPath := path.Join(localTempDir, myTarget.GetName(), masterScriptName)
99104
err = os.WriteFile(masterScriptPath, []byte(masterScript), 0600)
@@ -277,97 +282,126 @@ func scriptNameToFilename(name string) string {
277282

278283
// formMasterScript forms a master script that runs all parallel scripts in the background, waits for them to finish, then prints the output of each script.
279284
// Return values are the master script and a boolean indicating whether the master script requires elevated privileges.
280-
func formMasterScript(targetTempDirectory string, parallelScripts []ScriptDefinition) (string, bool) {
281-
// we write the stdout and stderr from each command to temporary files and save the PID of each command
282-
// in a variable named after the script
283-
var masterScript strings.Builder
285+
func formMasterScript(targetTempDirectory string, parallelScripts []ScriptDefinition) (string, bool, error) {
286+
// data model for template
287+
type tplScript struct {
288+
Name string
289+
Sanitized string
290+
NeedsKill bool
291+
Superuser bool
292+
}
293+
data := struct {
294+
TargetTempDir string
295+
Scripts []tplScript
296+
}{}
297+
data.TargetTempDir = targetTempDirectory
298+
needsElevated := false
299+
for _, s := range parallelScripts {
300+
if s.Superuser {
301+
needsElevated = true
302+
}
303+
data.Scripts = append(data.Scripts, tplScript{
304+
Name: s.Name, Sanitized: sanitizeScriptName(s.Name), NeedsKill: s.NeedsKill, Superuser: s.Superuser,
305+
})
306+
}
307+
const masterScriptTemplate = `#!/usr/bin/env bash
308+
set -o errexit
309+
set -o pipefail
284310
285-
masterScript.WriteString("#!/bin/bash\n")
311+
script_dir={{.TargetTempDir}}
312+
cd "$script_dir"
286313
287-
// set dir var and change working directory to dir in case any of the scripts write out temporary files
288-
masterScript.WriteString(fmt.Sprintf("script_dir=%s\n", targetTempDirectory))
289-
masterScript.WriteString("cd $script_dir\n")
314+
declare -a scripts=()
315+
declare -A needs_kill=()
316+
declare -A pids=()
317+
declare -A exitcodes=()
318+
declare -A orig_names=()
290319
291-
// function to print the output of each script
292-
masterScript.WriteString("\nprint_output() {\n")
293-
for _, script := range parallelScripts {
294-
masterScript.WriteString("\techo \"<---------------------->\"\n")
295-
masterScript.WriteString(fmt.Sprintf("\techo SCRIPT NAME: %s\n", script.Name))
296-
masterScript.WriteString(fmt.Sprintf("\techo STDOUT:\n\tcat %s\n", path.Join("$script_dir", sanitizeScriptName(script.Name)+".stdout")))
297-
masterScript.WriteString(fmt.Sprintf("\techo STDERR:\n\tcat %s\n", path.Join("$script_dir", sanitizeScriptName(script.Name)+".stderr")))
298-
masterScript.WriteString(fmt.Sprintf("\techo EXIT CODE: $%s_exitcode\n", sanitizeScriptName(script.Name)))
299-
}
300-
masterScript.WriteString("}\n")
320+
{{- range .Scripts}}
321+
scripts+=({{ .Sanitized }})
322+
needs_kill[{{ .Sanitized }}]={{ if .NeedsKill }}1{{ else }}0{{ end }}
323+
orig_names[{{ .Sanitized }}]="{{ .Name }}"
324+
{{ end }}
301325
302-
// function to handle SIGINT
303-
masterScript.WriteString("\nhandle_sigint() {\n")
304-
for _, script := range parallelScripts {
305-
// send SIGINT to the child script, if it is still running
306-
masterScript.WriteString(fmt.Sprintf("\tif ps -p \"$%s_pid\" > /dev/null; then\n", sanitizeScriptName(script.Name)))
307-
masterScript.WriteString(fmt.Sprintf("\t\tkill -SIGINT $%s_pid\n", sanitizeScriptName(script.Name)))
308-
masterScript.WriteString("\tfi\n")
309-
if script.NeedsKill { // this is primarily used for scripts that start commands in the background, some of which (processwatch) doesn't respond to SIGINT as expected
310-
// if the *cmd.pid file exists, check if the process is still running
311-
masterScript.WriteString(fmt.Sprintf("\tif [ -f %s_cmd.pid ]; then\n", sanitizeScriptName(script.Name)))
312-
masterScript.WriteString(fmt.Sprintf("\t\tif ps -p $(cat %s_cmd.pid) > /dev/null; then\n", sanitizeScriptName(script.Name)))
313-
// send SIGINT to the background process first, then SIGKILL if it doesn't respond to SIGINT
314-
masterScript.WriteString(fmt.Sprintf("\t\t\tkill -SIGINT $(cat %s_cmd.pid)\n", sanitizeScriptName(script.Name)))
315-
// give the process a chance to respond to SIGINT
316-
masterScript.WriteString("\t\t\tsleep 0.5\n")
317-
// if the background process is still running, send SIGKILL
318-
masterScript.WriteString(fmt.Sprintf("\t\t\tif ps -p $(cat %s_cmd.pid) > /dev/null; then\n", sanitizeScriptName(script.Name)))
319-
masterScript.WriteString(fmt.Sprintf("\t\t\t\tkill -SIGKILL $(cat %s_cmd.pid)\n", sanitizeScriptName(script.Name)))
320-
masterScript.WriteString(fmt.Sprintf("\t\t\t\t%s_exitcode=137\n", sanitizeScriptName(script.Name))) // 137 is the exit code for SIGKILL
321-
masterScript.WriteString("\t\t\telse\n")
322-
// if the background process has exited, set the exit code to 0
323-
masterScript.WriteString(fmt.Sprintf("\t\t\t\t%s_exitcode=0\n", sanitizeScriptName(script.Name)))
324-
masterScript.WriteString("\t\t\tfi\n")
325-
masterScript.WriteString("\t\telse\n")
326-
// if the script itself has exited, set the exit code to 0
327-
masterScript.WriteString(fmt.Sprintf("\t\t\t%s_exitcode=0\n", sanitizeScriptName(script.Name)))
328-
masterScript.WriteString("\t\tfi\n")
329-
masterScript.WriteString("\telse\n")
330-
// if the *cmd.pid file doesn't exist, set the exit code to 1
331-
masterScript.WriteString(fmt.Sprintf("\t\t%s_exitcode=0\n", sanitizeScriptName(script.Name)))
332-
masterScript.WriteString("\tfi\n")
333-
} else {
334-
masterScript.WriteString(fmt.Sprintf("\twait \"$%s_pid\"\n", sanitizeScriptName(script.Name)))
335-
masterScript.WriteString(fmt.Sprintf("\t%s_exitcode=$?\n", sanitizeScriptName(script.Name)))
336-
}
337-
}
338-
masterScript.WriteString("\tprint_output\n")
339-
masterScript.WriteString("\texit 0\n")
340-
masterScript.WriteString("}\n")
326+
start_scripts() {
327+
for s in "${scripts[@]}"; do
328+
bash "$script_dir/${s}.sh" > "$script_dir/${s}.stdout" 2> "$script_dir/${s}.stderr" &
329+
pids[$s]=$!
330+
done
331+
}
332+
333+
kill_script() {
334+
local s="$1"
335+
local pid="${pids[$s]:-}"
336+
[[ -z "$pid" ]] && return 0
337+
if ! ps -p "$pid" > /dev/null 2>&1; then return 0; fi
338+
if [[ "${needs_kill[$s]}" == "1" && -f "${s}_cmd.pid" ]]; then
339+
local bgpid
340+
bgpid="$(cat "${s}_cmd.pid" 2>/dev/null || true)"
341+
if [[ -n "$bgpid" && $(ps -p "$bgpid" -o pid= 2>/dev/null) ]]; then
342+
kill -SIGINT "$bgpid" 2>/dev/null || true
343+
sleep 0.5
344+
if ps -p "$bgpid" > /dev/null 2>&1; then
345+
kill -SIGKILL "$bgpid" 2>/dev/null || true
346+
exitcodes[$s]=137
347+
else
348+
exitcodes[$s]=0
349+
fi
350+
fi
351+
else
352+
kill -SIGINT "$pid" 2>/dev/null || true
353+
wait "$pid" 2>/dev/null || true
354+
if [[ -z "${exitcodes[$s]:-}" ]]; then exitcodes[$s]=130; fi
355+
fi
356+
}
341357
342-
// call handle_sigint func when SIGINT is received
343-
masterScript.WriteString("\ntrap handle_sigint SIGINT\n")
358+
wait_for_scripts() {
359+
for s in "${scripts[@]}"; do
360+
if wait "${pids[$s]}"; then
361+
exitcodes[$s]=0
362+
else
363+
ec=$?
364+
exitcodes[$s]=$ec
365+
fi
366+
done
367+
}
344368
345-
// run all parallel scripts in the background
346-
masterScript.WriteString("\n")
347-
needsElevatedPrivileges := false
348-
for _, script := range parallelScripts {
349-
if script.Superuser {
350-
needsElevatedPrivileges = true
351-
}
352-
masterScript.WriteString(
353-
fmt.Sprintf("bash %s > %s 2>%s &\n",
354-
path.Join("$script_dir", scriptNameToFilename(script.Name)),
355-
path.Join("$script_dir", sanitizeScriptName(script.Name)+".stdout"),
356-
path.Join("$script_dir", sanitizeScriptName(script.Name)+".stderr"),
357-
),
358-
)
359-
masterScript.WriteString(fmt.Sprintf("%s_pid=$!\n", sanitizeScriptName(script.Name)))
360-
}
369+
print_summary() {
370+
for s in "${scripts[@]}"; do
371+
echo "<---------------------->"
372+
echo "SCRIPT NAME: ${orig_names[$s]}"
373+
echo "STDOUT:"; cat "$script_dir/${s}.stdout" || true
374+
echo "STDERR:"; cat "$script_dir/${s}.stderr" || true
375+
echo "EXIT CODE: ${exitcodes[$s]:-1}"
376+
done
377+
}
361378
362-
// wait for all parallel scripts to finish then print their output
363-
masterScript.WriteString("\n")
364-
for _, script := range parallelScripts {
365-
masterScript.WriteString(fmt.Sprintf("wait \"$%s_pid\"\n", sanitizeScriptName(script.Name)))
366-
masterScript.WriteString(fmt.Sprintf("%s_exitcode=$?\n", sanitizeScriptName(script.Name)))
367-
}
368-
masterScript.WriteString("\nprint_output\n")
379+
handle_sigint() {
380+
echo "Received SIGINT; attempting graceful shutdown" >&2
381+
for s in "${scripts[@]}"; do
382+
kill_script "$s"
383+
done
384+
print_summary
385+
exit 0
386+
}
387+
388+
trap handle_sigint SIGINT
369389
370-
return masterScript.String(), needsElevatedPrivileges
390+
start_scripts
391+
wait_for_scripts
392+
print_summary
393+
`
394+
tmpl, err := template.New("master").Parse(masterScriptTemplate)
395+
if err != nil {
396+
slog.Error("failed to parse master script template", slog.String("error", err.Error()))
397+
return "", needsElevated, err
398+
}
399+
var out strings.Builder
400+
if err = tmpl.Execute(&out, data); err != nil {
401+
slog.Error("failed to execute master script template", slog.String("error", err.Error()))
402+
return "", needsElevated, err
403+
}
404+
return out.String(), needsElevated, nil
371405
}
372406

373407
// parseMasterScriptOutput parses the output of the master script that runs all parallel scripts in the background.

‎internal/script/script_master_test.go‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
// Copyright (C) 2021-2025 Intel Corporation
2+
// SPDX-License-Identifier: BSD-3-Clause
3+
//
4+
// Unit tests for formMasterScript and parseMasterScriptOutput.
5+
// These tests validate:
6+
// 1. Template structure contains expected sections and sanitized names.
7+
// 2. Elevated privilege flag logic (returns true if any script is superuser).
8+
// 3. Behavior with empty script slice.
9+
// 4. Integration: executing generated master script with stub child scripts and parsing output.
10+
11+
package script
12+
13+
import (
14+
"os"
15+
"os/exec"
16+
"path/filepath"
17+
"strings"
18+
"testing"
19+
)
20+
21+
// minimal ScriptDefinition stub (using existing type) – fields used: Name, Superuser, NeedsKill.
22+
23+
func TestFormMasterScriptTemplateStructure(t *testing.T) {
24+
scripts := []ScriptDefinition{
25+
{Name: "alpha script", Superuser: false},
26+
{Name: "beta-script", Superuser: true},
27+
}
28+
master, elevated, err := formMasterScript("/tmp/targetdir", scripts)
29+
if err != nil {
30+
t.Fatalf("error forming master script: %v", err)
31+
}
32+
if !elevated {
33+
t.Fatalf("expected elevated=true when at least one script is superuser")
34+
}
35+
// Shebang
36+
if !strings.HasPrefix(master, "#!/usr/bin/env bash") {
37+
t.Errorf("master script missing shebang")
38+
}
39+
// Functions present
40+
for _, fn := range []string{"start_scripts()", "kill_script()", "wait_for_scripts()", "print_summary()", "handle_sigint()"} {
41+
if !strings.Contains(master, fn) {
42+
t.Errorf("expected function %s in master script", fn)
43+
}
44+
}
45+
// Sanitized names appear (spaces and hyphens replaced with underscores)
46+
if !strings.Contains(master, "alpha_script") || !strings.Contains(master, "beta_script") {
47+
t.Errorf("sanitized script names not found in template output")
48+
}
49+
// Mapping of original names present (orig_names associative array entries)
50+
for _, mapping := range []string{"orig_names[alpha_script]=\"alpha script\"", "orig_names[beta_script]=\"beta-script\""} {
51+
if !strings.Contains(master, mapping) {
52+
t.Errorf("expected original name mapping %q in master script", mapping)
53+
}
54+
}
55+
// Delimiter used for parsing
56+
if !strings.Contains(master, "<---------------------->") {
57+
t.Errorf("expected delimiter for parsing in master script")
58+
}
59+
}
60+
61+
func TestFormMasterScriptNeedsElevatedFlag(t *testing.T) {
62+
scripts := []ScriptDefinition{{Name: "user", Superuser: false}, {Name: "also user", Superuser: false}}
63+
_, elevated, err := formMasterScript("/tmp/dir", scripts)
64+
if err != nil {
65+
t.Fatalf("error forming master script: %v", err)
66+
}
67+
if elevated {
68+
t.Fatalf("expected elevated=false when no scripts require superuser")
69+
}
70+
}
71+
72+
func TestFormMasterScriptEmptyScripts(t *testing.T) {
73+
master, elevated, err := formMasterScript("/tmp/dir", nil)
74+
if err != nil {
75+
t.Fatalf("error forming master script: %v", err)
76+
}
77+
if elevated {
78+
t.Fatalf("expected elevated=false with empty slice")
79+
}
80+
// Should still contain core function definitions even if no scripts.
81+
if !strings.Contains(master, "start_scripts()") || !strings.Contains(master, "print_summary()") {
82+
t.Errorf("template missing expected functions for empty slice")
83+
}
84+
t.Logf("MASTER SCRIPT EMPTY:\n%s", master)
85+
// No orig_names assignments lines for empty slice.
86+
if strings.Count(master, "orig_names[") > 0 {
87+
for line := range strings.SplitSeq(master, "\n") {
88+
if strings.Contains(line, "orig_names[") && strings.Contains(line, "]=") {
89+
// assignment line detected
90+
t.Errorf("no orig_names mappings should appear for empty slice")
91+
}
92+
}
93+
}
94+
}
95+
96+
func TestFormMasterScriptExecutionIntegration(t *testing.T) {
97+
// Integration test: create temp directory, stub two child scripts, run master script, parse output.
98+
tmp := t.TempDir()
99+
scripts := []ScriptDefinition{{Name: "alpha script"}, {Name: "beta-script"}}
100+
master, elevated, err := formMasterScript(tmp, scripts)
101+
if err != nil {
102+
t.Fatalf("error forming master script: %v", err)
103+
}
104+
if elevated { // none marked superuser
105+
t.Fatalf("did not expect elevated=true for non-superuser scripts")
106+
}
107+
// Create child scripts.
108+
for _, s := range scripts {
109+
sanitized := sanitizeScriptName(s.Name)
110+
childPath := filepath.Join(tmp, sanitized+".sh")
111+
content := "#!/usr/bin/env bash\n" + "echo STDOUT-" + sanitized + "\n" + "echo STDERR-" + sanitized + " 1>&2\n"
112+
if err := os.WriteFile(childPath, []byte(content), 0o700); err != nil {
113+
t.Fatalf("failed writing child script %s: %v", childPath, err)
114+
}
115+
}
116+
// Write master script.
117+
masterPath := filepath.Join(tmp, "parallel_master.sh")
118+
if err := os.WriteFile(masterPath, []byte(master), 0o700); err != nil {
119+
t.Fatalf("failed writing master script: %v", err)
120+
}
121+
// Run master script.
122+
out, err := runLocalBash(masterPath)
123+
if err != nil {
124+
// Read master script content for debugging
125+
content, _ := os.ReadFile(masterPath)
126+
t.Fatalf("error executing master script: %v\nstdout+stderr: %s\nMASTER SCRIPT:\n%s", err, out, string(content))
127+
}
128+
parsed := parseMasterScriptOutput(out)
129+
if len(parsed) != 2 {
130+
t.Fatalf("expected 2 parsed script outputs, got %d", len(parsed))
131+
}
132+
// Validate each output.
133+
for _, p := range parsed {
134+
if p.Exitcode != 0 { // child scripts exit 0
135+
t.Errorf("expected exit code 0 for %s, got %d", p.Name, p.Exitcode)
136+
}
137+
if !strings.Contains(p.Stdout, "STDOUT-"+sanitizeScriptName(p.Name)) {
138+
t.Errorf("stdout mismatch for %s: %q", p.Name, p.Stdout)
139+
}
140+
if !strings.Contains(p.Stderr, "STDERR-"+sanitizeScriptName(p.Name)) {
141+
t.Errorf("stderr mismatch for %s: %q", p.Name, p.Stderr)
142+
}
143+
}
144+
}
145+
146+
// runLocalBash executes a bash script locally and returns combined stdout.
147+
func runLocalBash(scriptPath string) (string, error) {
148+
outBytes, err := exec.Command("bash", scriptPath).CombinedOutput() // #nosec G204
149+
return string(outBytes), err
150+
}

0 commit comments

Comments
 (0)