Proposal: Support Sending Merge Key in the Patch by Clients #476

mengqiy · 2017-03-23T06:51:39Z

pwittrock · 2017-04-18T16:57:24Z

contributors/design-proposals/key-set-as-merge-key.md

+It must always be present and unique to perform the merge on the list of non-primitive types,
+and will be preserved.
+
+The issue in practice is that sometimes we cannot find a key that always be present and unique.


I'd replace this with:

The current implementation requires a single field that uniquely identifies each element in a list. For some element Kinds, the identity is defined using multiple fields. An example of this is the x.x.xPort, which is identified by protocol and port

pwittrock · 2017-04-18T17:01:48Z

contributors/design-proposals/key-set-as-merge-key.md

+### API Change
+
+We will update the struct tags for the API resources whose Merge Key is not guaranteed to be always
+present and unique. The keys will be seperated by "|", i.e. `patchMergeKey:"<key1>|<key2>|<key3>"`.


Why '|' instead of ','? What is more consistent with other tags and comments?

After checking types.go, I agree we should use ','.

pwittrock · 2017-04-18T23:52:29Z

contributors/design-proposals/key-set-as-merge-key.md

+### Strategic Merge Patch pkg
+
+Entries are considered as the same if and only if all the keys in the key set are identical.
+We allow keys to be missing in the key set as long as it is not empty.


What does this mean that it is ok for the field to be missing by not empty? Can you give an example?

I see this is answered following :)

How about

If one or more of the mergeKeys is unspecified, it is interpreted as an empty value for purposes of uniquely identifying the object.

If one or more of the mergeKeys is unspecified, it is interpreted as an empty value for purposes of uniquely identifying the object.

IMO we still want to distinguish unspecified and empty

- port: 12345

and

- name: "" port: 12345

The wording you give seems not support this?

Can you give me another wording?

pwittrock · 2017-04-18T23:54:10Z

contributors/design-proposals/key-set-as-merge-key.md

+spec:
+  type: NodePort
+  ports:
+    - protocol: UDP


comment that the name is missing here

pwittrock · 2017-04-18T23:56:25Z

contributors/design-proposals/key-set-as-merge-key.md

+
+The plan to not break backward compatibility:
+
+In 1.7, add the logic support for key set, but keep the APIs' tags unchanged.


pwittrock · 2017-04-18T23:56:39Z

contributors/design-proposals/key-set-as-merge-key.md

+- Need auditing APIs
+
+### Broken APIs
+(1a) `ContainerPort`: Change merge key from `containerPort` to `name|containerPort`.


update to ','

pwittrock · 2017-04-18T23:58:10Z

I think this is ready for a broader review. Would you present at the next sig-cli?

pwittrock · 2017-04-18T23:59:09Z

@liggitt Would you be willing to act as a reviewer for this proposal for supporting multiple-fields as a mergeKey?

mengqiy · 2017-04-19T01:07:30Z

I think this is ready for a broader review. Would you present at the next sig-cli?

Sure.

liggitt · 2017-04-20T17:15:11Z

contributors/design-proposals/key-set-as-merge-key.md

+
+Document what the developer should consider when adding an API with `mergeKey`.
+
+## Version Skew


again, the 1 minor version skew policy is kubectl-specific, not for the API in general. I'm not sure how to evolve mergeKey on existing fields compatibly without making all clients do field-level API discovery which sounds pretty painful.

I understood.
The usage of OpenAPI is not just for kubectl. It is actually for all clients. We want all clients to use OpenAPI instead of baked-in types in the future. It is also useful and important for TPR and service catalog.

Ref: kubernetes/kubernetes#44531

It is also useful and important for TPR and service catalog.

For discovering arbitrary resources, sure, but for a client that wants to patch exactly one thing on a known version of a known resource, forcing them to do openapi discovery to see if the way they are supposed to patch that field in that version has changed is a pretty high bar.

For example, if I want to patch a port on a v1 service, I would not expect to have to do openapi discovery. I'd expect the v1 API to remain compatible. If you wanted to change the mergeKey, that seems like the sort of thing you'd need to bump API versions for.

Yeah, in general. MergeKeys are forever I think.

but for a client that wants to patch exactly one thing on a known version of a known resource, forcing them to do openapi discovery to see if the way they are supposed to patch that field in that version has changed is a pretty high bar.

@liggitt Do you have specific client(s) in your mind?

@thockin FYI

To clarify, this is an issue generally with changing merge keys, not specifically with supporting tuples as merge keys.

A couple thoughts:

The ship may have sailed on this already, but I would think that the client should send the merge keys and merge strategies as part of the patch request so it could tell the server how the patch request is intended to be merged.

It maybe possible to introduce additional keys for fields while keeping backwards compatibility. Would need to think through the combinatorics, but I am wondering if some of the merge key tuple is missing from the patch request (e.g. the new part), if the server could fallback on the remaining pieces as the merge key.

If merge keys really are "forever", and we needed to change the one referenced in this case, would this then require bumping the version for everything that has a PodTemplate in it - e.g. every controller type? The cure may be worse than the disease in this case.

Given that we don't publish what the merge keys actually are for resource kinds in any user facing documentation besides the types.go files (correct me if I am wrong here), are they considered first class pieces of the API, or are they an implementation detail? Does our API definition include metadata about fields that is only defined in code? Do we have any guess as to which clients besides kubectl calculate patch requests?

Let me qualify - mergeKeys are forever if a patch which previously "worked" (in the sense that a client uses it to accomplish a concrete mutation successfully) is broken when we change the mergeKey. If the patch was impossible or broken (envVar is a clear example of "we were totally broken because order is meaningful in env vars by our spec"), then I don't think a mergeKey change counts as a break.

We could consider versioning the patch strategy per type. Or the micro version approach Dan was championing for fields, but with restricted scope - v1.Pod has mergeKey "foo" for field "bar" at version 1, but "baz" for field "bar" at version 2. And we simply default the merge version.

Thanks for the clarification. IMHO the ServicePort is broken in a similar fashion as envVar - that is - the patch is impossible / broken for things we support in our spec - e.g. having a list containing 2 different ServicePorts with the same port value and different protocol values. However some clients may not be encountering these issues because they don't rely on those capabilities.

Thinking on this more, I am pretty sure we can introduce additional fields for merge keys while retaining backwards compatibility by having the server fallback to use the present merge keys in the event the additional merge key fields are missing from the patch request.

jamiehannaford · 2017-04-21T09:40:01Z

contributors/design-proposals/key-set-as-merge-key.md

+
+The current implementation requires a single field that uniquely identifies each element in a list.
+For some element Kinds, the identity is defined using multiple fields.
+An [example](https://github.com/kubernetes/kubernetes/issues/39188 of this is the service.spec.ports, which is identified by `protocol` and `port`.


Link MD is not terminated

I'd emphasise both here:

identified by both protocol and port

jamiehannaford · 2017-04-21T09:42:15Z

contributors/design-proposals/key-set-as-merge-key.md

+The current implementation requires a single field that uniquely identifies each element in a list.
+For some element Kinds, the identity is defined using multiple fields.
+An [example](https://github.com/kubernetes/kubernetes/issues/39188 of this is the service.spec.ports, which is identified by `protocol` and `port`.
+We need to support a key set as Merge Key. A key set must contain at least 1 key.


How about:

As a result we need to also support a set of keys as a Merge Key. A key set must be a list of strings at least 1 element long

What about ints?

The key is always a string, because we require all maps to be map[string]interface{}.

jamiehannaford · 2017-04-21T09:43:53Z

contributors/design-proposals/key-set-as-merge-key.md

+
+### API Change
+
+We will update the struct tags for the API resources whose Merge Key is not guaranteed to be always


I'm wondering whether this sentence makes sense. How about:

for API resources that cannot be effectively merged with a single merge key

jamiehannaford · 2017-04-21T09:44:10Z

contributors/design-proposals/key-set-as-merge-key.md

+We will update the struct tags for the API resources whose Merge Key is not guaranteed to be always
+present and unique. The keys will be seperated by ",", i.e. `patchMergeKey:"<key1>,<key2>,<key3>"`.
+
+E.g. `Ports` in `ServiceSpec`.


Can we link to the source code struct?

jamiehannaford · 2017-04-21T09:46:27Z

contributors/design-proposals/key-set-as-merge-key.md

+
+### Open API
+
+Update Open API schema to have a string that contains multiple keys for `patchMergeKey`.


What does this mean, that the type of patchMergeKey needs to change? Can you give an example of the OpenAPI change?

jamiehannaford · 2017-04-21T09:49:13Z

contributors/design-proposals/key-set-as-merge-key.md

+
+Suppose we are using `name` and `port` as merge key as mentioned in section [API Change](#api-change)
+
+We have live config:


This config currently exists:

jamiehannaford · 2017-04-21T09:49:33Z

contributors/design-proposals/key-set-as-merge-key.md

+      nodePort: 30420
+```
+
+We want to add another port, and the user's local config file becomes:


The user then wants to add a new port. They will send this manifest:

jamiehannaford · 2017-04-21T09:49:51Z

contributors/design-proposals/key-set-as-merge-key.md

+      nodePort: 30420
+```
+
+The patch we send should be:


The patch manifest that will be sent is:

jamiehannaford · 2017-04-21T09:50:32Z

contributors/design-proposals/key-set-as-merge-key.md

+  ports:
+    - protocol: UDP
+      port: 30420
+      name: udpport


What happens if the user omits the name in their manifest? Will the existing UDP port stay with an empty name?

The operation will fail because of the validation error. Name is required if there are more than one ports.

mengqiy · 2017-05-02T21:04:36Z

Added edge cases.
@pwittrock There is case when deleting maybe you will want to think about it. 5e9717f#diff-7bf73a741414e64cb422a883e2de7f0eR182

mengqiy · 2017-05-06T00:20:17Z

PTAL

liggitt · 2017-05-09T17:49:58Z

contributors/design-proposals/key-set-as-merge-key.md

+An example patch will look like:
+```yaml
+list:
+- $patchMergeKey:


in this example, $patchMergeKey is included in the first item's definition... is that intended?

I think $patchMergeKey should be in each entry in the list. This simple example only has one entry.

liggitt · 2017-05-09T18:02:19Z

contributors/design-proposals/key-set-as-merge-key.md

+
+When `$patchMergeKey` only has part of all merge keys,
+the server will apply the patch to all the matching items.
+So an update or a deletion may apply to multiple items in the list.


Accepting a $patchMergeKey that does not uniquely identify elements is what this proposal is trying to fix... I don't think a $patchMergeKey key that does not uniquely identify items should be accepted.

If it was, how would this interact with the proposed $setElementOrder directive, which is supposed to contain a list of keys? Keys that identify more than one item in the list mean that directive becomes ambiguous:

server items:

ports: - port: 8080 protocol: TCP name: http-tcp - port: 9090 protocol: TCP - port: 8080 protocol: UDP name: http-udp

patch:

$setElementOrder/ports: - port: 8080 - port: 8080 - port: 9090 ports: - port: 8080 protocol: UDP name: http-udp-renamed

It's not clear which 8080 port was ordered where.

I don't think a $patchMergeKey key that does not uniquely identify items should be accepted.

You are right.

$setElementOrder should always use all of the recommended merge keys that are present, so each entry in $setElementOrder list can uniquely identify an entry in the live list.

$setElementOrder/ports: - port: 8080 protocol: TCP - port: 8080 protocol: UDP - port: 9090 protocol: TCP ports: - port: 8080 protocol: UDP name: http-udp-renamed

I will add a section for $setElementOrder.

pwittrock · 2017-05-09T21:47:00Z

The majority of the content in this doc is now about how to change a merge key value. Lets pull the multi-field merge key into a separate design doc and review that separately from the design around to how to change merge keys for fields and how to control the merge key from the client side

mengqiy · 2017-05-10T06:13:27Z

Updating:

Addressed comments
Pull supporting multi-fields merge key logic to Proposal: support multi-fields merge key #610 per Proposal: Support Sending Merge Key in the Patch by Clients #476 (comment)
Add content of how to move merge keys that are at wrong level (child level)

liggitt · 2017-05-10T14:26:31Z

cc @kubernetes/sig-cli-proposals @kubernetes/sig-api-machinery-proposals

k8s-ci-robot · 2017-05-10T14:26:32Z

@liggitt: These labels do not exist in this repository: sig/cli, sig/api-machinery.

In response to this:

cc @kubernetes/sig-cli-proposals @kubernetes/sig-api-machinery-proposals

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.

bgrant0607 · 2017-09-26T16:26:22Z

contributors/design-proposals/changing-merge-key.md

+
+## Goal
+
+Propose a solution of changing the merge keys for existing APIs without breaking backward compatibility.


We need to take a step back and think about this systematically.

I think we should consider building a more principled form of the patch syntax. We keep making ad-hoc fixes.

Yeah, the existing RFCs have their limitations but evolving one of those with the improvements from SMR seems like a stronger path than retrofitting into existing SMR.

castrojo · 2017-10-10T15:53:58Z

This change is

k8s-github-robot · 2017-12-13T08:01:28Z

This PR hasn't been active in 61 days. It will be closed in 28 days (Jan 11, 2018).

cc @fabianofranz @liggitt @mengqiy @monopole @pwittrock

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-03-13T08:43:33Z

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

bgrant0607 · 2018-03-13T23:20:43Z

@mengqiy @pwittrock @lavalamp I think we should close this.

lavalamp · 2018-03-13T23:22:27Z

Agree, we're fixing this class of things with a different mechanism entirely.

fejta-bot · 2018-04-13T00:14:55Z

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

mengqiy · 2018-04-13T03:22:20Z

Closing in favor of server-side apply.

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

mengqiy force-pushed the multiple_merge_key branch from ae49d7b to c650c37 Compare March 23, 2017 16:32

pwittrock self-requested a review April 11, 2017 15:12

pwittrock self-assigned this Apr 11, 2017

pwittrock reviewed Apr 18, 2017

View reviewed changes

pwittrock assigned monopole, fabianofranz and liggitt Apr 18, 2017

pwittrock reviewed Apr 18, 2017

View reviewed changes

liggitt reviewed Apr 20, 2017

View reviewed changes

mengqiy mentioned this pull request Apr 20, 2017

Services with same port, different protocol display wrongly in kubectl and have wrong merge key kubernetes/kubernetes#39188

Closed

jamiehannaford reviewed Apr 21, 2017

View reviewed changes

support multiple keys as merge key

958bdeb

mengqiy force-pushed the multiple_merge_key branch from 6234e8e to 958bdeb Compare May 2, 2017 05:13

add edge case

5e9717f

mengqiy force-pushed the multiple_merge_key branch from f1a6c7b to 5e9717f Compare May 2, 2017 21:01

introduce directive

27e8e88

liggitt reviewed May 9, 2017

View reviewed changes

mengqiy mentioned this pull request May 9, 2017

Proposal: support multi-fields merge key #610

Merged

Address comments & reorganize & add move merge key

7c9583e

mengqiy changed the title ~~Proposal: support multiple keys as merge key~~ Proposal: Support Sending Merge Key in the Patch by Clients May 10, 2017

mengqiy mentioned this pull request May 23, 2017

support multifields merge key kubernetes/kubernetes#46161

Closed

mengqiy mentioned this pull request Jun 9, 2017

Services allow duplicate port kubernetes/kubernetes#47221

Closed

mengqiy mentioned this pull request Jul 20, 2017

"kubectl edit svc foo" fails to update ports kubernetes/kubernetes#49295

Closed

xiangpengzhao mentioned this pull request Jul 21, 2017

Check dup NodePort with protocols when update services kubernetes/kubernetes#49258

Merged

mengqiy mentioned this pull request Aug 13, 2017

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

Closed

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 15, 2017

mengqiy mentioned this pull request Aug 28, 2017

PATCH merges Services with same port, different protocol kubernetes/kubernetes#47249

Closed

mengqiy mentioned this pull request Sep 21, 2017

Proposal for re-architecting apply #1028

Merged

bgrant0607 reviewed Sep 26, 2017

View reviewed changes

k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Feb 6, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 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 Apr 13, 2018

mengqiy closed this Apr 13, 2018


		The plan to not break backward compatibility:

		In 1.7, add the logic support for key set, but keep the APIs' tags unchanged.


		Document what the developer should consider when adding an API with `mergeKey`.

		## Version Skew


		### API Change

		We will update the struct tags for the API resources whose Merge Key is not guaranteed to be always


		### Open API

		Update Open API schema to have a string that contains multiple keys for `patchMergeKey`.


		Suppose we are using `name` and `port` as merge key as mentioned in section [API Change](#api-change)

		We have live config:


		## Goal

		Propose a solution of changing the merge keys for existing APIs without breaking backward compatibility.

Proposal: Support Sending Merge Key in the Patch by Clients #476

Proposal: Support Sending Merge Key in the Patch by Clients #476

Conversation

mengqiy commented Mar 23, 2017

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

pwittrock Apr 18, 2017 • edited Loading

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

pwittrock commented Apr 18, 2017

pwittrock commented Apr 18, 2017

mengqiy commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock Apr 21, 2017 • edited Loading

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

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

mengqiy commented May 2, 2017

mengqiy commented May 6, 2017

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

pwittrock commented May 9, 2017

mengqiy commented May 10, 2017

liggitt commented May 10, 2017

k8s-ci-robot commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castrojo commented Oct 10, 2017

k8s-github-robot commented Dec 13, 2017

fejta-bot commented Mar 13, 2018

bgrant0607 commented Mar 13, 2018

lavalamp commented Mar 13, 2018

fejta-bot commented Apr 13, 2018

mengqiy commented Apr 13, 2018

pwittrock Apr 18, 2017 •

edited

Loading

liggitt Apr 20, 2017 •

edited

Loading

pwittrock Apr 21, 2017 •

edited

Loading