add ability to specify base resourceVersion for three-way server-side patch

[lavalamp]

The PATCH handler has some ... interesting behavior. From the comments:

	// first time through,
	// 1. apply the patch
	// 2. save the original and patched to detect whether there were conflicting changes on retries

	// on a conflict (which is the only reason to have more than one attempt),
	// 1. build a strategic merge patch from originalJS and the patchedJS.  Different patch types can
	//    be specified, but a strategic merge patch should be expressive enough handle them.  Build the
	//    patch with this type to handle those cases.
	// 2. build a strategic merge patch from originalJS and the currentJS
	// 3. ensure no conflicts between the two patches
	// 4. apply the #1 patch to the currentJS object

I think there are three modes of operation which it potentially makes sense for patch to have:

1. User specifies RV; patch makes one try. Fail if RV no longer matches. (This makes sense if the user's patch depends on some part of the object in an unpredictable way.)
2. User does not specify RV; patch makes multiple tries, fails if the patch stops applying cleanly. (User is opting out of any locking--'.spec.replicas' should be X, I don't care what anyone else said.)
3. User specifies a base object and a patch; patch interprets the patch changes as having an implicit "from" precondition for every change. It retries as long as the patch with the implied preconditions continues to apply cleanly. (User wants concurrency-safe application and wants the server to do the retry loop. And the patch is predictable.)

Unfortunately the code today does NONE of those sensible things, and is instead a conflation of 2 and 3 above. edit: 1 and 2 were addressed by #63146

If a user tries case 1 today, we think that it still does 5 retries, even though if the first one has a conflict it is certain that the others will, too.

If a user today doesn't specify an RV, today's retry loop does a bunch of conflict detection that is pretty pointless: the user didn't specify any relation to the RV, therefore using the RV when the request first came in as a "base object" is an arbitrary choice. If a conflict is generated, user is just going to retry with the exact same patch.

And if a user wants behavior 3, there is no way to get it today. I personally recall 3 as being the originally advertised behavior of PATCH but this doesn't seem to be a universal memory.

So, I propose changes:

1. If the user specifies a RV in the patch, do not do any retries. This is backwards compatible because they all would have failed anyway. done in collapse patch conflict retry onto GuaranteedUpdate #63146
2. If the user doesn't specify an RV, don't do the conflict detection mentioned in the comments, just retry with the original patch. I think this is backwards compatible because all the failures this causes are actually spurious. done in collapse patch conflict retry onto GuaranteedUpdate #63146
3. Add a mechanism for the user to deliberately invoke behavior 3. This would be a "new" feature.

[lavalamp]

People who might be interested: @liggitt @wojtek-t @bgrant0607 @apelisse

[wojtek-t]

@lavalamp one question that I have is:
"how user can specify RV in the patch"

As part of the request, you're specifying only "patch that should be applied". Not really how the initial object looked like (and on what version it has). So I'm not really sure how that is possible.
[I agree that if it is possible, not retrying make sense here]

[liggitt]

"how user can specify RV in the patch"

like this: {"metadata":{"resourceVersion":"10"},"foo":"bar"}

/assign

[wojtek-t]

Wait, but that means that we want to overwrite RV to 10, isn't it?

[liggitt]

that takes whatever the object coming from etcd is, sets resourceVersion=10, and foo=bar, then tries to store it. that lets you set the resourceVersion=10 precondition, which gives you normal resourceVersion semantics when it tries to persist

[wojtek-t]

aah ok - I forgot that that object with rv=10 is effectively a precondition on that version.
Thanks for explanation.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale