Repository: kubernetes
Issue: AvailableConditionController escalatory check behaviour during network partitions
Labels: kind/bug,sig/network,sig/api-machinery,triage/accepted

### What happened?

### A Network Partition impacts our api extensions

We experienced a network issue affecting our kubernetes apiservers which partitioned one or more of our apiservers from the back-end end pods which were providing api extensions.  The issue affected the apiservice extensions “[v1.subresources.kubevirt.io](http://v1.subresources.kubevirt.io/)” and “[v1alpha3.subresources.kubevirt.io](http://v1alpha3.subresources.kubevirt.io/)” which were marked as unavailable, which are both extensions that hosted external to the apiservers through in-cluster services.

We observed a high frequency of work being queued by the three controllers associated with the kube-aggregator, “APIServiceRegistrationController”, “autoregister” and “AvailableConditionController” as seen in this graph of the rate of workqueue_adds_total metric emitted by these controllers: -

<img width="2005" alt="Pasted Graphic" src="https://user-images.githubusercontent.com/1453938/185146684-f97aa357-4e7c-459f-b4e1-a447fdb7beee.png">
￼
We also saw a high volume of discovery URL requests, e.g. “/apis/subresources.kubevirt.io/v1” on the backend and so looked into the behaviour of the “AvailableConditionController” to learn more. The “AvailableConditionController” is running on all apiservers running the kube-aggregator and sharing a status condition which is stored in the apiservice object in the kubernetes api.  If the condition is toggled between “Available” or “Unavailable” by an apiserver this will trigger a round of immediate tests across all of the apiservers.

If one or more of the apiservers are in disagreement as to the condition (i.e. some see it down and some it up) then they will toggle the condition back and forth between “Available” and “Unavailable” and trigger more checks as they go.  The number of times that the condition is toggled to “Unavailable” is recorded by the metric aggregator_unavailable_apiservice_total, but we can’t see the toggling back to “Available” because this is specifically not recorded: -
￼
<img width="2007" alt="Pasted Graphic 1" src="https://user-images.githubusercontent.com/1453938/185146749-140c1454-4758-4048-8053-156552aaed44.png">

We can see from the graph above a lot a changes of state from “Available” to “Unavailable” (especially towards the end of the issues) and that a single apiserver instance can be responsible for much or all of the toggling to “Unavailable”.  We might say that the apiservers are fighting over the status of the service because of the number of times the condition is changing and the number of re-checks being performed (across all of the apiservers).

### Rate-limiting Not Reducing Check Frequency

It is clear from the metrics that the failing apiservers are performing a great many checks despite the use of a rate limiting workqueue (workqueue.rateLimitingType) for the checks, and an exponential rate limiter (workqueue.ItemExponentialFailureRateLimiter)  for handling retries. See (https://github.com/kubernetes/kubernetes/blob/619f1cf55208212893504338f18c69f3dcfcdc66/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go#L165)

We observe that, whilst the controller adds an exponential backoff to slow retries for failing checks (by calling AddRateLimited() after a failing check to requeue it),  it does not apply any limit to the rate of new checks being scheduled through Informer events.  If a check is in the process of running or waiting the in the delayed queue a new check can be immediately added through the call to Add(): -

![unknown](https://user-images.githubusercontent.com/1453938/185146839-56a48d91-04c0-4b98-9974-cf6365e83407.png)
￼
Please note that in this example all updates are for the same object ‘X’, i.e. the same apiservice.

1. Informer event 1 triggers an Add() which adds X into the queue.
2. Informer event 2 triggers an Add() which does not add X into the queue because it is already queued.
3. Processing of event 1 starts and X is moved from the queue to processing.
4. Informer event 3 triggers Add() and is queued in the dirty set (but not the queue) because this is the behaviour of the queue. A new request can be queued for an item if it is being processed.
5. Processing of event 1 succeeds and it is marked Done() and Forgot() by the controller. The action of Done() means that event 3 is now added to the active queue. 
6. Processing of event 3 starts from the queue.
7. Processing of event 3 fails and so both Done() and AddRateLimited() are called. The call to AddRateLimited() put the item on the delayed queue.
8. Informer event 4 triggers an Add() which immediately queues X whilst the retry is still waiting in the delayed queue.

This queue behaviour explains why even with ratelimiting we are still seeing a large number of checks being queued and performed per-apiserver.  In the the scenario where the condition is changing often the exponential rate limiter is providing no value at all because all of the rechecks are being triggered by the apiservice changes.

We will absolutely be working to preventing the network partition from re-occurring, but feel that the “AvailableConditionController” could be improved to better handle this kind of failure scenario,  rather than actively making matters worse through the increased load on the service.

### What did you expect to happen?

### Possible Improvements

Whilst we want to recover quickly, we don’t want to hammer a service that is failing.

In the context of the “AvailableConditionController” there is no difference between a retry and a new check, they both amount to the same thing.  Both will re-read the apiservice from the apiServiceLister and so will get the latest condiguration and perform the test of the backend.  We believe that the rate limiting aspect can take advantage of the knowledge that retries and new checks amount to the same thing and perform additionally rate limit the checks to reduce the load.

### Option 1: Only check on .spec changes

We could compare the “.spec” of the apiservice object triggering the check if the new value does not reflect.DeepEqual() the existing value.
This would require us to implement an alternative mechanism for performing the checks every 30 seconds because the resyncs from the Informer would be filtered out.
This behaviour would be restricted to just the “apiservice” informer and changes to “services” and “endpoints” would continue to be checked immediately (so that we can quickly react to change which may restore service).

### Option 2: Block new checks whilst failing checks are retried

Also only targeting changes sent by the “apiservice” informer and not to “service” or “endpoints”.
Use the NumRequeues() method on the ratelimiting work queue to check whether the specific apiservice is in a failure-retry state and ignore new events if it is.
All existing triggers remain the same, including the periodic 30 second re-checks and checks caused by Condition changes, we are effectively restricting all incoming apiservice changes when the service is known to be failing.
We rely on the delayed retry with exponential back-off in order to give the backend a chance to stableize.
Even in maximum backoff it will be re-checked every 30 seconds.
When an apiservice check succeeds the call to Forget() will mean that all apiservice updates will trigger checks again.

This will slow down failing apiservers from toggling the condition to Unavailable which in turn will reduce the amount of fighting over the status and prevent the constant stream of rechecking.

### How can we reproduce it (as minimally and precisely as possible)?

Not so easy, you need to be able to force a partition in your apiservers.
You might achieve this by configuring a proxy for apiserver outbound traffic and then configuring it to return 503 to each request so that it fails fast with an error.

### Anything else we need to know?

I'm happy to submit a PR but would like to discuss if this work is of any interest/value?

### Kubernetes version

<details>

```console
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"19+", GitVersion:"v1.19.13-apple-004", GitCommit:"69a0abf33c45fadd7d943341e385204a5c1631b3", GitTreeState:"archive", BuildDate:"2022-01-14T01:01:07Z", GoVersion:"go1.15.14", Compiler:"gc", Platform:"linux/amd64"}
```

</details>


### Cloud provider

<details>
None
</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"

$ uname -a
Linux kmaster001.REDACTED 5.4.144-16.el7pie #1 SMP Mon Sep 20 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>
N/A
</details>


### Container runtime (CRI) and version (if applicable)

<details>
N/A
</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>
N/A
</details>