feat: run CPU isolation and the memory instrument inside the macro-agent sandbox - #456
Conversation
Merging this PR will not alter performance
|
Greptile SummaryThis PR makes two sandboxed-runner features work without host privileges: CPU isolation now uses hook scripts (
Confidence Score: 5/5Safe to merge; the isolation refactor preserves the systemd fallback and the dual-skeleton approach is mechanically correct across all probe sites. The dual-skeleton dispatch, namespace-aware PID helpers, and proc_fs fallback path are all consistently implemented and well-tested. The previously flagged UPROBE_ARGS_RET namespace bug is fixed here. The only finding is a single eprintln! in a test helper. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/executor/wall_time/isolation.rs | Tri-state Isolation enum replaces HookScriptsGuard; Hooks path runs pre/post-bench hooks via Drop and wraps the benchmark leaf with codspeed-wrap-bench, Systemd path falls back to systemd-run. |
| crates/memtrack/src/ebpf/c/utils/variant.h | New header providing UPROBE_SEC/URETPROBE_SEC and namespace-aware PID helpers; task_ns_tgid level<4 bound is correct for BPF verifier. |
| crates/memtrack/src/ebpf/c/utils/process_tracking.h | sched_process_fork migrated from classic tracepoint to tp_btf for token delegation; follow_fork uses task_ns_tgid correctly. |
| crates/memtrack/src/ebpf/c/allocator.h | All probe SEC() annotations parameterised; UPROBE_ARGS_RET and all other probes now use current_task_ids().tgid instead of global PID. |
| crates/memtrack/src/ebpf/memtrack/mod.rs | Dual-skeleton Skel enum with with_skel! dispatch; open_and_load! sets pidns rodata before load; variant selected by has_delegated_bpf_token(). |
| crates/memtrack/src/bpf_token.rs | Shared has_delegated_bpf_token() helper (is_dir check included) now used by both the eBPF loader and the memory executor. |
| crates/memtrack/src/ebpf/proc_fs.rs | candidate_paths() falls back from map_files to pathname column for privilege-free library discovery. |
| crates/memtrack/tests/shared.rs | for_each_variant cross-checks BPF variants; one eprintln! violates the project tracing-macro rule. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Isolation::resolve] --> B{CODSPEED_ISOLATION=false?}
B -- yes --> C[Isolation::None]
B -- no --> D{codspeed-wrap-bench executable?}
D -- yes --> E[run_pre_bench hook]
E --> F[Isolation::Hooks]
D -- no --> G{forced=true OR passwordless sudo?}
G -- yes --> H[Isolation::Systemd]
G -- no --> I[Isolation::None - log warning]
F --> J[wrap_bench: prepend wrap-bench]
H --> K[wrap_bench: systemd-run scope]
C --> L[wrap_bench: passthrough]
J --> M[benchmark child of profiler]
K --> N[benchmark reparented - profiler needs sudo]
F -.->|Drop| P[run post-bench hook]
Reviews (6): Last reviewed commit: "chore: make tests run release builds of ..." | Re-trigger Greptile
cd83132 to
1142235
Compare
1142235 to
217413d
Compare
228b86f to
266f658
Compare
|
@greptileai review |
not-matthias
left a comment
There was a problem hiding this comment.
Overall, already pretty good. I think we can improve the naming a bit and let's check if we can keep the tests unified.
Lmk if you have some other thoughts on some comments.
f83af31 to
2ee406c
Compare
9069db4 to
81c200a
Compare
Replace the runner's built-in CPU-isolation mechanisms with a single,
machine-driven one. The runner previously hard-coded `systemd-run --scope` and a
`CODSPEED_ISOLATION=CGROUP:<dir>` mode with per-spawn cgroup-dir plumbing, split
across a `HookScriptsGuard` (which ran the pre/post-bench hooks) and
`isolation.rs` (which did the wrapping).
Now a single `Isolation` type owns the whole lifecycle: `resolve()` runs the
pre-bench hook, `wrap_bench()` pins the benchmark leaf, and `Drop` runs
post-bench. Cpuset logic lives on the machine behind three hooks
(`codspeed-{pre,wrap,post}-bench`); the runner only invokes them and is otherwise
oblivious to how cores are attributed. Discovery is by hook presence:
- an executable `codspeed-wrap-bench` selects the hook path — unprivileged, and
the benchmark stays a descendant of the profiler so it records without sudo;
- its absence falls back to `systemd-run --scope --slice=codspeed.slice`, so
hosts without the hook keep working unchanged.
The pre-bench hook is invoked with the runner's PID so the machine places the
runner (and the profiler it spawns) onto the system cores; the runner makes no
cgroup writes of its own. The profiler's `wrap_command` flag is renamed
`isolate` -> `requires_sudo`, now true only for the systemd fallback.
Refs COD-3012
A BPF token only relaxes the capability checks made by the bpf() syscall itself, so uprobes attached with perf_event_open() can never be delegated: that path checks CAP_SYS_ADMIN/CAP_PERFMON against the init user namespace. uprobe_multi links go entirely through bpf(), where the token applies, but require kernel >= 6.6. Compile the BPF source into two skeletons instead of one, through thin wrappers that differ only in MEMTRACK_BPF_VARIANT_TOKEN: the token variant attaches uprobe_multi links, the legacy variant keeps perf_event_open for older kernels. MemtrackBpf::new picks a variant from token availability, and with_variant pins one explicitly. The programs also resolve PIDs in the tracker's PID namespace rather than always the init one. eBPF observes init-namespace PIDs, so a tracker running inside a namespace registered namespace-local PIDs the probes would never match. The memory executor now treats a token as sufficient privilege, and so skips the sudo capability grant when one is present. Refs COD-3047
The two variants are built from the same BPF source and should observe the same allocations, since they differ only in how uprobes attach. Run each test case under both and compare what they saw, so a divergence fails the test that exercises it rather than a separate equivalence test. Comparison is on the count of events per kind-and-size: addresses, timestamps, pids and event order all differ legitimately between two runs of the same workload. A variant that cannot attach on the host is skipped rather than failing, since the token variant needs uprobe_multi (kernel >= 6.6); the run fails if neither attaches. Test workloads are now passed as closures returning a Command, as each variant needs a fresh process. Refs COD-3047
Allocator classification opened the mapping only through /proc/<pid>/map_files/<range>. Opening that requires CAP_CHECKPOINT_RESTORE or CAP_SYS_ADMIN in the *init* user namespace: proc_map_files_get_link calls checkpoint_restore_ns_capable(&init_user_ns). Outside a sandbox memtrack passes that check because the runner setcaps CAP_SYS_ADMIN onto the binary (MEMTRACK_REQUIRED_CAPS), or it runs as root. A tracker in a nested user namespace cannot pass it at all. File capabilities are not honoured there, and a capability held in the nested namespace does not satisfy a check made against the init one. So every mapping got EPERM, even libc classified as "not an allocator", and no uprobe was attached. The run still exited 0 and wrote a valid empty artifact, reporting no allocations. Fall back to the maps pathname column, which needs no privilege beyond read access. map_files stays preferred: it references the mapped inode, so it holds when the path has been replaced or unlinked, whereas a reused name could resolve to different contents. A `[deleted]` or non-absolute pathname (`[heap]`, `[vdso]`) names no openable file and is skipped. Classification reads the whole file, so a failed open and a readable non-allocator were indistinguishable; openability is now probed separately. A mapping no candidate path can reach warns instead of staying silent, but does not abort: losing coverage of one unreachable library beats killing the run, which is what a fatal error here would do via record_fatal's SIGKILL. Refs COD-3047
c957484 to
e602ec8
Compare
Runner-side changes to make CPU isolation and the memory instrument work
unprivileged inside the macro-agent sandbox. Two issues, combined here because
they share the sandbox's privilege model and the same branch.
CPU isolation (COD-3012). Drop the runner's built-in
systemd-run/CGROUP:isolation for a hook-based one: anIsolationtype invokescodspeed-{pre,wrap,post}-benchand stays oblivious to how cores are attributed.If
codspeed-wrap-benchexists it takes the (unprivileged) hook path; otherwiseit falls back to
systemd-run --scope --slice=codspeed.slice, so non-sandboxhosts are unchanged.
Memory instrument (COD-3047). Let memtrack run off a delegated BPF token
instead of root: attach via
bpf()-native links (uprobe_multi+tp_btf),resolve tracked PIDs in the tracker's PID namespace, and accept
LIBBPF_BPF_TOKEN_PATHas a privilege source.Closes COD-3012
Closes COD-3047