trending/substrate
1
0
Fork 0
mirror of https://github.com/agent-substrate/substrate.git synced 2026-10-02 03:24:42 +08:00
Code Issues Packages Projects Releases Wiki Activity

ateapi: stamp the worker pool on successful suspend and pause (#1075)

Browse Source 
Fixes #957

`ate.actor.lifecycle.operation.duration` carried the pool pair on
suspend and pause only when they failed, so per-pool dashboards saw
those two operations exclusively as failures.

Both workflows record the histogram from a defer that reads the `actor`
variable, and the happy path reassigns it to the finalized record. The
finalize step commits the new state and the cleared `WorkerAssignment`
in one update, so the defer found no assignment and dropped both keys. A
failure returns the pre-finalize record, which still names the worker.

Both now snapshot `lifecycleOpAttrs(...)` just before the finalize step
— the same snapshot-before-clear crash.go does for the crash counter.
Paths that end earlier keep the current computation.

`delete` stays without a pool: it only runs from SUSPENDED or CRASHED,
which already released the worker, so there is none to name.

TestLifecycleOpPoolAttributesOnSuccess drives a real suspend and pause
through the gRPC service; both subtests fail without the fix.

- [x] Tests pass
- [x] Appropriate changes to documentation are included in the PR
This commit is contained in:
Jeff Luo
2026-08-20 11:55:18 -04:00
committed by GitHub
parent 86a736c90c
commit bc59db783a
5 changed files with 138 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/ateapi/internal/controlapi/actor.go
+2 -1
View File
@@ -334,7 +334,8 @@ func (s *Service) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorRequ
}
start := time.Now()
// Template dims only once the record resolved: the request names only the
// actor, so failures before the load carry none.
// actor, so failures before the load carry none. No pool pair: delete only
// runs from SUSPENDED or CRASHED, which already released the worker.
defer func() {
var attrs []attribute.KeyValue
if deleted != nil {
cmd/ateapi/internal/controlapi/functionaltest/actor_test.go
+103
View File
@@ -32,6 +32,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/sdk/metric/metricdata"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
@@ -3035,3 +3036,105 @@ func TestResumeActor_RelocatesAfterSuspendFromPaused(t *testing.T) {
t.Errorf("worker-2 node = %q, want node2", got)
}
}
// TestLifecycleOpPoolAttributesOnSuccess is the regression test for #957: a
// successful suspend and pause must stamp the pool they ran on. Both recorded
// the histogram from a defer that read the finalized record, whose assignment
// the finalize step had already cleared, so the pair landed only on failures.
func TestLifecycleOpPoolAttributesOnSuccess(t *testing.T) {
tests := []struct {
name string
op string
// run performs the operation on an actor that is already RUNNING.
run func(t *testing.T, tc *testContext, actor *ateapipb.ObjectRef)
}{
{
name: "suspend",
op: ateattr.OperationSuspend,
run: func(t *testing.T, tc *testContext, actor *ateapipb.ObjectRef) {
t.Helper()
if _, err := tc.client.SuspendActor(context.Background(), &ateapipb.SuspendActorRequest{Actor: actor}); err != nil {
t.Fatalf("SuspendActor failed: %v", err)
}
},
},
{
name: "pause",
op: ateattr.OperationPause,
run: func(t *testing.T, tc *testContext, actor *ateapipb.ObjectRef) {
t.Helper()
if _, err := tc.client.PauseActor(context.Background(), &ateapipb.PauseActorRequest{Actor: actor}); err != nil {
t.Fatalf("PauseActor failed: %v", err)
}
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ns := namespaceForTest("ns-lifecycle-pool-" + tt.name)
tc := setupTest(t, ns)
defer tc.cleanup()
createTemplate(t, tc, ns)
createWorkerPod(t, tc, ns, "worker-1", "node1", "pool1")
actorRef := &ateapipb.ObjectRef{Atespace: testAtespace, Name: "id1"}
if _, err := tc.client.CreateActor(context.Background(), &ateapipb.CreateActorRequest{Actor: &ateapipb.Actor{
Metadata: &ateapipb.ResourceMetadata{Atespace: actorRef.GetAtespace(), Name: actorRef.GetName()},
ActorTemplateNamespace: ns,
ActorTemplateName: "tmpl1",
}}); err != nil {
t.Fatalf("CreateActor failed: %v", err)
}
if _, err := tc.client.ResumeActor(context.Background(), &ateapipb.ResumeActorRequest{Actor: actorRef}); err != nil {
t.Fatalf("ResumeActor failed: %v", err)
}
tt.run(t, tc, actorRef)
attrs := lifecycleOpAttributes(t, tc, tt.op)
if got, ok := attrs.Value(ateattr.WorkerPoolNamespaceKey); !ok || got.AsString() != ns {
t.Errorf("%s = %q (present: %v), want %q", ateattr.WorkerPoolNamespaceKey, got.AsString(), ok, ns)
}
if got, ok := attrs.Value(ateattr.WorkerPoolNameKey); !ok || got.AsString() != "pool1" {
t.Errorf("%s = %q (present: %v), want %q", ateattr.WorkerPoolNameKey, got.AsString(), ok, "pool1")
}
// error.type's absence marks a success, so its presence would mean the
// datapoint under test is not the happy path.
if _, ok := attrs.Value(ateattr.ErrorTypeKey); ok {
t.Errorf("%s is set on the %s datapoint, want the successful operation", ateattr.ErrorTypeKey, tt.op)
}
})
}
}
// lifecycleOpAttributes returns the attribute set of the single
// ate.actor.lifecycle.operation.duration datapoint recorded for op.
func lifecycleOpAttributes(t *testing.T, tc *testContext, op string) attribute.Set {
t.Helper()
var rm metricdata.ResourceMetrics
if err := tc.metricReader.Collect(context.Background(), &rm); err != nil {
t.Fatalf("collect metrics: %v", err)
}
var got []attribute.Set
for _, sm := range rm.ScopeMetrics {
for _, m := range sm.Metrics {
if m.Name != "ate.actor.lifecycle.operation.duration" {
continue
}
hist, ok := m.Data.(metricdata.Histogram[float64])
if !ok {
t.Fatalf("%s data type = %T, want a float64 histogram", m.Name, m.Data)
}
for _, dp := range hist.DataPoints {
if v, ok := dp.Attributes.Value(ateattr.ActorOperationNameKey); ok && v.AsString() == op {
got = append(got, dp.Attributes)
}
}
}
}
if len(got) != 1 {
t.Fatalf("datapoints for %s = %d (%v), want exactly one", op, len(got), got)
}
return got[0]
}
cmd/ateapi/internal/controlapi/functionaltest/common_test.go
+5 -1
View File
@@ -91,6 +91,8 @@ type testContext struct {
// setupAteletOnNode waits on it so a test never dials a node whose atelet the
// informer has not seen yet.
ateletIndexer cache.Indexer
// metricReader collects what the service's instruments recorded.
metricReader *sdkmetric.ManualReader
}
// setupTest sets up a fully isolated test environment.
@@ -171,7 +173,8 @@ func setupTestWithVolumePlugins(t *testing.T, ns string, plugins map[string]volu
return insecure.NewCredentials(), nil
}))
instruments, err := controlapi.NewInstruments(sdkmetric.NewMeterProvider(sdkmetric.WithReader(sdkmetric.NewManualReader())).Meter("ateapi"))
metricReader := sdkmetric.NewManualReader()
instruments, err := controlapi.NewInstruments(sdkmetric.NewMeterProvider(sdkmetric.WithReader(metricReader)).Meter("ateapi"))
if err != nil {
cancel()
mr.Close()
@@ -263,6 +266,7 @@ func setupTestWithVolumePlugins(t *testing.T, ns string, plugins map[string]volu
workerPoolLister: workerPoolLister,
sandboxConfigLister: sandboxConfigLister,
ateletIndexer: ateletInformer.GetIndexer(),
metricReader: metricReader,
}
}
cmd/ateapi/internal/controlapi/workflow_pause.go
+14 -2
View File
@@ -27,6 +27,7 @@ import (
"github.com/agent-substrate/substrate/internal/resources"
atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1"
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"go.opentelemetry.io/otel/attribute"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
@@ -39,10 +40,16 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, actorRef resources.Actor
var actor *ateapipb.Actor
var actorTemplate *atev1alpha1.ActorTemplate
var wireSnapshotScope string
// Set just before finalize; nil until then, so earlier exits label
// themselves from the record they hold.
var finalAttrs []attribute.KeyValue
defer func() {
w.instruments.recordLifecycleOp(ctx, ateattr.OperationPause, start, err,
lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)...)
attrs := finalAttrs
if attrs == nil {
attrs = lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)
}
w.instruments.recordLifecycleOp(ctx, ateattr.OperationPause, start, err, attrs...)
}()
lockCtx, lock, err := w.acquireActorLock(ctx, actorRef)
@@ -58,6 +65,8 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, actorRef resources.Actor
if actor.GetStatus().GetState() == ateapipb.ActorState_ACTOR_STATE_PAUSED {
// Fully paused already: FinalizePaused commits PAUSED and the cleared
// worker assignment in a single update, so there is nothing left to do.
// This success reports no pool, and cannot: the previous attempt
// released the worker, so the record names none (#957).
return actor, nil
}
var marked *ateapipb.Actor
@@ -74,6 +83,9 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, actorRef resources.Actor
if err = w.ensureVolumesDetached(lockCtx, actor, actorTemplate, "DetachVolumesForPause", ateattr.OperationPause); err != nil {
return nil, err
}
// FinalizePaused clears the WorkerAssignment the labels read, so snapshot
// them here, as crash.go does for the crash counter.
finalAttrs = lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)
var finalized *ateapipb.Actor
if finalized, err = w.ensurePausedFinalized(lockCtx, actorRef, actorTemplate); err != nil {
return nil, err
cmd/ateapi/internal/controlapi/workflow_suspend.go
+14 -3
View File
@@ -27,6 +27,7 @@ import (
"github.com/agent-substrate/substrate/internal/resources"
atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1"
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"go.opentelemetry.io/otel/attribute"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
@@ -41,10 +42,16 @@ func (w *ActorWorkflow) SuspendActor(ctx context.Context, actorRef resources.Act
var actor *ateapipb.Actor
var actorTemplate *atev1alpha1.ActorTemplate
var wireSnapshotScope string
// Set just before finalize; nil until then, so earlier exits label
// themselves from the record they hold.
var finalAttrs []attribute.KeyValue
defer func() {
w.instruments.recordLifecycleOp(ctx, ateattr.OperationSuspend, start, err,
lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)...)
attrs := finalAttrs
if attrs == nil {
attrs = lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)
}
w.instruments.recordLifecycleOp(ctx, ateattr.OperationSuspend, start, err, attrs...)
}()
lockCtx, lock, err := w.acquireActorLock(ctx, actorRef)
@@ -60,7 +67,8 @@ func (w *ActorWorkflow) SuspendActor(ctx context.Context, actorRef resources.Act
if actor.GetStatus().GetState() == ateapipb.ActorState_ACTOR_STATE_SUSPENDED {
// Fully suspended already: FinalizeSuspended commits SUSPENDED and the
// cleared worker assignment in a single update, so there is nothing
// left to do.
// left to do. This success reports no pool, and cannot: the previous
// attempt released the worker, so the record names none (#957).
return actor, nil
}
// Decided before marking: once SUSPENDING is committed, the loaded status
@@ -82,6 +90,9 @@ func (w *ActorWorkflow) SuspendActor(ctx context.Context, actorRef resources.Act
if err = w.ensureVolumesDetached(lockCtx, actor, actorTemplate, "DetachVolumes", ateattr.OperationSuspend); err != nil {
return nil, err
}
// FinalizeSuspended clears the WorkerAssignment the labels read, so snapshot
// them here, as crash.go does for the crash counter.
finalAttrs = lifecycleOpAttrs(actor, actorTemplate, "", wireSnapshotScope)
var finalized *ateapipb.Actor
if finalized, err = w.ensureSuspendedFinalized(lockCtx, actorRef, actorTemplate); err != nil {
return nil, err