Repository navigation
rename ateom -> ateworker - #2254
Benjamin Elder (BenTheElder) wants to merge 16 commits into
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
cbdecb7 to
8964f28
Compare
| echo " for each WorkerPool pod (default: unset, the pod is unsized)." | ||
| echo " --wait-timeout SECONDS Forwarded to workloads/deploy.sh. The timeout in seconds for" | ||
| echo " waiting for the ateom workers to be ready (default: 300)" | ||
| echo " waiting for the ateworker workers to be ready (default: 300)" |
There was a problem hiding this comment.
So this was a direct find and replace (automated at that) but it reads weird. instead
| echo " waiting for the ateworker workers to be ready (default: 300)" | |
| echo " waiting for the workers to be ready (default: 300)" |
8964f28 to
a6d3602
Compare
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
TL;DR: the places where we just refer to the abstract idea of "a worker" should just say "worker". E.g. if someone could replace our baked-in ateworker with their own worker and the sentence would still make sense, "worker". "ateworker" should refer to specifically our main implementation(s).
But also, most of this is internal - comments etc. Focus on public facing stuff -- proto names and methods and the SPIFFE stuff.
| @@ -188,7 +188,7 @@ func retryable(err error) bool { | |||
| func requestOnce(ctx context.Context, conn grpc.ClientConnInterface, actor Actor) error { | |||
| callCtx, cancel := context.WithTimeout(ctx, requestTimeout) | |||
| defer cancel() | |||
| _, err := ateletpb.NewAteomSupportClient(conn).RequestActorSuspend(callCtx, &ateletpb.RequestActorSuspendRequest{ | |||
| _, err := ateletpb.NewWorkerSupportClient(conn).RequestActorSuspend(callCtx, &ateletpb.RequestActorSuspendRequest{ | |||
There was a problem hiding this comment.
In some places "Ateom" -> "Worker" and in others "AteWorker". e.g. certificateSource.MintAteWorkerCertificate(ctx) I can't discern a clear rule.
| // otelOverrideDeployments are the control plane Deployments that read | ||
| // ate-otel-config. ate-controller additionally copies the values onto the | ||
| // ateom worker pods it creates, so one patch reaches the whole system. | ||
| // ateworker worker pods it creates, so one patch reaches the whole system. |
There was a problem hiding this comment.
super nit: "ateworker worker" -> "worker" ?
| crashMessageWorkerIneligible = "assigned worker no longer satisfies the actor's placement constraints" | ||
| crashMessageWorkerPodGone = "worker pod went away while hosting the actor" | ||
| crashMessageAteomRestarted = "ateom restarted while hosting the actor" | ||
| crashMessageAteWorkerRestarted = "ateworker restarted while hosting the actor" |
There was a problem hiding this comment.
Above says "worker pod" this says "ateworker" - I don't love embedding the word "pod" here but "worker" seems correct in both
|
|
||
| req := &ateletpb.TerminateRequest{ | ||
| TargetAteomUid: assignment.GetWorkerPodUid(), | ||
| TargetAteWorkerUid: assignment.GetWorkerPodUid(), |
There was a problem hiding this comment.
Here's a place where "Worker" is clearly more correct - it's the UID of the Worker resource.
| // into the snapshot manifest. | ||
| req := &ateletpb.CheckpointRequest{ | ||
| TargetAteomUid: assignment.GetWorkerPodUid(), | ||
| TargetAteWorkerUid: assignment.GetWorkerPodUid(), |
There was a problem hiding this comment.
Another place where "Worker" is clearly more correct - it's the UID of the Worker resource.
| // run and one more for each restart. 0 means it has not been reported. | ||
| // | ||
| // A restarted ateom has lost the sandboxes of the Actors it was hosting, so | ||
| // A restarted ateworker has lost the sandboxes of the Actors it was hosting, so |
| @@ -2152,7 +2152,7 @@ message WorkerStatus { | |||
|
|
|||
| // observed_epoch is the latest epoch whose earlier Actors the control plane | |||
| // has crashed and released. While it is below epoch, Actors placed before | |||
| // the ateom's last restart may still be reported as running. | |||
| // the ateworker's last restart may still be reported as running. | |||
| @@ -2225,19 +2225,19 @@ message ActorAssignment { | |||
| // only to an atelet, and only for the Workers on its own node. | |||
| service WorkerService { | |||
| // SetWorkerCapacity records what a Worker can hold. Capacity is the Worker's | |||
| // to report rather than the control plane's to infer: it is what the ateom | |||
| // to report rather than the control plane's to infer: it is what the ateworker | |||
| // on behalf of a particular actor. | ||
| // | ||
| // SPIFFE URI: spiffe://${trustdomain}/ateom-for-actor/${atespace}/${actor} | ||
| rpc MintAteomActorCertificate(MintAteomActorCertificateRequest) returns (MintAteomActorCertificateResponse); | ||
| // SPIFFE URI: spiffe://${trustdomain}/ateworker-for-actor/${atespace}/${actor} |
There was a problem hiding this comment.
This is a very consequential one - should SPIFFE say "ateworker" or "worker"? I think "worker".
| return &url.URL{ | ||
| Scheme: "spiffe", | ||
| Host: ActorSPIFFETrustDomain, | ||
| // TODO(identity): Prefix with "atunnel" to prevent | ||
| // confusion between atunnel and an actor pretending to be | ||
| // an atunnel. | ||
| Path: path.Join("ateom-for-actor", r.Atespace, r.Name), | ||
| Path: path.Join("ateworker-for-actor", r.Atespace, r.Name), |
There was a problem hiding this comment.
Taahir Ahmed (@ahmedtd) this is a decision that will stick for a long time?
Agree.
Yeah that makes sense, we can punt internal cleanups to later. |
|
I do think we should rename the internal packages to match and that's going to cause churn but we can do that separately from nailing down the user-visible renames. |
722c093 to
d11206d
Compare
ateom was named when it hosted a single actor; a worker now hosts many. cmd/ateom-gvisor and cmd/ateom-microvm become ateworker-gvisor and ateworker-microvm, and so do their images.
The package name is part of every method name on the wire.
Ateom becomes ateworker.Worker and AteomSupport becomes atelet.WorkerSupport: the proto package already names the component. AteomHerder, which ateapi calls to drive actors on a node, becomes atelet.Atelet.
The field carries the worker pod's UID, matching worker_pod_uid in the public API.
Match the worker services.
The certificate asserts a worker acting for an actor, whatever the worker implementation.
ateworker is our implementation; these APIs describe any worker.
The container runs whatever image the WorkerPool names, so it is the worker container, not necessarily an ateworker.
The phases time the worker's restore and checkpoint calls, whatever the worker implementation.
Client-visible errors, the crash message, CRD and user docs, manifests and the metric registry say worker for the generic concept and ateworker for our implementation.
The keys are documented and joined by benchmarking tooling, so they follow the binary's name.
Operators read these, so they follow the same rule as the docs.
The worker's own leaf under its container cgroup becomes worker. The micro-VM guest's per-actor container parent becomes /actor, a name other runtimes can share.
Per-worker directories move to /var/lib/ate/workers/<pod-uid> with a worker.sock, and the support socket becomes worker-support.sock. The /var/lib/ate hostPath volume is ate-base everywhere, and the capacity volume is worker-capacity.
Covers the authz relation, e2e fixtures, benchmark names and queries, and the remaining docs. Internal Go package names are left for later.
Code outside the ateworker binaries says worker; the binaries say ateworker for themselves. Go package names are left for later.
d11206d to
4702dca
Compare
Breaking change
Renames the binaries and RPCs ahead of GA. Breaking change.
Fixes #2065