Skip to content
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

Allow ChildReconciler to work without owner references #235

Merged
merged 2 commits into from May 16, 2022

Conversation

scothis
Copy link
Contributor

@scothis scothis commented May 13, 2022

Owner references only work within the same namespace (or cluster scope).
Finalizers give us the ability to clean up state in other scopes, where
owner references are not valid.

ChildReconciler can now opt-out of owner references with
SkipOwnerReference, using a child Finalizer implies SkipOwnerReference.
Since we can no longer link parent and child resources via the owner
reference, the child is tracked explicitly and the OurChild method must
be defined and able to uniquely distinguish the child resource that
belongs to the parent from other resources of the same kind.

Resolves #83

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #235 (0089c31) into main (ed9214d) will decrease coverage by 0.19%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
- Coverage   66.34%   66.14%   -0.20%     
==========================================
  Files           9        9              
  Lines        1144     1158      +14     
==========================================
+ Hits          759      766       +7     
- Misses        360      365       +5     
- Partials       25       27       +2     
Impacted Files Coverage Δ
reconcilers/reconcilers.go 80.75% <44.44%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed9214d...0089c31. Read the comment docs.

Owner references only work within the same namespace (or cluster scope).
Finalizers give us the ability to clean up state in other scopes, where
owner references are not valid.

ChildReconciler can now opt-out of owner references with
SkipOwnerReference, using a child Finalizer implies SkipOwnerReference.
Since we can no longer link parent and child resources via the owner
reference, the child is tracked explicitly and the OurChild method must
be defined and able to uniquely distinguish the child resource that
belongs to the parent from other resources of the same kind.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
Copy link
Collaborator

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand it correctly, then this allows to have a parent reconcile into a child beyond the namespace boundary, but within the same cluster.

(recapping for self) In total that gives the following reconciler options

  • owned child resource in the same ns
  • finalized child resource in another ns
  • sync anything
  • finalized sync anything

(thinking out loud) I wonder if there's a way to have more type safety to the first two. ChildReconciler needs to have the right combination of Finalize, OurChild and SkipOwnerReference. It all revolves around "child management". Maybe ChildReconciler.ChildManager could return smth that implements a FinalizerManager or OwnerManager... meh. Unsure.
It may just needlessly complicate things. And finalization is new feature anyway.

LGTM 👍

@@ -919,6 +940,10 @@ func (r *ChildReconciler) validate(ctx context.Context) error {
return fmt.Errorf("ChildReconciler %q must implement OurChild: nil | func(%s, %s) bool, found: %s", r.Name, reflect.TypeOf(castParentType), reflect.TypeOf(r.ChildType), fn)
}
}
if r.OurChild == nil && r.SkipOwnerReference {
// OurChild is required when SkipOwnerReference is true
return fmt.Errorf("ChildReconciler %q must implement OurChild", r.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion]: Let the error document why this ChildReconciler needs to implement OurChild.


// normally tracks should occur before API operations, but when creating a resource with a
// generated name, we need to know the actual resource name.
if err := c.Tracker.TrackChild(ctx, parent, desired, c.Scheme()); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question]: How is this use of TrackChild an exception to the rule stated in the comment above? The API operation happens after this line, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create call is on line 1074

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@scothis scothis merged commit 75bd54f into vmware-labs:main May 16, 2022
@scothis scothis deleted the skip-child-owner-ref branch May 16, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-owner referenced child resources
3 participants