-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ implement helpers for add/remove finalizer #545
Conversation
It actually might make sense for the It also might make sense to handle the |
/assign @pwittrock |
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.
minor nit inline on naming. otherwise lgtm
|
||
// AddFinalizerWithError tries to convert a runtime object to a metav1 object and add the provided finalizer. | ||
// It returns an error if the provided object cannot provide an accessor. | ||
func AddFinalizerWithError(o runtime.Object, finalizer string) error { |
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.
nit: this name is not great. Perhaps AddFinalizerIfPossible
?
dunno why I assigned @pwittrock, blindly following the robot I guess /unassign @pwittrock |
squash the fixes, otherwise lgtm. |
0ba00d6
to
487bb8b
Compare
// RemoveFinalizer accepts a metav1 object and removes the provided finalizer if present. | ||
func RemoveFinalizer(o metav1.Object, finalizer string) { | ||
f := o.GetFinalizers() | ||
for i, e := range f { |
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.
Nit: This is nicer with k8s.io/apimachinery/pkg/util/sets.String
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.
found this today coincidentally. can try swapping it out 👍
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.
better?
487bb8b
to
a100c1b
Compare
|
||
// AddFinalizer accepts a metav1 object and adds the provided finalizer if not present. | ||
func AddFinalizer(o metav1.Object, finalizer string) { | ||
finalizers := sets.NewString(o.GetFinalizers()...) |
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 hate to go back an forth here, but this was actually probably fine with the linear search -- namely, the list of finalizers is almost always tiny, which means the linear search is probably fast, and we avoid what's actually probably more "work" plus an allocation building the set (plus, we don't re-order the list of finalizers). FWIW, k8s uses a normal linear search like the old code -- look for NeedsLBFinalizer and the finalizer code in the volume controller.
pkg/util/slice in apimachinery has a RemoveString and ContainsString that we can use to still make this simple to look at.
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.
makes sense. I noticed that most controller implementations (including the function you mentioned) make a DeepCopy to avoid mutating the informer cache. Is that something I should add here?
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.
err, i'm also not seeing those methods (or a pkg/util/slice in apimachinery). I think you mean these? https://github.com/kubernetes/kubernetes/blob/4d9e4f0b458c746f43836f558ebe61ec68bb7133/pkg/util/slice/slice.go
Happy to slice those out of k/k into apimachinery or somewhere else (pun heavily intended).
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 don't really understand the proper usage of controllers w.r.t informer cache because I saw this a lot pre-controller runtime, but don't see it as much in controllers using CR. I thought it might be under the hood somehow in CR (copying the object for you to avoid mutating the cache), but I didn't find it in internal/controller or in the manager code for injecting the cache.
Would love to understand more deeply to make sure I'm doing the right thing.
tiny PR, but learning lots... 😄
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 noticed most controller implementations make a DeepCopy
We automatically deepcopy before returning the object to you from the cache to avoid these sorts of issues: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/cache/internal/cache_reader.go#L75
I think you mean these?
Yeah, those are the ones that I meant :-)
CR (copying the object for you to avoid mutating the cache), but I didn't find it in internal/controller or in the manager code for injecting the cache.
See above :-)
97f03e6
to
a7084d4
Compare
// AddFinalizer accepts a metav1 object and adds the provided finalizer if not present. | ||
func AddFinalizer(o metav1.Object, finalizer string) { | ||
newFinalizers := AddString(o.GetFinalizers(), finalizer) | ||
o.SetFinalizers(append(newFinalizers, finalizer)) |
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.
huh? Doesn't this add the finalizer twice?
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.
pushed early, fixed
|
||
// AddString returns a []string with s appended if it is not already found in the provided slice. | ||
func AddString(slice []string, s string) []string { | ||
newSlice := make([]string, 0) |
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.
No reason to make a copy here:
for i, item := range slice {
if item == s {
return slice
}
}
return append(slice, s)
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.
also make these private
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 was wondering if there was a reason the k/k version made a copy, I stared at it a bit and decided no => already reduce to what you posted
} | ||
|
||
// RemoveString returns a newly created []string that contains all items from slice that are not equal to s. | ||
func RemoveString(slice []string, s string) []string { |
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.
make this private too
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.
fixed
7176ea4
to
e7e24af
Compare
@DirectXMan12 bump? 🙂 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeldeib, DirectXMan12 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 |
Skip update op when instance is created
#485
I don't have a particular preference between the runtime.Object and metav1.Object in the signature. I see both reasons from the linked issue as valid. Figured i'd open as such and ask for feedback.