Repository navigation
A gVisor actor going over its memory limit no longer takes down every actor on its worker - #3106
Conversation
Benjamin Elder (BenTheElder)
added
kind/bug
| defer usageStdout.Close() | ||
| ateworkerService.usage = workerstats.NewUsageEmitter(lp, usageStdout, pool) | ||
| defer workerstats.StartSampler(ctx, *usageSampleInterval, func(ctx context.Context) { ateworkerService.sweepUsage(ctx) })() | ||
| if cgroupDelegated { |
There was a problem hiding this comment.
this concept is detritus, but I'm filing a separate cleanup for that. we ONLY run non-privileged workers, will have to rebase one of them but it's not really in scope for this PR
There was a problem hiding this comment.
#3110 will fix this, we have a bunch of unnecessary complexity from trying to support a mode that hasn't existed for most releases at this point (privileged: true worker pods)
f5094f7 to
cdeb11b
Compare
Workers are always unprivileged now, so delegation never fails to apply. Fail at startup if it can't rather than quietly running actors without their cgroups, which drops their cpu limits and checkpoint cache isolation, and drop the flag both workers threaded through. <!-- The PR title becomes the release note. Write it for users. --> Why is this needed? (Or link the issue. Open one first for features, API, install or CI changes.) eliminates dead code, reduces unnecessary complexity for implementing PRs like #3106 (comment) Fixes # <!-- e.g. "AI-assisted, I reviewed and tested it." No AI co-author trailers. --> AI use: AI assisted, I reviewed and tested it. <!-- Breaking change? Describe what users must do on upgrade and ask for the `breaking-change` label. --> - [x] Tests pass - [x] Docs updated --------- Co-authored-by: Davanum Srinivas <[email protected]>
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
None of this is really specific to gVisor, right? The uVM VMM could just as easily run away with memory. That would be OUR problem, not the user's, but if it kills the whole pod it's ALL of our problem.
This is going to be a subsystem that warrants some deeper work post 1.0. Factoring in requests/limits and overcommit and near-realtime behavior, this is both subtle and complex.
| // https://github.com/opencontainers/cgroups/blob/19a241326c83a5100dff94732733791a0c9e4720/systemd/v2.go#L366-L382 | ||
|
|
||
| // memoryWatchInterval is how often sandbox leaves are checked. | ||
| const memoryWatchInterval = 250 * time.Millisecond |
There was a problem hiding this comment.
OK for GA, but do the math: 4 QPS * num_actors could be 4K - 40K QPS polling -- we will need a more scalable way to do this, pretty soon.
There was a problem hiding this comment.
OK for GA, but do the math: 4 QPS * num_actors could be 4K - 40K QPS polling -- we will need a more scalable way to do this, pretty soon.
I agree we should work to make this cheap, but we cap at 1k actors (which is perhaps unrealistic at the moment anyhow) so 4k, not 40k? Are you saying with a further 10x overcommit later?
We're only reading cgroupfs at least, not proc.
We could use:
- PSI triggers
- We would need to add
CAP_SYS_RESOURCEwhich is going to be problematic after GA so we should consider it now
- We would need to add
- inotify watches
- ... that's a lot more complex, and inotify watches have a problem with the limits not being namespaced and therefore the limits being easily exhausted
- epoll
- plausible, probably still needs a periodic sweep fallback
I'd suggested epoll as a follow-up because this code is reasonably well tested and we need to mitigate this and then next mitigate uVM properly. epoll could land on top of the uVM follow-up, and if it doesn't make GA that's ok, the behavior stays the same and we're probably not running quite that dense yet.
There was a problem hiding this comment.
Note that sometimes kernel vritual FS's do not play well with inotify and epoll. If it works, cool. I just don't want 1000 goroutines trying to do 4QPS each perpetually.
There was a problem hiding this comment.
ACK.
That's dependent on changes in the other PR thread, but yes and no. uVM is basically covered by the overhead currently and we don't cap the processed outside the worker, most of that is page cache, we need to, but that's another PR. Most memory is inside the guest, which has it's own oomkiller and hard cap not visible to the host cgroups. |
it decides from how long a cgroup stalled on memory while hitting its own memory.high, or from how far over the limit it got, so a worker can kill it itself rather than leave it to the kernel's OOM killer.
…on its worker on nodes with runc 1.5.2+ and systemd 253-258 any kernel OOM kill under the worker stops the whole worker, so sandbox cgroups get memory.high at the actor's limit instead of memory.max, and the worker kills a sandbox the watch finds stuck with cgroup.kill, which systemd ignores.
- remove a sandbox cgroup an earlier activation left behind, runsc joins an existing one without applying its limits - kill in the background and time each check on its own, so one slow kill can't skew or stall the others - keep memory.max when the kernel has no cgroup memory pressure (PSI), and warn - pause the watch while a checkpoint moves the sentry out of its sandbox cgroup
b1dc1c0 to
3e17de3
Compare
|
[rebase onto #3110 which simplifies this a bit, dropping the handling of privileged containers / we're always in delegate mode now] |
… down its worker (agent-substrate#3429) A microVM actor's cloud-hypervisor and virtiofsd run in the worker pod with no memory limit of their own. If they grow past what the actor booked, they take the pod to its limit, and the OOM kill takes out the worker and every actor on it. agent-substrate#3106 fixed this for gVisor, this PR fixes it for microVM. Part of agent-substrate#3092. - each actor's cgroup gets `memory.high` at what it books (its memory limit plus the runtime overhead from agent-substrate#3104). The guest can't use more than its memory limit, so anything past that is cloud-hypervisor or virtiofsd. - the worker kills a cgroup stuck at its `memory.high` with `cgroup.kill`, using the same rule as agent-substrate#3106, and reports the actor crashed with "killed for exceeding its memory limit". Other actors keep running. - the watch pauses during snapshot writes. When the pod can lend the whole write, the cgroup's `memory.high` is lifted for the write and restored after, same as agent-substrate#3353. - without PSI the worker sets no limit and warns at startup, like gVisor - the gVisor watch from agent-substrate#3106 moves into a shared `cgroup.MemoryGuard` that both workers use, gVisor's behavior is unchanged Tested on GKE with a fresh install: - a 1GiB actor filled to 925MiB peaked at 1007MiB against a `memory.high` of 1036MiB, and survived 3/3 suspend/resume cycles with no kills - a 400MiB process added to one 512Mi actor's cgroup got that actor killed and reported crashed. Its neighbour kept answering and the worker didn't restart. AI use: AI-assisted, I reviewed and tested it. - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Benjamin Elder <[email protected]>
When a container inside a micro-VM goes over its memory limit (or the guest runs out of RAM), the guest kernel kills it and nothing on the host notices: the actor stays RUNNING, no worker log fires, and the only symptom is the workload going silent. gVisor reports the same condition (agent-substrate#3106); microVM had no equivalent. Wire up kata's GetOOMEvent: ateworker now runs the same long-blocked call the kata shim uses, one goroutine per running actor, alongside watchVM and the stdout/stderr forwarding (boot and restore both). On an event it logs a WARN with the actor's ateattr identity and the killed container's name, falling back to the raw guest container id when unknown. The watch multiplexes over the existing agent connection and ends when the connection does: teardown nils and closes the agent under actorsMu (the watcher copies it out the same way the graceful shutdown drain does), and a VM death fails the in-flight call while watchVM reports the crash. This covers the observability step of agent-substrate#3615. The follow-ups the issue lists - moving the actor out of RUNNING on a guest kill, and logging the console.log tail before cleanup deletes it - are left to the issue: the state decision builds on agent-substrate#2972 (container exit -> CRASHED), and the console tail is tracked by agent-substrate#2212 item 6. Updates agent-substrate#3615 Signed-off-by: yjjnls <[email protected]>
On some nodes, one gVisor actor using too much memory takes down every actor on its worker, not just itself. See #3092 for details.
We can't turn that off from inside the pod, so this makes sure the kernel never OOM-kills inside the worker. Instead:
memory.highat its memory limit instead ofmemory.max, so going over the limit slows the actor down instead of killing itcgroup.kill, which systemd ignores"Stuck" means the actor spent at least a quarter of the last 2 seconds stalled on memory (from
memory.pressure) while hitting its own limit. We also kill right away if it gets more than 16MiB (or 1/16 of its limit, if smaller) over, since a throttled actor keeps creeping up slowly. Just sampling memory usage didn't work, the kernel keeps pulling a throttled actor back just under the limit between allocations.I searched for prior art like meta's oomd, systemd-oomd, and lmkd. Those projects have a slightly different goal/tuning with machine-wide oom, so we borrow some ideas but we have tighter time thresholds.
Part of #3092. The other half of that issue, the whole worker pod running out of memory, is separate.
This is pre-factored into the shared cgroup package, microVM doesn't currently limit cgroups outside of the microVM, I will sequence that change next, keeping this PR scoped to the mechanism, and the critical gVisor update.
AI use: AI-assisted, I reviewed and tested it.
Tested on GKE 1.37.1 (runc 1.5.2, systemd 254): an actor allocating 400MiB against a 256MiB limit was killed alone about a second after hitting its limit, and its neighbour and the worker kept running. Without this change the same thing restarts the whole worker.