Replace actor template namespace/name pair with ActorTemplate atespace and name pair in internal APIs - #1304
Conversation
71b19ba to
dfe7e8c
Compare
|
|
||
| message TerminateWorkloadRequest { | ||
| reserved 4, 5; | ||
| reserved "actor_template_namespace", "actor_template_name"; |
There was a problem hiding this comment.
Don't reserve fields. Just drop them and reuse the IDs. We are breaking the API anyways.
There was a problem hiding this comment.
Done. I renamed "actor_template_namespace" to "actor_template_atespace" instead.
|
|
||
| // ObjectRef references a Substrate resource by its (atespace, name) identity. | ||
| // Mirrors ateapipb.ObjectRef, which internal protos do not import. | ||
| message ObjectRef { |
There was a problem hiding this comment.
ObjectRef is a concept of the control plane API (mgmt plane) where we need a way for resources to reference another resources. We could introduce the same concept in the atelet API (control plane), although I'm not sure how useful that is.
If we adopt it, we should use it consistently, for example, we have atespace, actor_name , which could be an ObjectRef too, but for actors we also have an actor_uid which isn't in the ObjectRef (I guess that could be a separate top level field alongside actor ?
ObjectRef actor;
string actor_uid;
ObjectReg actor_template;
There was a problem hiding this comment.
Dropped ObjectRef concept in internal API for now, since the internal APIs only need the ActorTemplate's name and atepsace for telemetry for now.
| return nil | ||
| } | ||
| return &ateletpb.ObjectRef{Atespace: ref.GetAtespace(), Name: ref.GetName()} | ||
| } |
There was a problem hiding this comment.
This drops the CRD fallback, so actors now might send nil and ate.template.* becomes empty on all the atelet/ateom metrics and actor logs.
Here:
substrate/cmd/ateapi/internal/controlapi/workflow_resume.go
Lines 519 to 526 in 6982894
we already do the if/else against actorTemplateObjectRef, can we do the same until the legacy one is gone?
There was a problem hiding this comment.
I restricted the scope of this PR to just renaming the actor template "Namespace" to "Atespace" in internal APIs. Will fix this in a follow up PR.
The legacy one will be gone in the next 2 days. I'll have a PR to do the cutover so we won't have to maintain 2 branches.
| op := snapshotOp{ | ||
| templateNamespace: req.GetActorTemplateNamespace(), | ||
| templateName: req.GetActorTemplateName(), | ||
| templateNamespace: req.GetActorTemplate().GetAtespace(), |
There was a problem hiding this comment.
ActorMetricAttributes coerces an empty template to TemplateUnknown, this, and at a few other places below emit "". Can we pick one to make it consistent?
There was a problem hiding this comment.
The TemplateUnknown was a temporary hack to fix e2e test during the transition period. Will have a separate PR to use the new substrate atespace everywhere.
| snapshotSizeBytes.Record(ctx, fi.Size(), metric.WithAttributes( | ||
| semconv.FileNameKey.String(file), | ||
| ateattr.TemplateNamespaceKey.String(atNamespace), | ||
| ateattr.TemplateNameKey.String(atName), | ||
| ateattr.TemplateNamespaceKey.String(templateRef.GetAtespace()), | ||
| ateattr.TemplateNameKey.String(templateRef.GetName()), |
There was a problem hiding this comment.
Note that ate.template.namespace is documented like this:
substrate/docs/metrics/registry/metrics.yaml
Lines 74 to 78 in 6982894
but it's an atespace now. Weaver only checks spellings so
verify/metrics.sh stays green either way, we should fix the brief in this PR.
| // Actor identity makes a flat snapshot self-identifying if control-plane | ||
| // persistence is unavailable. | ||
| Atespace string `json:"atespace,omitempty"` | ||
| ActorName string `json:"actorName,omitempty"` | ||
| ActorUID string `json:"actorUid,omitempty"` | ||
| ActorTemplateNamespace string `json:"actorTemplateNamespace,omitempty"` | ||
| ActorTemplateName string `json:"actorTemplateName,omitempty"` | ||
| Atespace string `json:"atespace,omitempty"` | ||
| ActorName string `json:"actorName,omitempty"` | ||
| ActorUID string `json:"actorUid,omitempty"` | ||
| ActorTemplate *actorTemplateRecord `json:"actorTemplate,omitempty"` |
There was a problem hiding this comment.
Can we lose template identity before/after this PR? Probably fine at this point though.
There was a problem hiding this comment.
Yes, during the switch there are some places where the template might log empty. I'll fix it as part of the full cutover.
| templateNamespace: sample.GetActorTemplate().GetAtespace(), | ||
| templateName: sample.GetActorTemplate().GetName(), |
There was a problem hiding this comment.
If the ref is nil every actor collapses into one ("", "") bucket here, so the per-template memory/cpu gauges merge instead of just going unlabeled. Similar issue to #761
The actor template identity carried on atelet/ateom requests, snapshot manifests, and stats samples now names the substrate ActorTemplate (atespace + name), sourced from the resolved ActorTemplate's metadata, instead of the legacy CRD-backed template's kube namespace/name pair.
dfe7e8c to
9e173b3
Compare
9bddf66
into
agent-substrate:main
Both Atelet and Ateom internal APIs consume actor template namespace/name pair to telemetry. Updated them to use the new Substrate ActorTemplate proto instead of the legacy CRD one.