Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
26fb1e4
Start documenting patch strategy markers
mariantalla Nov 5, 2019
ab2dd84
Document list/map/structType, specify listType=map (not associative)
mariantalla Dec 16, 2019
56a601f
(minor) Clarify explanation of patch strategies
mariantalla Dec 30, 2019
2473333
Add a section on topology update considerations
mariantalla Dec 30, 2019
09c31c1
Split directives in separate lines
mariantalla Jan 21, 2020
957240d
Correct marker references, clarify types
mariantalla Jan 23, 2020
df4289b
Fix typo touse -> to use
mariantalla Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions contributors/devel/sig-architecture/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,23 @@ 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 and Object Topology
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 lists, maps and structs (`//+patchStrategy` and `//+patchMergeKey`). Work to move the `apply` functionality to the control plane ([KEP](https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/0006-apply.md)) has also introduced markers to describe the topology of lists, maps and structs (`//+listType`, `//+listMapKeys`, `//+mapType`, `//+structType`).

| Marker | Definition |
|--------|------------|
| `//+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. |
mariantalla marked this conversation as resolved.
Show resolved Hide resolved
| `// +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). |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `//+listType=set` | Applicable to objects that are of type list, and the elements are scalar. Different actors can update individual elements in the list. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

| `//+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. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `//+mapType=atomic` | A single actor can only replace the map entirely. |
| `//+mapType=granular` (default) | Multiple actors can update individual map entries separately. |
| `//+structType=atomic` | A single actor can only replace the struct entirely. |
| `//+structType=granular` (default) | Multiple actors can update individual struct fields separately. |

## Idempotency

All compatible Kubernetes APIs MUST support "name idempotency" and respond with
Expand Down