Fix original object mutation on patch retry #43871

liggitt · 2017-03-30T20:11:10Z

When applying a patch, the original state of the object and patch are captured so that retries can determine if an overlapping update was made to the object, and the patch API call should abort with a conflict.

However, the strategicPatchObject -> applyPatchToObject -> StrategicMergeMapPatch -> mergeMap call mutates both the original object map and the patch map, making them unsuitable for reusing for subsequent calls.

We saw this in a downstream test that exercises patch conflict retries, where the data being submitted in a patch was showing up in the original object data map.

Since mergeMap also mutates the patch map passed in (deletes patch directives as it encounters them), we also cannot reuse the patch map in applyPatchToObject once it has been used.

This PR:

Builds originalObjMap separate from the initial patch application for later use in conflict detection
Changes the originalPatchMap to a helper that returns a fresh map from original sources
Adds an integration test that exercises retry of non-overlapping patches with patches containing $patch directives

Fix incorrect conflict errors applying strategic merge patches to resources.

k8s-reviewable · 2017-03-30T20:11:15Z

This change is

liggitt · 2017-03-30T20:11:24Z

cc @kubernetes/sig-api-machinery-pr-reviews @sttts @ncdc

liggitt · 2017-03-31T00:49:15Z

@wojtek-t @deads2k PTAL

k8s-cherrypick-bot · 2017-03-31T03:30:12Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

liggitt · 2017-03-31T03:34:41Z

Tagging for cherry-pick since this was a regression in 1.6

sttts · 2017-03-31T10:34:30Z

Looks like this was failed communication: here is another one: #43902, basically the same thing. We can choose one.

liggitt · 2017-03-31T10:39:05Z

The patch map also gets mutated. This fixed and tests that issue as well

deads2k · 2017-03-31T17:20:38Z

I think this does what it sets out to do, but the code in this area is really sticky to read.

liggitt · 2017-03-31T17:37:41Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -86,21 +86,21 @@ func strategicPatchObject(
 	patchJS []byte,
 	objToUpdate runtime.Object,
 	versionedObj runtime.Object,
-) (originalObjMap map[string]interface{}, patchMap map[string]interface{}, retErr error) {
-	originalObjMap = make(map[string]interface{})
+) error {


I just removed the return values here since they are unsuitable for use after having used them to apply the patch

Yeah - that sounds good.

wojtek-t

Sorry for long delay - I added some comment to a test.

wojtek-t · 2017-04-05T07:51:00Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -86,21 +86,21 @@ func strategicPatchObject(
 	patchJS []byte,
 	objToUpdate runtime.Object,
 	versionedObj runtime.Object,
-) (originalObjMap map[string]interface{}, patchMap map[string]interface{}, retErr error) {
-	originalObjMap = make(map[string]interface{})
+) error {


Yeah - that sounds good.

wojtek-t · 2017-04-05T08:52:17Z

test/integration/apiserver/patch_test.go

+	}
+	wg.Wait()
+
+	if successes < handlers.MaxRetryWhenPatchConflicts {


I think I don't understand this. We have 2N concurrent goroutines trying to do a patch. Why at least N of them has to succeed? Can you explain it (and add comment about it)?

https://github.com/kubernetes/kubernetes/pull/43902/files#diff-a98175d31a994ac0386ef984acfe13f5R111

@liggitt wants to copy that into his patch. Took some time to get the reasoning there.

BTW - I just looked into the other PR, and I think I like the approach from #43902 better. So maybe we should proceed with that one and close this one?
@liggitt @sttts - thoughts?

I can rename the function here as well. That one doesn't address the patch map mutation issue

I see. But the test is still passing with the other PR, which means it's not really testing everything, right?

Anyway - I'm fine with proceeding with this one too - just want to understand it.

the test is still passing with the other PR

the test in the other PR doesn't include the patch directives... the test in this PR would fail against the fix in the other PR

OK - sorry for confusion then.

Can you then please add a comment (similar than the one from the other PR) and I will lgtm this PR.

liggitt · 2017-04-06T16:39:00Z

added comment at https://github.com/kubernetes/kubernetes/pull/43871/files#diff-a98175d31a994ac0386ef984acfe13f5R60

I added a mutation warning to the StrategicMergePatch godoc for now, rather than rename

wojtek-t · 2017-04-06T17:55:21Z

I added a mutation warning to the StrategicMergePatch godoc for now, rather than rename

That's fine. Thanks.

/lgtm

k8s-github-robot · 2017-04-06T17:55:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/apiserver/pkg/endpoints/OWNERS~~ [liggitt,wojtek-t]
~~test/OWNERS~~ [liggitt,wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

liggitt · 2017-04-06T18:12:37Z

@k8s-bot kops aws e2e test this

k8s-github-robot · 2017-04-06T19:36:23Z

Automatic merge from submit-queue (batch tested with PRs 43871, 44053)

…1-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #43871 Cherry pick of #43871 on release-1.6. #43871: Fix original object mutation on patch retry ```release-note Fix incorrect conflict errors applying strategic merge patches to resources. ```

k8s-cherrypick-bot · 2017-04-07T04:24:33Z

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…k-of-#43871-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of kubernetes#43871 Cherry pick of kubernetes#43871 on release-1.6. kubernetes#43871: Fix original object mutation on patch retry ```release-note Fix incorrect conflict errors applying strategic merge patches to resources. ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 30, 2017

liggitt added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 30, 2017

k8s-github-robot assigned xiang90 Mar 30, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 30, 2017

liggitt assigned sttts and wojtek-t Mar 30, 2017

liggitt force-pushed the patch-mutation branch 2 times, most recently from 115546c to e06a37d Compare March 30, 2017 21:06

k8s-github-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 Mar 30, 2017

liggitt force-pushed the patch-mutation branch from e06a37d to dcfef02 Compare March 31, 2017 00:45

liggitt assigned deads2k Mar 31, 2017

liggitt added this to the v1.6.1 milestone Mar 31, 2017

liggitt added the cherrypick-candidate label Mar 31, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Mar 31, 2017

liggitt modified the milestones: v1.6, v1.6.1 Mar 31, 2017

liggitt added the cherrypick-candidate label Mar 31, 2017

ncdc mentioned this pull request Mar 31, 2017

Fix strategic merge patch input mutation leading to broken PATCH retry logic #43902

Closed

liggitt commented Mar 31, 2017

View reviewed changes

wojtek-t reviewed Apr 5, 2017

View reviewed changes

Fix original object mutation on patch retry

c415e4a

liggitt force-pushed the patch-mutation branch from dcfef02 to c415e4a Compare April 6, 2017 16:30

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2017

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 6, 2017

liggitt mentioned this pull request Apr 6, 2017

Automated cherry pick of #43871 #44173

Merged

k8s-github-robot merged commit 6497318 into kubernetes:master Apr 6, 2017

liggitt mentioned this pull request Apr 6, 2017

Rebase 1.6.1 openshift/origin#13653

Merged

13 tasks

liggitt deleted the patch-mutation branch April 6, 2017 20:38

k8s-cherrypick-bot removed the cherrypick-candidate label Apr 7, 2017

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix original object mutation on patch retry #43871

Fix original object mutation on patch retry #43871

liggitt commented Mar 30, 2017 •

edited by enisoc

Loading

k8s-reviewable commented Mar 30, 2017

liggitt commented Mar 30, 2017

liggitt commented Mar 31, 2017

k8s-cherrypick-bot commented Mar 31, 2017

liggitt commented Mar 31, 2017

sttts commented Mar 31, 2017

liggitt commented Mar 31, 2017

deads2k commented Mar 31, 2017

liggitt Mar 31, 2017

wojtek-t Apr 5, 2017

wojtek-t left a comment

wojtek-t Apr 5, 2017

wojtek-t Apr 5, 2017

sttts Apr 5, 2017

wojtek-t Apr 5, 2017

liggitt Apr 5, 2017

wojtek-t Apr 6, 2017

liggitt Apr 6, 2017

wojtek-t Apr 6, 2017

wojtek-t Apr 6, 2017

liggitt commented Apr 6, 2017

wojtek-t commented Apr 6, 2017

k8s-github-robot commented Apr 6, 2017

liggitt commented Apr 6, 2017

k8s-github-robot commented Apr 6, 2017

k8s-cherrypick-bot commented Apr 7, 2017

Fix original object mutation on patch retry #43871

Fix original object mutation on patch retry #43871

Conversation

liggitt commented Mar 30, 2017 • edited by enisoc Loading

k8s-reviewable commented Mar 30, 2017

liggitt commented Mar 30, 2017

liggitt commented Mar 31, 2017

k8s-cherrypick-bot commented Mar 31, 2017

liggitt commented Mar 31, 2017

sttts commented Mar 31, 2017

liggitt commented Mar 31, 2017

deads2k commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 6, 2017

wojtek-t commented Apr 6, 2017

k8s-github-robot commented Apr 6, 2017

liggitt commented Apr 6, 2017

k8s-github-robot commented Apr 6, 2017

k8s-cherrypick-bot commented Apr 7, 2017

liggitt commented Mar 30, 2017 •

edited by enisoc

Loading