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.

Sign up for GitHub

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

Jump to bottom

Fix duplicate controller references #99

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Fix duplicate controller references #99

merged 1 commit into from
Aug 3, 2018

Conversation

calind
Copy link
Contributor

@calind calind commented Jul 31, 2018

Fixes #68

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 31, 2018
@calind calind mentioned this pull request Jul 31, 2018
Closed
@droot
Copy link
Contributor

droot commented Jul 31, 2018

@calind Thanks for the PR. Looks good, Can we add a few tests to verify the behavior.

@calind
Copy link
Contributor Author

calind commented Jul 31, 2018

Will do. Can you take a look at #100 as well since the tests fail because of that.

Thanks!

@calind
Copy link
Contributor Author

calind commented Aug 1, 2018

@droot, since only one OwnerReference can have Controller=true what should be the semantics of this function?

  1. Return an error is another OwnerReference is a controller
  2. Remove the Controller=true flag from existing OwnerReference
  3. Remove any OwnerReference with Controller=true

I think that first option is the one to go.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 1, 2018
droot
droot reviewed Aug 1, 2018
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Return an error is another OwnerReference is a controller

This looks reasonable to me. Added a comment about the returning error.

pkg/controller/controllerutil/controllerutil.go Outdated
@@ -28,6 +28,8 @@ import (
// SetControllerReference sets owner as a Controller OwnerReference on owned.
// This is used for garbage collection of the owned object and for
// reconciling the owner object on changes to owned (with a Watch + EnqueueRequestForOwner).
// Since only one OwnerRecerence can be a controller, it returns an error if
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OwnerRecerence/OwnerReference

pkg/controller/controllerutil/controllerutil.go Outdated
if r.Kind == gvk.Kind && r.Name == owner.GetName() && r.UID == owner.GetUID() {
fi = i
} else if r.Controller != nil && *r.Controller {
return fmt.Errorf("Object %s/%s is already owned by another %s controller %s", object.GetNamespace(), object.GetName(), r.Kind, r.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

May be we should consider creating an error-type for this error because users would like to detect this error-type and kick-in some business logic for adopting of these objects (re-parenting to new owner). @DirectXMan12 what do you think ?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2018
@calind
Copy link
Contributor Author

calind commented Aug 2, 2018

@droot implemented the feedback, also cleared out #100 and now travis tests are passing.

droot
droot suggested changes Aug 2, 2018
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Thanks for improving it further. Few nits about the new changes, then good to go.

pkg/controller/controllerutil/controllerutil.go Outdated
@@ -25,9 +25,31 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
)

// AlreadyOwnedError is an error returned if the object you are trying to assign
// an controller reference is already owned by another controller
Copy link
Contributor

Choose a reason for hiding this comment

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

s/an/a

pkg/controller/controllerutil/controllerutil.go Outdated
}

// NewAlreadyOwnedError creates a new AlreadyOwnedError
func NewAlreadyOwnedError(Object v1.Object, Owner v1.OwnerReference) *AlreadyOwnedError {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fn doesn't need to be exported so pl. lowercase it.

pkg/controller/controllerutil/controllerutil.go Outdated
return nil
}

// Returns true if a and b point to the same object
func sameOwnerReference(a, b v1.OwnerReference) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since its comparing the GVKs, better name should be gvkEquals(a,b). Current name reads as a, b has same owner ?

I wonder if something like this exists the apimachinery already ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've looked trough apimachinery but didn't found a comparison function for OwnerReference objects. As for GVK they are directly comparable since they are just structs made of strings :)

@droot
Copy link
Contributor

droot commented Aug 3, 2018

@calind This is good to go. Can you pl. squash the commits ?. Thank you so much for this PR.

@calind
Copy link
Contributor Author

calind commented Aug 3, 2018

@droot done. Thanks for the feedback.

@droot droot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: calind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 471f9cd into kubernetes-sigs:master Aug 3, 2018
justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018
@k8s-ci-robot
Merge pull request kubernetes-sigs#99 from presslabs/fix-set-owner-re…
5eb4f11 
…ference

Fix duplicate controller references
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
@droot
Merge pull request #99 from fanzhangio/issue50
fd34223 
Add binary path in testEnvironment for kubebuilder integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droot droot droot requested changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@calind @droot @k8s-ci-robot