Skip to content

Commit 8bacabd

Browse files
jjamrogaEItanya
authored andcommitted
Decouple actor lock TTL from workflow deadline via heartbeat (#14)
ActorWorkflow.ResumeActor and SuspendActor used to derive their workflow ctx from the Redis lock TTL via acquireActorLock(ctx, id, 30s, 2s) — the workflow deadline and the lock TTL were a single 28s knob. That meant image pulls / restores that legitimately need more than 28s death-looped forever, while raising the knob also raised how long peers wait to retry an actor after a crashed ateapi replica. Split the two concerns: - Lock TTL stays short (30s constant, internal). Bounds peer failover. - Workflow deadline is a separate operator-configurable knob via the new --actor-workflow-deadline pflag (default 5m). Bounds a single Resume/Suspend. - A heartbeat goroutine refreshes the lock every lockTTL/3 (~10s) for the full workflow duration. On RefreshLock=false or any Redis error (peer stole the lock, Redis blip), the workflow ctx is cancelled with errLostActorLock as the cause so in-flight steps unwind cleanly and the mutual-exclusion invariant is preserved. - The release function stops the heartbeat (waits for goroutine exit) before best-effort ReleaseLock. Adds store.Interface.RefreshLock with a Redis CAS Lua script mirroring the existing ReleaseLock script.
1 parent 805a583 commit 8bacabd

7 files changed

Lines changed: 76 additions & 8 deletions

File tree

‎charts/substrate-crds/templates/ate.dev_actortemplates.yaml‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,26 @@ spec:
6464
description: A single application container that you want to run
6565
within a WorkerPool.
6666
properties:
67+
args:
68+
description: |-
69+
Arguments to the entrypoint. Not executed within a shell. The container
70+
image's CMD is used if this is not provided (unless command is set,
71+
which discards the image's CMD).
72+
73+
Unlike Kubernetes, variable references $(VAR_NAME) are NOT expanded.
74+
items:
75+
type: string
76+
maxItems: 64
77+
type: array
78+
x-kubernetes-list-type: atomic
6779
command:
68-
description: Entrypoint array. Not executed within a shell.
80+
description: |-
81+
Entrypoint array. Not executed within a shell. The container image's
82+
ENTRYPOINT is used if this is not provided; if it is provided, the
83+
image's ENTRYPOINT and CMD are both ignored and the process argv is
84+
command + args.
85+
86+
Unlike Kubernetes, variable references $(VAR_NAME) are NOT expanded.
6987
items:
7088
type: string
7189
maxItems: 64

‎cmd/ateapi/internal/controlapi/functional_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ func setupTest(t *testing.T, ns string) *testContext {
314314
}
315315

316316
dialer := NewAteletDialer(workerInformer.GetIndexer(), ateletInformer.GetIndexer())
317-
service := NewService(persistence, wc, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, k8sClient)
317+
service := NewService(persistence, wc, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, k8sClient, 30*time.Second)
318318

319319
// 5. Start REAL gRPC Server for ATE API
320320
grpcServer := grpc.NewServer(grpc.UnaryInterceptor(ateinterceptors.ServerUnaryInterceptor))

‎cmd/ateapi/internal/controlapi/service.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package controlapi
1616

1717
import (
18+
"time"
19+
1820
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
1921
"github.com/agent-substrate/substrate/cmd/ateapi/internal/workercache"
2022
listersv1alpha1 "github.com/agent-substrate/substrate/pkg/client/listers/api/v1alpha1"
@@ -34,7 +36,8 @@ type Service struct {
3436

3537
var _ ateapipb.ControlServer = (*Service)(nil)
3638

37-
// NewService creates a service.
39+
// NewService creates a service. actorWorkflowDeadline bounds how long a single
40+
// Resume/Suspend workflow can run end-to-end.
3841
func NewService(
3942
persistence store.Interface,
4043
workerCache *workercache.Cache,
@@ -43,13 +46,14 @@ func NewService(
4346
sandboxConfigLister listersv1alpha1.SandboxConfigLister,
4447
dialer *AteletDialer,
4548
kubeClient kubernetes.Interface,
49+
actorWorkflowDeadline time.Duration,
4650
) *Service {
4751
s := &Service{
4852
persistence: persistence,
4953
actorTemplateLister: actorTemplateLister,
5054
workerPoolLister: workerPoolLister,
5155
dialer: dialer,
52-
actorWorkflow: NewActorWorkflow(persistence, workerCache, dialer, actorTemplateLister, workerPoolLister, sandboxConfigLister, kubeClient),
56+
actorWorkflow: NewActorWorkflow(persistence, workerCache, dialer, actorTemplateLister, workerPoolLister, sandboxConfigLister, kubeClient, actorWorkflowDeadline),
5357
}
5458

5559
return s

‎cmd/ateapi/internal/controlapi/workflow.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"errors"
2020
"fmt"
21+
"time"
2122

2223
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
2324
"github.com/agent-substrate/substrate/cmd/ateapi/internal/workercache"
@@ -136,9 +137,12 @@ type ActorWorkflow struct {
136137
sandboxConfigLister listersv1alpha1.SandboxConfigLister
137138
kubeClient kubernetes.Interface
138139
secretCache *envSecretCache
140+
// workflowDeadline is the maximum duration of a single actor workflow.
141+
workflowDeadline time.Duration
139142
}
140143

141-
// NewActorWorkflow creates a new ActorWorkflow.
144+
// NewActorWorkflow creates a new ActorWorkflow. workflowDeadline bounds how
145+
// long a single Resume/Suspend can run end-to-end.
142146
func NewActorWorkflow(
143147
store store.Interface,
144148
workerCache *workercache.Cache,
@@ -147,6 +151,7 @@ func NewActorWorkflow(
147151
workerPoolLister listersv1alpha1.WorkerPoolLister,
148152
sandboxConfigLister listersv1alpha1.SandboxConfigLister,
149153
kubeClient kubernetes.Interface,
154+
workflowDeadline time.Duration,
150155
) *ActorWorkflow {
151156
return &ActorWorkflow{
152157
store: store,
@@ -157,6 +162,7 @@ func NewActorWorkflow(
157162
sandboxConfigLister: sandboxConfigLister,
158163
kubeClient: kubeClient,
159164
secretCache: newEnvSecretCache(envSecretCacheTTL),
165+
workflowDeadline: workflowDeadline,
160166
}
161167
}
162168

@@ -247,14 +253,17 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, atespace, name string) (
247253

248254
func (w *ActorWorkflow) acquireActorLock(ctx context.Context, atespace, name string) (context.Context, *store.Lock, error) {
249255
lockKey := "lock:actor:" + atespace + ":" + name
256+
workflowCtx, cancel := context.WithTimeout(ctx, w.workflowDeadline)
250257

251-
lock, err := w.store.AcquireLock(ctx, lockKey)
258+
lock, err := w.store.AcquireLock(workflowCtx, lockKey)
252259
if err != nil {
260+
cancel()
253261
if errors.Is(err, store.ErrLockConflict) {
254262
return nil, nil, status.Error(grpcCodes.Aborted, "another operation is in progress for this actor")
255263
}
256264
return nil, nil, fmt.Errorf("while acquiring lock: %w", err)
257265
}
258266

267+
context.AfterFunc(lock.Context(), cancel)
259268
return lock.Context(), lock, nil
260269
}

‎cmd/ateapi/internal/controlapi/workflow_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ import (
2121
"time"
2222

2323
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
24+
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store/ateredis"
25+
"github.com/alicebob/miniredis/v2"
26+
"github.com/redis/go-redis/v9"
2427
"google.golang.org/grpc/codes"
2528
"google.golang.org/grpc/status"
2629
"k8s.io/apimachinery/pkg/util/wait"
@@ -205,3 +208,33 @@ func TestRunWorkflow_RetryOnPersistenceConflict(t *testing.T) {
205208
}
206209
})
207210
}
211+
212+
func TestAcquireActorLockWorkflowDeadline(t *testing.T) {
213+
mr, err := miniredis.Run()
214+
if err != nil {
215+
t.Fatalf("miniredis.Run: %v", err)
216+
}
217+
defer mr.Close()
218+
219+
rdb := redis.NewClusterClient(&redis.ClusterOptions{Addrs: []string{mr.Addr()}})
220+
defer rdb.Close()
221+
w := &ActorWorkflow{
222+
store: ateredis.NewPersistence(rdb),
223+
workflowDeadline: 20 * time.Millisecond,
224+
}
225+
226+
ctx, lock, err := w.acquireActorLock(context.Background(), "space", "actor")
227+
if err != nil {
228+
t.Fatalf("acquireActorLock: %v", err)
229+
}
230+
defer lock.Close()
231+
232+
select {
233+
case <-ctx.Done():
234+
if !errors.Is(ctx.Err(), context.DeadlineExceeded) {
235+
t.Fatalf("context error = %v, want DeadlineExceeded", ctx.Err())
236+
}
237+
case <-time.After(time.Second):
238+
t.Fatal("workflow context did not reach its deadline")
239+
}
240+
}

‎cmd/ateapi/internal/controlapi/workflow_testutil_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"slices"
2020
"testing"
21+
"time"
2122

2223
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
2324
atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1"
@@ -41,7 +42,7 @@ func newTestActorWorkflow(t *testing.T, st store.Interface, tmplNamespace, tmplN
4142
}); err != nil {
4243
t.Fatalf("add template to indexer: %v", err)
4344
}
44-
return NewActorWorkflow(st, nil, nil, listersv1alpha1.NewActorTemplateLister(indexer), nil, nil, nil)
45+
return NewActorWorkflow(st, nil, nil, listersv1alpha1.NewActorTemplateLister(indexer), nil, nil, nil, time.Minute)
4546
}
4647

4748
// seedWorkflowActor stores an actor with the given status, bound to the given

‎cmd/ateapi/main.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ var (
7777
sessionIDCAPoolFile = pflag.String("session-id-ca-pool", "", "The file that contains the CA pool for signing session JWTs")
7878
workerpoolCACerts = pflag.String("workerpool-ca-certs", "", "The file that contains the CA for verifying workerpool client certificates.")
7979

80+
actorWorkflowDeadline = pflag.Duration("actor-workflow-deadline", 5*time.Minute, "Maximum wall-clock duration of a single Resume/Suspend workflow; raise it for slow image registries.")
81+
8082
showVersion = pflag.Bool("version", false, "Print version and exit.")
8183
authMode = pflag.String("auth-mode", "mtls", "Auth mode for incoming gRPC: mtls|jwt. 'mtls' (default) relies on transport-level mTLS for client identity. 'jwt' additionally requires a Kubernetes ServiceAccount Bearer token on every RPC. Substrate will drop support for JWT auth mode once the Pod Certificates feature is enabled by default in the minimum supported Kubernetes version.")
8284
clientJWTCAFile = pflag.String("client-jwt-ca-cert", ateapiauth.DefaultServiceAccountCAFile, "CA cert file used to verify TLS when fetching the OIDC discovery document and JWKS for JWT authentication. Defaults to the in-cluster service account CA.")
@@ -167,7 +169,7 @@ func main() {
167169
}
168170

169171
dialer := controlapi.NewAteletDialer(workerPodInformer.GetIndexer(), ateletPodInformer.GetIndexer())
170-
sm := controlapi.NewService(redisPersistence, workerCache, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, clientset)
172+
sm := controlapi.NewService(redisPersistence, workerCache, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, clientset, *actorWorkflowDeadline)
171173

172174
jwtIssuerDiscoveryClient := buildK8sServiceAccountIssuerDiscoveryClient(ctx, *clientJWTCAFile, *clientJWTIssuer)
173175
if authModeParsed == ateapiauth.ModeJWT && jwtIssuerDiscoveryClient == nil {
@@ -259,6 +261,7 @@ func logFlagValues(ctx context.Context) {
259261
slog.String("session-id-ca-pool", *sessionIDCAPoolFile),
260262
slog.String("workerpool-ca-certs", *workerpoolCACerts),
261263
slog.String("auth-mode", *authMode),
264+
slog.Duration("actor-workflow-deadline", *actorWorkflowDeadline),
262265
)
263266
}
264267

0 commit comments

Comments
 (0)