Skip to content

Commit ef41e9a

Browse files
sreyasreya
committed
pr comments
1 parent 2fe9c70 commit ef41e9a

File tree

9 files changed

+147
-129
lines changed

9 files changed

+147
-129
lines changed

‎agent/agent.go

Lines changed: 28 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ const (
5353
ProtocolDial = "dial"
5454
)
5555

56-
// EnvProcMemNice determines whether we attempt to manage
56+
// EnvProcPrioMgmt determines whether we attempt to manage
5757
// process CPU and OOM Killer priority.
58-
const EnvProcMemNice = "CODER_PROC_MEMNICE_ENABLE"
58+
const EnvProcPrioMgmt = "CODER_PROC_PRIO_MGMT"
5959

6060
type Options struct {
6161
Filesystem afero.Fs
@@ -217,7 +217,7 @@ type agent struct {
217217
metrics *agentMetrics
218218
syscaller agentproc.Syscaller
219219

220-
// podifiedProcs is used for testing process priority management.
220+
// modifiedProcs is used for testing process priority management.
221221
modifiedProcs chan []*agentproc.Process
222222
// processManagementTick is used for testing process priority management.
223223
processManagementTick <-chan time.Time
@@ -1281,41 +1281,34 @@ func (a *agent) startReportingConnectionStats(ctx context.Context) {
12811281
var prioritizedProcs = []string{"coder"}
12821282

12831283
func (a *agent) manageProcessPriorityLoop(ctx context.Context) {
1284-
if val := a.envVars[EnvProcMemNice]; val == "" || runtime.GOOS != "linux" {
1285-
a.logger.Info(ctx, "process priority not enabled, agent will not manage process niceness/oom_score_adj ",
1286-
slog.F("env_var", EnvProcMemNice),
1284+
if val := a.envVars[EnvProcPrioMgmt]; val == "" || runtime.GOOS != "linux" {
1285+
a.logger.Debug(ctx, "process priority not enabled, agent will not manage process niceness/oom_score_adj ",
1286+
slog.F("env_var", EnvProcPrioMgmt),
12871287
slog.F("value", val),
12881288
slog.F("goos", runtime.GOOS),
12891289
)
12901290
return
12911291
}
12921292

1293-
manage := func() {
1293+
if a.processManagementTick == nil {
1294+
ticker := time.NewTicker(time.Second)
1295+
defer ticker.Stop()
1296+
a.processManagementTick = ticker.C
1297+
}
1298+
1299+
for {
12941300
procs, err := a.manageProcessPriority(ctx)
12951301
if err != nil {
12961302
a.logger.Error(ctx, "manage process priority",
1297-
slog.F("dir", agentproc.DefaultProcDir),
12981303
slog.Error(err),
12991304
)
13001305
}
13011306
if a.modifiedProcs != nil {
13021307
a.modifiedProcs <- procs
13031308
}
1304-
}
1305-
1306-
// Do once before falling into loop.
1307-
manage()
1308-
1309-
if a.processManagementTick == nil {
1310-
ticker := time.NewTicker(time.Second)
1311-
defer ticker.Stop()
1312-
a.processManagementTick = ticker.C
1313-
}
13141309

1315-
for {
13161310
select {
13171311
case <-a.processManagementTick:
1318-
manage()
13191312
case <-ctx.Done():
13201313
return
13211314
}
@@ -1324,48 +1317,46 @@ func (a *agent) manageProcessPriorityLoop(ctx context.Context) {
13241317

13251318
func (a *agent) manageProcessPriority(ctx context.Context) ([]*agentproc.Process, error) {
13261319
const (
1327-
procDir = agentproc.DefaultProcDir
13281320
niceness = 10
13291321
oomScoreAdj = -500
13301322
)
13311323

1332-
procs, err := agentproc.List(a.filesystem, a.syscaller, agentproc.DefaultProcDir)
1324+
procs, err := agentproc.List(a.filesystem, a.syscaller)
13331325
if err != nil {
13341326
return nil, xerrors.Errorf("list: %w", err)
13351327
}
13361328

1337-
modProcs := []*agentproc.Process{}
1329+
var (
1330+
modProcs = []*agentproc.Process{}
1331+
logger slog.Logger
1332+
)
1333+
13381334
for _, proc := range procs {
1335+
logger = a.logger.With(
1336+
slog.F("name", proc.Name()),
1337+
slog.F("pid", proc.PID),
1338+
)
1339+
13391340
// Trim off the path e.g. "./coder" -> "coder"
13401341
name := filepath.Base(proc.Name())
13411342
// If the process is prioritized we should adjust
13421343
// it's oom_score_adj and avoid lowering its niceness.
13431344
if slices.Contains(prioritizedProcs, name) {
13441345
err = proc.SetOOMAdj(oomScoreAdj)
13451346
if err != nil {
1346-
a.logger.Error(ctx, "unable to set proc oom_score_adj",
1347-
slog.F("name", proc.Name()),
1348-
slog.F("pid", proc.PID),
1347+
logger.Warn(ctx, "unable to set proc oom_score_adj",
13491348
slog.F("oom_score_adj", oomScoreAdj),
13501349
slog.Error(err),
13511350
)
13521351
continue
13531352
}
13541353
modProcs = append(modProcs, proc)
1355-
1356-
a.logger.Debug(ctx, "decreased process oom_score",
1357-
slog.F("name", proc.Name()),
1358-
slog.F("pid", proc.PID),
1359-
slog.F("oom_score_adj", oomScoreAdj),
1360-
)
13611354
continue
13621355
}
13631356

13641357
score, err := proc.Niceness(a.syscaller)
13651358
if err != nil {
1366-
a.logger.Error(ctx, "unable to get proc niceness",
1367-
slog.F("name", proc.Name()),
1368-
slog.F("pid", proc.PID),
1359+
logger.Warn(ctx, "unable to get proc niceness",
13691360
slog.Error(err),
13701361
)
13711362
continue
@@ -1376,30 +1367,21 @@ func (a *agent) manageProcessPriority(ctx context.Context) ([]*agentproc.Process
13761367
// Getpriority actually returns priority for the nice value
13771368
// which is niceness + 20, so here 20 = a niceness of 0 (aka unset).
13781369
if score != 20 {
1379-
a.logger.Error(ctx, "skipping process due to custom niceness",
1380-
slog.F("name", proc.Name()),
1381-
slog.F("pid", proc.PID),
1370+
logger.Debug(ctx, "skipping process due to custom niceness",
13821371
slog.F("niceness", score),
13831372
)
13841373
continue
13851374
}
13861375

13871376
err = proc.SetNiceness(a.syscaller, niceness)
13881377
if err != nil {
1389-
a.logger.Error(ctx, "unable to set proc niceness",
1390-
slog.F("name", proc.Name()),
1391-
slog.F("pid", proc.PID),
1378+
logger.Warn(ctx, "unable to set proc niceness",
13921379
slog.F("niceness", niceness),
13931380
slog.Error(err),
13941381
)
13951382
continue
13961383
}
13971384

1398-
a.logger.Debug(ctx, "deprioritized process",
1399-
slog.F("name", proc.Name()),
1400-
slog.F("pid", proc.PID),
1401-
slog.F("niceness", niceness),
1402-
)
14031385
modProcs = append(modProcs, proc)
14041386
}
14051387
return modProcs, nil

‎agent/agent_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2422,15 +2422,15 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
24222422
// score should be untouched.
24232423
var proc agentproc.Process
24242424
if i == 0 {
2425-
proc = agentproctest.GenerateProcess(t, fs, agentproc.DefaultProcDir,
2425+
proc = agentproctest.GenerateProcess(t, fs,
24262426
func(p *agentproc.Process) {
24272427
p.CmdLine = "./coder\x00agent\x00--no-reap"
24282428
p.PID = 1
24292429
},
24302430
)
24312431
} else {
24322432
// The rest are peasants.
2433-
proc = agentproctest.GenerateProcess(t, fs, agentproc.DefaultProcDir)
2433+
proc = agentproctest.GenerateProcess(t, fs)
24342434
syscaller.EXPECT().SetPriority(proc.PID, 10).Return(nil)
24352435
syscaller.EXPECT().GetPriority(proc.PID).Return(20, nil)
24362436
}
@@ -2444,7 +2444,7 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
24442444
_, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) {
24452445
o.Syscaller = syscaller
24462446
o.ModifiedProcesses = modProcs
2447-
o.EnvironmentVariables = map[string]string{agent.EnvProcMemNice: "1"}
2447+
o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"}
24482448
o.Filesystem = fs
24492449
o.Logger = logger
24502450
o.ProcessManagementTick = ticker
@@ -2480,7 +2480,7 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
24802480

24812481
// Create some processes.
24822482
for i := 0; i < 2; i++ {
2483-
proc := agentproctest.GenerateProcess(t, fs, agentproc.DefaultProcDir)
2483+
proc := agentproctest.GenerateProcess(t, fs)
24842484
syscaller.EXPECT().
24852485
Kill(proc.PID, syscall.Signal(0)).
24862486
Return(nil)
@@ -2500,7 +2500,7 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
25002500
_, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) {
25012501
o.Syscaller = syscaller
25022502
o.ModifiedProcesses = modProcs
2503-
o.EnvironmentVariables = map[string]string{agent.EnvProcMemNice: "1"}
2503+
o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"}
25042504
o.Filesystem = fs
25052505
o.Logger = logger
25062506
o.ProcessManagementTick = ticker
@@ -2518,7 +2518,7 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
25182518
}
25192519

25202520
var buf bytes.Buffer
2521-
log := slog.Make(sloghuman.Sink(&buf))
2521+
log := slog.Make(sloghuman.Sink(&buf)).Leveled(slog.LevelDebug)
25222522

25232523
_, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) {
25242524
o.Logger = log
@@ -2537,13 +2537,13 @@ func TestAgent_ManageProcessPriority(t *testing.T) {
25372537
}
25382538

25392539
var buf bytes.Buffer
2540-
log := slog.Make(sloghuman.Sink(&buf))
2540+
log := slog.Make(sloghuman.Sink(&buf)).Leveled(slog.LevelDebug)
25412541

25422542
_, _, _, _, _ = setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) {
25432543
o.Logger = log
25442544
// Try to enable it so that we can assert that non-linux
25452545
// environments are truly disabled.
2546-
o.EnvironmentVariables = map[string]string{agent.EnvProcMemNice: "1"}
2546+
o.EnvironmentVariables = map[string]string{agent.EnvProcPrioMgmt: "1"}
25472547
})
25482548
require.Eventually(t, func() bool {
25492549
return strings.Contains(buf.String(), "process priority not enabled")

‎agent/agentproc/agentproctest/proc.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
"github.com/coder/coder/v2/cryptorand"
1212
)
1313

14-
func GenerateProcess(t *testing.T, fs afero.Fs, dir string, muts ...func(*agentproc.Process)) agentproc.Process {
14+
func GenerateProcess(t *testing.T, fs afero.Fs, muts ...func(*agentproc.Process)) agentproc.Process {
1515
t.Helper()
1616

1717
pid, err := cryptorand.Intn(1<<31 - 1)
@@ -38,7 +38,7 @@ func GenerateProcess(t *testing.T, fs afero.Fs, dir string, muts ...func(*agentp
3838
mut(&process)
3939
}
4040

41-
process.Dir = fmt.Sprintf("%s/%d", dir, process.PID)
41+
process.Dir = fmt.Sprintf("%s/%d", "/proc", process.PID)
4242

4343
err = fs.MkdirAll(process.Dir, 0o555)
4444
require.NoError(t, err)

‎agent/agentproc/proc_other.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
//go:build !linux
2+
// +build !linux
3+
4+
package agentproc
5+
6+
import (
7+
"github.com/spf13/afero"
8+
)
9+
10+
func (p *Process) SetOOMAdj(score int) error {
11+
return errUnimplimented
12+
}
13+
14+
func (p *Process) Niceness(sc Syscaller) (int, error) {
15+
return 0, errUnimplimented
16+
}
17+
18+
func (p *Process) SetNiceness(sc Syscaller, score int) error {
19+
return errUnimplimented
20+
}
21+
22+
func (p *Process) Name() string {
23+
return ""
24+
}
25+
26+
func List(fs afero.Fs, syscaller Syscaller) ([]*Process, error) {
27+
return nil, errUnimplimented
28+
}

‎agent/agentproc/proc_test.go

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,20 @@ func TestList(t *testing.T) {
2525
fs = afero.NewMemMapFs()
2626
sc = agentproctest.NewMockSyscaller(gomock.NewController(t))
2727
expectedProcs = make(map[int32]agentproc.Process)
28-
rootDir = agentproc.DefaultProcDir
2928
)
3029

3130
for i := 0; i < 4; i++ {
32-
proc := agentproctest.GenerateProcess(t, fs, rootDir)
31+
proc := agentproctest.GenerateProcess(t, fs)
3332
expectedProcs[proc.PID] = proc
3433

3534
sc.EXPECT().
3635
Kill(proc.PID, syscall.Signal(0)).
3736
Return(nil)
3837
}
3938

40-
actualProcs, err := agentproc.List(fs, sc, rootDir)
39+
actualProcs, err := agentproc.List(fs, sc)
4140
require.NoError(t, err)
42-
require.Len(t, actualProcs, 4)
41+
require.Len(t, actualProcs, len(expectedProcs))
4342
for _, proc := range actualProcs {
4443
expected, ok := expectedProcs[proc.PID]
4544
require.True(t, ok)
@@ -56,11 +55,10 @@ func TestList(t *testing.T) {
5655
fs = afero.NewMemMapFs()
5756
sc = agentproctest.NewMockSyscaller(gomock.NewController(t))
5857
expectedProcs = make(map[int32]agentproc.Process)
59-
rootDir = agentproc.DefaultProcDir
6058
)
6159

6260
for i := 0; i < 3; i++ {
63-
proc := agentproctest.GenerateProcess(t, fs, rootDir)
61+
proc := agentproctest.GenerateProcess(t, fs)
6462
expectedProcs[proc.PID] = proc
6563

6664
sc.EXPECT().
@@ -70,14 +68,14 @@ func TestList(t *testing.T) {
7068

7169
// Create a process that's already finished. We're not adding
7270
// it to the map because it should be skipped over.
73-
proc := agentproctest.GenerateProcess(t, fs, rootDir)
71+
proc := agentproctest.GenerateProcess(t, fs)
7472
sc.EXPECT().
7573
Kill(proc.PID, syscall.Signal(0)).
7674
Return(xerrors.New("os: process already finished"))
7775

78-
actualProcs, err := agentproc.List(fs, sc, rootDir)
76+
actualProcs, err := agentproc.List(fs, sc)
7977
require.NoError(t, err)
80-
require.Len(t, actualProcs, 3)
78+
require.Len(t, actualProcs, len(expectedProcs))
8179
for _, proc := range actualProcs {
8280
expected, ok := expectedProcs[proc.PID]
8381
require.True(t, ok)
@@ -94,11 +92,10 @@ func TestList(t *testing.T) {
9492
fs = afero.NewMemMapFs()
9593
sc = agentproctest.NewMockSyscaller(gomock.NewController(t))
9694
expectedProcs = make(map[int32]agentproc.Process)
97-
rootDir = agentproc.DefaultProcDir
9895
)
9996

10097
for i := 0; i < 3; i++ {
101-
proc := agentproctest.GenerateProcess(t, fs, rootDir)
98+
proc := agentproctest.GenerateProcess(t, fs)
10299
expectedProcs[proc.PID] = proc
103100

104101
sc.EXPECT().
@@ -108,14 +105,14 @@ func TestList(t *testing.T) {
108105

109106
// Create a process that doesn't exist. We're not adding
110107
// it to the map because it should be skipped over.
111-
proc := agentproctest.GenerateProcess(t, fs, rootDir)
108+
proc := agentproctest.GenerateProcess(t, fs)
112109
sc.EXPECT().
113110
Kill(proc.PID, syscall.Signal(0)).
114111
Return(syscall.ESRCH)
115112

116-
actualProcs, err := agentproc.List(fs, sc, rootDir)
113+
actualProcs, err := agentproc.List(fs, sc)
117114
require.NoError(t, err)
118-
require.Len(t, actualProcs, 3)
115+
require.Len(t, actualProcs, len(expectedProcs))
119116
for _, proc := range actualProcs {
120117
expected, ok := expectedProcs[proc.PID]
121118
require.True(t, ok)
@@ -136,15 +133,14 @@ func TestProcess(t *testing.T) {
136133

137134
var (
138135
fs = afero.NewMemMapFs()
139-
dir = agentproc.DefaultProcDir
140-
proc = agentproctest.GenerateProcess(t, fs, agentproc.DefaultProcDir)
136+
proc = agentproctest.GenerateProcess(t, fs)
141137
expectedScore = -1000
142138
)
143139

144140
err := proc.SetOOMAdj(expectedScore)
145141
require.NoError(t, err)
146142

147-
actualScore, err := afero.ReadFile(fs, fmt.Sprintf("%s/%d/oom_score_adj", dir, proc.PID))
143+
actualScore, err := afero.ReadFile(fs, fmt.Sprintf("/proc/%d/oom_score_adj", proc.PID))
148144
require.NoError(t, err)
149145
require.Equal(t, fmt.Sprintf("%d", expectedScore), strings.TrimSpace(string(actualScore)))
150146
})

0 commit comments

Comments
 (0)