address comments

Author: ymqytw

contributors/design-proposals/preserve-order-in-strategic-merge-patch.md

@@ -17,14 +17,35 @@ issue [40373](https://github.com/kubernetes/kubernetes/issues/40373).
 
 ## Proposed Change
 
+We will use the following notions through the doc.
+Notion:
+list to be merged: same as live list, which is the list current in the server.
+parallel list: the list with `$setElementOrder` directive in the patch.
+patch list: the list in the patch that contains the value changes.
+
 Changes are all in strategic merge patch package.
 The proposed solution is similar to the solution used for deleting elements from lists of primitives.
 
 Add to the current patch, a directive ($setElementOrder) containing a list of element keys -
 either the patch merge key, or for primitives the value. When applying the patch,
 the server ensures that the relative ordering of elements matches the directive.
 
-Here is an simple example:
+The server will reject the patch if it doesn't satisfy the following 2 requirement.
+- the relative order of any two items in the `$setElementOrder` list
+matches that in the patch list if they present.
+- the items in the patch list must be a subset or the same as the `$setElementOrder` list if the directive presents.
+
+The relative order of two items are determined by the following order:
+
+1. relative order in the $setElementOrder if both items are present
+2. else relative order in the patch if both items are present
+3. else relative order in the server-side list if both items are present
+4. else append to the end
+
+If the relative order of the live config in the server is different from the order of the parallel list,
+the user's patch will always override the order in the server.
+
+Here is an simple example of the patch format:
 
 Suppose we have a type called list. The patch will look like below.
 The order from the parallel list ($setElementOrder/list) will be respected.
@@ -33,40 +54,32 @@ The order from the parallel list ($setElementOrder/list) will be respected.
 $setElementOrder/list:
 - A
 - B
+- C
 list:
-- B
 - A
+- C
 ```
 
-All the items in the server's live list but not in the parallel list will be append to the end of the parallel list.
+All the items in the server's live list but not in the parallel list will be come before the parallel list.
 The relative order between these appended items are kept.
-If the relative order of the live config in the server is different from the order of the parallel list,
-the user's patch will always override the order in the server.
 
 The patched list will look like:
 
 ```
 mergingList:
+- serverOnlyItem1 \
+  ...              |===> items in the server's list but not in the parallel list
+- serverOnlyItemM /
 - parallelListItem1 \
   ...                |===> items from the parallel list
 - parallelListItemN /
-- otherItem1 \
-  ...         |===> items in the server's list but not in the parallel list
-- otherItemN /
 ```
 
 ### When $setElementOrder is not present and patching a list
 
 The new directive $setElementOrder is optional.
-The new server applying a patch request without the directive will be
-a best-effort operation to keep the order of elements that are already in the list:
-
-For changes and deletions, the server will just merge the change without sorting them.
-So the items in the list will retain the order.
-
-For additions, the items will be appended to the end of the list.
-
-So it changes a little from previous releases, but is still fully backward compatible with old clients.
+When the $setElementOrder is missing,
+relative order in the patch list will be respected.
 
 Examples where A and C have been changed, B has been deleted and D has been added.
 
@@ -75,26 +88,26 @@ Patch:
 ```yaml
 list:
 - A'
-- $patch: delete
-  B
+- B'
 - D
 ```
 
 Live:
 
 ```yaml
 list:
-- C
 - B
+- C
 - A
 ```
 
 Result:
 
 ```yaml
 list:
-- C
-- A
+- C # server-only item comes first
+- A'
+- B'
 - D
 ```
 
@@ -132,10 +145,12 @@ list:
 ### When the list to be merged contains elements not found in `$setElementOrder`
 
 If the list to be merged contains elements not found in $setElementOrder,
-they will come after all elements defined in $setElementOrder, but keep their relative ordering.
+they will come before all elements defined in $setElementOrder, but keep their relative ordering.
 
 Example where A & B have been changed:
 
+Patch:
+
 ```yaml
 $setElementOrder/list:
 - A
@@ -145,6 +160,8 @@ list:
 - B
 ```
 
+Live:
+
 ```yaml
 list:
 - C
@@ -158,11 +175,11 @@ Result:
 
 ```yaml
 list:
-- A
-- B
 - C
 - D
 - E
+- A
+- B
 ```
 
 ### When `$setElementOrder` contains elements not found in the list to be merged
@@ -207,7 +224,9 @@ Patch requests without the directive will change a little,
 but still be fully backward compatible.
 
 ### kubectl
-If an old kubectl sends a old patch to a new server, the server will not preserve the order which behave as an old server.
+If an old kubectl sends a old patch to a new server,
+the server will honor the order in the list as mentioned above.
+The behavior is a little different from before but is not a breaking change.
 
 If a new kubectl sends a new patch to an old server, the server doesn't recognise the parallel list and will drop it.
 So it will behave the same as before.
@@ -284,20 +303,20 @@ env:
   value: new-env
 ```
 
-After server applying the patch:
+After server applying the new patch:
 
 ```yaml
 env:
+- name: ENV5
+  value: server-added-2
+- name: ENV4
+  value: server-added-1
 - name: ENV1
   value: foo
 - name: ENV2
   value: bar
 - name: ENV6
   value: new-env
-- name: ENV5
-  value: server-added-2
-- name: ENV4
-  value: server-added-1
 ```
 
 ### List of Primitives
@@ -361,11 +380,11 @@ After server applying the patch:
 
 ```yaml
 finalizers:
+- e
+- d
 - a
 - b
 - f
-- e
-- d
 ```