Repository navigation
require actor cpu and memory limits - #2285
Conversation
Benjamin Elder (BenTheElder)
added
area/api
0c0e541 to
693965e
Compare
|
DO NOT MERGE for a chance for input, this is kind of a big change. I think the ideal API might look more like:
That has some edge cases to reason about and some tricky memory footprint awareness. So I propose we make our existing We could default limits instead, but I don't think we'll get a better default than users at this stage. |
|
In the same spirit that some think a best practice is to not set CPU limits in pods, why do we think is a good idea to put cpu limits on actors? Why its so critical to set limit now vs. leaving configurable in terms of GA API change? |
Because we need to be able to migrate sandboxes between machines, the reasons are in both the PR body and the comment above. Right now if you don't set limits, you don't even have requests, because requests are missing. If we had more time, we'd add requests now, I think we'll have to add that post-GA.
Because we cannot reliably schedule actors without a more complex implementation in the short term. |
I thought actorTemplate.resources (* ResourceRequirements) is where requests are specified and use for scheduling today |
| // +k8s:maxItems=2 | ||
| // +k8s:listType=map | ||
| // +k8s:listMapKey=name | ||
| repeated Limits limits = 1; |
There was a problem hiding this comment.
Carlos Santana (@csantanapr) there are only limits, no requests, that type is unchanged by this PR.
693965e to
955642a
Compare
Benjamin Elder (BenTheElder)
removed
the
DO NOT MERGE
|
We had a general ACK in the community call today, this still needs code review though. |
5825f32 to
5e357bd
Compare
5e357bd to
a4b3d23
Compare
the scheduler books these against worker capacity, so every template should declare its size.
an actor without limits fit on any worker and booked nothing. micro-VM also bakes its size into snapshots, so the old 2GiB/1 vCPU fallback could resume onto a worker too small to hold it. drop the micro-VM default sizing.
a4b3d23 to
ed2079f
Compare
|
|
||
| // ValidateCustom_ActorTemplate_Resources requires both a cpu and a memory | ||
| // limit, which the scheduler books against worker capacity. | ||
| func ValidateCustom_ActorTemplate_Resources(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *ateapipb.Resources) field.ErrorList { |
There was a problem hiding this comment.
Hmm, DV has a way to specify list-items but apparently it doesn't compose with "required". TODO.
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
I am anxious about the implications of this, but it seems better than nothing.
Yeah. This is extremely high on my post-GA list, I think we need a complete revamp of how we track actor footprint and offer requests + limits and make the scheduler aware of the snapshot size for actors without limits. But I don't think we have the bandwidth to review an API change like this before GA. CPU throttling is probably the biggest risk here, we could probably prioritize relaxing the CPU limit requirement first if we need, since it doesn't have the same degree of placement risk for resume. |
We can still consider more flexible overcommit in the future, but I think leaving actors uncapped is unsafe, and it's hard to pick a reasonable universal default, it's too workload dependent.
Actor authors should really be aware of how much resources their actor requires, and tuning them can have significant performance impacts.
In the future we could do something like determining how much memory was in use at snapshot time and consider that for scheduling resumes.
Breaking change
ActorTemplates must declare CPU/memory limits.
Previously
microvmactors had defaulting, and gvisor actors were simply uncapped.This made the scheduler unaware of placement for gvisor.
Requiring it also ensures actor owners are aware of how much resources they cost.
Setting actor size will be important for performance particularly with microvms where higher memory limits have a real cost for e.g. snapshot/resume.