preserve order of list in strategicMergePatch #467

mengqiy · 2017-03-16T22:17:44Z

This is kind of a small proposal of how to preserve the order of list in strategicMergePatch.

Old behavior: we never preserve the order for list with merge patchStrategy. Instead we sort them by the mergeKey.

Related issue: #40373

cc: @pwittrock @lavalamp

mengqiy · 2017-03-23T22:52:20Z

@lavalamp Ping

pwittrock · 2017-03-29T16:26:47Z

contributors/devel/api-conventions.md

+and list with replace `patchStrategy`, list with merge `patchStrategy`),
+we always preserve the order of the list.
+
+It is obvious that list with replace `patchStrategy` will preserve the order.


Delete this sentence. No need to state that the obvious is obvious.

pwittrock · 2017-03-29T16:28:04Z

contributors/devel/api-conventions.md

@@ -595,6 +595,16 @@ Strategic Merge Patch also supports special operations as listed below.

 ### List Operations

+For all lists (including list of maps, list of primitives


This isn't strictly true or possible. What does preserve the order even mean when you merge lists:

AB

AC

pwittrock · 2017-03-29T16:29:01Z

contributors/devel/api-conventions.md

@@ -595,6 +595,16 @@ Strategic Merge Patch also supports special operations as listed below.

 ### List Operations

+For all lists (including list of maps, list of primitives
+and list with replace `patchStrategy`, list with merge `patchStrategy`),
+we always preserve the order of the list.


This is only true if we send the parallel list, which isn't required.

pwittrock · 2017-03-29T16:47:33Z

contributors/devel/api-conventions.md

+
+It is obvious that list with replace `patchStrategy` will preserve the order.
+
+For list with merge `patchStrategy`, we send a parallel reorder list when there are changes in the list.


Specifying element ordering for a strategic merge patch on a list [Alpha]

By default, patching a list using the Strategic Merge strategy will provide no guarantees to the final ordering of elements in the patched list.

As of Kubernetes 1.7, Alpha support exists for explicitly specifying the ordering of elements in a during a Strategic Merge Patch. This can be done by providing a parallel list containing all of the patchMergeKeys of elements that need to be ordered. Any elements missing from the parallel list are guaranteed to appear after elements appearing in the parallel list.

When patching Strategic Merge lists, kubectl apply sends a parallel list containing keys for each of the elements in the configuration file.

mengqiy · 2017-03-29T21:31:48Z

Updated.

pwittrock · 2017-04-03T20:57:08Z

@apelisse Could you give this a review?

apelisse · 2017-04-03T21:03:31Z

contributors/devel/api-conventions.md

+
+By default, patching a list using the Strategic Merge strategy will provide no guarantees to the final ordering of elements in the patched list.
+
+As of Kubernetes 1.7, Alpha support exists for explicitly specifying the ordering of elements in a during a Strategic Merge Patch.


missing word "list" ?

Alpha support exists for explicitly specifying the ordering of elements in a LIST during a Strategic Merge Patch.

apelisse · 2017-04-03T22:40:25Z

contributors/devel/api-conventions.md

+
+As of Kubernetes 1.7, Alpha support exists for explicitly specifying the ordering of elements in a during a Strategic Merge Patch.
+This can be done by providing a parallel list containing all of the patchMergeKeys of elements that need to be ordered.
+Any elements missing from the parallel list are guaranteed to appear after elements appearing in the parallel list.


Not sure if this is a relevant use-case, but what happens if the server adds many?

Original:

- A

Server adds B to F:

- A - B - C - D - E - F

User applies:

- A - B

We only guarantee that A and B will keep order?

Or if the server reorders things in the meantime. Intent gets really muddled here.

Original:
A B C D E

how does a client indicate it want to move A to the end, tolerating any server-side reordering of the other elements?

We guaranteed the order from the user's config will always be preserved in the server.

We only guarantee that A and B will keep order?

Yes

how does a client indicate it want to move A to the end, tolerating any server-side reordering of the other elements?

If the server add some items in the list, a client cannot move something to the end without specify the whole list using this approach.

lavalamp · 2017-04-03T23:02:53Z

contributors/devel/api-conventions.md

@@ -595,6 +595,16 @@ Strategic Merge Patch also supports special operations as listed below.

 ### List Operations

+#### Specifying element ordering for a strategic merge patch on a list [Alpha]


It is super confusing to put this here. This section of the doc is about when the verb is "list" (i.e., GET on a collection). Please make this part of the strategic merge patch section. Also should we really be editing the doc before it's implemented? I guess I would have expected a proposal.

Wait, sorry, I am wrong.

I think we should specify strategic merge patch elsewhere. This is not the right doc to define it.

Yes. Pulled strategic merge patch doc out.
#535

lavalamp · 2017-04-03T23:04:28Z

contributors/devel/api-conventions.md

+By default, patching a list using the Strategic Merge strategy will provide no guarantees to the final ordering of elements in the patched list.
+
+As of Kubernetes 1.7, Alpha support exists for explicitly specifying the ordering of elements in a during a Strategic Merge Patch.
+This can be done by providing a parallel list containing all of the patchMergeKeys of elements that need to be ordered.


I don't understand what this means. An example would be helpful. Something that explicitly states the syntax of the patch would be best.

lavalamp · 2017-04-03T23:12:26Z

contributors/devel/api-conventions.md

+    value: foo
+  - name: ENV2
+    value: bar
+  - name: ENV5


I am not sure that this is a useful feature. It is very complex and I'm not sure why it is necessary, or even why it is an improvement? I can see adding an "append" command.

lavalamp · 2017-04-03T23:13:42Z

contributors/devel/api-conventions.md

 #### List of Primitives

 We have two patch strategies for lists of primitives: replace and merge.
 Replace is the default patch strategy, which will replace the whole list on update and it will preserve the order;
-while merge strategy works as an unordered set. In this section, we call a primitive list with merge strategy an unordered set.
+while merge strategy works as an ordered set. In this section, we call a primitive list with merge strategy an ordered set.


Will it still be possible to treat these items as an unordered set?

lavalamp · 2017-04-03T23:22:14Z

Apologies for delay in looking at this. I am lost in a sea of github notifications, next time chat me up on slack (or internally).

Is there a reason why kubernetes/kubernetes#40373 can't just use the replace strategy? We don't have random things mutating the environment variables, I don't think.

liggitt · 2017-04-04T03:14:43Z

contributors/devel/api-conventions.md

 The patch strategy is defined in the go struct tag of the API objects,
 e.g. `finalizers` uses `merge` as patch strategy.
 ```go
 Finalizers []string `json:"finalizers,omitempty" patchStrategy:"merge" protobuf:"bytes,14,rep,name=finalizers"`
 ```

-##### Unordered Set
+##### Ordered Set

 There are 3 operations: add, delete, replace.


If it's truly an ordered set, then there should be more than three operations (or add should be insert)

Yeah, we shouldn't get rid of the current unordered set, we should add a way to deal with lists instead.

liggitt · 2017-04-04T04:18:17Z

Is there a reason why kubernetes/kubernetes#40373 can't just use the replace strategy? We don't have random things mutating the environment variables, I don't think.

kubectl edit submits a patch computed by CreateTwoWayMergePatch, which means clients have baked-in structs with merge strategies, so they will only send the added/deleted items, which means we can't change the strategy to replace server-side or we'll drop all the envvars they didn't change.

lavalamp · 2017-04-04T16:06:22Z

Yeah that's a fair point, we should stop computing these from compiled in structs. I talked with Phil & Mengqi some yesterday and suggested explicit inserts, deletes, and moves instead of this ordering suggestion.

…

On Mon, Apr 3, 2017 at 9:18 PM, Jordan Liggitt ***@***.***> wrote: Is there a reason why kubernetes/kubernetes#40373 <kubernetes/kubernetes#40373> can't just use the replace strategy? We don't have random things mutating the environment variables, I don't think. kubectl edit submits a patch computed by CreateTwoWayMergePatch, which means clients have baked-in structs with merge strategies, so they will only send the added/deleted items, which means we can't change the strategy to replace server-side or we'll drop all the envvars they didn't change. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglga6Xg5ddWeAMY61VpSGwh4NpN05ks5rscSNgaJpZM4Mf8vH> .

mengqiy · 2017-04-14T20:49:36Z

Move to a proposal PR in #537. PTAL

* init * cant be in both lists

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

k8s-ci-robot assigned lavalamp and pwittrock Mar 16, 2017

pwittrock reviewed Mar 29, 2017

View reviewed changes

preserve order when patching

5d81174

mengqiy force-pushed the preserve_order_SMP branch from 45687de to 5d81174 Compare March 29, 2017 21:31

pwittrock assigned lavalamp and apelisse and unassigned lavalamp and pwittrock Apr 3, 2017

apelisse reviewed Apr 3, 2017

View reviewed changes

lavalamp reviewed Apr 3, 2017

View reviewed changes

liggitt reviewed Apr 4, 2017

View reviewed changes

mengqiy mentioned this pull request Apr 14, 2017

Proposal: preserving order in strategic merge patch #537

Merged

mengqiy closed this Apr 14, 2017

mengqiy deleted the preserve_order_SMP branch September 6, 2017 21:30

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Add me as developer (kubernetes#467)

efe4ef7

* init * cant be in both lists

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preserve order of list in strategicMergePatch #467

preserve order of list in strategicMergePatch #467

mengqiy commented Mar 16, 2017

mengqiy commented Mar 23, 2017

pwittrock Mar 29, 2017

pwittrock Mar 29, 2017

pwittrock Mar 29, 2017

pwittrock Mar 29, 2017

mengqiy commented Mar 29, 2017

pwittrock commented Apr 3, 2017

apelisse Apr 3, 2017

apelisse Apr 3, 2017

liggitt Apr 3, 2017

mengqiy Apr 14, 2017

lavalamp Apr 3, 2017

lavalamp Apr 3, 2017

lavalamp Apr 3, 2017

mengqiy Apr 14, 2017

lavalamp Apr 3, 2017

lavalamp Apr 3, 2017

lavalamp Apr 3, 2017

lavalamp commented Apr 3, 2017

liggitt Apr 4, 2017 •

edited

Loading

lavalamp Apr 4, 2017

liggitt commented Apr 4, 2017

lavalamp commented Apr 4, 2017 via email

mengqiy commented Apr 14, 2017

		@@ -595,6 +595,16 @@ Strategic Merge Patch also supports special operations as listed below.

		### List Operations

		For all lists (including list of maps, list of primitives


		It is obvious that list with replace `patchStrategy` will preserve the order.

		For list with merge `patchStrategy`, we send a parallel reorder list when there are changes in the list.


		By default, patching a list using the Strategic Merge strategy will provide no guarantees to the final ordering of elements in the patched list.

		As of Kubernetes 1.7, Alpha support exists for explicitly specifying the ordering of elements in a during a Strategic Merge Patch.

		@@ -595,6 +595,16 @@ Strategic Merge Patch also supports special operations as listed below.

		### List Operations

		#### Specifying element ordering for a strategic merge patch on a list [Alpha]

preserve order of list in strategicMergePatch #467

preserve order of list in strategicMergePatch #467

Conversation

mengqiy commented Mar 16, 2017

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

Specifying element ordering for a strategic merge patch on a list [Alpha]

mengqiy commented Mar 29, 2017

pwittrock commented Apr 3, 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

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

lavalamp commented Apr 3, 2017

liggitt Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 4, 2017

lavalamp commented Apr 4, 2017 via email

mengqiy commented Apr 14, 2017

liggitt Apr 4, 2017 •

edited

Loading