Don't recreate resources as we're tearing a thing down.

[mattmoor]

We recently started registering DeleteFunc handlers for our resources, which enables us to quickly recreate resources if they are deleted from out from under us.

A byproduct of this (still needs confirmation) is that I noticed that sometimes when a Revision is being torn down, that right at the moment the Deployments pod starts Terminating another Pod (under a different ReplicaSet) appears. At first I thought this was us updating the Deployment (e.g. #2632), but we are clearly creating two Deployments in our logs:

I think what's happening is that we Reconcile the Revision when it has a DeletionTimestamp (mid delete) and see no Deployment, so we recreate it.

cc @lichuqiang @dgerd

[dgerd]

Your explanation makes the most sense to me. Is this reproducible all the time? If so it should be easy to add some instrumentation to confirm. Happy to take a stab at doing this.

serving/pkg/reconciler/v1alpha1/revision/reconcile_resources.go

Lines 43 to 56 in 678373d

	ns := rev.Namespace
	deploymentName := resourcenames.Deployment(rev)
	logger := logging.FromContext(ctx).With(zap.String(logkey.Deployment, deploymentName))
	deployment, err := c.deploymentLister.Deployments(ns).Get(deploymentName)
	if apierrs.IsNotFound(err) {
	// Deployment does not exist. Create it.
	rev.Status.MarkDeploying("Deploying")
	deployment, err = c.createDeployment(ctx, rev)
	if err != nil {
	logger.Errorf("Error creating deployment %q: %v", deploymentName, err)
	return err
	}
	logger.Infof("Created deployment %q", deploymentName)

[dgerd]

/milestone Serving 0.4

[dgerd]

/assign @dgerd

[dgerd]

From @mattmoor: "I think the key is that we didn't make Reconcile() sensitive to metadata.deletionTimestamp when we added the OnDelete hooks. I believe deletionTimestamp gets set as a tombstone as Forground GC runs"

[dgerd]

I added some logging and attempted to reproduce this at HEAD by creating a runLatest Service and then deleting the service. I tried multiple times waiting: immediately after, after a few seconds, and after a few minutes. I have yet to reproduce this.

I am using kubectl apply -f to create and kubectl delete -f to delete the service. I will try throwing traffic at it before deleting to see if that changes the behavior at all.

Let me know if you have any other reproduction advice.

[mattmoor]

So I can see this in the scale test (at least cranked up to 150 as I have it right now), e.g.

scale-00150-015-zztulrto-00001-deployment-6977d4bcc-xd749    1/2     Terminating   0          3m
scale-00150-015-zztulrto-00001-deployment-97785665f-8t659    1/2     Terminating   0          1m

Interpreting from the suffixes, these pods came from distinct ReplicaSets, and the main way that would happen would be if we created a second deployment while the pods from the first were still tearing down.

[mattmoor]

I added some logic in the revision controller to log when we see Revisions with a DeletionTimestamp, and running N=100 a bunch, I'm not seeing my log statement. The only other thing that comes to mind would be adding a delay to the queuing of Deployment deletion events using this.

[mattmoor]

I'd initially thought this was us doing something obviously wrong, but given that DeletionTimestamp is unset when this happens this seems cosmetic and somewhat tough to fix. Moving out of v1.