Ignore resources when DeletionTimestamp is set. (#3036)

When we had finalizers previously we would race with K8s GC to recreate our children as K8s reaped them.

The simplest way to test this is to enable "foreground" deletion in our e2e tests, which is implemented as a finalizer.

Fixes: #2678

pkg/reconciler/v1alpha1/autoscaling/hpa/hpa.go

@@ -131,6 +131,10 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 func (c *Reconciler) reconcile(ctx context.Context, key string, pa *pav1alpha1.PodAutoscaler) error {
 	logger := logging.FromContext(ctx)
 
+	if pa.GetDeletionTimestamp() != nil {
+		return nil
+	}
+
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result
 	// in this getting written back to the API Server, but lets downstream logic make

pkg/reconciler/v1alpha1/autoscaling/hpa/hpa_test.go

@@ -55,6 +55,13 @@ func TestReconcile(t *testing.T) {
 			pa(testRevision, testNamespace, WithKPAClass),
 		},
 		Key: key(testRevision, testNamespace),
+	}, {
+		Name: "nop deletion reconcile",
+		// Test that with a DeletionTimestamp we do nothing.
+		Objects: []runtime.Object{
+			pa(testRevision, testNamespace, WithHPAClass, WithPADeletionTimestamp),
+		},
+		Key: key(testRevision, testNamespace),
 	}, {
 		Name:    "delete when pa does not exist",
 		Objects: []runtime.Object{},

pkg/reconciler/v1alpha1/autoscaling/kpa/kpa.go

@@ -173,6 +173,10 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error {
 	logger := logging.FromContext(ctx)
 
+	if pa.GetDeletionTimestamp() != nil {
+		return nil
+	}
+
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result
 	// in this getting written back to the API Server, but lets downstream logic make

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

@@ -174,6 +174,9 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.ClusterIngress) (*v1alpha1.C
 
 func (c *Reconciler) reconcile(ctx context.Context, ci *v1alpha1.ClusterIngress) error {
 	logger := logging.FromContext(ctx)
+	if ci.GetDeletionTimestamp() != nil {
+		return nil
+	}
 
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result

pkg/reconciler/v1alpha1/configuration/configuration.go

@@ -149,6 +149,9 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 
 func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configuration) error {
 	logger := logging.FromContext(ctx)
+	if config.GetDeletionTimestamp() != nil {
+		return nil
+	}
 
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result

pkg/reconciler/v1alpha1/configuration/configuration_test.go

@@ -60,6 +60,13 @@ func TestReconcile(t *testing.T) {
 	}, {
 		Name: "key not found",
 		Key:  "foo/not-found",
+	}, {
+		Name: "nop deletion reconcile",
+		// Test that with a DeletionTimestamp we do nothing.
+		Objects: []runtime.Object{
+			cfg("foo", "delete-pending", 1234, WithConfigDeletionTimestamp),
+		},
+		Key: "foo/delete-pending",
 	}, {
 		Name: "create revision matching generation",
 		Objects: []runtime.Object{

pkg/reconciler/v1alpha1/revision/revision.go

@@ -350,6 +350,9 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1alpha1.Revision
 
 func (c *Reconciler) reconcile(ctx context.Context, rev *v1alpha1.Revision) error {
 	logger := commonlogging.FromContext(ctx)
+	if rev.GetDeletionTimestamp() != nil {
+		return nil
+	}
 
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result

pkg/reconciler/v1alpha1/revision/table_test.go

@@ -52,6 +52,13 @@ func TestReconcile(t *testing.T) {
 		Name: "key not found",
 		// Make sure Reconcile handles good keys that don't exist.
 		Key: "foo/not-found",
+	}, {
+		Name: "nop deletion reconcile",
+		// Test that with a DeletionTimestamp we do nothing.
+		Objects: []runtime.Object{
+			rev("foo", "delete-pending", WithRevisionDeletionTimestamp),
+		},
+		Key: "foo/delete-pending",
 	}, {
 		Name: "first revision reconciliation",
 		// Test the simplest successful reconciliation flow.

pkg/reconciler/v1alpha1/route/route.go

@@ -222,7 +222,6 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 
 func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
 	logger := logging.FromContext(ctx)
-
 	if r.GetDeletionTimestamp() != nil {
 		// Check for a DeletionTimestamp.  If present, elide the normal reconcile logic.
 		return c.reconcileDeletion(ctx, r)

pkg/reconciler/v1alpha1/service/service.go

@@ -160,6 +160,9 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 
 func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) error {
 	logger := logging.FromContext(ctx)
+	if service.GetDeletionTimestamp() != nil {
+		return nil
+	}
 
 	// We may be reading a version of the object that was stored at an older version
 	// and may not have had all of the assumed defaults specified.  This won't result

pkg/reconciler/v1alpha1/service/service_test.go

@@ -44,6 +44,13 @@ func TestReconcile(t *testing.T) {
 	}, {
 		Name: "key not found",
 		Key:  "foo/not-found",
+	}, {
+		Name: "nop deletion reconcile",
+		// Test that with a DeletionTimestamp we do nothing.
+		Objects: []runtime.Object{
+			svc("delete-pending", "foo", WithServiceDeletionTimestamp),
+		},
+		Key: "foo/delete-pending",
 	}, {
 		Name: "incomplete service",
 		Objects: []runtime.Object{

pkg/reconciler/v1alpha1/testing/functional.go

@@ -97,6 +97,12 @@ var (
 	}
 )
 
+// WithServiceDeletionTimestamp will set the DeletionTimestamp on the Service.
+func WithServiceDeletionTimestamp(r *v1alpha1.Service) {
+	t := metav1.NewTime(time.Unix(1e9, 0))
+	r.ObjectMeta.SetDeletionTimestamp(&t)
+}
+
 // WithRunLatestRollout configures the Service to use a "runLatest" rollout.
 func WithRunLatestRollout(s *v1alpha1.Service) {
 	s.Spec = v1alpha1.ServiceSpec{
@@ -422,6 +428,12 @@ func WithRouteLabel(key, value string) RouteOption {
 // ConfigOption enables further configuration of a Configuration.
 type ConfigOption func(*v1alpha1.Configuration)
 
+// WithConfigDeletionTimestamp will set the DeletionTimestamp on the Config.
+func WithConfigDeletionTimestamp(r *v1alpha1.Configuration) {
+	t := metav1.NewTime(time.Unix(1e9, 0))
+	r.ObjectMeta.SetDeletionTimestamp(&t)
+}
+
 // WithBuild adds a Build to the provided Configuration.
 func WithBuild(cfg *v1alpha1.Configuration) {
 	cfg.Spec.Build = &v1alpha1.RawExtension{
@@ -517,6 +529,12 @@ func WithConfigLabel(key, value string) ConfigOption {
 // RevisionOption enables further configuration of a Revision.
 type RevisionOption func(*v1alpha1.Revision)
 
+// WithRevisionDeletionTimestamp will set the DeletionTimestamp on the Revision.
+func WithRevisionDeletionTimestamp(r *v1alpha1.Revision) {
+	t := metav1.NewTime(time.Unix(1e9, 0))
+	r.ObjectMeta.SetDeletionTimestamp(&t)
+}
+
 // WithInitRevConditions calls .Status.InitializeConditions() on a Revision.
 func WithInitRevConditions(r *v1alpha1.Revision) {
 	r.Status.InitializeConditions()
@@ -722,6 +740,12 @@ func WithNoTraffic(reason, message string) PodAutoscalerOption {
 	}
 }
 
+// WithPADeletionTimestamp will set the DeletionTimestamp on the PodAutoscaler.
+func WithPADeletionTimestamp(r *autoscalingv1alpha1.PodAutoscaler) {
+	t := metav1.NewTime(time.Unix(1e9, 0))
+	r.ObjectMeta.SetDeletionTimestamp(&t)
+}
+
 // WithHPAClass updates the PA to add the hpa class annotation.
 func WithHPAClass(pa *autoscalingv1alpha1.PodAutoscaler) {
 	if pa.Annotations == nil {

test/clients.go

@@ -131,6 +131,11 @@ func (clients *ServingClients) Delete(routes []string, configs []string, service
 		{clients.Services, services},
 	}
 
+	propPolicy := v1.DeletePropagationForeground
+	dopt := &v1.DeleteOptions{
+		PropagationPolicy: &propPolicy,
+	}
+
 	for _, deletion := range deletions {
 		if deletion.client == nil {
 			continue
@@ -141,7 +146,7 @@ func (clients *ServingClients) Delete(routes []string, configs []string, service
 				continue
 			}
 
-			if err := deletion.client.Delete(item, nil); err != nil {
+			if err := deletion.client.Delete(item, dopt); err != nil {
 				return err
 			}
 		}