-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not reconcile images and builds being deleted #1820
base: main
Are you sure you want to change the base?
Do not reconcile images and builds being deleted #1820
Conversation
When an image is being deleted, k8s does not delete the object immediately, instead it sets the deletion timestamp awaiting finalization. See [k8s docs](https://github.com/kubernetes/apimachinery/blob/45d29dc4d66fc2ac83e736e79752ad81a9c6195f/pkg/apis/meta/v1/types.go#L190-L209) for details If the kpack image (or its builds) has a finalizer being added by an external component (such as [Korifi](https://github.com/cloudfoundry/korifi/blob/17557eb68fed830f3f57abd651882a712fc25f5f/kpack-image-builder/controllers/webhooks/finalizer/finalizer_webhook.go#L25)), 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. Signed-off-by: Danail Branekov <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1820 +/- ##
===========================================
- Coverage 67.34% 35.33% -32.02%
===========================================
Files 140 257 +117
Lines 8886 19968 +11082
===========================================
+ Hits 5984 7055 +1071
- Misses 2393 12311 +9918
- Partials 509 602 +93 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels kinda awkward to have this short circuit on only the Build and Image, I wonder if we should just add this to all our reconcilers.
I imagine we can make use of the FilterFunc
to skip any resource with non-nil DeletionTimestamps
kpack/pkg/reconciler/image/image.go
Lines 66 to 71 in d3e70fe
imageInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) | |
buildInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ | |
FilterFunc: controller.FilterControllerGK(buildapi.SchemeGroupVersion.WithKind(Kind).GroupKind()), | |
Handler: controller.HandleAll(impl.EnqueueControllerOf), | |
}) |
Something like pkg/reconciler/filter.go
:
func FilterDeletionTimestamp(obj interface{}) bool {
object, ok := obj.(metav1.Object)
if !ok {
return false
}
return object.GetDeletionTimestamp() == nil
}
And in each reconciler:
- informer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+ informer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+ FilterFunc: reconciler.FilterDeletionTimestamp,
+ Handler: controller.HandleAll(impl.Enqueue),
+ })
This way we can also get away without modifying all the reconciler tests
When an image is being deleted, k8s does not delete the object immediately, instead it sets the deletion timestamp awaiting finalization. See
k8s docs for details
If the kpack image (or its builds) has a finalizer being added by an external component (such as
Korifi), 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.
In order to address this, this PR "short-circuits" the image and build reconcilers to immediately return whenever an image or a build has their deletion timestamp set, thus preventing builds and build pods being rescheduled.