deleting a service should delete all pods, immediately

[duglin]

In what area(s)?

/area autoscale

What version of Knative?

HEAD as of 7/17

Note: you'll need to modify your cluster so that you can execute long running requests - in this case at least 10 minutes. Aside from tweaking my networking to not timeout idle connections, I also added these to the config-defaults configMap:

  max-revision-timeout-seconds: "86400"
  revision-timeout-seconds: "86400"

Using this script:

#!/bin/bash

set -ex

kn service delete echo > /dev/null 2>&1 || true
sleep 3
kn service create echo --image duglin/echo | tee out
URL=$(tail -1 out)

curl -s $URL?sleep=600 -d "sleep 600" &
sleep 1
kn service delete echo
sleep 1
kn service list
kubectl get -w pods

it will create a ksvc, curl it and tell the request to sleep for 10 minutes. Meanwhile, in parallel, after 1 second we kill the ksvc.

The ksvc is deleted and the pod is put into terminating state. However, the pod lives for the full 10 minutes rather than be deleted immediately. I understand why this happens, but I don't think it's correct.

Deleting a ksvc is not the same as deleting an instance, where we want (sometimes) to allow for the current requests to finish. In this case the user has specifically asked for all resources associated with the ksvc to be deleted, and therefore all requests should be stopped as well.

This is critical for users to at least two reasons:
1 - they may wish to halt the execution of the pods and stop their processing. If the pods are misbehaving then the user may need to stop that processing.
2 - once the user has tried to delete a ksvc, they should reasonable expect to no longer be charged for the pods running for the ksvc. It's fair for the provider to charge them while the pods are busy (even when in terminating state), so it becomes important from a cost perspective to allow the user to have control over their costs by halting the pods immediately.

[vagababov]

Not sure if this is autoscale. It's more like API or core serving.

[duglin]

Maybe - wasn't sure

[mattmoor]

This is complicated for a wide variety of reasons. Generally, we aren't in the business of deleting basically anything. We create things and lean (heavily) on K8s GC for basically everything else. However, there are many reasons that Pods are deleted, it could be that the node is being taken offline, or its parent ReplicaSet may be scaling down. In the grand scheme of thing cascaded deletion is possibly (probably?) an incredibly niche reason for Pod death.

So I think the questions in my mind is two-fold:

1. Why isn't this a justification for Kubernetes to implement different pod-termination behavior when cascading deletion (vs. other reasons for deleting pods)?
2. Depending a lot on the answer to Validate all incoming objects (need to modify webhook to watch for others, make webhook generic) #1, how would we justify a departure from the K8s deletion semantics?

[duglin]

That's why I made a distinction between deleting an instance and deleting the ksvc. I agree that there could be reasons for a graceful shutdown of an instance (where we wait until requests are finished), but I think deleting the ksvc itself isn't part of those cases. It seems fair to say that if a user deletes a ksvc that they're done with it and want it's resources deleted. If we want to head down the path of allowing for a graceful deletion then I think we'd need to offer a flag to give the user the choice of "immediate" vs "graceful" shutdown. But w/o that choice I think the more likely semantics that people would expect is that "delete" == "everything is deleted, now". Otherwise, they have no way to control their costs with a run-away request/ksvc.

To your questions: I'm not sure this is a kube thing since kube will try to delete the pod right away. We have logic in the QP that pauses it and waits for the current requests to close down. When I comment out that code and re-run my testcase I see the pod go away immediately.

[mattmoor]

Right, you're talking about logic that exists to support graceful shutdown and avoid 5xx errors in the much more common case that we are scaling things down, and we aren't given any signal to distinguish between those.

K8s knows the difference, and has modes where it can bypass this graceful shutdown, which sounds like exactly what you want. However, the question is why we should implement that vs. pushing for that to be the K8s semantic when cascading deletion.

[duglin]

I’m not familiar with those “modes”. What does Kube do differently in those cases? Will it skip sending the TERM signal?

The more appropriate, long term, solution might be a Kube thing, but in the meantime it might be good to consider another “lemonade” to hold us over. Seems like there might be some options for us to get the semantics we want in the meantime without being too disruptive. Perhaps something like sending a message to each ksvc’s pod to tell it that a delete of the ksvc is in progress so a TERM shouldn’t be graceful, then we can delete the deployment. I’m sure we can think of something...

[mattmoor]

From kubectl delete pod --help:

Some resources, such as pods, support graceful deletion. These resources define a default period before they are forcibly terminated (the grace period) but you may override that value with the --grace-period flag, or pass --now to set a grace-period of 1. Because these resources often represent entities in the cluster, deletion may not be acknowledged immediately. If the node hosting a pod is down or cannot reach the API server, termination may take significantly longer than the grace period. To force delete a resource, you must specify the --force flag. Note: only a subset of resources support graceful deletion. In absence of the support, --grace-period is ignored.

We'd have to dig to figure out what kubectl is doing, but even knowing that we'd have to reach around up to 4 layers of abstraction to achieve what you are after, and there's really no guarantees that those layers won't "fight back". The thing with "lemonade" was that we had line of sight to a solution in Kube, it was just a year or so out from being something we could adopt. I'm also receptive to "leading the ball" with stuff like what we were hoping to do with the graceful shutdown KEP, assuming we could get some traction going upstream as the long-term solution. This feels more tenuous than the graceful shutdown KEP, but if there's a longer-term plan upstream for how we offload the semantic, then I think it'd be a conversation worth having.

[duglin]

ah ok - I was thinking there was some super secret "mode" thingy I hadn't heard of :-)

Just FYI and for my own education... did a quick test, and when I delete one of our pods with gracePeriod=0 the TERM signal is still received by the queue proxy but then it appears to be killed even before we get a chance to shutdown the servers. So that's nice.

However, deleting the rs or deployment with gracePeriod=0 and force=true doesn't seem to propagate those flags down to the dependent pod's delete(). Asking in kube-land to see why that is...

[julz]

Looks intentional upstream, unfortunately :/

kubernetes/kubernetes#24964

kubernetes/kubernetes#25055

I guess a not-super-great-at-all option is to have a kn plugin that hard-deletes any pods in a ksvc using the pod-level k8s apis. User would need permissions and visibility to pods for that to work obviously, though.

[mattmoor]

Well, the other half of my point here is that if you have a ksvc -> Configuration -> Revision -> Deployment -> ReplicaSet -> Pods, you have to cascade things pretty far... 😅

If you delete ksvc on a K8s under moderate load, the GC is pretty slow to cascade even the basic tombstone bit, so if a plugin were to reach around and delete the pods, it is entirely plausible that the replicaset would actually spin up new ones! 🙃

If anything upstream, I'd probably expect this to be a mode on "foreground deletion", which already follows a somewhat specialized path.

[julz]

Could a plug-in just list pods for a ksvc (via selector) in Terminating state and shut them down forcefully? I think that avoids any races because you're just speeding up terminations that're already in progress. Of course, more can be created, but this is just solving the case where you shut down the ksvc but stuff is still running (in terminating state) and you'd like it not to be. Certainly not as a top level solution, but as a kn plug-in seems like it might solve a bit of pain for some people.

[mattmoor]

I'd see what happens with the above kubectl command when run on a Pod that's already terminating. 🤷

[duglin]

I'd see what happens with the above kubectl command when run on a Pod that's already terminating

if you mean, what happens if you try to delete a terminating pod again, but this time with grace-period=0.... it seems to have no effect. I guess the grace-period needs to be on the initial delete.