[WIP] Document patch strategy markers in api-conventions #4218

mariantalla · 2019-11-05T18:10:46Z

Should eventually address #3752.
Also related: kubernetes-sigs/structured-merge-diff#115

This PR adds a section in contributors/devel/sig-architecture/api-conventions.md that references available go markers that can be used to define merge strategies during PATCH operations.

Would love feedback on:

Above all, any false information and/or unclear language:)
Does the format of collecting everything in this doc work (as opposed to pointing to other docs as applicable, e.g. the strategic merge patch documentation)
Does it make sense to add a couple of syntax examples in this doc, or are these better placed elsewhere?

Remaining tasks in this PR:

(once the general direction seems fine) Add information for listType, mapType, structType.

/cc @apelisse
/kind documentation
/sig api-machinery
/sig architecture
(^^as owners of the doc)

k8s-ci-robot · 2019-11-05T18:11:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mariantalla
To complete the pull request process, please assign jbeda
You can assign the PR to them by writing /assign @jbeda in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

contributors/devel/sig-architecture/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-11-05T18:11:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mariantalla
To complete the pull request process, please assign jbeda
You can assign the PR to them by writing /assign @jbeda in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

contributors/devel/sig-architecture/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2019-11-05T20:48:45Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

contributors/devel/sig-architecture/api-conventions.md

apelisse · 2019-12-16T18:05:49Z

OK looks good to me, @liggitt is this relevant here?

It mixes up a couple of different concepts though...needs some more work.

apelisse · 2020-01-08T00:35:26Z

contributors/devel/sig-architecture/api-conventions.md

@@ -591,6 +591,33 @@ saved. For more details on how to use Merge Patch, see the RFC.
 detailed explanation of how it works and why it needed to be introduced, see
 [here](/contributors/devel/sig-api-machinery/strategic-merge-patch.md).

+#### Declaring Patch Strategy
+In the context of [strategic merge patch](/contributors/devel/sig-api-machinery/strategic-merge-patch.md), a set of go markers can be used by API authors to declare the patch strategy of collections of fields (lists, maps and structs).


I don't know if this sentence is correct, listType etc don't specifically make sense in the context of strategic merge.

liggitt · 2020-01-08T13:38:31Z

contributors/devel/sig-architecture/api-conventions.md

@@ -591,6 +591,33 @@ saved. For more details on how to use Merge Patch, see the RFC.
 detailed explanation of how it works and why it needed to be introduced, see
 [here](/contributors/devel/sig-api-machinery/strategic-merge-patch.md).

+#### Declaring Patch Strategy
+In the context of [strategic merge patch](/contributors/devel/sig-api-machinery/strategic-merge-patch.md), a set of go markers can be used by API authors to declare the patch strategy of collections of fields (lists, maps and structs).


Some of these directives are only relevant for server-side apply, not strategic merge patch. @apelisse, can you give guidance on which apply to which?

Agreed, I made pretty much the same statement above.

liggitt · 2020-01-08T13:40:16Z

contributors/devel/sig-architecture/api-conventions.md

+
+| Marker | Definition |
+|--------|------------|
+| `//+patchStrategy=merge/replace` |  Defines that the merge strategy will be to: merge elements of the two lists into one (if set to merge), or use the list provided in the patch literally rather than merging (if set to replace). Default is `replace`. |


I think it might be more readable to give each directive/value pair its own line

liggitt · 2020-01-08T13:41:25Z

contributors/devel/sig-architecture/api-conventions.md

+
+A newer version of an API may decide to redefine the patch strategy or topology for a list, map, or struct. This section lists some scenarios and expected behaviours.
+
+* A `listType` can only be updated from `atomic` to `set` if the list elements are scalars. Similarly, `listType` updates from `atomic` to `map` will only succeed if the list items are key-value pairs, and a `listMapKeys` marker is added to define the list of keys that uniquely identify a list entry.


clarify can vs should... you can do some things that you should not do

liggitt · 2020-01-08T13:42:15Z

contributors/devel/sig-architecture/api-conventions.md

+* A `listType` can only be updated from `atomic` to `set` if the list elements are scalars. Similarly, `listType` updates from `atomic` to `map` will only succeed if the list items are key-value pairs, and a `listMapKeys` marker is added to define the list of keys that uniquely identify a list entry.
+* When updating `listType` from `set` to `atomic`, the next actor (e.g. `kubectl`) to apply the list will entirely replace it. The operation will not give out conflicts, even if the actor has't previously issued requests against this field.
+* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16):
+  * if `listType` is set to `set` or `map`


what does this line mean?

Hm, I think I've stopped mid-sentence there, sorry :/ I've removed this whole section on updates while we figure them out.

liggitt · 2020-01-08T13:42:55Z

contributors/devel/sig-architecture/api-conventions.md

+* When updating `listType` from `set` to `atomic`, the next actor (e.g. `kubectl`) to apply the list will entirely replace it. The operation will not give out conflicts, even if the actor has't previously issued requests against this field.
+* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16):
+  * if `listType` is set to `set` or `map`
+  * if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged.


@apelisse is this true? that's really confusing if the fields were considered atomic previously

liggitt · 2020-01-08T13:43:33Z

contributors/devel/sig-architecture/api-conventions.md

+* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16):
+  * if `listType` is set to `set` or `map`
+  * if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged.
+* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`.


@apelisse - being set to atomic (visible in the API) or treated as atomic (behavior in the server)?

liggitt · 2020-01-08T13:45:05Z

contributors/devel/sig-architecture/api-conventions.md

+  * if `listType` is set to `set` or `map`
+  * if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged.
+* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`.
+* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step.


this seems like it falls under the same category of things you should not do

configure to atomic as a middle step

what does this accomplish?

liggitt · 2020-01-08T13:46:26Z

contributors/devel/sig-architecture/api-conventions.md

+* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`.
+* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step.
+  * From `set` to `map`: while new objects can be created without problems following the updated topology, existing objects can no longer be changed.
+  * From `map` to `set`: there's high risk of losing or corrupting data of existing objects. Just reconfiguring `listType` without reapplying objects will empty the contents of existing objects.


Just reconfiguring listType without reapplying objects will empty the contents of existing objects.

@apelisse - Is this accurate? I'm not aware of any process that does this. Does this actually mean "empty contents" or does it mean "deduplicate items to form a set"?

liggitt · 2020-01-08T13:47:47Z

contributors/devel/sig-architecture/api-conventions.md

+* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step.
+  * From `set` to `map`: while new objects can be created without problems following the updated topology, existing objects can no longer be changed.
+  * From `map` to `set`: there's high risk of losing or corrupting data of existing objects. Just reconfiguring `listType` without reapplying objects will empty the contents of existing objects.
+* Updating `listMapKeys` for a `listType=map` will succeed as long as the spec of the existing objects is valid by both key validations; the original and the update. For example, with an original configuration of `listMapKeys=port,protocol` and an updated configuration of `listMapKeys=name,version`, list entries of an existing object must have unique, not omitted combinations of both `port,protocol` and `name,version`; otherwise the object can't be updated.


Updating listMapKeys for a listType=map will succeed as long as the spec of the existing objects is valid by both key validations;

This makes it sound like the server will check existing data for you and prevent you from making unsafe changes to the schema which is not the case. There is no validation of existing objects performed when updating a custom resource schema.

apelisse · 2020-01-08T20:35:57Z

I see that you've added the part about compatibility, which is great. But I'd much rather see the test before to be convinced about the validity. I'll look for the tests first. Can we maybe split this PR in 2? 1 part that explains how the markers work, and later another PR that explains the compatibility?

for readability

mariantalla · 2020-01-21T17:38:36Z

Thanks for the feedback! I've removed the compatibility part while we sort it out, and rephrased points that were unclear or wrong. Let me know how that looks!

apelisse · 2020-01-21T20:00:07Z

contributors/devel/sig-architecture/api-conventions.md

+| `//+patchStrategy=merge` |  Defines that the merge strategy will be to merge elements of the two lists into one. |
+| `//+patchStrategy=replace` (default)|  Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. |
+| `// +patchMergeKey=<key-name>` |  Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. |
+| `//+listType=atomic/set/map` |  Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). |


Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to atomic, different actors can update individual elements in the list.

I find this a little confusing, since you start by saying that a signle actor can replace the entire list.
What about this:

Applicable to objects that are of type list. Defines if list are owned as a whole by a single actor of if each individual item is owned by a different actor. atomic means that the list is owned/changed entirely by a single actor. set means that each item in the list can be owned by a different actor, the items of the list must be atomic/scalar. map means that the list is an associative list, each actor can own a separate item in the list, the keys have to be scalars.

Whoops, I meant to make that line talk about atomic only, but didn't do it all the way. Fixed now, I think.

apelisse · 2020-01-21T20:00:37Z

contributors/devel/sig-architecture/api-conventions.md

+| `//+patchStrategy=replace` (default)|  Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. |
+| `// +patchMergeKey=<key-name>` |  Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. |
+| `//+listType=atomic/set/map` |  Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). |
+| `//+listType=set` |  Applicable to objects that are of type list, and the elements are scalar. Different actors can update individual elements in the list.  |


Elements can also be 'atomic' things. And also, elements have to be unique.

Added the uniqueness, not sure about the atomic things though - doesn't elements are scalar guarantee that? Do you mean e.g. having a set with atomic maps? Or did I misunderstand your comment? 🤔

apelisse · 2020-01-21T20:01:12Z

contributors/devel/sig-architecture/api-conventions.md

+| `//+listType=atomic/set/map` |  Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). |
+| `//+listType=set` |  Applicable to objects that are of type list, and the elements are scalar. Different actors can update individual elements in the list.  |
+| `//+listType=map` |  Applicable to objects that are of type list, and elements are map-like, i.e. lists that are used as dictionaries. Different actors can update individual elements in the list. |
+| `//+listMapKeys=<list of keys>` |  Applicable in the context where `listType=map`. `listMapKeys` a list of strings, that defines which combination of keys should be used as the identifier of each element. |


combinations of keys have to be unique, keys have to be scalar. I don't know if they strictly have to be strings, but they should definitely be scalars.

Added the "combinations must be unique" point. The keys' "names" have to be strings* but the values can indeed be anything that's scalar - I've clarified that. Thank you!

*e.g. listMapKeys: [1,2] is invalid

apelisse · 2020-01-27T22:05:08Z

contributors/devel/sig-architecture/api-conventions.md

+| `//+patchStrategy=replace` (default)|  Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. |
+| `// +patchMergeKey=<key-name>` |  Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. |
+| `//+listType=atomic` |  Applicable to objects that are of type list. A single actor owns and can replace the entire list, but cannot change individual elements of it. |
+| `//+listType=set` |  Applicable to objects that are of type list, and the elements are unique and scalar. Different actors can update individual elements in the list. |


I think the restriction for set is that elements have to be unique and atomic (scalar or atomic list/struct/map).

I think I agree with that restriction. But, I think currently we only correctly handle the case of scalar set elements, and have a TODO in the structured-merge-diff repo to support any atomic type as an element.

https://github.com/kubernetes-sigs/structured-merge-diff/blob/9818ffe7e8e1c6765e6484b9ce087c8656cc60cc/typed/helpers.go#L210

apelisse · 2020-01-27T22:58:45Z

@jennybuckley and I were talking about lists semantic. I was curious what a "random list with various items" should be? It sounds like it should be "atomic". We should make it clear that UNLESS you know that your list is a set or a map, then it should be atomic. I'm curious if we should make atomic the default.

contributors/devel/sig-architecture/api-conventions.md

apelisse · 2020-02-03T20:41:25Z

/lgtm

Jordan, anything to add? Thanks.

mariantalla · 2020-02-04T09:21:52Z

@jennybuckley and I were talking about lists semantic. I was curious what a "random list with various items" should be? It sounds like it should be "atomic". We should make it clear that UNLESS you know that your list is a set or a map, then it should be atomic. I'm curious if we should make atomic the default.

Where it gets confusing (in my head at least) is that set/map seem to refer to the elements of the list, and describe whether they're scalar or nested, and atomic/granular seem to refer to the replacement behaviour supported by the list (and describe if the list must be replaced as a whole or can be replaced element by element).

In theory the two could be orthogonal, i.e. it could be valid to have an atomic or granular set, atomic or granular map. Whether that would make sense in practice is a different conversation :)

apelisse · 2020-02-04T22:50:07Z

that set/map seem to refer to the elements of the list, and describe whether they're scalar or nested

I don't know if that's true. set doesn't make the elements atomic, it should fail if they are not. The types should be atomic (and tagged appropriately, before their struct definition) to be used in a set.

MikeSpreitzer · 2020-02-11T16:33:59Z

Do we still need to document the patch stuff in both comments and the field's tag? For example, I currently see this in https://github.com/kubernetes/api/blob/master/apps/v1/types.go:

	// Represents the latest available observations of a statefulset's current state.
	// +optional
	// +patchMergeKey=type
	// +patchStrategy=merge
	Conditions []StatefulSetCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,10,rep,name=conditions"`

apelisse · 2020-02-11T17:14:13Z

I don't think so. I'm not even sure why we still have these? @mariantalla that's also something we could be doing, look at the code to understand if this is still required and provide guidance for the future.

fejta-bot · 2020-05-11T17:41:30Z

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

mrbobbytables · 2020-06-10T15:41:46Z

/remove-lifecycle stale

Just checking in on this -- at this point is the only remaining item the comment from @MikeSpreitzer and @apelisse brought up?

fejta-bot · 2020-09-08T16:26:34Z

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

markjacksonfishing · 2020-09-08T16:27:57Z

/remove-lifecycle stale

mrbobbytables · 2020-10-23T20:38:47Z

This is now approaching a year old. =/
@mariantalla is this something you still want to pursue?
I'm going to flag as rotten so it will close automatically if not.
/lifecycle rotten

apelisse · 2020-10-23T22:04:30Z

We have some of that in the docs, not in the api convention, which may or may not be good enough.

apelisse · 2020-11-21T04:33:31Z

@mengqiy Do you know if the apiserver still uses the struct tags or the openapi to merge SMP? I don't remember how it's choosing one or another.

fejta-bot · 2020-12-21T04:49:59Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-12-21T04:50:13Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Start documenting patch strategy markers

26fb1e4

k8s-ci-robot requested a review from apelisse November 5, 2019 18:10

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2019

apelisse reviewed Nov 6, 2019

View reviewed changes

contributors/devel/sig-architecture/api-conventions.md Outdated Show resolved Hide resolved

Document list/map/structType, specify listType=map (not associative)

ab2dd84

mariantalla added 2 commits December 30, 2019 11:20

(minor) Clarify explanation of patch strategies

56a601f

Add a section on topology update considerations

2473333

It mixes up a couple of different concepts though...needs some more work.

mariantalla mentioned this pull request Jan 7, 2020

Document how to use list-type and list-keys in a CRD, including in existing clusters. kubernetes-sigs/structured-merge-diff#115

Open

apelisse reviewed Jan 8, 2020

View reviewed changes

liggitt reviewed Jan 8, 2020

View reviewed changes

Split directives in separate lines

09c31c1

for readability

apelisse reviewed Jan 21, 2020

View reviewed changes

Correct marker references, clarify types

957240d

apelisse reviewed Jan 27, 2020

View reviewed changes

This was referenced Jan 28, 2020

Invalid listTypes in FlowSchema cause errors in apiserver logs kubernetes/kubernetes#87594

Closed

api-conventions.md should explain patchStrategy #3752

Open

MikeSpreitzer reviewed Jan 29, 2020

View reviewed changes

contributors/devel/sig-architecture/api-conventions.md Outdated Show resolved Hide resolved

Fix typo touse -> to use

df4289b

k8s-ci-robot assigned apelisse Feb 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2020

MikeSpreitzer mentioned this pull request Feb 11, 2020

Introduce ExtendedObjectMetadata, holding Writes MikeSpreitzer/kube-examples#87

Merged

hasheddan mentioned this pull request May 6, 2020

Continuous update of remote resources causes Azure LB provisioner to fail crossplane/crossplane#1446

Closed

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

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 10, 2020

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

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 8, 2020

k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 23, 2020

k8s-ci-robot closed this Dec 21, 2020


		A newer version of an API may decide to redefine the patch strategy or topology for a list, map, or struct. This section lists some scenarios and expected behaviours.

		* A `listType` can only be updated from `atomic` to `set` if the list elements are scalars. Similarly, `listType` updates from `atomic` to `map` will only succeed if the list items are key-value pairs, and a `listMapKeys` marker is added to define the list of keys that uniquely identify a list entry.

[WIP] Document patch strategy markers in api-conventions #4218

[WIP] Document patch strategy markers in api-conventions #4218

Conversation

mariantalla commented Nov 5, 2019 • edited Loading

k8s-ci-robot commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

fejta-bot commented Nov 5, 2019

apelisse commented Dec 16, 2019

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

apelisse commented Jan 8, 2020

mariantalla commented Jan 21, 2020

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

apelisse commented Jan 27, 2020

apelisse commented Feb 3, 2020

mariantalla commented Feb 4, 2020

apelisse commented Feb 4, 2020

MikeSpreitzer commented Feb 11, 2020

apelisse commented Feb 11, 2020

fejta-bot commented May 11, 2020

mrbobbytables commented Jun 10, 2020

fejta-bot commented Sep 8, 2020

markjacksonfishing commented Sep 8, 2020

mrbobbytables commented Oct 23, 2020

apelisse commented Oct 23, 2020

apelisse commented Nov 21, 2020

fejta-bot commented Dec 21, 2020

k8s-ci-robot commented Dec 21, 2020

mariantalla commented Nov 5, 2019 •

edited

Loading