Do not reconcile objects that are being deleted

danail-branekov · georgethebeatle · danail-branekov · commit e4ef85e73b2d · 2025-02-25T12:51:33.000Z
If the kpack image (or its builds) has a finalizer being added by an
external component, then when the client request the object to be
deleted, the image/build reconcilers keep reconciling the image/build
causing new build pods to be created as the image is being deleted. Even
though eventually the image gets deleted, it takes significant amount of
time.

Co-authored-by: Georgi Sabev &lt;georgethebeatle@gmail.com&gt;
diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go
@@ -99,7 +99,10 @@ func NewController(
 
 	impl := controller.NewContext(ctx, c, controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger})
 
-	informer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	informer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 
 	podInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
 		FilterFunc: controller.FilterControllerGK(buildapi.SchemeGroupVersion.WithKind(Kind).GroupKind()),
diff --git a/pkg/reconciler/builder/builder.go b/pkg/reconciler/builder/builder.go
@@ -89,7 +89,10 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	builderInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	builderInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 
 	c.Tracker = tracker.New(impl.EnqueueKey, opt.TrackerResyncPeriod())
 	clusterStoreInformer.Informer().AddEventHandler(controller.HandleAll(
diff --git a/pkg/reconciler/buildpack/buildpack.go b/pkg/reconciler/buildpack/buildpack.go
@@ -36,7 +36,8 @@ func NewController(
 	opt reconciler.Options,
 	keychainFactory registry.KeychainFactory,
 	buildpackInformer buildinformers.BuildpackInformer,
-	storeReader StoreReader) *controller.Impl {
+	storeReader StoreReader,
+) *controller.Impl {
 	c := &Reconciler{
 		Client:          opt.Client,
 		BuildpackLister: buildpackInformer.Lister(),
@@ -55,7 +56,11 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	buildpackInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	buildpackInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
+
 	return impl
 }
 
diff --git a/pkg/reconciler/clusterbuilder/clusterbuilder.go b/pkg/reconciler/clusterbuilder/clusterbuilder.go
@@ -86,7 +86,10 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	clusterBuilderInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	clusterBuilderInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 
 	c.Tracker = tracker.New(impl.EnqueueKey, opt.TrackerResyncPeriod())
 	clusterBuildpackInformer.Informer().AddEventHandler(controller.HandleAll(
diff --git a/pkg/reconciler/clusterbuildpack/clusterbuildpack.go b/pkg/reconciler/clusterbuildpack/clusterbuildpack.go
@@ -36,7 +36,8 @@ func NewController(
 	opt reconciler.Options,
 	keychainFactory registry.KeychainFactory,
 	clusterBuildpackInformer buildinformers.ClusterBuildpackInformer,
-	storeReader StoreReader) *controller.Impl {
+	storeReader StoreReader,
+) *controller.Impl {
 	c := &Reconciler{
 		Client:                 opt.Client,
 		ClusterBuildpackLister: clusterBuildpackInformer.Lister(),
@@ -55,7 +56,10 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	clusterBuildpackInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	clusterBuildpackInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 	return impl
 }
 
diff --git a/pkg/reconciler/clusterstack/clusterstack.go b/pkg/reconciler/clusterstack/clusterstack.go
@@ -55,7 +55,10 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	clusterStackInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	clusterStackInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 	return impl
 }
 
diff --git a/pkg/reconciler/clusterstore/clusterstore.go b/pkg/reconciler/clusterstore/clusterstore.go
@@ -55,7 +55,10 @@ func NewController(
 		},
 		controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},
 	)
-	clusterStoreInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	clusterStoreInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 	return impl
 }
 
diff --git a/pkg/reconciler/filter.go b/pkg/reconciler/filter.go
@@ -0,0 +1,12 @@
+package reconciler
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+func FilterDeletionTimestamp(obj interface{}) bool {
+	object, ok := obj.(metav1.Object)
+	if !ok {
+		return false
+	}
+
+	return object.GetDeletionTimestamp() == nil
+}
diff --git a/pkg/reconciler/filter_test.go b/pkg/reconciler/filter_test.go
@@ -0,0 +1,40 @@
+package reconciler_test
+
+import (
+	"testing"
+	"time"
+
+	buildapi "github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
+	"github.com/pivotal/kpack/pkg/reconciler"
+	"github.com/sclevine/spec"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestFilterDeletionTimestamp(t *testing.T) {
+	spec.Run(t, "FilterDeletionTimestamp", testFilterDeletionTimestamp)
+}
+
+func testFilterDeletionTimestamp(t *testing.T, when spec.G, it spec.S) {
+	when("#FilterDeletionTimestamp", func() {
+		it("returns true", func() {
+			require.True(t, reconciler.FilterDeletionTimestamp(&buildapi.Build{}))
+		})
+
+		when("object is deleted", func() {
+			it("returns false", func() {
+				require.False(t, reconciler.FilterDeletionTimestamp(buildapi.Build{
+					ObjectMeta: metav1.ObjectMeta{
+						DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					},
+				}))
+			})
+		})
+
+		when("not an object", func() {
+			it("returns false", func() {
+				require.False(t, reconciler.FilterDeletionTimestamp("not an object"))
+			})
+		})
+	})
+}
diff --git a/pkg/reconciler/image/image.go b/pkg/reconciler/image/image.go
@@ -63,7 +63,10 @@ func NewController(
 
 	impl := controller.NewContext(ctx, c, controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger})
 
-	imageInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	imageInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 
 	buildInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
 		FilterFunc: controller.FilterControllerGK(buildapi.SchemeGroupVersion.WithKind(Kind).GroupKind()),
diff --git a/pkg/reconciler/sourceresolver/sourceresolver.go b/pkg/reconciler/sourceresolver/sourceresolver.go
@@ -57,7 +57,10 @@ func NewController(
 		delay:        opt.SourcePollingFrequency,
 	}
 
-	sourceResolverInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+	sourceResolverInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: reconciler.FilterDeletionTimestamp,
+		Handler:    controller.HandleAll(impl.Enqueue),
+	})
 
 	return impl
 }

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,10 @@ func NewController(`
`55`	`55`	`},`
`56`	`56`	`controller.ControllerOptions{WorkQueueName: ReconcilerName, Logger: logger},`
`57`	`57`	`)`
`58`		`- clusterStackInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))`
	`58`	`+ clusterStackInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{`
	`59`	`+ FilterFunc: reconciler.FilterDeletionTimestamp,`
	`60`	`+ Handler: controller.HandleAll(impl.Enqueue),`
	`61`	`+ })`
`59`	`62`	`return impl`
`60`	`63`	`}`
`61`	`64`