atenet: raise the default route timeout, and size shutdown separately - #1529
atenet: raise the default route timeout, and size shutdown separately#1529Maya Wang (mayawang) wants to merge 2 commits into
Conversation
The workload route shipped with a 10s ceiling, which is below the normal duration of the turns this platform exists to serve: an agent relaying a model completion holds the request open for the whole generation. A turn past the ceiling returns 504, and that 504 cannot safely be retried, because the actor did receive the turn and is still working on it. The same ceiling cut off gRPC server-streaming and bidi RPCs longer than 10s, so streaming effectively required raising the flag. The ceiling could not simply be raised, because the drain sequence derived its Envoy-drain window and its drain-timeout from the same constant, deliberately, so that a raised route ceiling could not stretch shutdown past terminationGracePeriodSeconds. Raising the constant alone would have pushed the derived drain to over five minutes against a 60s grace period, and the kubelet would SIGKILL mid-drain. Split the two. defaultRouteTimeout becomes 5m and governs the route only. drainRouteBudget stays at 10s and is what shutdown plans for, so the drain arithmetic is unchanged. A turn still running after the drain budget does not survive a shutdown, as before; operators who need one to raise --drain-timeout and the grace period together. The new default is exactly Envoy's stream idle default, so pin the route idle timeout at equality rather than leaving the two to race. A turn that sends nothing until it is done is idle by Envoy's reckoning, and an idle reset reaches the client as a torn stream rather than a timeout.
41f721c to
27fee2f
Compare
Lior Lieberman (LiorLieberman)
left a comment
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Whats the best way to reproduce it?
Easiest way is to look at what Envoy actually got, no workload needed: A stock install gives 10s. The same command against a cluster where I've added --route-timeout=5m gives 300s. That's the whole thing: nothing in manifests/ate-install/atenet-router.yaml sets the flag, so the route lands on the constant. For the failure itself, any actor that takes longer than ~15s to answer gets 504 upstream request timeout, the 10s route plus the 5s parking budget in front of it. I saw it 10 out of 10 on burst-wake during acceptance, between 15.9s and 18.4s. Happy to grab you a trace off that cluster if you'd rather see it than the config. The response-reuse half I can't hand you a recipe for. It showed up on 6 of 10 actors and I haven't pinned the trigger, which is why #1525 stays open after this merges. |
Raising the default unblocks long turns but does not give streaming and bidi RPCs a ceiling of their own, which is what the TODO asked for. Keep it, reworded for the new default, so the per-route work is not lost.
|
I am concerned that we seprate timeout configs more to even more constants. I already filled a previously #1291. Ideally we have a way to control this) Promised to review tonight. |
2 participants
Addresses the timeout half of #1525. Deliberately not a closing keyword: see Scope.
The problem
atenet-routershipped with the workload route capped at 10s, plus 5s of parking in front of it. That is below the normal duration of the turns this platform exists to serve, since an agent relaying a model completion holds the request open for the whole generation. A turn past the ceiling returns504 upstream request timeout, and the caller cannot safely retry it: the actor did receive the turn and is still working, so a retry runs it twice.Observed as 10 failures out of 10 on a burst-wake test during acceptance, each at 15.9s to 18.4s.
The same ceiling bounds gRPC server-streaming and bidi RPCs, which is what the
TODO(liorlieberman)above the constant is about. This does not resolve that TODO. It asks for streaming to get its ceiling without imposing one on every workload, and a raised default is the blunt version it warns against, so the TODO stays in place pointing at #1291.Why it was not a one-line change
defaultRouteTimeoutwas doing two jobs. It was the route default, and it was the anchor the drain sequence sized its Envoy-drain window and--drain-timeoutfrom.The
--route-timeoutflag never fed the drain, anddrainTimeout()carries a comment saying so. But the two constants were one, so raising it for the route would have taken the derived drain from ~20s to over 5m against a 60s grace period, and the kubelet would SIGKILL mid-drain. This splits them so the route number can move without the drain number following.What this does
Splits the constant.
defaultRouteTimeoutis now 5m and governs the route only.drainRouteBudgetis new, stays at 10s, and is what shutdown plans for. All the drain arithmetic is unchanged, indrainTimeout()and indataplaneWindow.The behaviour that does not change: a turn still running after the drain budget does not survive a shutdown. Operators who need one to raise
--drain-timeoutand the grace period together, as before, and the manifest still says so.One thing to flag, since it is the weak point. Envoy's stream idle default is also 5m and we never set it, so raising the route timeout alone would not have worked: a turn that sends nothing while the actor thinks is idle by that measure and would be reset before the route timeout was ever reached.
routeIdleTimeout()takes the larger of the two, which leaves everything below 5m exactly as it was.At exactly 5m, which is now the default, the two deadlines coincide and either may fire first. An idle-triggered end reaches the client as a stream reset rather than a 504, which is the worse of the two. Making the idle timer strictly later would settle it, but that changes behavior for anyone already running
--route-timeoutabove 5m, so I left it alone. Happy to take the other call.The commented-out
--route-timeout=5mcomes out of the router manifest, since it is now the default, and the comment above it is rewritten to describe lowering rather than raising.Scope
This does not fix the other half of #1525 and that issue should stay open. When a turn times out, the actor keeps working, finishes, and the response has nowhere to return to. It is then delivered to whoever speaks to that actor next, as a fluent
HTTP 200answering somebody else's question. We saw that on 6 of 10 actors. A longer ceiling makes it rarer, not impossible, and anyone whose turns exceed 5m still reaches it.Testing
go test ./cmd/... ./internal/...passes.TestRouterConfigDrainTimeoutIndependentOfRouteTimeoutis new. It pins the property this change turns on rather than the arithmetic: the derived drain must not track the route timeout, and the full sequence must fit inside the 60s grace period in the router manifest. It fails on the naive version of this change.TestXdsServer_RouteTimeout/IdleTimeoutAtTheDefaultRouteTimeoutcovers the equality case above, withIdleTimeoutTracksLongerRouteTimeoutandIdleTimeoutKeepsEnvoyDefaultWhenRouteTimeoutIsShortereither side of it.SetterOverridesnow sets 30s instead of 5m, which was silently passing against the new default whether or not the setter worked, and lowering is the direction an operator capping turn length actually goes.make verifydoes not complete in my environment:hack/verify/codegen.shfails creating the Locust codegen virtualenv becausepython3-venvis not installed locally.hack/verify/boilerplate.sh,gofmtandgo vetare clean.