Skip to content

feat: implement agent process management #9461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Sep 15, 2023
Merged

feat: implement agent process management #9461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4ed4069
feat: implement agent process management
sreya Aug 30, 2023
7e59db6
improve process detection
sreya Aug 31, 2023
8c65216
add agentproc tests
sreya Aug 31, 2023
760cbb3
some minor agent tests
sreya Sep 1, 2023
f4b864e
add a proper test for proc management
sreya Sep 1, 2023
cbcb854
custom nice
sreya Sep 1, 2023
8230247
refactor into build files
sreya Sep 8, 2023
3e1defd
tick tock
sreya Sep 8, 2023
2fe9c70
make fmt
sreya Sep 8, 2023
ef41e9a
pr comments
sreya Sep 12, 2023
05baba0
lint
sreya Sep 12, 2023
478d57c
whoops
sreya Sep 12, 2023
0ced5ce
skip non-linux
sreya Sep 13, 2023
8aaa6d5
skip non-linux
sreya Sep 13, 2023
cea4851
prevent race
sreya Sep 13, 2023
5020eb4
only prioritize ourselves
sreya Sep 14, 2023
11ab047
Revert "only prioritize ourselves"
sreya Sep 14, 2023
46ef05a
only prioritize coder agent
sreya Sep 14, 2023
d132480
remove oom_score_adj
sreya Sep 14, 2023
04ee5cb
defer first
sreya Sep 15, 2023
ffbeab9
avoid resetting niceness for already niced procs
sreya Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert "only prioritize ourselves" 
This reverts commit 5020eb4.
  • Loading branch information
@sreya
sreya committed Sep 14, 2023
commit 11ab047a6e8903c2ea72a0ab2694aa9f631c9719
13 changes: 5 additions & 8 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,6 @@ type Options struct {
ModifiedProcesses chan []*agentproc.Process
// ProcessManagementTick is used for testing process priority management.
ProcessManagementTick <-chan time.Time
// PrioritizedPIDs are processes that should have preferred CPU allocation
// and be de-prioritized for OOM Killer selection.
PrioritizedPIDs []int
}

type Client interface {
Expand Down Expand Up @@ -164,7 +161,6 @@ func New(options Options) Agent {
syscaller: options.Syscaller,
modifiedProcs: options.ModifiedProcesses,
processManagementTick: options.ProcessManagementTick,
prioritizedPIDs: options.PrioritizedPIDs,

prometheusRegistry: prometheusRegistry,
metrics: newAgentMetrics(prometheusRegistry),
Expand Down Expand Up @@ -225,9 +221,6 @@ type agent struct {
modifiedProcs chan []*agentproc.Process
// processManagementTick is used for testing process priority management.
processManagementTick <-chan time.Time
// prioritizedPIDs are processes that should have preferred CPU allocation
// and be de-prioritized for OOM Killer selection.
prioritizedPIDs []int
}

func (a *agent) TailnetConn() *tailnet.Conn {
Expand Down Expand Up @@ -1285,6 +1278,8 @@ func (a *agent) startReportingConnectionStats(ctx context.Context) {
}
}

var prioritizedProcs = []string{"coder"}

func (a *agent) manageProcessPriorityLoop(ctx context.Context) {
if val := a.envVars[EnvProcPrioMgmt]; val == "" || runtime.GOOS != "linux" {
a.logger.Debug(ctx, "process priority not enabled, agent will not manage process niceness/oom_score_adj ",
Expand Down Expand Up @@ -1342,9 +1337,11 @@ func (a *agent) manageProcessPriority(ctx context.Context) ([]*agentproc.Process
slog.F("pid", proc.PID),
)

// Trim off the path e.g. "./coder" -> "coder"
name := filepath.Base(proc.Name())
// If the process is prioritized we should adjust
// it's oom_score_adj and avoid lowering its niceness.
if slices.Contains(a.prioritizedPIDs, int(proc.PID)) {
if slices.Contains(prioritizedProcs, name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to specifically prioritize the agent and not other coder processes right? If I'm reading this code correctly it would treat coder server and coder stat the same as the agent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch, I don't see that as being a big deal but we can be more discriminate about which processes we want to prioritize by also parsing command arguments. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just check if its the current process?

err = proc.SetOOMAdj(oomScoreAdj)
if err != nil {
logger.Warn(ctx, "unable to set proc oom_score_adj",
Expand Down
21 changes: 8 additions & 13 deletions agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2411,13 +2411,12 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
}

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))
prioritizedProc = 123
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))
)

// Create some processes.
Expand All @@ -2430,15 +2429,12 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
proc = agentproctest.GenerateProcess(t, fs,
func(p *agentproc.Process) {
p.CmdLine = "./coder\x00agent\x00--no-reap"
p.PID = int32(prioritizedProc)
p.PID = 1
},
)
} else {
// The rest are peasants.
proc = agentproctest.GenerateProcess(t, fs)
if proc.PID == int32(prioritizedProc) {
proc.PID = 1234
}
syscaller.EXPECT().SetPriority(proc.PID, 10).Return(nil)
syscaller.EXPECT().GetPriority(proc.PID).Return(20, nil)
}
Expand All @@ -2456,7 +2452,6 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
o.Filesystem = fs
o.Logger = logger
o.ProcessManagementTick = ticker
o.PrioritizedPIDs = []int{prioritizedProc}
})
actualProcs := <-modProcs
require.Len(t, actualProcs, 4)
Expand All @@ -2465,7 +2460,7 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
expectedScore := "0"
expected, ok := expectedProcs[actual.PID]
require.True(t, ok)
if expected.PID == int32(prioritizedProc) {
if expected.PID == 1 {
expectedScore = "-500"
}

Expand Down
1 change: 0 additions & 1 deletion cli/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,6 @@ func (r *RootCmd) workspaceAgent() *clibase.Cmd {
// Intentionally set this to nil. It's mainly used
// for testing.
ModifiedProcesses: nil,
PrioritizedPIDs: []int{os.Getpid()},
})

prometheusSrvClose := ServeHandler(ctx, logger, prometheusMetricsHandler(prometheusRegistry, logger), prometheusAddress, "prometheus")
Expand Down