-
Notifications
You must be signed in to change notification settings - Fork 103
Conversation
@rlguarino @crimsonfaith91 This is ready for review now. You might want to start with the docs and examples to get an overview of the intended UX before diving into the implementation. |
@enisoc I reviewed all commits except the main commit (5th commit). Should we also add finalizer smoke tests for |
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.
I reviewed all commits except the main commit (5th commit).
Thanks! I've done a pass over the comments so far. Please hit "Resolve conversation" on each thread if your concern is addressed so we can skip it in the next pass.
Should we also add finalizer smoke tests for BlueGreen and IndexedJob?
For now, I focused on adding one test for a CompositeController and one for a DecoratorController. We can think later about whether those examples would be improved by adding finalizers, but my first instinct is that those particular use cases are already well-served by background GC.
// First check if we should instead call the finalize hook, | ||
// which has the same API as the sync hook except that it's | ||
// called while the object is pending deletion. | ||
if request.Parent.GetDeletionTimestamp() != nil && cc.Spec.Hooks.Finalize != nil { |
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.
Is it possible to factor out common functions for both composite and decorator controllers?
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 would be nice, but I'm not sure where we could draw a clean line of abstraction. For example, the logic on this particular line is a little bit different than in DecoratorController's version of callSyncHook
.
/lgtm |
This adds a
finalize
hook to both CompositeController and DecoratorController.If a
finalize
hook is defined, Metacontroller adds a controller-specific finalizer tometadata.finalizers
to block deletion of the parent object until the user'sfinalize
handler reports that it's satisfied. This enables reliable cleanup of state that the built-in garbage collector can't handle (e.g. resources in an external system).Closes #60