Autoscaling in Knative and in the Cluster

[vagababov]

Right now, when the cluster is full, Knative will try to deploy the revision for ProgressDeadline seconds (120s by default in Knative). If there are not enough resources and CAS does not do its job fast enough, the PD will expire and then Autoscaler will wait some 10s more and finally give up and scale to 0 basically marking the revision as failed.

@jonjohnsonjr added this clutch long ago to deal with the revisions that fail to ever progress and we end up with zombie revisions that can never succeed.

But in the case above it is possible that the deployment will succeed.

Now, simple suggestion might be to just crank the PD setting to 11. But this has unintended consequences of all deployments now waiting that much to fail even if they are to fail anyway.

Another suggestion is to check in Autoscaler after PD+10s passed, whether the reason for pod unavailability is resource insufficiency, and if so — wait additional Xs rather than mark the revision failed right away. Otherwise, behave as we do now.

Deployments already watch for quota and will deploy the resources if they become available (https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#failed-deployment)

This seems like a mostly simple change that will help us to avoid making random knative users tweak PD for this reason.

Or I am missing something :-)

/cc @julz @markusthoemmes @mattmoor

[markusthoemmes]

Sounds fair enough. This should fix #9432

[julz]

I'm a little torn. Part of me thinks we should maintain the upstream "k8s-native" (ie very declarative but user hostile ;) behaviour and have ProgressDeadline not be terminal at all. Otherwise we act subtly differently from other k8s things which eventually converge when resources become available, in particular Deployment itself.

For example, why special case insufficient resources with more time but not eg a registry image that isn't available yet? And isn't it odd that insufficient resources gives you a bit more time, but still fails if your cluster is a bit slower today? How do I ask for more time (or less) since we've already overloaded ProgressDeadline?

OTOH getting a clear "this didn't work and we gave up" is a much nicer UX, and being broken with cluster autoscaler is pretty incontrovertibly not a good thing so 🤷‍♂️. I guess tldr Id be ok if we special cased this, but I wish there was a nicer / more generic option.

[markusthoemmes]

To be clear, I don't like a special cased "let's wait some longer" either. If we can get a signal from the deployment that stuff eventually moved forward though, I don't see why we wouldn't pick that up and make the revision succeed too 🤔.

Retrying on a timer is obviously brittle but if the deployment eventually succeeds, we should get a reconcile event anyway, I think?

[vagababov]

Thing is we need to decide when to keep waiting for Deployment to progress (i.e. we still need to read the pod statuses and find out the reason is insufficient resources).
If we do find out then we can keep trying achieving initial scale (forever? or some other timeout?)

[julz]

Ah, my reading of the above was we wait the extra X seconds once if the we see insufficient resources. We could indeed wait infinitely for resources (just don't treat ProgressDeadlineExceeded as terminal if we see ResourcesUnavailable), though it still seems kind of odd that we wait infinitely for resources to become available but not, say, for a registry to come back, or for an image to become available, or for a dependent service to come up (or any of a variety of other things). Again, subtly different from upstream, which is a bit sad (but if it was the only want to make cluster autoscaler reliably work 🤷).

Just to ask the obvious question out loud.... "how awful would it be if.." we just generically dropped ProgressDeadlineExceeded being a terminal state? i.e. we dont scale to zero but maintain our desired scale until the user/client tells us to give up. Bonus: that'd also solve the race here which was causing quite a few flakes till we bumped the timeout in #8964 iirc. Opposite-of-bonus: backwards-incompatible behaviour if you expect a failed revision to stay failed!

[markusthoemmes]

That's sort of what I was getting at: If the deployment is able to recover any of those failure cases, why wouldn't we? I guess we're saying the same things @julz. And I ack that it should apply to all error scenarios the deployment can recover from itself.

The registry stuff is a little tougher I guess, given that we want to resolve the digest and it's unclear when we can retry that 🤔. I guess an exponential backoff in the background resolve code could solve that though.

[julz]

Ugh yeah I forgot about background resolve for image digests. I vote we pretend I never said images above and treat that as a different issue.

[vagababov]

So in general, I'd rather keep the number of cases we consider extendable, possibly, infinitely in pending state fixed (rather the other way around). Since in general it requires intimate knowledge of whether deployment will retry some errors and won't others (crashing container, cannot pull image)?

[jonjohnsonjr]

For posterity, #5077 and #4614 are where the scale to zero and propagate deployment status changes were introduced.

There was some discussion about further changes to the condition lifecycle in comments on the PR. I haven't kept up with this project, so I'm not sure if they're still interesting to the conversation.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.