Skip to content

fix(#496): verify cgroup delegation is active + state the real consequence - #514

Merged
shujaatTracebloc merged 5 commits into
developfrom
fix/496-cgroup-delegation-verify
Jul 31, 2026
Merged

fix(#496): verify cgroup delegation is active + state the real consequence#514
shujaatTracebloc merged 5 commits into
developfrom
fix/496-cgroup-delegation-verify

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

#496 — verify cgroup delegation is active (don't assume)

_write_cgroup_delegation wrote user@.service.d/delegate.conf + ran systemctl daemon-reload, then unconditionally printed "Delegated cpu/cpuset/io … A re-login may be needed." But daemon-reload re-reads unit files without restarting the running user@$(id -u).service, so cpu/cpuset/io aren't in effect for that session — and the k3d node inherits the delegation state from when it's created. So limit-bearing pods run unconstrained until a re-login and a cluster recreate, while the install looks green. The old wording ("may be needed") understated it.

Fix — the issue's recommended Option 3 (verify, don't assume)

Not Option 2 (systemctl restart user@…) — that kills the user's session processes; the issue says instruct, not perform.

  • New _cgroup_controllers_active reads the live cgroup.controllers of the user slice (/sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers; overridable for tests) and is true only when cpu+cpuset+io are all delegated (memory/pids are default).
  • After the reload, _write_cgroup_delegation checks it:
    • activesuccess "… active in this session."
    • not active → a loud warn: limits won't enforce until a re-login andk3d cluster delete <name> + re-run — "the install looks healthy, but limit-bearing workloads run unconstrained."

Acceptance

  • The gap is now observable (read the controllers) rather than inferred, and the message states the real consequence (re-login and recreate), not just "may be needed".

Context (from the issue)

Lower severity in practice: behind the opt-in TB_TIER1_ROOTLESS flag (not the validated default, no customer install takes it today), and the path was always non-fatal. This makes the under-stated case honest and verifiable.

Tests

_cgroup_controllers_active (all-present / missing-one / unreadable), and _write_cgroup_delegation's active-vs-not report. shellcheck --severity=error + check-style + check-drift clean; full setup-linux.bats green (126); scripts/manifest.sha256 regenerated (R8).

Closes#496


Note

Medium Risk
Changes rootless Tier 1 install/prepare-host behavior and messaging around cgroup delegation; incorrect checks could mislead users about limit enforcement, but scope is installer scripts and tests only.

Overview
The Linux installer no longer treats writing the user@.service cgroup delegation drop-in as success. It reads livecgroup.controllers under user@$UID.service (not the enclosing slice, which can falsely look delegated) and only reports delegation as active when cpu, cpuset, and io are present.

_write_cgroup_delegation and _ensure_cgroup_delegation always run _report_cgroup_delegation on every path, including when the drop-in already exists, so re-runs don't silently skip a written-but-not-yet-live delegation. Idempotent content checks use unprivileged cmp instead of sudo cmp.

Full installs get a clear warning that pod CPU/memory limits won't enforce until the user manager restarts (re-login or reboot with lingering) and the k3d cluster is recreated. prepare-host uses TB_PREPARE_HOST_MODE so messaging targets the researcher's next login and omits cluster-delete advice.

Bats coverage and scripts/manifest.sha256 are updated for the changed setup-linux.sh.

Reviewed by Cursor Bugbot for commit 78c9de2. Bugbot is set up for automated code reviews on this repo. Configure here.

…equence when it isn't
_write_cgroup_delegation wrote the user@.service.d/delegate.conf drop-in + ran
`systemctl daemon-reload`, then unconditionally printed "Delegated … A re-login may be
needed." But daemon-reload re-reads unit files WITHOUT restarting the running
user@$(id -u).service, so cpu/cpuset/io aren't in effect for that session — and the
k3d node inherits the delegation state from when it's CREATED. So limit-bearing pods
run unconstrained until a re-login AND a cluster recreate, while the install looks
green. The old message understated this ("may be needed").
Fix (the issue's recommended Option 3 — verify, don't assume; not the session-killing
`systemctl restart user@…`):
- New _cgroup_controllers_active reads the live cgroup.controllers of the user slice
(/sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers; overridable for
tests) and is true only when cpu+cpuset+io are all delegated.
- After the reload, _write_cgroup_delegation checks it: if active -> success ("active
in this session"); if not -> a LOUD warn stating limits won't enforce until a
re-login AND `k3d cluster delete <name>` + re-run.
Tests: _cgroup_controllers_active (all-present / missing / unreadable), and
_write_cgroup_delegation's active-vs-not report.
Closes#496
@shujaatTraceblocshujaatTracebloc self-assigned this Jul 31, 2026
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review July 31, 2026 10:59
…egation-verify
# Conflicts:
#	scripts/manifest.sha256
Comment threadscripts/lib/setup-linux.sh
Comment threadscripts/lib/setup-linux.sh
- Silent re-run skipped verification: _cgroup_controllers_active only ran after a fresh
drop-in write; the idempotent path (file already present) early-returned. So the
re-run the hint tells operators to do took the silent fast path — and with lingering
(the rootless path) a re-login often doesn't restart user@.service, leaving inactive
delegation invisible. Restructured so the report/verify runs on EVERY invocation
(fresh AND idempotent), and the wording now notes a reboot may be needed with linger.
- Wrong prepare-host remediation: _write_cgroup_delegation is shared with
run_prepare_host, but the warn told the admin to log out + `k3d cluster delete` —
prepare-host creates no cluster, and the verify read the ADMIN's user slice, not the
researcher's. Split out _report_cgroup_delegation, mode-aware: prepare-host just
confirms the drop-in is written (takes effect at the researcher's next login); the
full install verifies this session + gives the recreate remedy.
Tests: re-run over an existing drop-in still verifies (no silent fast path); prepare-host
mode uses researcher-login wording with no cluster-delete / no admin-slice judgement.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two things undercut the fix — both make the new behaviour dead on its real paths, and both are masked by tests that set state by hand. Details inline.

Comment threadscripts/lib/setup-linux.sh
Comment threadscripts/lib/setup-linux.sh
Comment threadscripts/lib/setup-linux.sh Outdated
…reviewer)
The round-2 verify/report worked in _write_cgroup_delegation's own tests but
not the actual wiring:
1. _ensure_cgroup_delegation (the only full-install caller) short-circuits at
its OWN fast path before ever reaching _write_cgroup_delegation. On a 2nd+
run over a written-but-not-yet-active drop-in — the exact #496 case — it just
logged "already present" and returned; the verify/warn never ran. Route the
fast path through _report_cgroup_delegation (an unprivileged read, so the
no-sudo property holds) so an inactive drop-in re-surfaces.
2. run_prepare_host resets TB_PREPARE_HOST_MODE right after install_docker_engine,
before the cgroup write — so the report fell through to the full-install branch
and printed "recreate the cluster" advice judged on the ADMIN's slice. Set the
mode around the cgroup write (mirrors install_docker_engine) so it reports in
prepare-host wording.
Tests exercise the real callers now, not the helper in isolation: fast path
re-surfaces an inactive drop-in (+ stays sudo-free), confirms an active one, and
run_prepare_host reports researcher-login wording with no cluster-delete advice.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e5a598e. Configure here.

Comment threadscripts/lib/setup-linux.sh
…Bugbot High)
_cgroup_controllers_active read user-$UID.slice/cgroup.controllers, but Delegate=
on user@.service enables the controllers INSIDE user@$UID.service (the path
runc/rootless-containers document). The slice node routinely lists cpu/io by
default (DefaultCPUAccounting), so the full-install path could print "active in
this session" while the user manager still lacked the delegation and limit-bearing
pods ran unconstrained. Read the manager's node instead; split the default path into
a unit-testable _cgroup_controllers_path.
Also (reviewer): drop the sudo from _write_cgroup_delegation's idempotent cmp — the
drop-in lives under /etc and is world-readable, so a plain cmp matches
_ensure_cgroup_delegation's unprivileged grep and avoids a needless elevation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@shujaatTracebloc
shujaatTracebloc merged commit 7dab299 into developJul 31, 2026
37 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the fix/496-cgroup-delegation-verify branch July 31, 2026 13:40
@LukasWodka

Copy link
Copy Markdown
Contributor

/fr-pass

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@shujaatTracebloc@LukasWodka@saadqbal