Skip to content

Extend the atelet drain time and tGPS for potential long checkpoint #770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Haven Xia (HavenXia) wants to merge 1 commit into from
+27 −12
Closed

Extend the atelet drain time and tGPS for potential long checkpoint #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions cmd/atelet/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ var (
logLevelFlag = pflag.String("log-level", "info", "Minimum log level: debug, info, warn, or error.")

drainDelay = pflag.Duration("drain-delay", 0, "How long to keep accepting new RPCs after SIGTERM before starting the gRPC drain.")
drainTimeout = pflag.Duration("drain-timeout", 5*time.Minute, "Deadline for the graceful gRPC drain on shutdown. In-flight RPCs still running past it are forcefully cancelled.")
drainTimeout = pflag.Duration("drain-timeout", 60*time.Minute, "Deadline for the graceful gRPC drain on shutdown. In-flight RPCs still running past it are forcefully cancelled. Sized to the worker pod's 60m termination grace: a Checkpoint/Restore still in flight when atelet is terminated must not be cancelled before the worker's own grace expires.")

@sfunkenhauser sfunkenhauser Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the ateom gets a long tGPS, initially we are only going to give actors 1 minute to shutdown.
https://github.com/agent-substrate/substrate/pull/764/changes#diff-8d90ca5fe4218dfa9c688e2ecf61f3f2c1a32c55e155f7c7dfb0c32f79b36da1R79

I think this timeout should correlate to the actor tGPS, and not necessarily the worker.

@HavenXia Haven Xia (HavenXia) Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

substrate/cmd/ateom-gvisor/main.go

Line 353 in c155efd

s.lock.Lock()

hmm it seems the checkpoint will hold the lock, so whatever how long the checkpoint takes, the actor will not see an signal of SIGTERM (with the 1min workloadGracePeriod), it need to wait for the checkpoint ends. And if the checkpoint takes longer than 5 min, atelet will cancel it, which is why I raise this PR.

@sfunkenhauser sfunkenhauser Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good point, but I think the underlying issue is that the ateom doesn't kill the container.

Ultimately whenever the ateom receives a SIGTERM, it should start a timer for the actor tGPS. If the actor hasn't exited by then it should be SIGKILLed, even if it's in the middle of a checkpoint. (So I likely need to make some improvements to my PR).

That's why I think this drain timer should be correlated to the actor tGPS. The container should be killed regardless of what's happening after the actor tGPS is elapsed, so having a larger drain timeout here shouldn't be necessary.

@HavenXia Haven Xia (HavenXia) Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, in SIGTERM path it's the actor's 1min grace decides the SIGKILL, so either worker pod's grace or atelet's tGPS is not necessary.

But I just thought of a scenario that what if it's atelet Daemonset got restarted during a normal checkpoint process? A harness is trying to checkpoint a large actor which takes 30mins, and at 20min the atelet get a restart so it will cut the upload RPC?

But honestly I don't have the data of a P95 or P95 checkpoint time, Max Smythe (@maxsmythe) do you know that number?

@maxsmythe Max Smythe (maxsmythe) Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give you it for very small images (and gVisor). For them it is well under 1 minute. P99 < 2.5 seconds, usually much less than that.

@HavenXia Haven Xia (HavenXia) Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, then we are safe to keep the current 5min

)

func main() {
Expand Down Expand Up @@ -248,12 +248,26 @@ func drainOnShutdown(ctx context.Context, srv *grpc.Server, readiness *serverboo
srv.GracefulStop()
close(drainComplete)
}()
select {
case <-drainComplete:
slog.InfoContext(ctx, "Drain completed within deadline")
case <-time.After(*drainTimeout):
slog.WarnContext(ctx, "Drain deadline exceeded; forcing stop")
srv.Stop()
// log progress so the pod does not just sit in Terminating silently.
start := time.Now()
deadline := time.NewTimer(*drainTimeout)
defer deadline.Stop()
progress := time.NewTicker(time.Minute)
defer progress.Stop()
for {
select {
case <-drainComplete:
slog.InfoContext(ctx, "Drain completed within deadline")
return
case <-progress.C:
slog.InfoContext(ctx, "Still draining",
slog.Duration("elapsed", time.Since(start).Round(time.Second)),
slog.Duration("remaining", (*drainTimeout-time.Since(start)).Round(time.Second)))
case <-deadline.C:
slog.WarnContext(ctx, "Drain deadline exceeded; forcing stop")
srv.Stop()
return
}
}
}()
return done
Expand Down
11 changes: 6 additions & 5 deletions manifests/ate-install/atelet.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,12 @@ spec:
spec:
serviceAccountName: atelet
# Budget for the full shutdown sequence: --drain-delay (0 for atelet) +
# --drain-timeout (5m). Sized long because Checkpoint/Restore stream
# multi-GiB snapshots and must not be force-cancelled mid-upload. The sum
# must fit within terminationGracePeriodSeconds, plus slack for the force
# --drain-timeout (60m). Sized to the worker pod's hardcoded 60m
# termination grace (workerTerminationGracePeriodSeconds): atelet drives
# the worker's Checkpoint/Restore, so a terminating atelet must not
# force-cancel one before the worker's own grace expires, plus slack for the force
# Stop() and the tracer/meter flush on exit.
terminationGracePeriodSeconds: 330
terminationGracePeriodSeconds: 3630
containers:
- name: atelet
image: ko://github.com/agent-substrate/substrate/cmd/atelet
Expand All @@ -80,7 +81,7 @@ spec:
# Graceful shutdown knobs. The sum must fit within
# terminationGracePeriodSeconds above.
- --drain-delay=0s
- --drain-timeout=5m
- --drain-timeout=60m
# atelet does no mounts, netlink, device, or namespace operations (those
# live in the ateom worker pod) — it only reads/writes the
# /var/lib/ateom-gvisor hostPath as root, so it needs no Linux
Expand Down
Loading