diff --git a/agent/agent.go b/agent/agent.go index 2d5b9a663202e..82ff9442bde3b 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -12,8 +12,6 @@ import ( "os" "os/user" "path/filepath" - "runtime" - "runtime/debug" "sort" "strconv" "strings" @@ -35,7 +33,6 @@ import ( "tailscale.com/util/clientmetric" "cdr.dev/slog" - "github.com/coder/coder/v2/agent/agentproc" "github.com/coder/coder/v2/agent/agentscripts" "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/agent/proto" @@ -82,12 +79,7 @@ type Options struct { PrometheusRegistry *prometheus.Registry ReportMetadataInterval time.Duration ServiceBannerRefreshInterval time.Duration - Syscaller agentproc.Syscaller - // ModifiedProcesses is used for testing process priority management. - ModifiedProcesses chan []*agentproc.Process - // ProcessManagementTick is used for testing process priority management. - ProcessManagementTick <-chan time.Time - BlockFileTransfer bool + BlockFileTransfer bool } type Client interface { @@ -147,10 +139,6 @@ func New(options Options) Agent { prometheusRegistry = prometheus.NewRegistry() } - if options.Syscaller == nil { - options.Syscaller = agentproc.NewSyscaller() - } - hardCtx, hardCancel := context.WithCancel(context.Background()) gracefulCtx, gracefulCancel := context.WithCancel(hardCtx) a := &agent{ @@ -178,9 +166,6 @@ func New(options Options) Agent { announcementBannersRefreshInterval: options.ServiceBannerRefreshInterval, sshMaxTimeout: options.SSHMaxTimeout, subsystems: options.Subsystems, - syscaller: options.Syscaller, - modifiedProcs: options.ModifiedProcesses, - processManagementTick: options.ProcessManagementTick, logSender: agentsdk.NewLogSender(options.Logger), blockFileTransfer: options.BlockFileTransfer, @@ -253,13 +238,7 @@ type agent struct { prometheusRegistry *prometheus.Registry // metrics are prometheus registered metrics that will be collected and // labeled in Coder with the agent + workspace. - metrics *agentMetrics - syscaller agentproc.Syscaller - - // modifiedProcs is used for testing process priority management. - modifiedProcs chan []*agentproc.Process - // processManagementTick is used for testing process priority management. - processManagementTick <-chan time.Time + metrics *agentMetrics } func (a *agent) TailnetConn() *tailnet.Conn { @@ -308,8 +287,6 @@ func (a *agent) init() { // may be happening, but regardless after the intermittent // failure, you'll want the agent to reconnect. func (a *agent) runLoop() { - go a.manageProcessPriorityUntilGracefulShutdown() - // need to keep retrying up to the hardCtx so that we can send graceful shutdown-related // messages. ctx := a.hardCtx @@ -1443,162 +1420,6 @@ func (a *agent) Collect(ctx context.Context, networkStats map[netlogtype.Connect return stats } -var prioritizedProcs = []string{"coder agent"} - -func (a *agent) manageProcessPriorityUntilGracefulShutdown() { - // process priority can stop as soon as we are gracefully shutting down - ctx := a.gracefulCtx - defer func() { - if r := recover(); r != nil { - a.logger.Critical(ctx, "recovered from panic", - slog.F("panic", r), - slog.F("stack", string(debug.Stack())), - ) - } - }() - - if val := a.environmentVariables[EnvProcPrioMgmt]; val == "" || runtime.GOOS != "linux" { - a.logger.Debug(ctx, "process priority not enabled, agent will not manage process niceness/oom_score_adj ", - slog.F("env_var", EnvProcPrioMgmt), - slog.F("value", val), - slog.F("goos", runtime.GOOS), - ) - return - } - - if a.processManagementTick == nil { - ticker := time.NewTicker(time.Second) - defer ticker.Stop() - a.processManagementTick = ticker.C - } - - oomScore := unsetOOMScore - if scoreStr, ok := a.environmentVariables[EnvProcOOMScore]; ok { - score, err := strconv.Atoi(strings.TrimSpace(scoreStr)) - if err == nil && score >= -1000 && score <= 1000 { - oomScore = score - } else { - a.logger.Error(ctx, "invalid oom score", - slog.F("min_value", -1000), - slog.F("max_value", 1000), - slog.F("value", scoreStr), - ) - } - } - - debouncer := &logDebouncer{ - logger: a.logger, - messages: map[string]time.Time{}, - interval: time.Minute, - } - - for { - procs, err := a.manageProcessPriority(ctx, debouncer, oomScore) - // Avoid spamming the logs too often. - if err != nil { - debouncer.Error(ctx, "manage process priority", - slog.Error(err), - ) - } - if a.modifiedProcs != nil { - a.modifiedProcs <- procs - } - - select { - case <-a.processManagementTick: - case <-ctx.Done(): - return - } - } -} - -// unsetOOMScore is set to an invalid OOM score to imply an unset value. -const unsetOOMScore = 1001 - -func (a *agent) manageProcessPriority(ctx context.Context, debouncer *logDebouncer, oomScore int) ([]*agentproc.Process, error) { - const ( - niceness = 10 - ) - - // We fetch the agent score each time because it's possible someone updates the - // value after it is started. - agentScore, err := a.getAgentOOMScore() - if err != nil { - agentScore = unsetOOMScore - } - if oomScore == unsetOOMScore && agentScore != unsetOOMScore { - // If the child score has not been explicitly specified we should - // set it to a score relative to the agent score. - oomScore = childOOMScore(agentScore) - } - - procs, err := agentproc.List(a.filesystem, a.syscaller) - if err != nil { - return nil, xerrors.Errorf("list: %w", err) - } - - modProcs := []*agentproc.Process{} - - for _, proc := range procs { - containsFn := func(e string) bool { - contains := strings.Contains(proc.Cmd(), e) - return contains - } - - // If the process is prioritized we should adjust - // it's oom_score_adj and avoid lowering its niceness. - if slices.ContainsFunc(prioritizedProcs, containsFn) { - continue - } - - score, niceErr := proc.Niceness(a.syscaller) - if niceErr != nil && !isBenignProcessErr(niceErr) { - debouncer.Warn(ctx, "unable to get proc niceness", - slog.F("cmd", proc.Cmd()), - slog.F("pid", proc.PID), - slog.Error(niceErr), - ) - } - - // We only want processes that don't have a nice value set - // so we don't override user nice values. - // Getpriority actually returns priority for the nice value - // which is niceness + 20, so here 20 = a niceness of 0 (aka unset). - if score != 20 { - // We don't log here since it can get spammy - continue - } - - if niceErr == nil { - err := proc.SetNiceness(a.syscaller, niceness) - if err != nil && !isBenignProcessErr(err) { - debouncer.Warn(ctx, "unable to set proc niceness", - slog.F("cmd", proc.Cmd()), - slog.F("pid", proc.PID), - slog.F("niceness", niceness), - slog.Error(err), - ) - } - } - - // If the oom score is valid and it's not already set and isn't a custom value set by another process then it's ok to update it. - if oomScore != unsetOOMScore && oomScore != proc.OOMScoreAdj && !isCustomOOMScore(agentScore, proc) { - oomScoreStr := strconv.Itoa(oomScore) - err := afero.WriteFile(a.filesystem, fmt.Sprintf("/proc/%d/oom_score_adj", proc.PID), []byte(oomScoreStr), 0o644) - if err != nil && !isBenignProcessErr(err) { - debouncer.Warn(ctx, "unable to set oom_score_adj", - slog.F("cmd", proc.Cmd()), - slog.F("pid", proc.PID), - slog.F("score", oomScoreStr), - slog.Error(err), - ) - } - } - modProcs = append(modProcs, proc) - } - return modProcs, nil -} - // isClosed returns whether the API is closed or not. func (a *agent) isClosed() bool { return a.hardCtx.Err() != nil @@ -1992,88 +1813,3 @@ func PrometheusMetricsHandler(prometheusRegistry *prometheus.Registry, logger sl } }) } - -// childOOMScore returns the oom_score_adj for a child process. It is based -// on the oom_score_adj of the agent process. -func childOOMScore(agentScore int) int { - // If the agent has a negative oom_score_adj, we set the child to 0 - // so it's treated like every other process. - if agentScore < 0 { - return 0 - } - - // If the agent is already almost at the maximum then set it to the max. - if agentScore >= 998 { - return 1000 - } - - // If the agent oom_score_adj is >=0, we set the child to slightly - // less than the maximum. If users want a different score they set it - // directly. - return 998 -} - -func (a *agent) getAgentOOMScore() (int, error) { - scoreStr, err := afero.ReadFile(a.filesystem, "/proc/self/oom_score_adj") - if err != nil { - return 0, xerrors.Errorf("read file: %w", err) - } - - score, err := strconv.Atoi(strings.TrimSpace(string(scoreStr))) - if err != nil { - return 0, xerrors.Errorf("parse int: %w", err) - } - - return score, nil -} - -// isCustomOOMScore checks to see if the oom_score_adj is not a value that would -// originate from an agent-spawned process. -func isCustomOOMScore(agentScore int, process *agentproc.Process) bool { - score := process.OOMScoreAdj - return agentScore != score && score != 1000 && score != 0 && score != 998 -} - -// logDebouncer skips writing a log for a particular message if -// it's been emitted within the given interval duration. -// It's a shoddy implementation used in one spot that should be replaced at -// some point. -type logDebouncer struct { - logger slog.Logger - messages map[string]time.Time - interval time.Duration -} - -func (l *logDebouncer) Warn(ctx context.Context, msg string, fields ...any) { - l.log(ctx, slog.LevelWarn, msg, fields...) -} - -func (l *logDebouncer) Error(ctx context.Context, msg string, fields ...any) { - l.log(ctx, slog.LevelError, msg, fields...) -} - -func (l *logDebouncer) log(ctx context.Context, level slog.Level, msg string, fields ...any) { - // This (bad) implementation assumes you wouldn't reuse the same msg - // for different levels. - if last, ok := l.messages[msg]; ok && time.Since(last) < l.interval { - return - } - switch level { - case slog.LevelWarn: - l.logger.Warn(ctx, msg, fields...) - case slog.LevelError: - l.logger.Error(ctx, msg, fields...) - } - l.messages[msg] = time.Now() -} - -func isBenignProcessErr(err error) bool { - return err != nil && - (xerrors.Is(err, os.ErrNotExist) || - xerrors.Is(err, os.ErrPermission) || - isNoSuchProcessErr(err)) -} - -func isNoSuchProcessErr(err error) bool { - return err != nil && strings.Contains(err.Error(), "no such process") -} diff --git a/agent/agent_test.go b/agent/agent_test.go index f0bd0bd8e97e4..f1dfcd8c42a02 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -21,9 +21,7 @@ import ( "runtime" "strconv" "strings" - "sync" "sync/atomic" - "syscall" "testing" "time" @@ -37,7 +35,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" - "go.uber.org/mock/gomock" "golang.org/x/crypto/ssh" "golang.org/x/exp/slices" "golang.org/x/xerrors" @@ -45,11 +42,8 @@ import ( "tailscale.com/tailcfg" "cdr.dev/slog" - "cdr.dev/slog/sloggers/sloghuman" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/agent" - "github.com/coder/coder/v2/agent/agentproc" - "github.com/coder/coder/v2/agent/agentproc/agentproctest" "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/agent/proto" @@ -2668,242 +2662,6 @@ func TestAgent_Metrics_SSH(t *testing.T) { require.NoError(t, err) } -func TestAgent_ManageProcessPriority(t *testing.T) { - t.Parallel() - - t.Run("OK", func(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skip("Skipping non-linux environment") - } - - var ( - expectedProcs = map[int32]agentproc.Process{} - fs = afero.NewMemMapFs() - syscaller = agentproctest.NewMockSyscaller(gomock.NewController(t)) - ticker = make(chan time.Time) - modProcs = make(chan []*agentproc.Process) - logger = slog.Make(sloghuman.Sink(io.Discard)) - ) - - requireFileWrite(t, fs, "/proc/self/oom_score_adj", "-500") - - // Create some processes. - for i := 0; i < 4; i++ { - // Create a prioritized process. - var proc agentproc.Process - if i == 0 { - proc = agentproctest.GenerateProcess(t, fs, - func(p *agentproc.Process) { - p.CmdLine = "./coder\x00agent\x00--no-reap" - p.PID = int32(i) - }, - ) - } else { - proc = agentproctest.GenerateProcess(t, fs, - func(p *agentproc.Process) { - // Make the cmd something similar to a prioritized - // process but differentiate the arguments. - p.CmdLine = "./coder\x00stat" - }, - ) - - syscaller.EXPECT().GetPriority(proc.PID).Return(20, nil) - syscaller.EXPECT().SetPriority(proc.PID, 10).Return(nil) - } - syscaller.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - - expectedProcs[proc.PID] = proc - } - - _, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.Syscaller = syscaller - o.ModifiedProcesses = modProcs - o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"} - o.Filesystem = fs - o.Logger = logger - o.ProcessManagementTick = ticker - }) - actualProcs := <-modProcs - require.Len(t, actualProcs, len(expectedProcs)-1) - for _, proc := range actualProcs { - requireFileEquals(t, fs, fmt.Sprintf("/proc/%d/oom_score_adj", proc.PID), "0") - } - }) - - t.Run("IgnoreCustomNice", func(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skip("Skipping non-linux environment") - } - - var ( - expectedProcs = map[int32]agentproc.Process{} - fs = afero.NewMemMapFs() - ticker = make(chan time.Time) - syscaller = agentproctest.NewMockSyscaller(gomock.NewController(t)) - modProcs = make(chan []*agentproc.Process) - logger = slog.Make(sloghuman.Sink(io.Discard)) - ) - - err := afero.WriteFile(fs, "/proc/self/oom_score_adj", []byte("0"), 0o644) - require.NoError(t, err) - - // Create some processes. - for i := 0; i < 3; i++ { - proc := agentproctest.GenerateProcess(t, fs) - syscaller.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - - if i == 0 { - // Set a random nice score. This one should not be adjusted by - // our management loop. - syscaller.EXPECT().GetPriority(proc.PID).Return(25, nil) - } else { - syscaller.EXPECT().GetPriority(proc.PID).Return(20, nil) - syscaller.EXPECT().SetPriority(proc.PID, 10).Return(nil) - } - - expectedProcs[proc.PID] = proc - } - - _, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.Syscaller = syscaller - o.ModifiedProcesses = modProcs - o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"} - o.Filesystem = fs - o.Logger = logger - o.ProcessManagementTick = ticker - }) - actualProcs := <-modProcs - // We should ignore the process with a custom nice score. - require.Len(t, actualProcs, 2) - for _, proc := range actualProcs { - _, ok := expectedProcs[proc.PID] - require.True(t, ok) - requireFileEquals(t, fs, fmt.Sprintf("/proc/%d/oom_score_adj", proc.PID), "998") - } - }) - - t.Run("CustomOOMScore", func(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skip("Skipping non-linux environment") - } - - var ( - fs = afero.NewMemMapFs() - ticker = make(chan time.Time) - syscaller = agentproctest.NewMockSyscaller(gomock.NewController(t)) - modProcs = make(chan []*agentproc.Process) - logger = slog.Make(sloghuman.Sink(io.Discard)) - ) - - err := afero.WriteFile(fs, "/proc/self/oom_score_adj", []byte("0"), 0o644) - require.NoError(t, err) - - // Create some processes. - for i := 0; i < 3; i++ { - proc := agentproctest.GenerateProcess(t, fs) - syscaller.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - syscaller.EXPECT().GetPriority(proc.PID).Return(20, nil) - syscaller.EXPECT().SetPriority(proc.PID, 10).Return(nil) - } - - _, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.Syscaller = syscaller - o.ModifiedProcesses = modProcs - o.EnvironmentVariables = map[string]string{ - agent.EnvProcPrioMgmt: "1", - agent.EnvProcOOMScore: "-567", - } - o.Filesystem = fs - o.Logger = logger - o.ProcessManagementTick = ticker - }) - actualProcs := <-modProcs - // We should ignore the process with a custom nice score. - require.Len(t, actualProcs, 3) - for _, proc := range actualProcs { - requireFileEquals(t, fs, fmt.Sprintf("/proc/%d/oom_score_adj", proc.PID), "-567") - } - }) - - t.Run("DisabledByDefault", func(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skip("Skipping non-linux environment") - } - - var ( - buf bytes.Buffer - wr = &syncWriter{ - w: &buf, - } - ) - log := slog.Make(sloghuman.Sink(wr)).Leveled(slog.LevelDebug) - - _, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.Logger = log - }) - - require.Eventually(t, func() bool { - wr.mu.Lock() - defer wr.mu.Unlock() - return strings.Contains(buf.String(), "process priority not enabled") - }, testutil.WaitLong, testutil.IntervalFast) - }) - - t.Run("DisabledForNonLinux", func(t *testing.T) { - t.Parallel() - - if runtime.GOOS == "linux" { - t.Skip("Skipping linux environment") - } - - var ( - buf bytes.Buffer - wr = &syncWriter{ - w: &buf, - } - ) - log := slog.Make(sloghuman.Sink(wr)).Leveled(slog.LevelDebug) - - _, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.Logger = log - // Try to enable it so that we can assert that non-linux - // environments are truly disabled. - o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"} - }) - require.Eventually(t, func() bool { - wr.mu.Lock() - defer wr.mu.Unlock() - - return strings.Contains(buf.String(), "process priority not enabled") - }, testutil.WaitLong, testutil.IntervalFast) - }) -} - -type syncWriter struct { - mu sync.Mutex - w io.Writer -} - -func (s *syncWriter) Write(p []byte) (int, error) { - s.mu.Lock() - defer s.mu.Unlock() - return s.w.Write(p) -} - // echoOnce accepts a single connection, reads 4 bytes and echos them back func echoOnce(t *testing.T, ll net.Listener) { t.Helper() @@ -2933,17 +2691,3 @@ func requireEcho(t *testing.T, conn net.Conn) { require.NoError(t, err) require.Equal(t, "test", string(b)) } - -func requireFileWrite(t testing.TB, fs afero.Fs, fp, data string) { - t.Helper() - err := afero.WriteFile(fs, fp, []byte(data), 0o600) - require.NoError(t, err) -} - -func requireFileEquals(t testing.TB, fs afero.Fs, fp, expect string) { - t.Helper() - actual, err := afero.ReadFile(fs, fp) - require.NoError(t, err) - - require.Equal(t, expect, string(actual)) -} diff --git a/agent/agentexec/cli_linux.go b/agent/agentexec/cli_linux.go index 4081882712a40..d729a53c826dc 100644 --- a/agent/agentexec/cli_linux.go +++ b/agent/agentexec/cli_linux.go @@ -23,7 +23,8 @@ const unset = -2000 // CLI runs the agent-exec command. It should only be called by the cli package. func CLI() error { // We lock the OS thread here to avoid a race condition where the nice priority - // we get is on a different thread from the one we set it on. + // we set gets applied to a different thread than the one we exec the provided + // command on. runtime.LockOSThread() // Nop on success but we do it anyway in case of an error. defer runtime.UnlockOSThread() @@ -68,12 +69,18 @@ func CLI() error { err = unix.Setpriority(unix.PRIO_PROCESS, 0, *nice) if err != nil { - return xerrors.Errorf("set nice score: %w", err) + // We alert the user instead of failing the command since it can be difficult to debug + // for a template admin otherwise. It's quite possible (and easy) to set an + // inappriopriate value for niceness. + printfStdErr("failed to adjust niceness to %q: %v", *nice, err) } err = writeOOMScoreAdj(*oom) if err != nil { - return xerrors.Errorf("set oom score: %w", err) + // We alert the user instead of failing the command since it can be difficult to debug + // for a template admin otherwise. It's quite possible (and easy) to set an + // inappriopriate value for oom_score_adj. + printfStdErr("failed to adjust oom score to %q: %v", *nice, err) } path, err := exec.LookPath(args[0]) @@ -143,3 +150,7 @@ func execArgs(args []string) []string { } return nil } + +func printfStdErr(format string, a ...any) { + _, _ = fmt.Fprintf(os.Stderr, "coder-agent: %s\n", fmt.Sprintf(format, a...)) +} diff --git a/agent/agentexec/exec.go b/agent/agentexec/exec.go index 253671aeebe86..fdb75b8ee4d13 100644 --- a/agent/agentexec/exec.go +++ b/agent/agentexec/exec.go @@ -10,6 +10,8 @@ import ( "strconv" "golang.org/x/xerrors" + + "github.com/coder/coder/v2/pty" ) const ( @@ -25,19 +27,39 @@ const ( // is returned. All instances of exec.Cmd should flow through this function to ensure // proper resource constraints are applied to the child process. func CommandContext(ctx context.Context, cmd string, args ...string) (*exec.Cmd, error) { + cmd, args, err := agentExecCmd(cmd, args...) + if err != nil { + return nil, xerrors.Errorf("agent exec cmd: %w", err) + } + return exec.CommandContext(ctx, cmd, args...), nil +} + +// PTYCommandContext returns an pty.Cmd that calls "coder agent-exec" prior to exec'ing +// the provided command if CODER_PROC_PRIO_MGMT is set, otherwise a normal pty.Cmd +// is returned. All instances of pty.Cmd should flow through this function to ensure +// proper resource constraints are applied to the child process. +func PTYCommandContext(ctx context.Context, cmd string, args ...string) (*pty.Cmd, error) { + cmd, args, err := agentExecCmd(cmd, args...) + if err != nil { + return nil, xerrors.Errorf("agent exec cmd: %w", err) + } + return pty.CommandContext(ctx, cmd, args...), nil +} + +func agentExecCmd(cmd string, args ...string) (string, []string, error) { _, enabled := os.LookupEnv(EnvProcPrioMgmt) if runtime.GOOS != "linux" || !enabled { - return exec.CommandContext(ctx, cmd, args...), nil + return cmd, args, nil } executable, err := os.Executable() if err != nil { - return nil, xerrors.Errorf("get executable: %w", err) + return "", nil, xerrors.Errorf("get executable: %w", err) } bin, err := filepath.EvalSymlinks(executable) if err != nil { - return nil, xerrors.Errorf("eval symlinks: %w", err) + return "", nil, xerrors.Errorf("eval symlinks: %w", err) } execArgs := []string{"agent-exec"} @@ -51,7 +73,7 @@ func CommandContext(ctx context.Context, cmd string, args ...string) (*exec.Cmd, execArgs = append(execArgs, "--", cmd) execArgs = append(execArgs, args...) - return exec.CommandContext(ctx, bin, execArgs...), nil + return bin, execArgs, nil } // envValInt searches for a key in a list of environment variables and parses it to an int. diff --git a/agent/agentproc/agentproctest/doc.go b/agent/agentproc/agentproctest/doc.go deleted file mode 100644 index 5007b36268f76..0000000000000 --- a/agent/agentproc/agentproctest/doc.go +++ /dev/null @@ -1,5 +0,0 @@ -// Package agentproctest contains utility functions -// for testing process management in the agent. -package agentproctest - -//go:generate mockgen -destination ./syscallermock.go -package agentproctest github.com/coder/coder/v2/agent/agentproc Syscaller diff --git a/agent/agentproc/agentproctest/proc.go b/agent/agentproc/agentproctest/proc.go deleted file mode 100644 index 4fa1c698b50bc..0000000000000 --- a/agent/agentproc/agentproctest/proc.go +++ /dev/null @@ -1,55 +0,0 @@ -package agentproctest - -import ( - "fmt" - "strconv" - "testing" - - "github.com/spf13/afero" - "github.com/stretchr/testify/require" - - "github.com/coder/coder/v2/agent/agentproc" - "github.com/coder/coder/v2/cryptorand" -) - -func GenerateProcess(t *testing.T, fs afero.Fs, muts ...func(*agentproc.Process)) agentproc.Process { - t.Helper() - - pid, err := cryptorand.Intn(1<<31 - 1) - require.NoError(t, err) - - arg1, err := cryptorand.String(5) - require.NoError(t, err) - - arg2, err := cryptorand.String(5) - require.NoError(t, err) - - arg3, err := cryptorand.String(5) - require.NoError(t, err) - - cmdline := fmt.Sprintf("%s\x00%s\x00%s", arg1, arg2, arg3) - - process := agentproc.Process{ - CmdLine: cmdline, - PID: int32(pid), - OOMScoreAdj: 0, - } - - for _, mut := range muts { - mut(&process) - } - - process.Dir = fmt.Sprintf("%s/%d", "/proc", process.PID) - - err = fs.MkdirAll(process.Dir, 0o555) - require.NoError(t, err) - - err = afero.WriteFile(fs, fmt.Sprintf("%s/cmdline", process.Dir), []byte(process.CmdLine), 0o444) - require.NoError(t, err) - - score := strconv.Itoa(process.OOMScoreAdj) - err = afero.WriteFile(fs, fmt.Sprintf("%s/oom_score_adj", process.Dir), []byte(score), 0o444) - require.NoError(t, err) - - return process -} diff --git a/agent/agentproc/agentproctest/syscallermock.go b/agent/agentproc/agentproctest/syscallermock.go deleted file mode 100644 index 1c8bc7e39c340..0000000000000 --- a/agent/agentproc/agentproctest/syscallermock.go +++ /dev/null @@ -1,83 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/coder/coder/v2/agent/agentproc (interfaces: Syscaller) -// -// Generated by this command: -// -// mockgen -destination ./syscallermock.go -package agentproctest github.com/coder/coder/v2/agent/agentproc Syscaller -// - -// Package agentproctest is a generated GoMock package. -package agentproctest - -import ( - reflect "reflect" - syscall "syscall" - - gomock "go.uber.org/mock/gomock" -) - -// MockSyscaller is a mock of Syscaller interface. -type MockSyscaller struct { - ctrl *gomock.Controller - recorder *MockSyscallerMockRecorder -} - -// MockSyscallerMockRecorder is the mock recorder for MockSyscaller. -type MockSyscallerMockRecorder struct { - mock *MockSyscaller -} - -// NewMockSyscaller creates a new mock instance. -func NewMockSyscaller(ctrl *gomock.Controller) *MockSyscaller { - mock := &MockSyscaller{ctrl: ctrl} - mock.recorder = &MockSyscallerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockSyscaller) EXPECT() *MockSyscallerMockRecorder { - return m.recorder -} - -// GetPriority mocks base method. -func (m *MockSyscaller) GetPriority(arg0 int32) (int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetPriority", arg0) - ret0, _ := ret[0].(int) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetPriority indicates an expected call of GetPriority. -func (mr *MockSyscallerMockRecorder) GetPriority(arg0 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPriority", reflect.TypeOf((*MockSyscaller)(nil).GetPriority), arg0) -} - -// Kill mocks base method. -func (m *MockSyscaller) Kill(arg0 int32, arg1 syscall.Signal) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Kill", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// Kill indicates an expected call of Kill. -func (mr *MockSyscallerMockRecorder) Kill(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Kill", reflect.TypeOf((*MockSyscaller)(nil).Kill), arg0, arg1) -} - -// SetPriority mocks base method. -func (m *MockSyscaller) SetPriority(arg0 int32, arg1 int) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetPriority", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// SetPriority indicates an expected call of SetPriority. -func (mr *MockSyscallerMockRecorder) SetPriority(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetPriority", reflect.TypeOf((*MockSyscaller)(nil).SetPriority), arg0, arg1) -} diff --git a/agent/agentproc/doc.go b/agent/agentproc/doc.go deleted file mode 100644 index 8b15c52c5f9fb..0000000000000 --- a/agent/agentproc/doc.go +++ /dev/null @@ -1,3 +0,0 @@ -// Package agentproc contains logic for interfacing with local -// processes running in the same context as the agent. -package agentproc diff --git a/agent/agentproc/proc_other.go b/agent/agentproc/proc_other.go deleted file mode 100644 index c57d7425d7986..0000000000000 --- a/agent/agentproc/proc_other.go +++ /dev/null @@ -1,24 +0,0 @@ -//go:build !linux -// +build !linux - -package agentproc - -import ( - "github.com/spf13/afero" -) - -func (*Process) Niceness(Syscaller) (int, error) { - return 0, errUnimplemented -} - -func (*Process) SetNiceness(Syscaller, int) error { - return errUnimplemented -} - -func (*Process) Cmd() string { - return "" -} - -func List(afero.Fs, Syscaller) ([]*Process, error) { - return nil, errUnimplemented -} diff --git a/agent/agentproc/proc_test.go b/agent/agentproc/proc_test.go deleted file mode 100644 index 0cbdb4d2bc599..0000000000000 --- a/agent/agentproc/proc_test.go +++ /dev/null @@ -1,166 +0,0 @@ -package agentproc_test - -import ( - "runtime" - "syscall" - "testing" - - "github.com/spf13/afero" - "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" - "golang.org/x/xerrors" - - "github.com/coder/coder/v2/agent/agentproc" - "github.com/coder/coder/v2/agent/agentproc/agentproctest" -) - -func TestList(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skipf("skipping non-linux environment") - } - - t.Run("OK", func(t *testing.T) { - t.Parallel() - - var ( - fs = afero.NewMemMapFs() - sc = agentproctest.NewMockSyscaller(gomock.NewController(t)) - expectedProcs = make(map[int32]agentproc.Process) - ) - - for i := 0; i < 4; i++ { - proc := agentproctest.GenerateProcess(t, fs) - expectedProcs[proc.PID] = proc - - sc.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - } - - actualProcs, err := agentproc.List(fs, sc) - require.NoError(t, err) - require.Len(t, actualProcs, len(expectedProcs)) - for _, proc := range actualProcs { - expected, ok := expectedProcs[proc.PID] - require.True(t, ok) - require.Equal(t, expected.PID, proc.PID) - require.Equal(t, expected.CmdLine, proc.CmdLine) - require.Equal(t, expected.Dir, proc.Dir) - } - }) - - t.Run("FinishedProcess", func(t *testing.T) { - t.Parallel() - - var ( - fs = afero.NewMemMapFs() - sc = agentproctest.NewMockSyscaller(gomock.NewController(t)) - expectedProcs = make(map[int32]agentproc.Process) - ) - - for i := 0; i < 3; i++ { - proc := agentproctest.GenerateProcess(t, fs) - expectedProcs[proc.PID] = proc - - sc.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - } - - // Create a process that's already finished. We're not adding - // it to the map because it should be skipped over. - proc := agentproctest.GenerateProcess(t, fs) - sc.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(xerrors.New("os: process already finished")) - - actualProcs, err := agentproc.List(fs, sc) - require.NoError(t, err) - require.Len(t, actualProcs, len(expectedProcs)) - for _, proc := range actualProcs { - expected, ok := expectedProcs[proc.PID] - require.True(t, ok) - require.Equal(t, expected.PID, proc.PID) - require.Equal(t, expected.CmdLine, proc.CmdLine) - require.Equal(t, expected.Dir, proc.Dir) - } - }) - - t.Run("NoSuchProcess", func(t *testing.T) { - t.Parallel() - - var ( - fs = afero.NewMemMapFs() - sc = agentproctest.NewMockSyscaller(gomock.NewController(t)) - expectedProcs = make(map[int32]agentproc.Process) - ) - - for i := 0; i < 3; i++ { - proc := agentproctest.GenerateProcess(t, fs) - expectedProcs[proc.PID] = proc - - sc.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(nil) - } - - // Create a process that doesn't exist. We're not adding - // it to the map because it should be skipped over. - proc := agentproctest.GenerateProcess(t, fs) - sc.EXPECT(). - Kill(proc.PID, syscall.Signal(0)). - Return(syscall.ESRCH) - - actualProcs, err := agentproc.List(fs, sc) - require.NoError(t, err) - require.Len(t, actualProcs, len(expectedProcs)) - for _, proc := range actualProcs { - expected, ok := expectedProcs[proc.PID] - require.True(t, ok) - require.Equal(t, expected.PID, proc.PID) - require.Equal(t, expected.CmdLine, proc.CmdLine) - require.Equal(t, expected.Dir, proc.Dir) - } - }) -} - -// These tests are not very interesting but they provide some modicum of -// confidence. -func TestProcess(t *testing.T) { - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skipf("skipping non-linux environment") - } - - t.Run("SetNiceness", func(t *testing.T) { - t.Parallel() - - var ( - sc = agentproctest.NewMockSyscaller(gomock.NewController(t)) - proc = &agentproc.Process{ - PID: 32, - } - score = 20 - ) - - sc.EXPECT().SetPriority(proc.PID, score).Return(nil) - err := proc.SetNiceness(sc, score) - require.NoError(t, err) - }) - - t.Run("Cmd", func(t *testing.T) { - t.Parallel() - - var ( - proc = &agentproc.Process{ - CmdLine: "helloworld\x00--arg1\x00--arg2", - } - expectedName = "helloworld --arg1 --arg2" - ) - - require.Equal(t, expectedName, proc.Cmd()) - }) -} diff --git a/agent/agentproc/proc_unix.go b/agent/agentproc/proc_unix.go deleted file mode 100644 index d35d9f1829722..0000000000000 --- a/agent/agentproc/proc_unix.go +++ /dev/null @@ -1,134 +0,0 @@ -//go:build linux -// +build linux - -package agentproc - -import ( - "errors" - "os" - "path/filepath" - "strconv" - "strings" - "syscall" - - "github.com/spf13/afero" - "golang.org/x/xerrors" -) - -func List(fs afero.Fs, syscaller Syscaller) ([]*Process, error) { - d, err := fs.Open(defaultProcDir) - if err != nil { - return nil, xerrors.Errorf("open dir %q: %w", defaultProcDir, err) - } - defer d.Close() - - entries, err := d.Readdirnames(0) - if err != nil { - return nil, xerrors.Errorf("readdirnames: %w", err) - } - - processes := make([]*Process, 0, len(entries)) - for _, entry := range entries { - pid, err := strconv.ParseInt(entry, 10, 32) - if err != nil { - continue - } - - // Check that the process still exists. - exists, err := isProcessExist(syscaller, int32(pid)) - if err != nil { - return nil, xerrors.Errorf("check process exists: %w", err) - } - if !exists { - continue - } - - cmdline, err := afero.ReadFile(fs, filepath.Join(defaultProcDir, entry, "cmdline")) - if err != nil { - if isBenignError(err) { - continue - } - return nil, xerrors.Errorf("read cmdline: %w", err) - } - - oomScore, err := afero.ReadFile(fs, filepath.Join(defaultProcDir, entry, "oom_score_adj")) - if err != nil { - if isBenignError(err) { - continue - } - - return nil, xerrors.Errorf("read oom_score_adj: %w", err) - } - - oom, err := strconv.Atoi(strings.TrimSpace(string(oomScore))) - if err != nil { - return nil, xerrors.Errorf("convert oom score: %w", err) - } - - processes = append(processes, &Process{ - PID: int32(pid), - CmdLine: string(cmdline), - Dir: filepath.Join(defaultProcDir, entry), - OOMScoreAdj: oom, - }) - } - - return processes, nil -} - -func isProcessExist(syscaller Syscaller, pid int32) (bool, error) { - err := syscaller.Kill(pid, syscall.Signal(0)) - if err == nil { - return true, nil - } - if err.Error() == "os: process already finished" { - return false, nil - } - - var errno syscall.Errno - if !errors.As(err, &errno) { - return false, err - } - - switch errno { - case syscall.ESRCH: - return false, nil - case syscall.EPERM: - return true, nil - } - - return false, xerrors.Errorf("kill: %w", err) -} - -func (p *Process) Niceness(sc Syscaller) (int, error) { - nice, err := sc.GetPriority(p.PID) - if err != nil { - return 0, xerrors.Errorf("get priority for %q: %w", p.CmdLine, err) - } - return nice, nil -} - -func (p *Process) SetNiceness(sc Syscaller, score int) error { - err := sc.SetPriority(p.PID, score) - if err != nil { - return xerrors.Errorf("set priority for %q: %w", p.CmdLine, err) - } - return nil -} - -func (p *Process) Cmd() string { - return strings.Join(p.cmdLine(), " ") -} - -func (p *Process) cmdLine() []string { - return strings.Split(p.CmdLine, "\x00") -} - -func isBenignError(err error) bool { - var errno syscall.Errno - if !xerrors.As(err, &errno) { - return false - } - - return errno == syscall.ESRCH || errno == syscall.EPERM || xerrors.Is(err, os.ErrNotExist) -} diff --git a/agent/agentproc/syscaller.go b/agent/agentproc/syscaller.go deleted file mode 100644 index fba3bf32ce5c9..0000000000000 --- a/agent/agentproc/syscaller.go +++ /dev/null @@ -1,21 +0,0 @@ -package agentproc - -import ( - "syscall" -) - -type Syscaller interface { - SetPriority(pid int32, priority int) error - GetPriority(pid int32) (int, error) - Kill(pid int32, sig syscall.Signal) error -} - -// nolint: unused // used on some but no all platforms -const defaultProcDir = "/proc" - -type Process struct { - Dir string - CmdLine string - PID int32 - OOMScoreAdj int -} diff --git a/agent/agentproc/syscaller_other.go b/agent/agentproc/syscaller_other.go deleted file mode 100644 index 2a355147e24c1..0000000000000 --- a/agent/agentproc/syscaller_other.go +++ /dev/null @@ -1,30 +0,0 @@ -//go:build !linux -// +build !linux - -package agentproc - -import ( - "syscall" - - "golang.org/x/xerrors" -) - -func NewSyscaller() Syscaller { - return nopSyscaller{} -} - -var errUnimplemented = xerrors.New("unimplemented") - -type nopSyscaller struct{} - -func (nopSyscaller) SetPriority(int32, int) error { - return errUnimplemented -} - -func (nopSyscaller) GetPriority(int32) (int, error) { - return 0, errUnimplemented -} - -func (nopSyscaller) Kill(int32, syscall.Signal) error { - return errUnimplemented -} diff --git a/agent/agentproc/syscaller_unix.go b/agent/agentproc/syscaller_unix.go deleted file mode 100644 index e63e56b50f724..0000000000000 --- a/agent/agentproc/syscaller_unix.go +++ /dev/null @@ -1,42 +0,0 @@ -//go:build linux -// +build linux - -package agentproc - -import ( - "syscall" - - "golang.org/x/sys/unix" - "golang.org/x/xerrors" -) - -func NewSyscaller() Syscaller { - return UnixSyscaller{} -} - -type UnixSyscaller struct{} - -func (UnixSyscaller) SetPriority(pid int32, nice int) error { - err := unix.Setpriority(unix.PRIO_PROCESS, int(pid), nice) - if err != nil { - return xerrors.Errorf("set priority: %w", err) - } - return nil -} - -func (UnixSyscaller) GetPriority(pid int32) (int, error) { - nice, err := unix.Getpriority(0, int(pid)) - if err != nil { - return 0, xerrors.Errorf("get priority: %w", err) - } - return nice, nil -} - -func (UnixSyscaller) Kill(pid int32, sig syscall.Signal) error { - err := syscall.Kill(int(pid), sig) - if err != nil { - return xerrors.Errorf("kill: %w", err) - } - - return nil -} diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 081056b4f4ebd..415674c9e2e95 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -30,6 +30,7 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/usershell" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/pty" @@ -725,7 +726,10 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) } } - cmd := pty.CommandContext(ctx, name, args...) + cmd, err := agentexec.PTYCommandContext(ctx, name, args...) + if err != nil { + return nil, xerrors.Errorf("pty command context: %w", err) + } cmd.Dir = s.config.WorkingDirectory() // If the metadata directory doesn't exist, we run the command diff --git a/agent/reconnectingpty/buffered.go b/agent/reconnectingpty/buffered.go index d53b22ffe2153..cde41fb227c9e 100644 --- a/agent/reconnectingpty/buffered.go +++ b/agent/reconnectingpty/buffered.go @@ -14,6 +14,7 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/pty" ) @@ -58,7 +59,11 @@ func newBuffered(ctx context.Context, cmd *pty.Cmd, options *Options, logger slo // Add TERM then start the command with a pty. pty.Cmd duplicates Path as the // first argument so remove it. - cmdWithEnv := pty.CommandContext(ctx, cmd.Path, cmd.Args[1:]...) + cmdWithEnv, err := agentexec.PTYCommandContext(ctx, cmd.Path, cmd.Args[1:]...) + if err != nil { + rpty.state.setState(StateDone, xerrors.Errorf("pty command context: %w", err)) + return rpty + } cmdWithEnv.Env = append(rpty.command.Env, "TERM=xterm-256color") cmdWithEnv.Dir = rpty.command.Dir ptty, process, err := pty.Start(cmdWithEnv) diff --git a/agent/reconnectingpty/screen.go b/agent/reconnectingpty/screen.go index ca3451fe33947..122ef1fffc792 100644 --- a/agent/reconnectingpty/screen.go +++ b/agent/reconnectingpty/screen.go @@ -9,7 +9,6 @@ import ( "io" "net" "os" - "os/exec" "path/filepath" "strings" "sync" @@ -20,6 +19,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/pty" ) @@ -210,7 +210,7 @@ func (rpty *screenReconnectingPTY) doAttach(ctx context.Context, conn net.Conn, logger.Debug(ctx, "spawning screen client", slog.F("screen_id", rpty.id)) // Wrap the command with screen and tie it to the connection's context. - cmd := pty.CommandContext(ctx, "screen", append([]string{ + cmd, err := agentexec.PTYCommandContext(ctx, "screen", append([]string{ // -S is for setting the session's name. "-S", rpty.id, // -U tells screen to use UTF-8 encoding. @@ -223,6 +223,9 @@ func (rpty *screenReconnectingPTY) doAttach(ctx context.Context, conn net.Conn, rpty.command.Path, // pty.Cmd duplicates Path as the first argument so remove it. }, rpty.command.Args[1:]...)...) + if err != nil { + return nil, nil, xerrors.Errorf("pty command context: %w", err) + } cmd.Env = append(rpty.command.Env, "TERM=xterm-256color") cmd.Dir = rpty.command.Dir ptty, process, err := pty.Start(cmd, pty.WithPTYOption( @@ -327,10 +330,10 @@ func (rpty *screenReconnectingPTY) sendCommand(ctx context.Context, command stri defer cancel() var lastErr error - run := func() bool { + run := func() (bool, error) { var stdout bytes.Buffer //nolint:gosec - cmd := exec.CommandContext(ctx, "screen", + cmd, err := agentexec.CommandContext(ctx, "screen", // -x targets an attached session. "-x", rpty.id, // -c is the flag for the config file. @@ -338,18 +341,21 @@ func (rpty *screenReconnectingPTY) sendCommand(ctx context.Context, command stri // -X runs a command in the matching session. "-X", command, ) + if err != nil { + return false, xerrors.Errorf("command context: %w", err) + } cmd.Env = append(rpty.command.Env, "TERM=xterm-256color") cmd.Dir = rpty.command.Dir cmd.Stdout = &stdout - err := cmd.Run() + err = cmd.Run() if err == nil { - return true + return true, nil } stdoutStr := stdout.String() for _, se := range successErrors { if strings.Contains(stdoutStr, se) { - return true + return true, nil } } @@ -359,11 +365,15 @@ func (rpty *screenReconnectingPTY) sendCommand(ctx context.Context, command stri lastErr = xerrors.Errorf("`screen -x %s -X %s`: %w: %s", rpty.id, command, err, stdoutStr) } - return false + return false, nil } // Run immediately. - if done := run(); done { + done, err := run() + if err != nil { + return err + } + if done { return nil } @@ -379,7 +389,11 @@ func (rpty *screenReconnectingPTY) sendCommand(ctx context.Context, command stri } return errors.Join(ctx.Err(), lastErr) case <-ticker.C: - if done := run(); done { + done, err := run() + if err != nil { + return err + } + if done { return nil } } diff --git a/agent/stats_internal_test.go b/agent/stats_internal_test.go index 76f41a9da113f..e032910ee36aa 100644 --- a/agent/stats_internal_test.go +++ b/agent/stats_internal_test.go @@ -1,10 +1,7 @@ package agent import ( - "bytes" "context" - "encoding/json" - "io" "net/netip" "sync" "testing" @@ -16,8 +13,6 @@ import ( "tailscale.com/types/netlogtype" - "cdr.dev/slog" - "cdr.dev/slog/sloggers/slogjson" "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/testutil" ) @@ -213,58 +208,3 @@ func newFakeStatsDest() *fakeStatsDest { resps: make(chan *proto.UpdateStatsResponse), } } - -func Test_logDebouncer(t *testing.T) { - t.Parallel() - - var ( - buf bytes.Buffer - logger = slog.Make(slogjson.Sink(&buf)) - ctx = context.Background() - ) - - debouncer := &logDebouncer{ - logger: logger, - messages: map[string]time.Time{}, - interval: time.Minute, - } - - fields := map[string]interface{}{ - "field_1": float64(1), - "field_2": "2", - } - - debouncer.Error(ctx, "my message", "field_1", 1, "field_2", "2") - debouncer.Warn(ctx, "another message", "field_1", 1, "field_2", "2") - // Shouldn't log this. - debouncer.Warn(ctx, "another message", "field_1", 1, "field_2", "2") - - require.Len(t, debouncer.messages, 2) - - type entry struct { - Msg string `json:"msg"` - Level string `json:"level"` - Fields map[string]interface{} `json:"fields"` - } - - assertLog := func(msg string, level string, fields map[string]interface{}) { - line, err := buf.ReadString('\n') - require.NoError(t, err) - - var e entry - err = json.Unmarshal([]byte(line), &e) - require.NoError(t, err) - require.Equal(t, msg, e.Msg) - require.Equal(t, level, e.Level) - require.Equal(t, fields, e.Fields) - } - assertLog("my message", "ERROR", fields) - assertLog("another message", "WARN", fields) - - debouncer.messages["another message"] = time.Now().Add(-2 * time.Minute) - debouncer.Warn(ctx, "another message", "field_1", 1, "field_2", "2") - assertLog("another message", "WARN", fields) - // Assert nothing else was written. - _, err := buf.ReadString('\n') - require.ErrorIs(t, err, io.EOF) -} diff --git a/cli/agent.go b/cli/agent.go index 43af444536c8f..f76f222d3053b 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -25,7 +25,7 @@ import ( "cdr.dev/slog/sloggers/slogjson" "cdr.dev/slog/sloggers/slogstackdriver" "github.com/coder/coder/v2/agent" - "github.com/coder/coder/v2/agent/agentproc" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/agent/reaper" "github.com/coder/coder/v2/buildinfo" @@ -171,6 +171,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { slog.F("auth", auth), slog.F("version", version), ) + client := agentsdk.New(r.agentURL) client.SDK.SetLogger(logger) // Set a reasonable timeout so requests can't hang forever! @@ -292,11 +293,20 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { environmentVariables := map[string]string{ "GIT_ASKPASS": executablePath, } - if v, ok := os.LookupEnv(agent.EnvProcPrioMgmt); ok { - environmentVariables[agent.EnvProcPrioMgmt] = v - } - if v, ok := os.LookupEnv(agent.EnvProcOOMScore); ok { - environmentVariables[agent.EnvProcOOMScore] = v + + enabled := os.Getenv(agentexec.EnvProcPrioMgmt) + if enabled != "" && runtime.GOOS == "linux" { + logger.Info(ctx, "process priority management enabled", + slog.F("env_var", agentexec.EnvProcPrioMgmt), + slog.F("enabled", enabled), + slog.F("os", runtime.GOOS), + ) + } else { + logger.Info(ctx, "process priority management not enabled (linux-only) ", + slog.F("env_var", agentexec.EnvProcPrioMgmt), + slog.F("enabled", enabled), + slog.F("os", runtime.GOOS), + ) } agnt := agent.New(agent.Options{ @@ -322,12 +332,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { Subsystems: subsystems, PrometheusRegistry: prometheusRegistry, - Syscaller: agentproc.NewSyscaller(), - // Intentionally set this to nil. It's mainly used - // for testing. - ModifiedProcesses: nil, - - BlockFileTransfer: blockFileTransfer, + BlockFileTransfer: blockFileTransfer, }) promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger) diff --git a/cmd/coder/main.go b/cmd/coder/main.go index 7d41563c18e68..46e0850574a99 100644 --- a/cmd/coder/main.go +++ b/cmd/coder/main.go @@ -1,12 +1,20 @@ package main import ( + "fmt" + "os" _ "time/tzdata" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/cli" ) func main() { + if len(os.Args) > 1 && os.Args[1] == "agent-exec" { + err := agentexec.CLI() + _, _ = fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } var rootCmd cli.RootCmd rootCmd.RunWithSubcommands(rootCmd.AGPL()) } diff --git a/enterprise/cmd/coder/main.go b/enterprise/cmd/coder/main.go index c7e19dfab96ef..c71e479fe0079 100644 --- a/enterprise/cmd/coder/main.go +++ b/enterprise/cmd/coder/main.go @@ -1,12 +1,21 @@ package main import ( + "fmt" + "os" _ "time/tzdata" + "github.com/coder/coder/v2/agent/agentexec" entcli "github.com/coder/coder/v2/enterprise/cli" ) func main() { + if len(os.Args) > 1 && os.Args[1] == "agent-exec" { + err := agentexec.CLI() + _, _ = fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + var rootCmd entcli.RootCmd rootCmd.RunWithSubcommands(rootCmd.EnterpriseSubcommands()) } diff --git a/scripts/rules.go b/scripts/rules.go index 46aebabab4a1a..57d067aeb5019 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -487,3 +487,39 @@ func workspaceActivity(m dsl.Matcher) { !m.File().Name.Matches(`_test\.go$`), ).Report("Updating workspace activity should always be done in the workspacestats package.") } + +// noExecInAgent ensures that packages under agent/ don't use exec.Command or +// exec.CommandContext directly. +// +//nolint:unused,deadcode,varnamelen +func noExecInAgent(m dsl.Matcher) { + m.Import("os/exec") + m.Match( + `exec.Command($*_)`, + `exec.CommandContext($*_)`, + ). + Where( + m.File().PkgPath.Matches("/agent/") && + !m.File().PkgPath.Matches("/agentexec") && + !m.File().Name.Matches(`_test\.go$`), + ). + Report("The agent and its subpackages should not use exec.Command or exec.CommandContext directly. Consider using agentexec.CommandContext instead.") +} + +// noPTYInAgent ensures that packages under agent/ don't use pty.Command or +// pty.CommandContext directly. +// +//nolint:unused,deadcode,varnamelen +func noPTYInAgent(m dsl.Matcher) { + m.Import("github.com/coder/coder/v2/pty") + m.Match( + `pty.Command($*_)`, + `pty.CommandContext($*_)`, + ). + Where( + m.File().PkgPath.Matches(`/agent/`) && + !m.File().PkgPath.Matches(`/agentexec`) && + !m.File().Name.Matches(`_test\.go$`), + ). + Report("The agent and its subpackages should not use pty.Command or pty.CommandContext directly. Consider using agentexec.PTYCommandContext instead.") +}