Skip to content
This repository has been archived by the owner on Jul 22, 2022. It is now read-only.

Add finalize hook #100

Merged
merged 8 commits into from
Oct 1, 2018
Merged

Add finalize hook #100

merged 8 commits into from
Oct 1, 2018

Conversation

enisoc
Copy link

@enisoc enisoc commented Sep 25, 2018

This adds a finalize hook to both CompositeController and DecoratorController.

If a finalize hook is defined, Metacontroller adds a controller-specific finalizer to metadata.finalizers to block deletion of the parent object until the user's finalize 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

@enisoc enisoc changed the title [WIP] Add finalize hook Add finalize hook Sep 26, 2018
@enisoc
Copy link
Author

enisoc commented Sep 26, 2018

@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.

@crimsonfaith91
Copy link
Contributor

@enisoc I reviewed all commits except the main commit (5th commit). Should we also add finalizer smoke tests for BlueGreen and IndexedJob?

Copy link
Author

@enisoc enisoc left a 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.

dynamic/clientset/clientset.go Show resolved Hide resolved
dynamic/object/metadata.go Show resolved Hide resolved
examples/catset/test.sh Outdated Show resolved Hide resolved
examples/service-per-pod/README.md Outdated Show resolved Hide resolved
examples/service-per-pod/test.sh Show resolved Hide resolved
// 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 {
Copy link
Contributor

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?

Copy link
Author

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.

@crimsonfaith91
Copy link
Contributor

/lgtm
Great work!

@enisoc enisoc merged commit 754e3fa into GoogleCloudPlatform:master Oct 1, 2018
@enisoc enisoc deleted the finalize branch October 1, 2018 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants