sig-api-machinery KEP: Pruning for CustomResources #709
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
kind/kep
|
/assign @apelisse |
|
I will update this today to match the vanilla OpenAPI subset #1002. |
|
This is updated now to match #1002. |
sttts
force-pushed
the
sttts-pruning
branch
5 times, most recently
from
ee7989c to
9c42677
Compare
sttts
force-pushed
the
sttts-pruning
branch
3 times, most recently
from
c82ed16 to
591edf5
Compare
sttts
changed the title
sttts
force-pushed
the
sttts-pruning
branch
5 times, most recently
from
e0cf9f9 to
caefa54
Compare
|
|
||
| **blockers for GA:** | ||
|
|
||
| * we verified that performance of pruning is adequat and not considerably reducing throughput. |
There was a problem hiding this comment.
Quantify this? Intuitively, I don't expect much difference in cost from a validation pass to a validation+pruning pass. So maybe saying within X% is sufficient here.
@roycaihw any thoughts from you're work on benchmarking validation?
There was a problem hiding this comment.
this is hard to quantify without defining test cases to compare against. It depends on the input CR instances and on the schema a lot.
My gut feeling is that with structural schemas and a handful of pruned fields, this is not expensive at all (compared to unmarshalling and value validation).
There was a problem hiding this comment.
Note that we don't have to fill this for alpha.
|
@jpbetz addressed your comments |
| // Defaults to false. | ||
| // Setting this field to true is considered an alpha API. | ||
| // Note: this will default to true in version v1. | ||
| PruneUnknownFields bool |
There was a problem hiding this comment.
needs to be a *bool (at least in v1beta1) so we can distinguish between unset and explicitly false
if explicitly false, is this equivalent to putting x-kubernetes-preserve-unknown-fields: true at every level of the schema?
There was a problem hiding this comment.
fixed.
And yes, x-kubernetes-preserve-unknown-fields: true at each object level.
There was a problem hiding this comment.
that equivalence is helpful to call out in the documentation
| // Defaults to false. | ||
| // Setting this field to true is considered an alpha API. | ||
| // Note: this will default to true in version v1. | ||
| PruneUnknownFields bool |
There was a problem hiding this comment.
Since "known fields" are derived from properties directives in the schema, would it be clearer to name it x-kubernetes-preserve-unknown-properties?
Also, would it be helpful to align the name and polarity of the field and the schema extension? This would let us omit the field in v1 when set to false, which is nice.
v1beta1:
// defaults to true
// +optional
PreserveUnknownProperties *bool `json:"preserveUnknownProperties,omitempty"v1:
// defaults to false
// +optional
PreserveUnknownProperties bool `json:"preserveUnknownProperties,omitempty"There was a problem hiding this comment.
not sure about field-vs-properties. @apelisse is using "fields" in his new tags as well.
Am fine with switching polarity.
There was a problem hiding this comment.
I like matching polarity and name, I don't feel strongly about properties vs fields
There was a problem hiding this comment.
I leave it at fields for the moment. We have time to find consensus before code freeze. This is not critical.
|
|
||
| **Pruning will be enabled by default for CRDs created in `v1`**. | ||
|
|
||
| Pruning requires _structural schemas_ (as described in [KEP Vanilla OpenAPI Subset: Structural Schema](https://github.com/kubernetes/enhancements/pull/1002)) for all defined versions. Validation will reject the CRD with `pruneUnknownFields: true` otherwise. |
There was a problem hiding this comment.
What is the behavior when submitting a v1 CRD without a validation schema?
There was a problem hiding this comment.
We will prune everything, other than TypeMeta and ObjectMeta.
|
if someone opts into pruning an existing v1beta1 CRD, should that signal something like the storage migrator that a read/write cycle should be done to prune existing etcd data? I had a similar question about the defaulting KEP. For CRDs with pruning enabled or with defaulting enabled, should the validation schema(s) factor into the published storageVersionHash, since they affect what is stored in etcd? |
|
we have a typo checker that could stall merge of this today. Really.... |
|
Glad to see this come together /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
|  | ||
|
|
||
| We will also prune after mutating webhooks have been run. |
There was a problem hiding this comment.
what about after reading the response from a conversion webhook?
Follow-up of kubernetes/community#2443.