From ca492c06556dbec4df1e5ac167f17124e771a746 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Tue, 24 Jul 2018 11:10:22 -0700 Subject: [PATCH 1/2] Document rules and best practices of default values --- contributors/devel/api_changes.md | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/contributors/devel/api_changes.md b/contributors/devel/api_changes.md index 4aa930a6c..3daf8eac1 100644 --- a/contributors/devel/api_changes.md +++ b/contributors/devel/api_changes.md @@ -102,7 +102,7 @@ An API change is considered forward and backward-compatible if it: * adds new functionality that is not required for correct behavior (e.g., does not add a new required field) * does not change existing semantics, including: - * default values *and behavior* + * the semantic meaning of default values *and behavior* * interpretation of existing API types, fields, and values * which fields are required and which are not * mutable fields do not become immutable @@ -296,6 +296,12 @@ was added. release, and do not make it the storage version. The latter is necessary so that a rollback of the apiserver doesn't render resources in etcd undecodable after rollback. +* Any field with a default value in one API version must have *non-nil default + values* in all API versions. If a new field with a default value is introduced + in a new API version, it is required to add a default value semantically + equivalent to an unset value in old API versions, to preserve the semantic + meaning of the value being unset in previous API versions. + ## Incompatible API changes There are times when this might be OK, but mostly we want changes that meet this @@ -365,9 +371,18 @@ being required otherwise. ### Edit defaults.go If your change includes new fields for which you will need default values, you -need to add cases to `pkg/apis///defaults.go` +need to add cases to `pkg/apis///defaults.go`. -*Note:* In the past the core v1 API +**Note:** When adding default values to new fields, you *must* also add default +values in all API versions, instead of leaving new fields unset (e.g. `nil`) in +old API versions. This is required because defaulting happens whenever a +serialized version is read (see [#66135]). When possible, pick meaningful values +as sentinels for unset values. For example, +`DeploymentSpec.ProgressDeadlineSeconds *int32` can have "max int32" (i.e. +2^31-1) as its default value, which is semantically equivalent to unset. When +using max int, be careful not to exceed 2^53-1 (see [#17095]). + +In the past the core v1 API was special. Its `defaults.go` used to live at `pkg/api/v1/defaults.go`. If you see code referencing that path, you can be sure its outdated. Now the core v1 api lives at `pkg/apis/core/v1/defaults.go` which follows the above convention. @@ -383,6 +398,9 @@ pick a default. Don't forget to run the tests! +[#66135]: https://github.com/kubernetes/kubernetes/issues/66135 +[#17095]: https://github.com/kubernetes/kubernetes/issues/17095 + ### Edit conversion.go Given that you have not yet changed the internal structs, this might feel From dd4ae8273f289e490bc0b95caad574f2d9ed90ed Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Thu, 23 Aug 2018 10:48:25 -0700 Subject: [PATCH 2/2] Further clarify defaulting rules & remove example --- contributors/devel/api_changes.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contributors/devel/api_changes.md b/contributors/devel/api_changes.md index 3daf8eac1..b4bc8c67d 100644 --- a/contributors/devel/api_changes.md +++ b/contributors/devel/api_changes.md @@ -297,10 +297,14 @@ was added. apiserver doesn't render resources in etcd undecodable after rollback. * Any field with a default value in one API version must have *non-nil default - values* in all API versions. If a new field with a default value is introduced - in a new API version, it is required to add a default value semantically - equivalent to an unset value in old API versions, to preserve the semantic - meaning of the value being unset in previous API versions. + values* in all API versions. If a default value is added to a field in one API + version, and the field didn't have a default value in previous API versions, + it is required to add a default value semantically equivalent to an unset + value to the field in previous API versions, to preserve the semantic + meaning of the value being unset. This includes: + * a new optional field with a default value is introduced in a new API version + * an old optional field without a default value (i.e. can be nil) has a + default value in a new API version ## Incompatible API changes @@ -377,10 +381,7 @@ need to add cases to `pkg/apis///defaults.go`. values in all API versions, instead of leaving new fields unset (e.g. `nil`) in old API versions. This is required because defaulting happens whenever a serialized version is read (see [#66135]). When possible, pick meaningful values -as sentinels for unset values. For example, -`DeploymentSpec.ProgressDeadlineSeconds *int32` can have "max int32" (i.e. -2^31-1) as its default value, which is semantically equivalent to unset. When -using max int, be careful not to exceed 2^53-1 (see [#17095]). +as sentinels for unset values. In the past the core v1 API was special. Its `defaults.go` used to live at `pkg/api/v1/defaults.go`. @@ -399,7 +400,6 @@ pick a default. Don't forget to run the tests! [#66135]: https://github.com/kubernetes/kubernetes/issues/66135 -[#17095]: https://github.com/kubernetes/kubernetes/issues/17095 ### Edit conversion.go