Conversation
|
This change is part of the following stack: Change managed by git-spice. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| r.incrementInFlight(fn, instance) | ||
| defer r.decrementInFlight(fn, instance) | ||
| return r.roundTripper.RoundTrip(req) | ||
| } |
There was a problem hiding this comment.
Bug: Retries Inflate In-Flight Request Counters
The forward method increments and decrements in-flight counters on each call, but it's invoked inside the retry loop in RoundTrip. When a request retries, the counters are incremented and decremented multiple times for the same logical request, causing incorrect in-flight metrics. The counters should be managed at the ServeHTTP level (once per logical request) rather than per retry attempt.
There was a problem hiding this comment.
I think the location is fine, when we do an attempt we count it as in-flight. We aren't double counting because if that call fails we decrement the in-flight requests.
a9699be to
dc9f046
Compare
| "go.opentelemetry.io/otel/attribute" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
These numbers were all picked at random
There was a problem hiding this comment.
I would turn these into flags so we can update them without having to make code changes.
| } | ||
| } | ||
| if len(combined.InFlightPerInstance) == 0 { | ||
| combined.InFlightPerInstance = nil |
There was a problem hiding this comment.
Bug: Heartbeat Timeout Breaks Scaling Decisions.
When all router heartbeats for a function have been garbage collected due to timeout, Combined returns a heartbeat with an empty Function field (zero value). This causes calculateDesiredInstances to use zero values for scale parameters like TargetInFlightRequests, MinInstances, and MaxInstances, leading to incorrect scaling decisions. The function should preserve the Function field from one of the heartbeats or handle the empty map case differently.
… going to any single sandbox and we can/want to scale up
dc9f046 to
1d47d37
Compare
|
I fear that using the heartbeats as a way to track the inflight count will be too slow. We don't heartbeat super often (5s) and the inflight count changes super frequently such that the controller is always going to have a somewhat out of date view of the inflight count. I think the result is that pods can still get hammered for 5s. What do you think about instead making the reloader-proxy enforce it server side within the pod, and reply with a 503 i can't serve this request right now, and have the router retry the other pods, and then if there are no pods left to try, ask the controller to scale up? |
scott-rc
left a comment
scott-rc
left a comment
There was a problem hiding this comment.
I left some comments, but the approach overall looks good 👍
| Name: "waiting_for_unassigned_pods", | ||
| Help: "The number of functions that are waiting for an unassigned pod", | ||
| }, []string{"function_deployment"}) | ||
| }, []string{"function_deployment", "function_tenant"}) |
There was a problem hiding this comment.
How do feel about moving these function_tenant label additions to a separate PR?
I avoided adding the function_tenant label in the first place because it's technically unbounded, we can have an infinite number of tenants, and prometheus advises against using labels with unbounded values:
| if len(combined.InFlightPerInstance) == 0 { | ||
| combined.InFlightPerInstance = map[string]int{} | ||
| } |
There was a problem hiding this comment.
nit: This is redundant.
| if len(combined.InFlightPerInstance) == 0 { | |
| combined.InFlightPerInstance = map[string]int{} | |
| } |
| if len(instances) == 0 { | ||
| http.Error(rw, "no ready instances", http.StatusServiceUnavailable) | ||
| return | ||
| } |
There was a problem hiding this comment.
nit: Let's log the error before returning it so we can easily track when this happens in Axiom.
| if len(instances) == 0 { | |
| http.Error(rw, "no ready instances", http.StatusServiceUnavailable) | |
| return | |
| } | |
| if len(instances) == 0 { | |
| log.Error(ctx, "no ready instances") | |
| http.Error(rw, "no ready instances", http.StatusServiceUnavailable) | |
| return | |
| } |
| combined := routerHeartbeats.Combined() | ||
| if len(combined.InFlightPerInstance) == 0 { | ||
| return map[string]int{} | ||
| } | ||
|
|
||
| loads := make(map[string]int, len(combined.InFlightPerInstance)) | ||
| for instance, count := range combined.InFlightPerInstance { | ||
| loads[instance] = count | ||
| } | ||
| return loads |
There was a problem hiding this comment.
nit: simpler to just do this
| combined := routerHeartbeats.Combined() | |
| if len(combined.InFlightPerInstance) == 0 { | |
| return map[string]int{} | |
| } | |
| loads := make(map[string]int, len(combined.InFlightPerInstance)) | |
| for instance, count := range combined.InFlightPerInstance { | |
| loads[instance] = count | |
| } | |
| return loads | |
| return maps.Clone(routerHeartbeats.Combined().InFlightPerInstance) |
| if !ctrl.isRecentlyScaling(fn) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I would remove this ctrl.isRecentlyScaling check and all its dependencies imo.
ctrl.markScaleActivityis only called on the controller that is responsible for the function in the hashring, so this could returnfalsewhen it's reallytrue.- We know all the existing instances are above their threshold and giving them a moment to complete some requests should help them recover regardless of whether we just scaled or not
| "go.opentelemetry.io/otel/attribute" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
I would turn these into flags so we can update them without having to make code changes.
This is true, but we can lower the heartbeat interval to 1s and see if that adds any issues -- I have a feeling it won't. If it does cause issues, I can try and get this gRPC implementation out which should make request overhead even smaller 😅 |
|
I still think that a 1s window is a long enough time for the inflight count to change pretty drastically. Scott how do you feel about the "make the sandbox feedback to any caller that there are too many open connections, and have the callers retry a different sandbox" approach instead? |
3 participants
This enables skipper to track how many requests each instance has in-flight as it hearbeats.
Note
Adds per-instance in-flight tracking and throttled, least-busy instance selection (with short backoff), plus tenant-labeled metrics and related test coverage.
handleInstancenow throttles when all instances exceedTargetInFlightRequests(short backoff) and picks the least-busy acceptable instance; scales if none ready.RouterHeartbeats.Combined()andinFlightPerInstance.scaleActivity,scaleActivityWindow) to gate throttling decisions.InFlightPerInstance; request forwarding wraps increments/decrements of per-instance and total in-flight counts.forward,incrementInFlight,decrementInFlight.waiting_for_unassigned_pods,assignments_total,scale_ups_total,scale_downs_total,heartbeats_total,requests_total,requests_in_flight).server_test.goandrouter_test.go.Written by Cursor Bugbot for commit 1d47d37. This will update automatically on new commits. Configure here.