multi-actor worker API - #1283
Conversation
|
x-ref: #1164 🙈 |
96e9c63 to
975778f
Compare
a7b750e to
0533447
Compare
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
I focused on API stuff. The atomic update of an assignment and the worker seems like something that will hurt us later, but maybe workers just not updated often enough for that to matter?
It's tricky logic and could do with more explanation (and specifically the "not updated often enough to matter" rational).
I did not focus on validation yet
| // declared resource limits. An unset message, or a zero field within it, means | ||
| // "unknown/unset" for that dimension: treated as unconstrained so placement is | ||
| // not blocked (matching the pre-capacity behavior). | ||
| // WorkerCapacity is what a worker pod has to give the Actors it hosts, and also |
There was a problem hiding this comment.
This comment is also unclear. I think you should clarify a resource model but "give/take" wording is sort of clunky.
IIUC this is the total capacity of the worker, independent of what may currently be allocated to actors. Is that right? So "available" is "worker.capacity - worker.status.allocated".
How would we model overcommit? Worker lies about capacity? Maybe add an overcommit scalar (e.g. 1.5)?
There was a problem hiding this comment.
How would we model overcommit? Worker lies about capacity? Maybe add an overcommit scalar (e.g. 1.5)?
My gut take was worker lies about capacity.
We will have to filter this stuff up to autoscaling, that approach seems easy enough to reason about but perhaps too limiting.
There was a problem hiding this comment.
We plan to consolidate this with status.
| if err != nil { | ||
| return fmt.Errorf("marshaling worker: %w", err) | ||
| } | ||
| _, err = tx.Exec(ctx, `UPDATE workers SET version = $1, proto = $2 WHERE name = $3`, |
There was a problem hiding this comment.
Shouldn't you do precondition here anyway? (where UID=... AND VERSION=...)
There was a problem hiding this comment.
The row lock handles this currently. But we should probably defensively cover it as well.
FWIW I've pushed 28k claims/s with a test of the storage layer, it scales pretty well with cores.. For overcomitted toy actors, 97k in 60s ... mostly bound by dataplane. This is only going to hurt us if the rest of the system gets really fast or if we have reason to do a lot of other writes to workers unrelated to scheduling. We're by far dataplane bound even at fairly considerable scale with this. We could do something else like ... a distributed scheduling architecture, but I'm not sure that's necessary. I've realized some other defects in this as-is when I got some time to sit and think over the weekend. Working on it.
|
7e01bfb to
725a108
Compare
|
I will iterate on this a bit more, including some cleanup to the commits etc, and notify when it's ready. For now, logging off. |
4644d42 to
cd33f4d
Compare
cd33f4d to
4e67536
Compare
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
Overall this makes sense, I still need to mull the whole thing over, but I am really liking the direction.
Codex found a couple potential races that we can confirm:
Binding commits before the Actor is updated to reference the Worker. If ateapi crashes in
between, the Worker holds an assignment that the Actor does not know about.
Later, DeleteActor skips Worker cleanup when actor.status.worker_assignment is empty. There is
no assignment-to-Actor foreign key or reconciler, so the capacity can remain leaked until the
Worker disappears.
Smallest sound fix: deletion must look up and release an assignment by Actor UID even when the
Actor lacks a backlink. Add a test covering bind → missing Actor update → delete.
When an Actor is already assigned to the same Worker, BindActorToWorker subtracts the old
resources and adds the replacement, but does not call admit.
Because Actor.actor_template is mutable, a stale claim can be resumed with larger limits and
push the Worker over capacity.
Smallest fix: subtract the previous reservation, run admission against that effective Worker,
then add the replacement.
| // The Worker being reported on. atespace is always empty; Workers are | ||
| // global-scoped. | ||
| // +k8s:opaqueType | ||
| ObjectRef worker = 1; |
There was a problem hiding this comment.
Doesn't this need to be inferred from the certificate anyway, is this redundant?
There was a problem hiding this comment.
We don't currently dial directly from the worker, it comes from ateom => atelet.
I have confidence that will change in the future but ... not sure when. Probably after we actually have multi-actor worker ... bit chicken and egg.
| // The Actor currently bound to this Worker, if any. | ||
| // The Actors bound to this Worker. | ||
| // | ||
| // Populated only by GetWorker. ListWorkers leaves it empty and reports |
There was a problem hiding this comment.
So to be clear, this field is stored in the new worker_assignments table, but represented in the API separately?
There was a problem hiding this comment.
Yes. Though we dropped that in a later commit / iteration, for the subresource with ListWorkerAssignments as a dedicated RPC.
| // +k8s:optional | ||
| // +k8s:minimum=1 | ||
| int64 cpu_milli = 1; | ||
| Resources resources = 1; |
There was a problem hiding this comment.
Feel free to throw this away. If we parse this into a map in go anyway, should it be a map in the proto as well?
There was a problem hiding this comment.
Tim Hockin (@thockin) had an earlier comment about reusing the Resources type. I'm on the fence. Later we probably need to model more things. I don't love the Resources type but I like that it matches what we have on ActorTemplate.
There was a problem hiding this comment.
So I lean towards keeping it for now. If we want to use cpu_milli / memory bytes consistently we should rewrite ActorTemplate too.
There was a problem hiding this comment.
I don't love Resources but it maps to k8s. There are ...issues... properly handling quantity strings, which I am chasing down on the k8s side. I think we should ultimately decide to either support quantity strings all over or enforce quantized int values (e.g. cpu_millicores, memory_mb) all over.
There was a problem hiding this comment.
I suggest we bulk edit that in a follow-up, it's somewhat orthogonal and I'm not totally sold either way yet.
We're also going to need more complex resource types O(soon) to express GPUs properly, a count really isn't sufficient.
| // The ordinary bind is a first bind, and the insert landing is proof | ||
| // nothing was there to account for: one statement, no read. |
There was a problem hiding this comment.
I really hate to be that guy but these comments are horrendously confusing
There was a problem hiding this comment.
fair :-)
4e67536 to
bfcd462
Compare
These are real and should be fixed. I need to do some other remaining cleanup. |
bfcd462 to
787f032
Compare
787f032 to
e56db3c
Compare
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
Proto seems straight forward at this point
| // | ||
| // +k8s:optional | ||
| // +k8s:minimum=1 | ||
| int32 page_size = 2; |
There was a problem hiding this comment.
Julian Gutierrez Oschmann (@juli4n) Should we bundle these into a generic ListOptions ?
| // | ||
| // +k8s:optional | ||
| // +k8s:immutable | ||
| WorkerCapacity capacity = 10; |
There was a problem hiding this comment.
As per discussion, my takeaway was this moves to status. If you disagree respond, else close this in due course. Or are we killing this entirely?
| } | ||
|
|
||
| message WorkerStatus { | ||
| reserved 2; // was assignments, now the ActorAssignment subresource |
There was a problem hiding this comment.
We really don't need this
| // What this Worker can supply to the Actors it hosts. | ||
| // | ||
| // Mutable: a Worker can be resized, and it reports its own actor ceiling. | ||
| // Clearing is rejected. Shrinking below allocated stops new placements and |
There was a problem hiding this comment.
If you want to prevent clearing: +k8s:update=NoUnset
| // +k8s:optional | ||
| // +k8s:minimum=1 | ||
| int64 cpu_milli = 1; | ||
| Resources resources = 1; |
There was a problem hiding this comment.
I don't love Resources but it maps to k8s. There are ...issues... properly handling quantity strings, which I am chasing down on the k8s side. I think we should ultimately decide to either support quantity strings all over or enforce quantized int values (e.g. cpu_millicores, memory_mb) all over.
| // What a Worker supplies, named as an ActorTemplate names what it asks for, | ||
| // so the two are one vocabulary and subtract directly. A name the Worker does | ||
| // not report is unconstrained rather than absent, so a Worker that has said | ||
| // nothing is not unschedulable. |
There was a problem hiding this comment.
I don't think this semantic works -- if I ask for 128 cores, you can't put me somewhere unless you know I have them. If I ask for GPU, you can't put me on a worker that has no GPUs.
There was a problem hiding this comment.
Agree ...
Aside: this is also a meh claude comment for sure.
my main takeaway from this PR is to cut off claude for API changes and stick to boilerplate fixes. even writing out what you want winds up with ... ehh not there yet.
next time I will entirely hand-craft the API commit at minimum.
| // Actor is placed only where free covers its limits in every dimension. | ||
| // | ||
| // A Worker overcommits by reporting more than it has. | ||
| message WorkerCapacity { |
There was a problem hiding this comment.
Do we need this intermediate struct if we use Resources?
| // carried them would grow with occupancy. The inverse of WorkerAssignment. | ||
| message ActorAssignment { | ||
| // atespace is always empty, as it is on the Worker these belong to. name is | ||
| // the Actor's UID, so the Actor that caused an assignment is what addresses |
There was a problem hiding this comment.
I thought we said that metadata.name would match metadata.uid but I am also fine with this approach.
| message SetWorkerCapacityRequest { | ||
| // The Worker being reported on. atespace is always empty; Workers are | ||
| // global-scoped. | ||
| // +k8s:opaqueType |
| // What the Worker can hold. An unset dimension keeps what is recorded rather | ||
| // than clearing it, so a reporter that knows only its actor ceiling does not | ||
| // erase the compute capacity taken from the pod's limits. | ||
| // +k8s:opaqueType |
status.assignment held a single Actor. It becomes repeated, reusing field 2 so an old record reads back as a set of one, and status.allocated carries the running total that placement reads for every Worker on every decision. WorkerCapacity now holds a Resources set, named the way an ActorTemplate names its limits so that the two subtract, alongside an actors ceiling for the costs that cpu and memory do not cover. ActorAssignment.resources records what an Actor was admitted for. Capacity can now change over a Worker's life, but it cannot be cleared. A Worker reports its own capacity through the new WorkerReporting service. That is separate from Control because it is served only to atelet, and only for the Workers on its own node.
Readers use resources.WorkerAssignmentFor, and binding and releasing update status.allocated alongside the assignment list. Accounting moves to named quantities in resources.Quantities, which emits them in sorted order. Resources is a repeated field, so two totals that differ only in order compare unequal and rewrite the record for no change. Placement compares what is free in each dimension, and eligibility is asked separately from room. A caller re-validating a Worker that already holds the Actor would otherwise be told there is no space and evict it. A Worker whose recorded capacity does not parse is treated as full. Clearing capacity is rejected by a hand-written check. Declarative validation can mark a field immutable, but it cannot allow a field to change while still requiring it to be set. ListWorkers omits the assignments and reports occupancy through allocated. kubectl-ate prints ASSIGNED(n/m).
atelet reports on behalf of the Workers on its node, authenticated by its own client certificate as it is for MintCert. A Worker on another node comes back as NOT_FOUND. The atelet authentication moves into a package both services share. Capacity is the Worker's to report rather than the control plane's to infer, so the syncer no longer derives cpu and memory from the pod. Until a report arrives, the ceiling CreateWorker reifies holds the Worker to one Actor. A reporter can leave out dimensions it does not know, and those keep whatever is already recorded. Re-sending an unchanged capacity does not write. Nothing calls this yet.
A Worker carried its assignments inside its own record, so the record, its change event, and every watcher's copy of it all grew with the number of Actors on it. Each assignment becomes a row of its own. status.allocated stays on the Worker, because placement reads it for every candidate. The store updates it in the same transaction as the assignment it counts. Binding does not first read the assignment it might be replacing. The insert landing is proof that nothing was there, and a read could not see a claim that commits after it.
Claiming a worker rewrote its whole record, so goroutines racing on the same worker refused one another: 21% of activations failed at 12 in flight, and 67% at 24. BindActorToWorker now takes an admit callback and asks inside the transaction that holds the worker's row lock, so the answer cannot go stale before the write, and a refusal rolls the speculative row back. That removes the version precondition, the fresh read, and the assignment lookup. The retry budget had to grow, because a claim refused for want of room re-runs scheduling.
e56db3c to
15394e9
Compare
4 participants
Part of #1266
This is a draft of the core API + data store changes.
It's still a large PR, apologies.
The "as rows" commit could be split out, but this takes it to ~all of the breaking changes we can't hide behind updating internals.
Same for the claimlock, but in both cases it seems these are worth understanding when considering the API shape.
They're loadbearing for performance once we actually have multi-actor workers.