Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix mergepatch.HasConflicts(). #43469

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Fix mergepatch.HasConflicts(). #43469

merged 1 commit into from
Apr 27, 2017

Conversation

enisoc
Copy link
Member

@enisoc enisoc commented Mar 21, 2017 •
edited
Loading

What this PR does / why we need it:

This fixes some false negatives:

  • If a map had multiple entries, only the first was checked.
  • If a list had multiple entries, only the first was checked.

Which issue this PR fixes:

Special notes for your reviewer:

Release note:

Fix some false negatives in detection of meaningful conflicts during strategic merge patch with maps and lists.

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Mar 21, 2017
@enisoc enisoc added this to the next-candidate milestone Mar 21, 2017
@enisoc
Copy link
Member Author

enisoc commented Apr 18, 2017

/assign @caesarxuchao

@caesarxuchao
Copy link
Member

/assign @mengqiy
/unassign @caesarxuchao

@k8s-ci-robot k8s-ci-robot assigned mengqiy and unassigned caesarxuchao Apr 20, 2017
mengqiy
mengqiy reviewed Apr 20, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go Outdated
@@ -17,6 +17,8 @@ limitations under the License.
package mergepatch

import (
"bytes"
"encoding/json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use "apimachinery/pkg/util/json"
Using this json pkg can avoid some number conversion issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mengqiy
Copy link
Member

mengqiy commented Apr 20, 2017

Two patches setting the same numeric value with different types were detected as conflicting.

@enisoc Can you give me an example?

@enisoc
Copy link
Member Author

enisoc commented Apr 20, 2017

@enisoc Can you give me an example?

There are some examples in the unit test.

@mengqiy
Copy link
Member

mengqiy commented Apr 21, 2017

Two patches setting the same numeric value with different types were detected as conflicting.

I think this WAI.

I think the reason why same numeric value with different types are passed to HasConflicts() is because of the bug in downstream, i.e. whoever calls this function. It should use apimachinery/pkg/util/json instead of encoding/json.
How and where do you find this issue? I have encounter similar issue before #42488.

@enisoc
Copy link
Member Author

enisoc commented Apr 21, 2017

I encountered the numeric value issue in the form of this error when attempting to PATCH:

there is a meaningful conflict (firstResourceVersion: "8517", currentResourceVersion: "8519"):
 diff1={"metadata":{"resourceVersion":"8519"},"spec":{"replicas":0},"status":{"conditions":null,"fullyLabeledReplicas":null,"replicas":0}}
, diff2={"spec":{"replicas":0}}

If you're saying that should be fixed at a different layer, that makes sense. I will revert the numeric part of this change.

@mengqiy
Copy link
Member

mengqiy commented Apr 21, 2017

This error message is interesting.
I will help dig into this.

@enisoc
Fix mergepatch.HasConflicts().
103cb17 
This fixes some false negatives:

* If a map had multiple entries, only the first was checked.
* If a list had multiple entries, only the first was checked.
@mengqiy
Copy link
Member

mengqiy commented Apr 21, 2017

In staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go,
this unmarshal added recently is using the golang json pkg.
I think this is the place cause you trouble.

@mengqiy
Copy link
Member

mengqiy commented Apr 21, 2017

Can you try to switch the json pkg in that file to see if it help your numeric value issue?

@enisoc
Copy link
Member Author

enisoc commented Apr 22, 2017

@mengqiy Yup you're right. I will send a separate PR to fix that. Can you review this one for the other fixes?

@enisoc enisoc mentioned this pull request Apr 22, 2017
Merged
@mengqiy
Copy link
Member

mengqiy commented Apr 24, 2017

/lgtm
Thanks for fixing this.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2017
@mengqiy
Copy link
Member

mengqiy commented Apr 24, 2017

@caesarxuchao Your turn for approval.

@mengqiy
Copy link
Member

mengqiy commented Apr 24, 2017

Do we need to cherrypick this to 1.6?

k8s-github-robot pushed a commit that referenced this pull request Apr 24, 2017
Merge pull request #44788 from enisoc/patch-numeric
a9454ba 
Automatic merge from submit-queue

PATCH: Fix erroneous meaningful conflict for numeric values.

The wrong json package was used, resulting in patches being unmarshaled with numbers as float64 rather than int64. This in turn confused `HasConflicts()` which expects numeric types to match.

The end result was false positives of meaningful conflicts, such as:

```
there is a meaningful conflict (firstResourceVersion: "8517", currentResourceVersion: "8519"):
 diff1={"metadata":{"resourceVersion":"8519"},"spec":{"replicas":0},"status":"conditions":null,"fullyLabeledReplicas":null,"replicas":0}}
, diff2={"spec":{"replicas":0}}
```

This is branched from a discussion on #43469.

```release-note
Fix false positive "meaningful conflict" detection for strategic merge patch with integer values.
```
@caesarxuchao
Copy link
Member

/lgtm
/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, enisoc, mengqiy

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 904b020 into kubernetes:master Apr 27, 2017
@enisoc enisoc removed this from the next-candidate milestone Apr 27, 2017
@enisoc 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 27, 2017
@enisoc enisoc mentioned this pull request Apr 27, 2017
Merged
@enisoc enisoc deleted the has-conflicts branch April 27, 2017 18:44
k8s-github-robot pushed a commit that referenced this pull request Apr 27, 2017
Merge pull request #45045 from enisoc/automated-cherry-pick-of-#43469…
c04488d 
…-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #43469

Cherry pick of #43469 on release-1.6.

#43469: Fix mergepatch.HasConflicts().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy left review comments

Assignees

@mengqiy mengqiy

@caesarxuchao caesarxuchao

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@enisoc @k8s-reviewable @caesarxuchao @mengqiy @k8s-github-robot @dchen1107 @k8s-ci-robot