It would be helpful to look at how other pod-creating controllers handle backoff in failure cases when the kubelet rejects a pod. For example:

Daemonset controller:

Job controller (I'm not sure if this handles the case where kubelet rejects pods):
https://github.com/kubernetes/kubernetes/blob/efac8fdea24d37aa86b1fd72f526e3f0acaf9e13/pkg/controller/job/job_controller.go#L901C31-L912

What do other pod-creating controllers (like statefulset) do? Should we make a distinct special behavior for replicaset controller, or try to do something common?

StatefulSet

StatefulSet has the same problem as ReplicaSet. It goes into a hotloop here

One advantage is, that it does not pollute the cluster with failed pods, so the problem is not as noticeable. But it is a strain on resources...

I think we could easily solve this with a backoff, same as we do in the DaemonSet.

Job

I think we do not have to solve this problem for a Job. The Job will usually just finish faster after going over a certain number of failures (BackoffLimit, and other fields...). It would still be useful to have a ReschedulePodsOnKubeletRejection feature here in the future though.

ReplicaSet

The difference from StatefulSet/DaemonSet is that we do not have a pod identity that we could use to base the backoff on. We only see that either a portion or all of the created replicas are failing. If we base the backoff on any failing pod, we would have scaling speed issues. So we need a custom backoff which I tried to explain in the code comments.

Job
It would still be useful to have a ReschedulePodsOnKubeletRejection feature here in the future though.

Actually, for Job we already have a mechanism to throttle pod re-creation in case of failed pods - we count the number of failed pods since the last successful pod, and backoff exponentially with base 10s (so the delays are 10s, 20s, 40), see docs and code. So, it is not clear to me what would be the benefit of ReschedulePodsOnKubeletRejection in Job.

btw, I think 10s is too much of a base delay, but that's another discussion :).

So, it is not clear to me what would be the benefit of ReschedulePodsOnKubeletRejection in Job.

The backoff is only applicable for jobs with .spec.backoffLimitPerIndex, right? (updated my comment above). But even in that case ReschedulePodsOnKubeletRejection would still help as it would not fail the pod at all and just reschedule it to a better node.

The backoff is only applicable for jobs with .spec.backoffLimitPerIndex, right?

Actually. the backoff delay applies both for classical "backoffLimit" and "backoffLimitPerIndex". The only difference is that for "backoffLimitPerIndex" the scope of counting the number of failed pods since the last successful is within the index.

• classical backoff
• per index backoff

This will require some testing, but from my local tests creating 200 replicas RS takes up to 10s:

I0317 19:12:43.159542       1 replica_set.go:779] ">>> slowStartBatch is starting..." logger="replicaset-controller" count=200
I0317 19:12:43.159579       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=1 remaining=200
I0317 19:12:43.194089       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=2 remaining=199
I0317 19:12:43.197996       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=4 remaining=197
I0317 19:12:43.202078       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=8 remaining=193
I0317 19:12:43.206846       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=16 remaining=185
I0317 19:12:43.414170       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=32 remaining=169
I0317 19:12:44.460584       1 request.go:752] "Waited before sending request" logger="replicaset-controller" delay="1.046108269s" reason="client-side throttling, not priority and fairness" verb="POST" URL="https://172.18.0.2:6443/api/v1/namespaces/default/pods"
I0317 19:12:45.063424       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=64 remaining=137
I0317 19:12:48.314100       1 replica_set.go:783] ">>> slowStartBatch ..." logger="replicaset-controller" batchSize=73 remaining=73
I0317 19:12:52.015248       1 replica_set.go:803] ">>> slowStartBatch ..." logger="replicaset-controller" succeeded=200

logs added for clarity in slowStartBatch method, which is responsible for creating pods in batches (starting with 1, and every successful create allows us to double the batch size, up to 500 limit in a single batch).

The discussion during today's sig-apps focus on expanding that method with the ability to periodically (every n-th iteration, not to overwhelm the creation with those checks) check the state of created pods, and eventually do not allow the doubling to happen (slowStartBatch will not double if we encounter an error). Currently only api errors when creating pods are the trigger for stopping the increased creations. We propose to expand that with pod status checks, which should allow us to also slow down the failure rate, if something like that happens.

This will not resolve the issue, but our goal is to slow down the problem so that other tools (monitoring, primarily) could notice an increase in the pod creation and react to the problem. Rather than allowing an avalanche of bad pods trashing the cluster.

I have experimented with an alternative solution and detect the failures in the slowStartBatch function. Please see the code in #131050, but have found couple of problesms.

We do not have a good controll over the backoff. When we try to create pods in some capacity and observe them fail, the new pods can trigger RS resync via the update handler before the RS backoff kicks in. And this new sync can create new pods again. Basically we can get into a loop as well. The bigger the ReplicaSet and the kubelet/scheduler delay, the bigger the chance of running into it.

The reasons for the loop are:

• It can potentially take a long time before we get from Pending -> Failed.
• We observe updates from pods that land on the good nodes.
• When we get to the pod GC, each pod deletion can trigger the RS sync as well.

Also for small replica sets, it is hard to preserve the backoff since we always reset it to 0 because we are unable to detect the failures in a single sync.

It seems the best solution is to first do a lookback and decide how many pods should have failed per unit of time before we start creating new pods as proposed in the the current PR. Please let me know if you have a better idea on how to do the lookback.

I could also make the lookback part of the slowStartBatch, but then we run into the problem of polluting the log with

since we can create no pods in empty syncs.

It seems the best solution is to first do a lookback and decide how many pods should have failed per unit of time before we start creating new pods as proposed in the the current PR. Please let me know if you have a better idea on how to do the lookback.

I could also make the lookback part of the slowStartBatch, but then we run into the problem of polluting the log with

The lookback being part of the slowStartBatch, to be exact here seems like a reasonable approach. Especially that recently we've added a pod indexer which will ensure that the pod listing and filtering the failed shouldn't be too heavy.

So your proposed solution from #131050 (assuming you'll use rsc.getRSPods(rs)) is the right path forward. Also, to eliminate looping through the pod list too many times, I'd suggest to do both the failed and starttime filtering in a single go.

Okay, make sense. I will make the lookback part of the slowStartBatch.

• it will be more responsive
• we need to do the first check even before creating the first pod
• I will try to use the backoff mechanism from this PR (as mentioned the one in #131050 does not work)

I'd like to re-use the default re-queuing mechanism, which already supports rate limiting, rather than introducing entirely new mechanism. See my other comment where I'm laying out what it should look like, and drop this entire piece of code.

This is to limit the absolute number of pods to create in a single burst if we observe failing replicas. We could limit this even further. By a half sounds as a good compromise.

• failed pods will limit the creation of new pods (e.g. weight 1)
• pods that failed in the current slowStartBatch execution have double the weight

Nit: can you comment what this value stands for. Also, maybe something like FailedPodsWindow or something more descriptive will be a better name.

Why do we need a feature gate? If you want to go with a feature gate you'll need to open a KEP and go through the entire graduation and release process. Because every FG is automatically a user-facing change, in this case it's the --feature-gate value that is to be configured by the cluster admin.

Rarely, we decide to go with feature gates for bug fixes, and this particular case is no different. I've thought about it for a bit yesterday, and I think we can safely assume this is fixing a bug, so the surface is small, and with sufficient tests ensuring that both the error path isn't worse than before, and the "happy path" is the same I think we can just proceed with just controller changes.

Unless I missed something, did I?

I did some more testing and carefully reviewing this code and I have to admit that this is unnecessarily complicating the replica set controller, weaving the backoff calculations deeply inside the controller (see the below piece around calculateTimeWeightedFailedPodsCount) and the usual mechanism which we have inside the queue, which in case of failures puts a particular RS in a queue rate limited, see

I would like to be able to re-use just that mechanism to slow down the creations, rather than introduce yet another one. To achieve that, I'm proposing:

1. Modify slowStartBatch function similarly to what you're doing here, introducing the batchPrecondition check. It will look like this batchPrecondition func(successes int) error. It accepts a number of successes and returns an error. This function implementation will be very simpler version of what you coded in this PR:
   • Count failed pods in the last unit of time, the hardcoded 20 min might be too long, so I'm open to suggestions. I was personally thinking about 5 min for starters, or we can try to get that information from the queue, and modify the window based on NumRequeue, for example.
   • If the number of failed pods is significantly exceeding the successes (I'm open to any particular implementation here, as long as the precondition function will be idempotent), return error stating that we're failing to create.
2. Inside slowStartBatch I'd invoke the function and break the loop every time the precondition returns an error. This will cause manageReplicas to finish with an error which will trigger the default backoff to kick in, the one I pointed above.

Let's extract the precondition function as a type:

type batchPreconditionFunc  func(successes int) (int, error)

and make sure to properly document the inputs and outputs, this will make the code easier to follow.

Drop these changes entirely, and rely on the fact that manageReplicas should return error and fall back to the default rate limited re-queuing.

In my mind, and what I wrote above I think this entire piece should look like this:

if batchPrecondition != nil {
	if err := batchPrecondition(successes); err != nil {
		return successes, err
	}
}

I'd like to re-use the default re-queuing mechanism, which already supports rate limiting, rather than introducing entirely new mechanism. See my other comment where I'm laying out what it should look like, and drop this entire piece of code.