metrics staleness check should be moved from saturation detector to getting candidate pods per request

[nirrozenbaum]

What happened:
today SaturationDetector is checking 3 conditions per pod -

1. Metrics are fresh (not stale).
2. WaitingQueueSize <= QueueDepthThreshold.
3. KVCacheUsagePercent <= KVCacheUtilThreshold.

consider the case where one pod came up and hasn't reported metrics for a while, assume that last time that it was reporting, it was in great shape (e.g., on startup, waiting queue size = 0, kvcache = 0).

so we have:
pod1 - able to serve requests. reports as healthy. but hasn't reported metrics for a while and last time we collected metrics from it, it's was idle.
pod2 - partially busy.
pod3 - partially busy.

every incoming request that will go through the saturation detection identify that pod1 shouldn't be considered as a final target, but still pod1 may be selected to serve requests and its stale metrics that shows it's idle, so pod1 has high chances of getting selected.
this may happen continuously, so multiple requests can be sent to pod1, leading to unpredicted state (pod1 may not be able to handle the load).

What you expected to happen:
pod freshness of metrics shouldn't be part of saturation detector, but rather these pods should be filtered when getting the candidate pods from the datastore. stale metrics is not a sign of saturation, it's a sign of invalid pod that shouldn't be taken into consideration for the request serving.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):
• Inference extension version (use git describe --tags --dirty --always):
• Cloud provider or hardware configuration:
• Install tools:
• Others:

cc: @LukeAVanDrie

[LukeAVanDrie]

Stale metrics is not a sign of saturation, it's a sign of invalid pod that shouldn't be taken into consideration for the request serving.

With your recent change to the IsSaturated contract, this SGTM. Before, when we had a direct datastore dependency, I used freshness as a safeguard. At the time it felt safer to consider "saturation state unknown" as "saturated". The candidates filtering is a cleaner approach though.

[nirrozenbaum]

With your recent change to the IsSaturated contract, this SGTM. Before, when we had a direct datastore dependency, I used freshness as a safeguard. At the time it felt safer to consider "saturation state unknown" as "saturated". The candidates filtering is a cleaner approach though.

I'll push a PR for this change (not urgent). will tag you 👍

[LukeAVanDrie]

As an aside, #1293 breaks the FlowController dependency on the SaturationDetector. We use an interface for DI in flowcontrol/contracts/saturationdetector.go rather than accepting the concrete type from the saturationdetector package.

I will need to update FlowController usage before wiring it up this week.

I see two different approaches to resolve this:

1. Pass the candidate pods on the types.FlowControlRequest accepted by the Flow Controller's public EnqueueAndWait method (WIP -- PR will be out for this shortly).

• Pros: Isolates the datastore dependency to the Request Control layer. We already need to get the candidate pods in the Request Control layer to pass to the Scheduler (and Saturation Detector for the case where the experimental Flow Control layer is disabled). This prevents us from calculating it twice (once in Request Control and once in Flow Control).
• Cons: It's a bit leaky. The Flow Controller shouldn't really care about a request's pod candidates.

2. Add a datastore dependency to the Flow Controller.

• Pros: No changes to the types.FlowControlRequest interface. We are not mixing concerns on that type.
• Cons: The Flow Controller really shouldn't have access to the entire data store. This is bad dependency management. We also need to calculate candidate pods in two places.

I am pretty strongly leaning toward option 1 right now. Since requests will be queued for a short duration, I just want to confirm that a request's pods candidates are idempotent and do not benefit from being refreshed at any point.

[LukeAVanDrie]

I also suspect long term that the Saturation Detector may be absorbed into the flowcontrol/... module as an internal implementation detail once the layer matures and is no longer disabled by default. I see no other use cases for the component right now than admission control unless it gets enriched with more advanced features like SLO awareness or exposes some saturation observation methods/metrics that would be useful for other callers.

[nirrozenbaum]

yup, saturation detector as is might be fold into one of the two packages - either requestcontrol or flow control.
either way, I'd recommend to start simple (the simplest thing we can do to make it work) and try to improve as we make progress.

[liu-cong]

pod freshness of metrics shouldn't be part of saturation detector, but rather these pods should be filtered when getting the candidate pods from the datastore. stale metrics is not a sign of saturation, it's a sign of invalid pod that shouldn't be taken into consideration for the request serving.

I am not sure if we can enforce a global staleness threshold. Perhaps different use cases have different thresholds? For example, the metrics collector can tolerate a longer staleness than load aware routing. I am inclined on having the client being responsible for setting the threshold. We can, however, modify the datastore interface to something like PodList(metricsStalenessThreshold) to enforce the clients to explicitly set a threshold.

Another point is that if we just consider pods with stale metrics as invalid, then that basically becomes another health check.

[nirrozenbaum]

pod freshness of metrics shouldn't be part of saturation detector, but rather these pods should be filtered when getting the candidate pods from the datastore. stale metrics is not a sign of saturation, it's a sign of invalid pod that shouldn't be taken into consideration for the request serving.

I am not sure if we can enforce a global staleness threshold. Perhaps different use cases have different thresholds? For example, the metrics collector can tolerate a longer staleness than load aware routing. I am inclined on having the client being responsible for setting the threshold. We can, however, modify the datastore interface to something like PodList(metricsStalenessThreshold) to enforce the clients to explicitly set a threshold.

Another point is that if we just consider pods with stale metrics as invalid, then that basically becomes another health check.

@liu-cong I was referring to the saturation detector, which defines its own staleness threshold in its config and then uses it to decide whether a pod is considered saturated or not. see here:
https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/epp/saturationdetector/saturationdetector.go#L105

at this point, if there is a situation where pod's health code is responding correctly, but we're not able to collect metrics from some reason, the same pod might receive all incoming requests (e.g., if the snapshot of its metrics is "stuck" as the best pod to serve request). I prefer to avoid such a scenario if possible and to filter out pods that do not have reliable metrics.

[liu-cong]

if there is a situation where pod's health code is responding correctly, but we're not able to collect metrics from some reason, the same pod might receive all incoming requests

There is some nuance here. Health checks have its own configuration of timeout, threshold, etc. , which may not be aligned with metrics. So my point is we should decouple these.

I think of them this way: health check answers whether an endpoint CAN serve a request, metrics is more about how GOOD an endpoint can serve a request, and the freshness of the metrics plays a key role in determining how good an endpoint is. Take an hypothetical scenario (I haven't seen this in practice), suppose all endpoins are having high CPU utilization and requests including metrics are taking longer to process, it may still be considered healthy because it can still serve requests, but metrics are stale, then we cannot score the endpoint reliably, but we can fall back to random picking or round robin instead of fail the request.

[nirrozenbaum]

in such a case, current saturation detector logic will mark the system as saturated if all endpoints are having high CPU and an error will be returned.
I think it comes back to the discussion about saturation detector, of whether saturation detection should return an error or allow to bypass even if the check identifies saturation (which of course is configurable).
but this is probably a different discussion.

when opening this issue, I was thinking about how to improve current logic.