Old unreachable revision is causing new pods to get created when it should scale down

[SaschaSchwarze0]

/area autoscale

What version of Knative?

1.10 (but exists in older versions as well)

Expected Behavior

When an old revision is unreachable, no more pods should be created for it

Actual Behavior

New (and many) pods are created if the old revision has a large minScale.

Steps to Reproduce the Problem

1. kn quickstart kind (issue also happens outside of KinD)
2. kn service create testsvc --image ghcr.io/knative/helloworld-go:latest --scale 9
3. Start to watch the pods by using kubectl or a tool like k9s
4. kn service update testsvc --scale 1

After the second revision is ready, it starts to terminate the old pods, but also brings up many additional pods for the old revision. Depending on how long pods are stuck in Terminating you can reach more than 60 existing pods for the old revision.

--

The problem is the IsReady function of the ServerlessService which is returning false if the generation is different in metadata and status.

Due to it temporarily not being ready, the PodAutoscaler is set to SKSReady=Unknown here.

The PodAutoscaler then goes Ready=False because SKSReady is part of Ready here.

The revision inherits the PodAutoscaler status and sets Active=Unknown here.

This causes the PodAutoscaler's Reachability to be set to Unknown here.

Once the PodAutoscaler is not anymore marked as unreachable, the scaling boundary will be set to the min value from the annotation again here. For an unreachable revision, it would otherwise always use min = 0 so that scaleDown happens. But now it increases it to 9 again.

I will open a PR in a few minutes that changes IsReady of the ServerlessService to only check the condition. This fixes the problem and I have not seen negative impact. Though, it undos the work of IsReady should take ObservedGeneration into account #8004 which sounds to me as if it tried to resolve an abstract issue while I here have a real one. Alternative could be to leave IsReady unchanged und look at the condition directly in kpa.go.

So, I need somebody with experience to carefully assess if there are side-effects.

[dprotaso]

FYI - this is probably a duplicate of #13677

[dprotaso]

The problem is the IsReady function of the ServerlessService which is returning false if the generation is different in metadata and status.

We can only guarantee a resource is Ready=True is accurate when observed generation is equal to the metadata.generation.

I will open a PR in a few minutes that changes IsReady of the ServerlessService to only check the condition. This fixes the problem and I have not seen negative impact.

Alternative could be to leave IsReady unchanged und look at the condition directly in kpa.go.

I'd rather we not change the semantics of IsReady in this way but find an alternative as you suggest

[SaschaSchwarze0]

Thanks @dprotaso for checking.

Alternative could be to leave IsReady unchanged und look at the condition directly in kpa.go.

I'd rather we not change the semantics of IsReady in this way but find an alternative as you suggest

That's fine. The alternative that I had in mind is to change https://github.com/knative/serving/blob/v0.37.2/pkg/reconciler/autoscaling/kpa/kpa.go#L169 from

	if sks.IsReady() {

to

	if sks.Status.GetCondition(nv1alpha1.ServerlessServiceConditionReady).IsTrue() {

Then we can leave IsReady unchanged, but still ignore the "transition phase" caused by the generation difference (kind of pending reconcile).

[dprotaso]

Let me think of the consequences of that change and get back to you

[SaschaSchwarze0]

FYI - this is probably a duplicate of #13677

The scenario at least sounds different. And the podautoscaler in unknown status seems to be perstent there for a longer time while it here is only in that status for a millisecond at most.

Reminds me that I forgot to mention this ... eventually, in my scenario, an old revision would get fully cleaned up also without any code change. It just takes a few or many minutes and in the meantime many pods had been created and deleted. A much higher value of max-scale-down-rate in the autoscaler config actually helps once it allows the autoscaler to go from current to 1 directly.

[SaschaSchwarze0]

I closed the PR with the change in the networking library and moved the logic change to the KPA reconciler.