metrics: report a worker with no sandbox class as unknown - #1484
metrics: report a worker with no sandbox class as unknown#1484Jeff Luo (JeffLuoo) wants to merge 1 commit into
Conversation
|
Actually, before making this change I have a question about the scheduling - Julian Gutierrez Oschmann (@juli4n) Worker.sandbox_class is optional: substrate/pkg/proto/ateapipb/ateapi.proto Lines 1688 to 1693 in a223367 and it doesn't have a default value. But this field will be used by scheduler to schedule the worker. CreateWorker accepts "" and nothing fills it in. Should we validate it from the API level? |
Control.CreateWorker does not validate Worker.sandbox_class, so a client can register a worker carrying an empty one, and any worker written before this keeps it. RegisterWorkerCount tallied that raw value, emitting ate.workerpool.workers with ate.sandbox.class="" -- not a member of the attribute's registry vocabulary -- next to the pool's seeded series at 0. Normalize the worker's class with ateattr.NormalizeSandboxClass, so an empty class reports as unknown. That is the rule the same helper already applies to this attribute on the atelet side, so the codebase keeps one rule rather than two. The worker is deliberately not folded into the series of the pool it names. Scheduling matches sandbox_class exactly, so a worker with no class can host no actor; counting it as pool capacity would inflate the idle state and silence an idle == 0 alert while every resume fails for want of a worker. Under unknown it stays visible and alertable, and the pool's own counts stay truthful. The empty class enters through the worker record, not the WorkerPool CR: the CRD defaults sandboxClass to gvisor and rejects "" by enum. Validating Worker.sandbox_class at the API boundary would stop the empty state existing at all; that is a scheduling fix, and a separate change.
9bd647a to
365c075
Compare
Jeff Luo (JeffLuoo)
changed the title
|
So, I also want to mention that I think workers will have multiple sandbox classes in the future. Actors will be on a specific sandbox class but the idea that we reserve compute only for gvisor or microVM is an artificial distinction that probably won't hold up as we incrementally improve the dataplane. would that suggest a different approach here? |
Thank you for the context. I patch the PR to drop the change in earlier revision had an empty-class worker inherit its pool's class. This PR only normalizes empty string sandbox class to unknown like other metrics. For the impact of "workers will have multiple sandbox classes in the future" to the metric cc Krisztian F (@krisztianfekete) we may want to revisit some metrics when this happens. |
2 participants
Fixes #1475
Control.CreateWorkerdoes not validateWorker.sandbox_class, so a client can register a worker with an empty one (and any record written before this keeps it).RegisterWorkerCounttallied that raw value, soate.workerpool.workersemittedate_sandbox_class=""— not a member of theate.sandbox.classregistry vocabulary — alongside the pool's seeded series at 0.This change normalizes the worker's class with the existing
ateattr.NormalizeSandboxClass, so an empty class reports asunknown.