Extend the atelet drain time and tGPS to the worker checkpoint grace - #770
Extend the atelet drain time and tGPS to the worker checkpoint grace#770Haven Xia (HavenXia) wants to merge 1 commit into
Conversation
|
|
||
| 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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
substrate/cmd/ateom-gvisor/main.go
Line 353 in c155efd
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
2 participants
Large actor checkpoints may take a long time to finish, but the checkpoint itself runs through atelet, whose shutdown will cancel in-flight RPCs after 5 minutes.
Increase
--drain-timeoutto 60m, and atelet's ownterminationGracePeriodSecondsto 3630s so kubelet won't kill atelet before the drain deadline, and log progress once a minute during long drains.Also add a log since the draining become extremely long now.