Move patchMerge tag locations (from key to slice) #46547

aaronlevy · 2017-05-26T23:29:42Z

What this PR does / why we need it:

It looks like for a few api types the patchMerge tags were placed on the "key" of the object, rather than on slices of that object (like most other patchMerge tags). I would expect that the patchMergeKey and patchMergeStrategy would be set for the slice, rather than directly on the object key.

This was surfaced by some (brief) tests trying to merge tolerations via kubectl apply. I was expecting new/existing tolerations to be merged, but instead the existing tolerations were being replaced.

I might be misunderstanding the intention here, but when I looked into the object definitions, I was expecting to see the merge tags on the slices themselves (e.g. podSpec []Toleration)

k8s-github-robot · 2017-05-26T23:41:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaronlevy
We suggest the following additional approver: dchen1107

Assign the PR to them by writing /assign @dchen1107 in a comment when ready.

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

pkg/api/OWNERS

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

k8s-ci-robot · 2017-05-27T00:40:40Z

@aaronlevy: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`3f60cad`	link	`@k8s-bot pull-kubernetes-bazel test this`
pull-kubernetes-unit	`3f60cad`	link	`@k8s-bot pull-kubernetes-unit test this`
pull-kubernetes-verify	`3f60cad`	link	`@k8s-bot pull-kubernetes-verify test this`
pull-kubernetes-e2e-gce-etcd3	`3f60cad`	link	`@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-06-22T22:31:16Z

@aaronlevy PR needs rebase

gmarek · 2017-07-06T13:48:54Z

Can you please add regression tests?

aaronlevy · 2017-07-07T18:04:50Z

@gmarek sorry for letting this go stale. I might not be able to come back to this for a bit -- but in case I don't, @diegs has expressed interest in taking another look. Anyway, one of us will take a look (and add tests).

gmarek · 2017-08-07T09:58:19Z

@kubernetes/sig-api-machinery-pr-reviews

deads2k · 2017-08-07T11:50:57Z

@sttts you've been in the generators recently. Just a bug before?

sttts · 2017-08-07T12:07:24Z

@deads2k that's not even generation, isn't it? examples/deepcopy-gen/vendor/k8s.io/apimachinery/third_party/forked/golang/json/fields.go reads the tags.

sttts · 2017-08-07T12:18:36Z

Openapi-gen (now in another repo) references those tags: https://github.com/kubernetes/kube-openapi/search?utf8=%E2%9C%93&q=merge&type=

sttts · 2017-08-07T12:32:48Z

/cc @mbohlool can you comment here?

mbohlool · 2017-08-11T10:30:45Z

This is the content of the API which is not really controlled by api-machinery and OpenAPI generator. The generator uses those comment tags (that should be in sync with struct tags) to generate OpenAPI extensions (and I guess kubectl consume those extensions). As long as you move both struct and comment tag to the correct field, it should be fine. @mengqiy should be able to comment on kubectl usage of those extensions.

liggitt · 2017-08-11T13:36:13Z

pkg/api/v1/types.go

-	MatchExpressions []NodeSelectorRequirement `json:"matchExpressions" protobuf:"bytes,1,rep,name=matchExpressions"`
+	// +patchMergeKey=key
+	// +patchStrategy=merge
+	MatchExpressions []NodeSelectorRequirement `json:"matchExpressions" patchStrategy:"merge" patchMergeKey:"key" protobuf:"bytes,1,rep,name=matchExpressions"`


has this field shipped in a previous release this way? if so, clients are computing patches without a merge strategy (meaning they expect the server to replace the contents of the slice with whatever is given), and servers are applying patches without a merge strategy

a new client with this merge strategy would compute a patch that would only send a changed item to the server. if that patch were sent to an old server, the old server would replace the entire slice with the patch content (e.g. drop all the unchanged items)

an old client without this merge strategy would send the full contents of the slice if any item changed, not compute removals. if that patch were sent to a new server, the new server would merge in the items in the slice to the existing slice (e.g. make it impossible for an old client to remove items from the slice by patching).

@liggitt I agree that the behavior with skewed versions would be as you describe (I didn't check when this code went in, but it's definitely shipped).

On the other hand, client's expectations (based on the misplaced annotations) is that it should perform merges based on those keys. So I suppose the question is whether we would be willing to tolerate different (and admittedly slightly worse) broken behavior when using skewed clients/servers in exchange for fixing the behavior to match expectations.

Otherwise I think this is going to be broken forever and the correct fix is to just remove the misplaced annotations so that it doesn't confuse people.

cc @aaronlevy

I agree the misleading annotations should be removed.

We need some way to change and make our API correct without cycling the version of everything that has a PodSpec in it.

One way of doing this is to have the client send the merge key and strategy that it actually used to calculate the patch as part of the patch itself. The server would also send the recommended merge keys / strategies to the client as part of the open API before the client calculates the patch.

This would require supporting 2 versions of the strategy / key

"static / implicit" (will never change and even if it doesn't do the right thing)

"dynamic / explicit" (explicitly communicated by the patch author and recommended by the server)

New clients:

Fetch merge strategy / key obtained from open API

Already exists, but need to come up with way to make this deviate from the static tags

Calculate patch request using this strategy / key

In progress, hopefully 1.9

Include strategy / key in the patch request

Server applies patch using the provided strategy / key

Old clients:

Use merge strategy / key obtained from tribal knowledge

Calculate patch request using this strategy / key

Don't include information about what the strategy / key is in the patch

Server applies patch using implicit legacy key

I have an open proposal kubernetes/community#476 that has been summarized above,
trying to solve this issue without breaking backward compatibility.

@pwittrock @mengqiy @liggitt That sounds like an interesting (and big) change. For the sake of this issue (which I've taken over in #50257) should we just remove the offending directives? Or should we keep these "wrong" ones in forever... (they have no effect and are therefore misleading to users)

Removing the offending tags should have no effects. I'm OK to remove them.

I agree. Deciding whether and how to actually set those fields to use a merge strategy can be decided separately.

SG, updated #50257 to only remove the bad labels.

Directives were misplaced for the following types: - MatchExpressions - Taints - Tolerations Per the discussion in kubernetes#46547, we cannot fix these because it would cause backwards-compatibility problems. Instead, remove the incorrect ones so they don't mislead users. This has no impact on behavior.

@aaronlevy

Automatic merge from submit-queue (batch tested with PRs 50257, 50247, 50665, 50554, 51077) Remove incorrect patch-merge directives. **What this PR does / why we need it**: Directives were misplaced for the following types: - MatchExpressions - Taints - Tolerations Per the discussion in #46547, we cannot fix these because it would cause backwards-compatibility problems. Instead, remove the incorrect ones so they don't mislead users. This has no impact on behavior. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: Takes over from #46547 by @aaronlevy **Release note**: ```release-note NONE ```

k8s-github-robot · 2017-08-30T21:51:59Z

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

aaronlevy · 2017-09-18T17:51:53Z

Any reason to keep this open for discussion sake, or should I go ahead and close?

k8s-github-robot · 2017-11-18T08:10:14Z

This PR hasn't been active in 60 days. It will be closed in 29 days (Dec 17, 2017).

cc @aaronlevy @sttts

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

fejta-bot · 2018-02-16T08:55:01Z

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

fejta-bot · 2018-03-18T09:41:50Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten
/remove-lifecycle stale

aaronlevy · 2018-03-20T22:34:29Z

/close

Fixup patchMergeKeys

3f60cad

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

aaronlevy changed the title ~~Fix patchMerge tag locations (from key to slice)~~ Move patchMerge tag locations (from key to slice) May 26, 2017

k8s-github-robot assigned gmarek and dchen1107 May 26, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 26, 2017

gmarek added this to the next-candidate milestone May 29, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 7, 2017

gmarek assigned sttts and unassigned gmarek and dchen1107 Aug 7, 2017

diegs mentioned this pull request Aug 7, 2017

Remove incorrect patch-merge directives. #50257

Merged

mbohlool requested a review from mengqiy August 11, 2017 10:30

liggitt reviewed Aug 11, 2017

View reviewed changes

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 18, 2018

k8s-ci-robot closed this Mar 20, 2018

aaronlevy deleted the mergeFixup branch March 20, 2018 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move patchMerge tag locations (from key to slice) #46547

Move patchMerge tag locations (from key to slice) #46547

aaronlevy commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-ci-robot commented May 27, 2017

k8s-github-robot commented Jun 22, 2017

gmarek commented Jul 6, 2017

aaronlevy commented Jul 7, 2017

gmarek commented Aug 7, 2017

deads2k commented Aug 7, 2017

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

mbohlool commented Aug 11, 2017

liggitt Aug 11, 2017 •

edited

Loading

diegs Aug 11, 2017 •

edited

Loading

liggitt Aug 12, 2017

pwittrock Aug 13, 2017

mengqiy Aug 13, 2017

diegs Aug 15, 2017 •

edited

Loading

mengqiy Aug 16, 2017

liggitt Aug 16, 2017

diegs Aug 16, 2017

k8s-github-robot commented Aug 30, 2017

aaronlevy commented Sep 18, 2017

k8s-github-robot commented Nov 18, 2017

fejta-bot commented Feb 16, 2018

fejta-bot commented Mar 18, 2018

aaronlevy commented Mar 20, 2018

Move patchMerge tag locations (from key to slice) #46547

Move patchMerge tag locations (from key to slice) #46547

Conversation

aaronlevy commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-ci-robot commented May 27, 2017

k8s-github-robot commented Jun 22, 2017

gmarek commented Jul 6, 2017

aaronlevy commented Jul 7, 2017

gmarek commented Aug 7, 2017

deads2k commented Aug 7, 2017

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

mbohlool commented Aug 11, 2017

liggitt Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

diegs Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt Aug 12, 2017

Choose a reason for hiding this comment

pwittrock Aug 13, 2017

Choose a reason for hiding this comment

mengqiy Aug 13, 2017

Choose a reason for hiding this comment

diegs Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

mengqiy Aug 16, 2017

Choose a reason for hiding this comment

liggitt Aug 16, 2017

Choose a reason for hiding this comment

diegs Aug 16, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Aug 30, 2017

aaronlevy commented Sep 18, 2017

k8s-github-robot commented Nov 18, 2017

fejta-bot commented Feb 16, 2018

fejta-bot commented Mar 18, 2018

aaronlevy commented Mar 20, 2018

liggitt Aug 11, 2017 •

edited

Loading

diegs Aug 11, 2017 •

edited

Loading

diegs Aug 15, 2017 •

edited

Loading