Proposal: preserving order in strategic merge patch #537
Conversation
|
Anyone help to review this? |
k8s-ci-robot
added
the
cncf-cla: yes
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
6de8ca2
to
0547f63
Compare
|
|
||
| ## Motivation | ||
|
|
||
| The order of lists with a merge strategy is important, because an item in the list may depends on an item before it. |
There was a problem hiding this comment.
The Kubernetes API may apply semantic meaning to the ordering of items within a list, however the strategic merge patch does not keeping the ordering of elements. Ordering has semantic meaning for Environment variables, as later environment variables may reference earlier environment variables, but not the other way around.
There was a problem hiding this comment.
For example ordering has semantic meaning for environment variables.
I would like to change to this, semantic meaning is not very common.
| ## Proposed Change | ||
|
|
||
| Changes are all in strategic merge patch package. | ||
| It will be similar to how we solve the problem of deleting items from a list of primitives. |
There was a problem hiding this comment.
It will be similar to how we solve the problem of deleting items from a list of primitives.
The proposed solution is similar to the solution used for deleting elements from lists of primitives.
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
0547f63
to
cdeca8b
Compare
| Changes are all in strategic merge patch package. | ||
| It will be similar to how we solve the problem of deleting items from a list of primitives. | ||
|
|
||
| Always send a parallel list along with the patch for list: |
There was a problem hiding this comment.
Add to the current patch, a directive containing a list of element keys - either the patch merge key, or for primitives the value. When applying the patch, the server we ensure that the relative ordering of elements matches the directive.
| - otherItemN / | ||
| ``` | ||
|
|
||
| ## Version Skew |
There was a problem hiding this comment.
Version Skew and Backwards Compatibility
|
|
||
| Suppose we define a list of environment variables and we call them | ||
| the original environment variables: | ||
| ```yaml |
| - When patching a list of maps, the parallel list contains a list of items. Each item contains only the merge key. | ||
| - When patching a list of primitives, the parallel list contains a full list from user's config file. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
Before this give a simple example
| - When patching a list of primitives, the parallel list contains a full list from user's config file. | ||
|
|
||
| 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. | ||
| The relative order between these appended items are kept. |
There was a problem hiding this comment.
Add these as separate sections:
$setElementOrder may contain elements not present in the patch list
The $setElementOrder value may contain elements that are not present in the patch but present in the list to be merge to reorder the elements as part of the merge. Example where A & B have not changed:
Patch:
$setElementOrder/list:
- A
- BLive:
list:
- B
- AResult:
list:
- A
- BWhen 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.
Example where A & B have been changed:
$setElementOrder/list:
- A
- B
list:
- A
- Blist:
- C
- A
- D
- B
- EResult:
list:
- A
- B
- C
- D
- EExplain why this would happen. Explain what happens. Give an example.
When $setElementOrder contains elements not found in the list to be merged
Patch:
$setElementOrder/list:
- C
- A
- B
list:
- A
- BLive:
list:
- A
- BResult:
list:
- A
- BExplain why this would happen. Explain what happens. Given an example
|
|
||
| The patch will looks like: | ||
| ```yaml | ||
| $preserveOrderList/env: |
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
cdeca8b
to
843e051
Compare
| as later environment variables may reference earlier environment variables, | ||
| but not the other way around. | ||
|
|
||
| An use case is the environment variables. We don't preserve the order which causes |
|
|
||
| Add to the current patch, a directive containing a list of element keys - | ||
| either the patch merge key, or for primitives the value. When applying the patch, | ||
| the server we ensure that the relative ordering of elements matches the directive. |
There was a problem hiding this comment.
the server we ensure
the server ensures
|
|
||
| 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. | ||
| The relative order between these appended items are kept. | ||
| If the relative order of live config in the server is different from the order of the parallel list, |
There was a problem hiding this comment.
If the relative order of live config
If the relative order of the live config
| 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. | ||
| The relative order between these appended items are kept. | ||
| If the relative order of live config in the server is different from the order of the parallel list, | ||
| user's patch will always override the order in the server. |
There was a problem hiding this comment.
user's patch will al
the user's patch will al
| The new patch has one additional parallel list which will be dropped by the old server. | ||
|
|
||
| The logic for old version patch is still kept, | ||
| so the patch from any arbitrary old clients used to work before will continue to function. |
There was a problem hiding this comment.
so the patch from any arbitrary old clients used to work before will continue to function.
The new directive is optional. Patch requests without the directive will retain the behavior from previous releases. This will ensure backward compatibility with old clients.
| Then the server appends two finalizers and reorder the list: | ||
|
|
||
| ```yaml | ||
| finalizers: |
There was a problem hiding this comment.
This is a really helpful example. Good job.
|
Minor nits on wording. Otherwise LGTM. |
|
Updated. |
|
This looks good to me. |
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
7380028
to
23721e4
Compare
|
Squashed commits. |
| The new patch has one additional parallel list which will be dropped by the old server. | ||
|
|
||
| The new directive is optional. | ||
| Patch requests without the directive will retain the behavior from previous releases. |
| - E | ||
| ``` | ||
|
|
||
| ### When `$setElementOrder` contains elements not found in the list to be merged |
There was a problem hiding this comment.
Add a section for
When $setElementOrder is not present and patching a list.
| ``` | ||
|
|
||
|
|
||
| # Alternative Considered |
There was a problem hiding this comment.
Another alternative, is to send the mergeKeys as empty elements in the list, and have the server maintain the order of the provided items. In this case, we don't need to introduce any new directives, and the patch should be totally compatible. The patch list would always be reordered to come before all elements missing from the patch.
There was a problem hiding this comment.
Thinking on this more, if we did that, we might end up creating elements that we just want to order - e.g. if we want to modify "A", and send "A, B, C" as the main list to keep ordering, "C" would get recreated if it was deleted or missing, which we would not want.
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
d42784d
to
0861047
Compare
|
Updated. PTAL. |
| 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 - |
There was a problem hiding this comment.
not every list wants to keep ordering, whether we will consider add a directive to type definition?
There was a problem hiding this comment.
add a directive to type definition
I don't understand what you mean here. Can you give me an example?
There was a problem hiding this comment.
@adohe I think you mean that the ordering is not semantically important for every list, and should we add a comment tag to the field definition indicating whether order has semantic meaning. (e.g. The containers list in a PodTemplate does not have semantically meaningful order).
I think this would be be interesting to do in a follow up. Since this is totally optional and sent by the client, the client should decide whether it wants to control the order or not for a given patch (yes it should want to do so)
There was a problem hiding this comment.
@pwittrock yep, that's exactly what I mean. We could do this in a follow up and I am just wondering how the client can decide whether or not to keep list order.
| 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, |
There was a problem hiding this comment.
any consideration about multiple key as merge key?
There was a problem hiding this comment.
The mergeKey is a key-value pair in the map. If we are not using any special format, I think new additional merge keys will simply be new key-value pair in the map.
|
@fabianofranz @liggitt I will be merging this today if there are no other comments. |
| 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. |
There was a problem hiding this comment.
If the server has [A,B,C]
and the client sends [X,A,B,C]
always appending to the end has this counter-intuitive result: [A,B,C,X]
Are old clients known to scramble the order when computing the patch, or was the scrambling happening server-side?
I would have expected the order of two items to be determined by something like the following:
- relative order in the $setElementOrder if both items are present
- else relative order in the patch if both items are present
- else relative order in the server-side list if both items are present
- else append to the end
There was a problem hiding this comment.
Are old clients known to scramble the order when computing the patch, or was the scrambling happening server-side?
Both. We sort the list by mergeKey when diffing and merging in current strategic merge patch implementation.
There was a problem hiding this comment.
@liggitt What should the result be when applying patch [c,b,a] to server's list [a,x,b,y,c]?
There was a problem hiding this comment.
I cannot find a way to keep relative order of (a,x) and (x,b) at the same time.
There was a problem hiding this comment.
@liggitt Your later comment addresses this I think - all items in the list must appear in the orderList
There was a problem hiding this comment.
I was expecting something like https://play.golang.org/p/tJLW1Zj0h7 where the directive is used if present, otherwise the relative order in the patch list is honored for item pairs which are both present, and finally the relative server-side order is honored.
that is less compelling if old clients sent randomly ordered lists
There was a problem hiding this comment.
Thanks a lot. This is super helpful.
| ### 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. |
There was a problem hiding this comment.
if the patch contains $setElementOrder, it seems like an error for $setElementOrder to be missing items that are present in the list (or to have items in a different order than the list)
There was a problem hiding this comment.
it seems like an error for $setElementOrder to be missing items that are present in the list
I'm OK to fail the operation. I will update it soon.
or to have items in a different order than the list
Do you mean we should always verify the order in patch list and the order in $setElementOrder match before merging?
There was a problem hiding this comment.
I think @liggitt was suggesting both are required. I think that is probably fine.
| 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. |
There was a problem hiding this comment.
can we not do better here by honoring relative order of items in the patch?
There was a problem hiding this comment.
As mentioned above #537 (comment), a patch sent by old kubectl doesn't has any useful order in it. Because it has been sorted by mergeKey.
What should we do in this case?
There was a problem hiding this comment.
@liggitt I was thinking about this as well.
I think you are saying, can you just use the ordering in the patch list instead of sending another directive.
The issue here is: If you have a list of [a,b,c] and want to patch b you would either send a patch [b] or send a patch [a,b,c]. The former lacks the ordering, and the latter will recreate [a,c] if they were deleted from the server.
There was a problem hiding this comment.
I don't think it removes the need for the separate order-indicating directive, but it seems really counter-intuitive to ignore relative order in the list
There was a problem hiding this comment.
@liggitt Now sure how much of an issue you think this is. I think it is an edge case, but one that be annoying to track down if it is encountered.
|
This change is |
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
0861047
to
060b30b
Compare
|
PTAL |
mengqiy
force-pushed
the
preserve_order_proposal
branch
from
060b30b
to
ec0d060
Compare
|
Squashed commits. |
|
@LiGgit merging this since it looks like you comments have been addressed |
Move from #467, since we want to move strategic merge patch doc out of the api-convention doc. WIP PR
Addressed comments in #467.
cc: @pwittrock @lavalamp @liggitt @apelisse