Skip to content

Commit 854cc74

Browse files
mafredrimafredri
committed
Merge branch 'main' into mafredri/recreate-template
2 parents 3ad2f50 + 289b989 commit 854cc74

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+949
-232
lines changed

‎.github/ISSUE_TEMPLATE/external_bug_report.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<!--- Provide a general summary of the issue in the Title above -->
2+
3+
## Expected Behavior
4+
5+
<!--- Tell us what should happen -->
6+
7+
## Current Behavior
8+
9+
<!--- Tell us what happens instead of the expected behavior -->

‎agent/reaper/doc.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// Package reaper contains logic for reaping subprocesses. It is
2+
// specifically used in the agent to avoid the accumulation of
3+
// zombie processes.
4+
package reaper

‎agent/reaper/reaper_stub.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
//go:build !linux
2+
3+
package reaper
4+
5+
import "github.com/hashicorp/go-reap"
6+
7+
// IsChild returns true if we're the forked process.
8+
func IsChild() bool {
9+
return false
10+
}
11+
12+
// IsInitProcess returns true if the current process's PID is 1.
13+
func IsInitProcess() bool {
14+
return false
15+
}
16+
17+
func ForkReap(pids reap.PidCh) error {
18+
return nil
19+
}

‎agent/reaper/reaper_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
//go:build linux
2+
3+
package reaper_test
4+
5+
import (
6+
"os"
7+
"os/exec"
8+
"testing"
9+
"time"
10+
11+
"github.com/hashicorp/go-reap"
12+
"github.com/stretchr/testify/require"
13+
14+
"github.com/coder/coder/agent/reaper"
15+
)
16+
17+
func TestReap(t *testing.T) {
18+
t.Parallel()
19+
20+
// Don't run the reaper test in CI. It does weird
21+
// things like forkexecing which may have unintended
22+
// consequences in CI.
23+
if _, ok := os.LookupEnv("CI"); ok {
24+
t.Skip("Detected CI, skipping reaper tests")
25+
}
26+
27+
// Because we're forkexecing these tests will try to run twice...
28+
if reaper.IsChild() {
29+
t.Skip("I'm a child!")
30+
}
31+
32+
// OK checks that's the reaper is successfully reaping
33+
// exited processes and passing the PIDs through the shared
34+
// channel.
35+
t.Run("OK", func(t *testing.T) {
36+
pids := make(reap.PidCh, 1)
37+
err := reaper.ForkReap(pids)
38+
require.NoError(t, err)
39+
40+
cmd := exec.Command("tail", "-f", "/dev/null")
41+
err = cmd.Start()
42+
require.NoError(t, err)
43+
44+
cmd2 := exec.Command("tail", "-f", "/dev/null")
45+
err = cmd2.Start()
46+
require.NoError(t, err)
47+
48+
err = cmd.Process.Kill()
49+
require.NoError(t, err)
50+
51+
err = cmd2.Process.Kill()
52+
require.NoError(t, err)
53+
54+
expectedPIDs := []int{cmd.Process.Pid, cmd2.Process.Pid}
55+
56+
deadline := time.NewTimer(time.Second * 5)
57+
for i := 0; i < len(expectedPIDs); i++ {
58+
select {
59+
case <-deadline.C:
60+
t.Fatalf("Timed out waiting for process")
61+
case pid := <-pids:
62+
require.Contains(t, expectedPIDs, pid)
63+
}
64+
}
65+
})
66+
}

‎agent/reaper/reaper_unix.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
//go:build linux
2+
3+
package reaper
4+
5+
import (
6+
"fmt"
7+
"os"
8+
"syscall"
9+
10+
"github.com/hashicorp/go-reap"
11+
"golang.org/x/xerrors"
12+
)
13+
14+
// agentEnvMark is a simple environment variable that we use as a marker
15+
// to indicated that the process is a child as opposed to the reaper.
16+
// Since we are forkexec'ing we need to be able to differentiate between
17+
// the two to avoid fork bombing ourselves.
18+
const agentEnvMark = "CODER_DO_NOT_REAP"
19+
20+
// IsChild returns true if we're the forked process.
21+
func IsChild() bool {
22+
return os.Getenv(agentEnvMark) != ""
23+
}
24+
25+
// IsInitProcess returns true if the current process's PID is 1.
26+
func IsInitProcess() bool {
27+
return os.Getpid() == 1
28+
}
29+
30+
// ForkReap spawns a goroutine that reaps children. In order to avoid
31+
// complications with spawning `exec.Commands` in the same process that
32+
// is reaping, we forkexec a child process. This prevents a race between
33+
// the reaper and an exec.Command waiting for its process to complete.
34+
// The provided 'pids' channel may be nil if the caller does not care about the
35+
// reaped children PIDs.
36+
func ForkReap(pids reap.PidCh) error {
37+
// Check if the process is the parent or the child.
38+
// If it's the child we want to skip attempting to reap.
39+
if IsChild() {
40+
return nil
41+
}
42+
43+
go reap.ReapChildren(pids, nil, nil, nil)
44+
45+
args := os.Args
46+
// This is simply done to help identify the real agent process
47+
// when viewing in something like 'ps'.
48+
args = append(args, "#Agent")
49+
50+
pwd, err := os.Getwd()
51+
if err != nil {
52+
return xerrors.Errorf("get wd: %w", err)
53+
}
54+
55+
pattrs := &syscall.ProcAttr{
56+
Dir: pwd,
57+
// Add our marker for identifying the child process.
58+
Env: append(os.Environ(), fmt.Sprintf("%s=true", agentEnvMark)),
59+
Sys: &syscall.SysProcAttr{
60+
Setsid: true,
61+
},
62+
Files: []uintptr{
63+
uintptr(syscall.Stdin),
64+
uintptr(syscall.Stdout),
65+
uintptr(syscall.Stderr),
66+
},
67+
}
68+
69+
//#nosec G204
70+
pid, _ := syscall.ForkExec(args[0], args, pattrs)
71+
72+
var wstatus syscall.WaitStatus
73+
_, err = syscall.Wait4(pid, &wstatus, 0, nil)
74+
for xerrors.Is(err, syscall.EINTR) {
75+
_, err = syscall.Wait4(pid, &wstatus, 0, nil)
76+
}
77+
78+
return nil
79+
}

‎cli/agent.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/url"
99
"os"
1010
"path/filepath"
11+
"runtime"
1112
"time"
1213

1314
"cloud.google.com/go/compute/metadata"
@@ -18,6 +19,7 @@ import (
1819
"cdr.dev/slog/sloggers/sloghuman"
1920

2021
"github.com/coder/coder/agent"
22+
"github.com/coder/coder/agent/reaper"
2123
"github.com/coder/coder/cli/cliflag"
2224
"github.com/coder/coder/codersdk"
2325
"github.com/coder/retry"
@@ -51,6 +53,23 @@ func workspaceAgent() *cobra.Command {
5153
}
5254
defer logWriter.Close()
5355
logger := slog.Make(sloghuman.Sink(cmd.ErrOrStderr()), sloghuman.Sink(logWriter)).Leveled(slog.LevelDebug)
56+
57+
isLinux := runtime.GOOS == "linux"
58+
59+
// Spawn a reaper so that we don't accumulate a ton
60+
// of zombie processes.
61+
if reaper.IsInitProcess() && !reaper.IsChild() && isLinux {
62+
logger.Info(cmd.Context(), "spawning reaper process")
63+
err := reaper.ForkReap(nil)
64+
if err != nil {
65+
logger.Error(cmd.Context(), "failed to reap", slog.Error(err))
66+
return xerrors.Errorf("fork reap: %w", err)
67+
}
68+
69+
logger.Info(cmd.Context(), "reaper process exiting")
70+
return nil
71+
}
72+
5473
client := codersdk.New(coderURL)
5574

5675
if pprofEnabled {

‎cli/parameterslist.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,23 @@ func parameterList() *cobra.Command {
4545
return xerrors.Errorf("get workspace template: %w", err)
4646
}
4747
scopeID = template.ID
48-
49-
case codersdk.ParameterScopeImportJob, "template_version":
50-
scope = string(codersdk.ParameterScopeImportJob)
48+
case codersdk.ParameterImportJob, "template_version":
49+
scope = string(codersdk.ParameterImportJob)
5150
scopeID, err = uuid.Parse(name)
5251
if err != nil {
5352
return xerrors.Errorf("%q must be a uuid for this scope type", name)
5453
}
54+
55+
// Could be a template_version id or a job id. Check for the
56+
// version id.
57+
tv, err := client.TemplateVersion(cmd.Context(), scopeID)
58+
if err == nil {
59+
scopeID = tv.Job.ID
60+
}
61+
5562
default:
5663
return xerrors.Errorf("%q is an unsupported scope, use %v", scope, []codersdk.ParameterScope{
57-
codersdk.ParameterWorkspace, codersdk.ParameterTemplate, codersdk.ParameterScopeImportJob,
64+
codersdk.ParameterWorkspace, codersdk.ParameterTemplate, codersdk.ParameterImportJob,
5865
})
5966
}
6067

0 commit comments

Comments
 (0)