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.

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

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

Closed
wants to merge 7 commits into from
27 changes: 27 additions & 0 deletions contributors/devel/sig-architecture/api-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I made pretty much the same statement above.


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

Choose a reason for hiding this comment

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

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

| `// +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. If set to `atomic`, it defines that 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). |
| `//+listMapKeys=<list of keys>` | Applicable in the context of lists being used as maps/dictionaries. It's a list of strings, that defines which combination of keys should be used as the identifier of each element. |
| `//+mapType=atomic/granular` | If set to `atomic`, a single actor can only replace the map entirely. If set to `granular`, multiple actors can update individual map entries separately. |
| `//+structType=atomic/granular` | If set to `atomic`, a single actor can only replace the struct entirely. If set to `granular`, multiple actors can update individual struct fields separately. |

##### Considerations on updates to patch strategies/topologies, and server-side apply

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

Choose a reason for hiding this comment

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

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

* 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`
Copy link
Member

Choose a reason for hiding this comment

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

what does this line mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged.
Copy link
Member

Choose a reason for hiding this comment

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

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

* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`.
Copy link
Member

Choose a reason for hiding this comment

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

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

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

configure to atomic as a middle step

what does this accomplish?

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.


## Idempotency

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