✨ Add Patch MergeFrom optimistic locking option #969

vincepri · 2020-05-27T16:21:03Z

Signed-off-by: Vince Prignano [email protected]

fabriziopandini

@vincepri thanks for this change, I'm looking forward to getting this merged!
only a minor nit from my side, but not blocking

pkg/client/client_test.go

benmoss

Just some lil nits

benmoss · 2020-05-27T17:02:43Z

pkg/client/client_test.go

@@ -1209,6 +1209,42 @@ var _ = Describe("Client", func() {
 				close(done)
 			})

+			It("should patch an existing object from a go struct, using optimistic locking", func(done Done) {


Maybe out of the scope of this PR but why are all these tests async? I thought this was only useful if you were testing asynchronous code

I'm not sure, what do you mean?

That they accept the done Done parameter, it's an optional feature of Ginkgo It: https://onsi.github.io/ginkgo/#asynchronous-tests

Not 100% sure, I guess they can all run in parallel

pkg/client/patch.go

vincepri · 2020-05-27T18:40:46Z

cc @alvaroaleman @alexeldeib

alvaroaleman · 2020-05-28T15:12:43Z

@vincepri Can you change the PR to target master? We can still cherry-pick it after that but we shouldn't do the initial merge into an older release branch

vincepri · 2020-05-28T15:15:43Z

@alvaroaleman Yeah I can rework the branch, unfortunately when I worked on the patch I was already on release-0.5 and forgot to change the base branch :/

alvaroaleman · 2020-05-28T15:17:58Z

pkg/client/patch.go

+		if !ok {
+			return nil, fmt.Errorf("cannot use OptimisticLock, from object %q is not a valid metav1.Object", s.from)
+		}
+		resourceVersion := fromMeta.GetResourceVersion()


How does patch with a RV precondition differ from just doing Update?

That's a good question, I think when it comes down to patch is easier to merge only the fields a client has updated. It does keep the scope down to only things that have changes, so it's still possible to retry to patch only things necessary.

This particular use case came up from Cluster API conditions work, where we need to make sure that when patching an object we aren't overwriting other controllers. Adding resource version on patch keeps the scope down to the patch and we can still try to do a best-effort merge.

But with this precondition, if anything on the object was changed its RV gets increased and this patch fails. Hence, it has the exact same semantics as UPDATE unless I am missing something.

It seems to me a bit that what you actually want is a precondition on the .status.conditions and not the RV?

Yeah, it's the same update semantic, although in our case it'll allow us to only focus on a single field and try merging that field without bothering about the rest

alvaroaleman · 2020-05-28T15:19:31Z

pkg/client/patch.go

@@ -59,8 +63,29 @@ func ConstantPatch(patchType types.PatchType, data []byte) Patch {
 	return RawPatch(patchType, data)
 }

+type MergeFromWithOptimisticLock struct{}


I had to read the code to understand what you meant by OptimisticLock. This definitely needs godocs.

I also don't understand how that differs from Update except that you put less data on the wire and trade it for a local computation of the patch which seems like an implementation detail to me that should rarely if ever be relevant.

+1 I'll add a godoc

alvaroaleman · 2020-05-28T16:24:59Z

Yeah but how is this different from just doing `Update(newObject)`? I guess I don't properly understand the use case. Do you have an example somewhere where this does what you want and Update does not?

…

On Thu, May 28, 2020 at 12:06 PM Vince Prignano ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/client/patch.go <#969 (comment)> : > - return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) + data, err := jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) + if err != nil { + return nil, err + } + + if s.opts.OptimisticLock { + dataMap := map[string]interface{}{} + if err := json.Unmarshal(data, &dataMap); err != nil { + return nil, err + } + fromMeta, ok := s.from.(metav1.Object) + if !ok { + return nil, fmt.Errorf("cannot use OptimisticLock, from object %q is not a valid metav1.Object", s.from) + } + resourceVersion := fromMeta.GetResourceVersion() Yeah, it's the same update semantic, although in our case it'll allow us to only focus on a single field and try merging that field without bothering about the rest — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#969 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRR6ZHBFZKKETJ66P2E2E3RT2DYXANCNFSM4NMJF4CA> .

vincepri · 2020-05-28T17:13:21Z

It's not different in terms of functionality, apart from the need to set the whole spec/status vs only a single field, although we're trying to integrate this logic in https://github.com/kubernetes-sigs/cluster-api/blob/master/util/patch/patch.go#L84 and it'd be a little strange to issue Update within a Patch helper

vincepri · 2020-05-28T17:16:01Z

@alvaroaleman Do you see this as something that you wouldn't want exposed in controller runtime? I guess we can bring this in Cluster API itself if it wouldn't be useful for others as well.

Although, given that the functionality of using resource version with Patch is there, and kubectl also allows it, I thought it'd be good to expose it for users that want to go down this path.

alvaroaleman · 2020-05-28T17:44:02Z

Do you see this as something that you wouldn't want exposed in controller runtime? I guess we can bring this in Cluster API itself if it wouldn't be useful for others as well.

Although, given that the functionality of using resource version with Patch is there, and kubectl also allows it, I thought it'd be good to expose it for users that want to go down this path.

Well to my understanding the semantic difference between Update and Patch is that the latter can be applied even if someone else modified the object. With this precondition, that is not true anymore and the Patch operation is just a different way of doing an Update.

My objection here is that its not clear to me how this differs from an Update operation. So sooner or later someone will ask which of the two to use. And unless we have an answer to that that is better than Its implemented a bit differently but it behaves identically I don't think we should add this to our API because it will just result in confusion but not really help much.

ncdc · 2020-05-28T18:02:07Z

There is a difference between update and patch that I believe justifies this PR. Let's say you add a new field to an object without changing the API version. And then you populate that field. If you have a client that is using an older copy of the code that doesn't have this new field, when it sends an update (e.g. maybe you were setting an annotation), it won't have the new field included. Because update is a wholesale replace, the contents of that new field are now lost.

If you use patch, you can target just the changes you want to make, and it won't result in fields that aren't present in the older copy of the go structs being dropped. Adding support for optional optimistic currency control in this patch seems reasonable to me because of this short-coming that update has.

alvaroaleman · 2020-05-28T18:31:00Z

okay, that makes sense. Can we add this information to the godoc ("like update but avoids dropping fields the apiserver has but that aren't present in your go types")?

vincepri · 2020-05-28T18:41:51Z

@alvaroaleman @ncdc added godoc

vincepri · 2020-05-28T18:43:02Z

/retest

pkg/client/patch.go

Signed-off-by: Vince Prignano <[email protected]>

alvaroaleman

Thanks for your work and the explanations!

vincepri · 2020-05-29T14:44:56Z

@alvaroaleman good to merge?

alvaroaleman · 2020-05-29T14:57:10Z

@alvaroaleman good to merge?

Oh yeah sorry, I am very used to "github appove adds lgtm and approve"

/lgtm
/approve

k8s-ci-robot · 2020-05-29T14:57:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

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~~ [vincepri]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 27, 2020

k8s-ci-robot requested review from alenkacz and mengqiy May 27, 2020 16:21

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2020

fabriziopandini approved these changes May 27, 2020

View reviewed changes

pkg/client/client_test.go Outdated Show resolved Hide resolved

vincepri force-pushed the add-optimistic-locking branch from 5994e61 to f9ce06d Compare May 27, 2020 17:13

benmoss reviewed May 27, 2020

View reviewed changes

alvaroaleman reviewed May 28, 2020

View reviewed changes

vincepri force-pushed the add-optimistic-locking branch 2 times, most recently from 4b04fc3 to 6ec57bf Compare May 28, 2020 18:40

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2020

vincepri changed the base branch from release-0.5 to master May 28, 2020 18:40

vincepri closed this May 28, 2020

vincepri reopened this May 28, 2020

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2020

ncdc reviewed May 28, 2020

View reviewed changes

pkg/client/patch.go Outdated Show resolved Hide resolved

vincepri force-pushed the add-optimistic-locking branch from 6ec57bf to 62b2f0c Compare May 28, 2020 19:42

✨ Add Patch MergeFrom optimistic locking option

8c6919c

Signed-off-by: Vince Prignano <[email protected]>

vincepri force-pushed the add-optimistic-locking branch from 62b2f0c to 8c6919c Compare May 28, 2020 19:50

vincepri added this to the v0.6.x milestone May 28, 2020

alvaroaleman approved these changes May 28, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman May 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2020

k8s-ci-robot merged commit f66de86 into kubernetes-sigs:master May 29, 2020

✨ Add Patch MergeFrom optimistic locking option #969

✨ Add Patch MergeFrom optimistic locking option #969

Uh oh!

Conversation

vincepri commented May 27, 2020

Uh oh!

fabriziopandini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benmoss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincepri commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvaroaleman commented May 28, 2020

Uh oh!

vincepri commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman commented May 28, 2020 via email

Uh oh!

vincepri commented May 28, 2020

Uh oh!

vincepri commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvaroaleman commented May 28, 2020

Uh oh!

ncdc commented May 28, 2020

Uh oh!

alvaroaleman commented May 28, 2020

Uh oh!

vincepri commented May 28, 2020

Uh oh!

vincepri commented May 28, 2020

Uh oh!

Uh oh!

alvaroaleman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincepri commented May 29, 2020

Uh oh!

alvaroaleman commented May 29, 2020

Uh oh!

k8s-ci-robot commented May 29, 2020

Uh oh!

Uh oh!

vincepri commented May 27, 2020 •

edited

Loading

alvaroaleman May 28, 2020 •

edited

Loading

alvaroaleman May 28, 2020 •

edited

Loading

vincepri commented May 28, 2020 •

edited

Loading

alvaroaleman left a comment •

edited

Loading