✨ implement helpers for add/remove finalizer #545

alexeldeib · 2019-07-29T21:44:56Z

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.

alexeldeib · 2019-07-30T00:52:47Z

It actually might make sense for the WithError version to call client.Update()? that way, you get one error to handle, or you can do the granular management on your own metav1 object.

It also might make sense to handle the contains checking in a helper to facilitate writing loops similar to the book tutorial.

alexeldeib · 2019-07-30T05:52:33Z

/assign @pwittrock

DirectXMan12

minor nit inline on naming. otherwise lgtm

DirectXMan12 · 2019-08-06T20:48:46Z

pkg/controller/controllerutil/controllerutil.go

+
+// 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 {


nit: this name is not great. Perhaps AddFinalizerIfPossible?

alexeldeib · 2019-08-07T06:50:14Z

dunno why I assigned @pwittrock, blindly following the robot I guess

/unassign @pwittrock

DirectXMan12 · 2019-08-14T00:15:06Z

squash the fixes, otherwise lgtm.

pkg/controller/controllerutil/controllerutil.go

alvaroaleman · 2019-09-16T22:02:46Z

pkg/controller/controllerutil/controllerutil.go

+// 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 {


Nit: This is nicer with k8s.io/apimachinery/pkg/util/sets.String

found this today coincidentally. can try swapping it out 👍

DirectXMan12 · 2019-09-17T01:50:28Z

pkg/controller/controllerutil/controllerutil.go

+
+// 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()...)


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.

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?

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

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

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 :-)

DirectXMan12 · 2019-09-17T21:23:48Z

pkg/controller/controllerutil/controllerutil.go

+// 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))


huh? Doesn't this add the finalizer twice?

pushed early, fixed

DirectXMan12 · 2019-09-17T21:28:56Z

pkg/controller/controllerutil/controllerutil.go

+
+// 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)


No reason to make a copy here:

for i, item := range slice { if item == s { return slice } } return append(slice, s)

also make these private

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

DirectXMan12 · 2019-09-17T21:31:44Z

pkg/controller/controllerutil/controllerutil.go

+}
+
+// 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 {


make this private too

alexeldeib · 2019-10-17T09:48:21Z

@DirectXMan12 bump? 🙂

DirectXMan12 · 2019-10-18T21:06:51Z

/approve
/lgtm

k8s-ci-robot · 2019-10-18T21:06:59Z

[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:

~~OWNERS~~ [DirectXMan12]

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

Skip update op when instance is created

k8s-ci-robot requested review from DirectXMan12 and pwittrock July 29, 2019 21:44

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2019

k8s-ci-robot assigned pwittrock Jul 30, 2019

DirectXMan12 suggested changes Aug 6, 2019

View reviewed changes

k8s-ci-robot unassigned pwittrock Aug 7, 2019

DirectXMan12 mentioned this pull request Aug 14, 2019

Migrate Cluster Controller to Kubebuilder v2 kubernetes-sigs/cluster-api#1252

Merged

guymguym reviewed Aug 14, 2019

View reviewed changes

pkg/controller/controllerutil/controllerutil.go Show resolved Hide resolved

alexeldeib force-pushed the ace/finalizer branch from 0ba00d6 to 487bb8b Compare August 15, 2019 13:38

alvaroaleman reviewed Sep 16, 2019

View reviewed changes

alexeldeib force-pushed the ace/finalizer branch from 487bb8b to a100c1b Compare September 16, 2019 22:19

DirectXMan12 suggested changes Sep 17, 2019

View reviewed changes

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 Sep 17, 2019

alexeldeib force-pushed the ace/finalizer branch 3 times, most recently from 97f03e6 to a7084d4 Compare September 17, 2019 21:20

DirectXMan12 suggested changes Sep 17, 2019

View reviewed changes

✨ implement helpers for add/remove finalizer

e7e24af

alexeldeib force-pushed the ace/finalizer branch from 7176ea4 to e7e24af Compare October 10, 2019 03:05

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2019

k8s-ci-robot assigned DirectXMan12 Oct 18, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2019

k8s-ci-robot merged commit ca33b65 into kubernetes-sigs:master Oct 18, 2019

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #545 from xychu/skip-update

cb3a7e0

Skip update op when instance is created

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ implement helpers for add/remove finalizer #545

✨ implement helpers for add/remove finalizer #545

alexeldeib commented Jul 29, 2019

alexeldeib commented Jul 30, 2019 •

edited

alexeldeib commented Jul 30, 2019

DirectXMan12 left a comment

DirectXMan12 Aug 6, 2019

alexeldeib commented Aug 7, 2019

DirectXMan12 commented Aug 14, 2019

alvaroaleman Sep 16, 2019

alexeldeib Sep 16, 2019 •

edited

alexeldeib Sep 16, 2019

DirectXMan12 Sep 17, 2019

alexeldeib Sep 17, 2019

alexeldeib Sep 17, 2019

alexeldeib Sep 17, 2019 •

edited

DirectXMan12 Sep 17, 2019

DirectXMan12 Sep 17, 2019

alexeldeib Sep 17, 2019

DirectXMan12 Sep 17, 2019

DirectXMan12 Sep 17, 2019

alexeldeib Sep 17, 2019

DirectXMan12 Sep 17, 2019

alexeldeib Sep 17, 2019

alexeldeib commented Oct 17, 2019

DirectXMan12 commented Oct 18, 2019

k8s-ci-robot commented Oct 18, 2019

✨ implement helpers for add/remove finalizer #545

✨ implement helpers for add/remove finalizer #545

Conversation

alexeldeib commented Jul 29, 2019

alexeldeib commented Jul 30, 2019 • edited

alexeldeib commented Jul 30, 2019

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Aug 7, 2019

DirectXMan12 commented Aug 14, 2019

Choose a reason for hiding this comment

alexeldeib Sep 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib Sep 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Oct 17, 2019

DirectXMan12 commented Oct 18, 2019

k8s-ci-robot commented Oct 18, 2019

alexeldeib commented Jul 30, 2019 •

edited

alexeldeib Sep 16, 2019 •

edited

alexeldeib Sep 17, 2019 •

edited