neonvm: Use crictl to change container CPU, ditch cgroup

[sharnoff]

We recently realized¹ that under cgroups v2, kubernetes uses cgroup namespaces which has a few effects:

1. The output of /proc/self/cgroup shows as if the container were at the root of the hierarchy
2. It's very difficult for us to determine the actual cgroup that the container corresponds to on the host
3. We still can't directly create a cgroup in the container's namespace because /sys/fs/cgroup is mounted read-only

So, neonvm-runner currently does not work as expected with cgroups v2; it creates a new cgroup for the VM, at the top of the hierarchy, and doesn't clean it up on exit.

How do we fix this? The aim of this PR is to remove the special cgroup handling entirely, and "just" go through the Container Runtime Interface (CRI) exposed by containerd to modify the existing container we're running in.

This requires access to /run/containerd/containerd.sock, which a malicious user could use to perform priviledged operations on the host (or in any other container on the host). Obviously we'd like to prevent that as much as possible, so the CPU handling is now runs alongside neonvm-runner as a separate container. neonvm-runner does not have access to the containerd socket.

On the upside, one key benefit we get from this is being able to set cpu shares, the abstraction underlying container resources.requests. The other options weren't looking so great², so if this works, this would be a nice compromise.

---

Honestly have no clue whether this'll work. Haven't tested it at all (except for some ad-hoc interactions with crictl from ssh-ing into a kind node). Wanted to open this for visibility, in case there's something silly I missed.

---

Fixes #755.
Fixes #591.

Footnotes

1. https://neondb.slack.com/archives/C03TN5G758R/p1705092611188719 ↩

2. https://github.com/neondatabase/autoscaling/issues/591 ↩

[Omrigan]

Looks good. I guess the difference will be is that neonvm-runner will always be subject to cgroup restrictions, not just QEMU process. But that shouldn't be a problem. Right?

Also, why do we have to set container limits through crictl, isn't there a proper k8s way of setting pod's resource limits on a cluster scale?

[sharnoff]

neonvm-runner will always be subject to cgroup restrictions, not just QEMU process

Yeah- this is different, but I figured probably better anyways — it's more thorough resource limiting, or something like that.

isn't there a proper k8s way of setting pod's resource limits on a cluster scale?

Unfortunately, k8s <1.27 doesn't allow changing a pod/container's resources after it's created (partly why we're in this whole mess in the first place), and the upgrade process is complex. See also #591, neondatabase/cloud#7922

[cicdteam]

LGTM, but... I still don't understand why we need this whole cgroup dance if we have cpuLimitOvercommitFactor = 4 (IIUC, which means that CPU cgroup limits are not relevant, since qemu limits the guest itself by CPU cores)

[areyou1or0]

Replying to your questions from Slack:

Big risk comes from providing direct access to containerd API from within the pod, but in a separate, dedicated container.

In case of exploiting a vulnerability on the CPU handling container, an attacker could gain access to the CRI and potentially escalate their privileges. (container breakout, bypassing network controls etc.)

One way to prevent this scenario besides isolating the CPU handling container is to scanning the CPU handling container for vulnerabilities (which we already do) & monitoring pod activity regularly. (I'm skeptical on the current capability with vuln.scanner, perhaps guardduty may cover this as well)

From the pod-to-pod exploitation, in case of a cross-container attack where an attacker exploit a vulnerability on another container in the pod, could access to CRI and to CPU handling container.

Most obvious way to prevent this scenario is implementing least privilege, restricting access to the CRI., and implementing strong network security controls.

Wondering whether it adequately prevents privilege escalation (are there exploits that allow moving laterally within a pod but not further?)

Moving CPU handling to a separate container naturally helps preventing PE scenarios within the pod itself at some level, but in case of gaining unauthorized access to the CRI via another possible vulnerability can still lead to escalate privileges in theory.

[sharnoff]

I still don't understand why we need this whole cgroup dance if we have cpuLimitOvercommitFactor = 4 (IIUC, which means that CPU cgroup limits are not relevant, since qemu limits the guest itself by CPU cores)

There's two reasons:

1. QEMU can use more CPU time than the number of CPUs would indicate, often when doing IO (this is actually the original reason for the change - see neonvm/runner: Add overcommit factor for CPU limits #509)
2. We do want to have some upper bound on the CPU usage, just in case.

This also would resolve #591 (see there for more info).

[Omrigan]

#711 suggests a hypothesis the number of auxiliary containers might impact compute cold start time. So this might make the situation worse.

[sharnoff]

@Omrigan IIUC the impact is from adidtional Spec.InitContainers, not Spec.Containers, right? (iiuc, impact is specifically because k8s doesn't create the next InitContainer until the previous one finished, so sandbox creation isn't pipelined)

[shayanh]

Update reserved volume names here:

autoscaling/neonvm/apis/neonvm/v1/virtualmachine_webhook.go

Line 118 in 65e4393

"sysfscgroup",

[shayanh]

Looks good in general. Added several small things. Also I have a question about testing. Have you checked out cAdvisor metrics to work as expected here?

[Omrigan]

@Omrigan IIUC the impact is from adidtional Spec.InitContainers, not Spec.Containers, right? (iiuc, impact is specifically because k8s doesn't create the next InitContainer until the previous one finished, so sandbox creation isn't pipelined)

Not sure yet. At least, additional "main" container will put more pressure on the containerd, potentially delaying other parallel requests.

[sharnoff]

@shayanh I'm going to resolve the comments you left about the http handling for now. All the code there was directly copied from the existing implementation in runner/main.go, and I'd rather either move or modify, not both (this also means that it keeps the same API).

Happy to address things in a follow-up.

[shayanh]

@sharnoff makes sense. I'll make a note to open an issue regarding them later.

[sharnoff]

PR in its current state hasn't replicated this — so, right now there's a regression where a 0.25 CPU VM starts at 1 CPU and needs to scale down.

Probably need to add milli CPU as an arg to container-mgr or something, and then do a similar "set on startup". (and actually, that'd also remove the need for nil-able cpu returned from the runner pod as well, so can revert 5acf536)

[sharnoff]

fixed in 2af7af5

[shayanh]

I don't understand why we need the new arguments here. Can't we just set the initial limits once container-mgr is starting?

[sharnoff]

In theory, yeah. In practice, it seemed like it wasn't simple to make sure that we didn't get any weird transitory states — we'd have to update the runner before moving from Pending to Running, or e.g. if .status.cpus is nil, update the runner before fetching the info.

The PR in its present form (reverted nillable runner CPU; container-mgr has the new arg) doesn't need any of the meat of the reconcile logic to be updated (with the notable exception of #749). That's my primary motivation (because the existing logic is so complex).

[sharnoff]

suggestion from @kelvich: use a flag to enable this behavior, so that we can do very slow rollout (e.g. 1 week in a region before moving on).

Edit: this has now been implemented, in 5c93572

[shayanh]

I added a few more comments. With the new commits the changes here are quite large and tricky (particularly, dealing with multiple containers in the controller).

[shayanh]

Looks good to me. There is only a small thing mentioned in this comment:
#738 (comment)

[sharnoff]

only a small thing mentioned in this comment

Ah right, good catch - thanks!

[sharnoff]

Current status:

• Blocked on (approving and merging) neonvm: Use container statuses, not pod phase, to trigger restart #749
• Need to test on staging — as yet, haven't checked it works on EKS (although I expect it should be ok)
  • Feature works, but it substantially increased reconcile times. Commentary here.

Currently, the behavior is disabled by default, with a neonvm-controller CLI flag to enable it. The plan will be to gradually enable that flag, region-by-region, over the course of 1-2 weeks.

[sharnoff]

Follow-up that was brought up in a meeting: We should try out using the client library directly, rather than via the CLI. May substantially reduce latency.

[shayanh]

Related: I spent a couple of hours setting up autoscaling on a fresh Ubuntu bare metal machine and it didn't work out first due to our legacy cgroup stuff. Apparently, we don't use the right cgroup version in some cases, which doesn't allow the VMs to start. It's nice that we are moving away from manually reading and modifying cgroup files.