fix: Ensure consistent breaker state for unhealthy hosts with infligh…

[elijah-rou]

This PR references the discussion in the CNCF knative-serving Slack channel, which can be found here: https://cloud-native.slack.com/archives/C04LMU0AX60/p1740420658746939

Issue

There has been a long-standing issue in KNative, with Issues dating back to 2022 of the Load Balancer erroneously sending requests to pods which are already busy, even though their set max concurrency should not permit the request to be sent through to that pod. I believe I have found (at least one of) the causes.

In specific scenarios, particularly long-running requests, the breaker state for a podTracker will be functionally reset if the revision backend temporarily registers the pod as "unhealthy". There is no explicit logic that dictates this behaviour. Rather, this is a result of how throttler.go updates the state for its podTrackers:

serving/pkg/activator/net/throttler.go

Line 335 in c09ff6c

func (rt *revisionThrottler) updateThrottlerState(backendCount int, trackers []*podTracker, clusterIPDest *podTracker) {

To summarise, the new state is determined by completely remaking its []podTrackers list from scratch, using information it retrieves from the respective revision backend. Only the healthy hosts from the revision backend are used to make this podTrackers list, and therefore, any unhealthy podTrackers are effectively removed. This is the case even if the "unhealthy" pod is currently busy with a request. If the pod then becomes healthy timely, the revision backend will report it, and the throttler will re-add a brand new podTracker for the pod, effectively removing any previous state held by the podTracker (ie setting InFlight to 0). The pod therefore becomes a valid target for the loadBalancing policy, even though in reality it is still busy with a request.

Proposed Changes

This PR seeks to address this issue fairly simply, by not relinquishing the state of the podTrackers until a host is both unhealthy AND finished with in-flight requests. Technically, this amounts to the following:

• Change the core data-structure for storing all pod trackers to be a map[string]*podTracker as opposed to a []*podTracker. This allows us to easily manage viable podTrackers by referencing them with their dest at any point. It has the added benefit on not re-creating the map on every assignSlice call.
• Update the Breaker interface to have new Pending method. This involves moving the current definition for InFlight to be the inflight value associated with the semaphore (similarly to Capacity), and creating a new Pending method which references a new pending attribute (the old inFlight attribute) of a Breaker.
• Update the PodTracker struct with a new attribute healthy, which a load balancing policy can use to skip unhealthy podTrackers.
• When updating Throttler state, use both healthy pods obtained by the revision backend and the current podTracker list to not only add new podTrackers, but to mark podTrackers which are unhealthy but still have inFlight requests as determined by the current state of the respective podTracker's breaker semaphore-inflight as "unhealthy". If a podTracker is both unhealthy and has no in-flights, the tracker can be removed.
• Update all load balancing policies to skip pod trackers that are unhealthy, and also to skip podTrackers in the assignedTrackers list which are nil (which can now be possible due to us removing the reference for the podTracker in updateThrottlerState

There are 5 more things to note in this PR that are tangential/important but not directly related to the functionality:

1. I have updated the Capacity and InFlight calls to return uint64 as indicated by a TODO comment
2. The default minimum number of activators for the KPA was set to 2. I have updated this value to 1, as there can be setups with 1 activator present in the proxy path.
3. The changes introduce a dependency from golang.org/x/exp/maps, in order to do map manipulation more easily. (I believe this can be removed though when the project updates its go version)
4. Since a podTracker can now be nil, the RandomChoice policy may now not succeed. I have updated it to continue to try until it randomly selects a podTracker that is not nil. I'm not married to this implementation, we can definitely do it another way. I am aware the unit test for this is currently broken, but I will await feedback to change the actual implementation here.
5. I have added some debug logs for podTracker acquisition for a specific request. This also requires that X-Request-Id be passed in for the log to be sensical.

The implementation of this functionality is open to debate. I am happy to refine this towards an implementation that the core maintainers are more amenable to.

[knative-prow]

Hi @elijah-rou. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[knative-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elijah-rou
Once this PR has been reviewed and has the lgtm label, please assign skonto for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dprotaso]

/ok-to-test

[knative-prow-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[dprotaso]

I've created a PR to bump serving to v1.24 - #15856

[dprotaso]

The default minimum number of activators for the KPA was set to 2. I have updated this value to 1, as there can be setups with 1 activator present in the proxy path.

This default min of two is there to prevent a single point of failure when a single activator goes down or becomes unhealthy. We shouldn't change it. Note when multiple activators front a knative revision the target pods are sharded.