From 567b090166af84c4422d8fb91c10221e90bf7d43 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 23 Mar 2020 11:43:55 -0400 Subject: [PATCH 1/3] provide recommended .status.conditions schema --- .../1623-standardize-conditions/README.md | 144 ++++++++++++++++++ .../1623-standardize-conditions/kep.yaml | 19 +++ 2 files changed, 163 insertions(+) create mode 100644 keps/sig-api-machinery/1623-standardize-conditions/README.md create mode 100644 keps/sig-api-machinery/1623-standardize-conditions/kep.yaml diff --git a/keps/sig-api-machinery/1623-standardize-conditions/README.md b/keps/sig-api-machinery/1623-standardize-conditions/README.md new file mode 100644 index 00000000000..30cda0937f9 --- /dev/null +++ b/keps/sig-api-machinery/1623-standardize-conditions/README.md @@ -0,0 +1,144 @@ +# KEP-1623: Standardize Conditions. + + +- [Release Signoff Checklist](#release-signoff-checklist) +- [Summary](#summary) +- [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) +- [Proposal](#proposal) + - [Noteworthy choices](#noteworthy-choices) + - [Graduation Criteria](#graduation-criteria) + - [Upgrade / Downgrade Strategy](#upgrade--downgrade-strategy) + - [Version Skew Strategy](#version-skew-strategy) +- [Implementation History](#implementation-history) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) + + +## Release Signoff Checklist + + + +- [ ] Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR) +- [ ] KEP approvers have approved the KEP status as `implementable` +- [ ] Design details are appropriately documented +- [ ] Test plan is in place, giving consideration to SIG Architecture and SIG Testing input +- [ ] Graduation criteria is in place +- [ ] "Implementation History" section is up-to-date for milestone +- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] +- [ ] Supporting documentation e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes + + + +[kubernetes.io]: https://kubernetes.io/ +[kubernetes/enhancements]: https://git.k8s.io/enhancements +[kubernetes/kubernetes]: https://git.k8s.io/kubernetes +[kubernetes/website]: https://git.k8s.io/website + +## Summary + +While many Kuberentes APIs have `.status.conditions`, the schema of `condition` varies a lot between them. +There is very little commonality at the level of serialization, proto-encoding, and required vs optional. +Conditions are central enough to the API to make a common golang type with a fixed schema. +The schema can be a strong recommendation to all API authors. + +## Motivation + +Allow general consumers to expect a common schema for `.status.conditions` and share golang logic for common Get, Set, Is for `.status.conditions`. +The pattern is well-established and we have a good sense of the schema we now want. + +### Goals + + 1. For all new APIs, have a common type for `.status.conditions`. + 2. Provide common utility methods for `HasCondition`, `IsConditionTrue`, `SetCondition`, etc. + 3. Provide recommended defaulting functions that set required fields and can be embedded into conversion/default functions. + +### Non-Goals + + 1. Update all existing APIs to make use of the new condition type. + +## Proposal + +Introduce a type into k8s.io/apimachinery/pkg/apis/meta/v1 for `Condition` that looks like +```go +type Condition struct { + // Type of condition in CamelCase. + // +required + Type string `json:"type" protobuf:"bytes,1,opt,name=type"` + // Status of the condition, one of True, False, Unknown. + // +required + Status ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status"` + // Last time the condition transitioned from one status to another. + // This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + // +required + LastTransitionTime metav1.Time `json:"lastTransitionTime" protobuf:"bytes,3,opt,name=lastTransitionTime"` + // The reason for the condition's last transition in CamelCase. + // The specific API may choose whether or not this field is considered a guaranteed API. + // +required + Reason string `json:"reason" protobuf:"bytes,4,opt,name=reason"` + // A human readable message indicating details about the transition. + // This field is never considered a guaranteed API and may be empty/missing. + // +optional + Message string `json:"message,omitempty" protobuf:"bytes,5,opt,name=message"` +} +``` + +This is not strictly compatible with any of our existing conditions because of either proto ordinals, +required vs optional, or omitEmpty or not. +However, it encapsulates the best of what we've learned and will allow new APIs to have a unified type. + +### Noteworthy choices + 1. `lastTransitionTime` is required. + Some current implementations allow this to be missing, but this makes it difficult for consumers. + By requiring it, the actor setting the field can set it to the best possible value instead of having clients try to guess. + 2. `reason` is required. + The actor setting the value should always describe why the condition is the way it is, even if that value is, unknown unknowns. + No other actor has the information to make a better choice. + 3. `lastHeartbeatTime` is removed. + This field caused excessive write loads as we scaled. + If an API needs this concept, it should codify it separately and possibly using a different resource. + +### Graduation Criteria + +Because meta/v1 APIs are necessarily v1, this would go direct to GA. +Using a meta/v1beta1 isn't a meaningful distinction since this type is embedded into other types which own their own versions. + +### Upgrade / Downgrade Strategy + +This KEP isn't proposing that existing types be changed. +This means that individual upgrade/downgrade situations will be handled discretely. +By providing recommended defaulting functions, individual APIs will be able to more easily transition to the new condition type. + +### Version Skew Strategy + +Standard defaulting and conversion will apply. +APIs which have extra values for this type may have to go through an intermediate version that drops them or accept +that certain optional fields of their conditions will be dropped. +Depending on the individual APIs and when their extra fields are deprecated, this could be acceptable choice. + +## Implementation History + +## Drawbacks + + 1. There may be some one-time pain when new versions are created for APIs that wish to consume this common schema. + Switching is not strictly required, but it is encouraged. + +## Alternatives + + 1. We could recommend a schema and not provide one. This doesn't seem very nice to consumers. + diff --git a/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml b/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml new file mode 100644 index 00000000000..80d2bf3bf8e --- /dev/null +++ b/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml @@ -0,0 +1,19 @@ +title: KEP Template +kep-number: 1623 +authors: + - "@deads2k" +owning-sig: sig-apimachinery +participating-sigs: + - sig-apimachinery + - sig-apps + - sig-architecture +status: implementable +creation-date: 2020-03-23 +reviewers: + - "@lavalamp" + - "@smarterclayton" +approvers: + - "@derekwaynecarr" + - "@liggitt" +see-also: +replaces: From 45f87a414c66187d9733ce1959af1cfa1346728e Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 24 Apr 2020 09:08:51 -0400 Subject: [PATCH 2/3] add conditions.observedgeneration and clarify usage --- .../1623-standardize-conditions/README.md | 28 ++++++++++++------- .../1623-standardize-conditions/kep.yaml | 4 +-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/keps/sig-api-machinery/1623-standardize-conditions/README.md b/keps/sig-api-machinery/1623-standardize-conditions/README.md index 30cda0937f9..8232ae455a5 100644 --- a/keps/sig-api-machinery/1623-standardize-conditions/README.md +++ b/keps/sig-api-machinery/1623-standardize-conditions/README.md @@ -77,24 +77,32 @@ The pattern is well-established and we have a good sense of the schema we now wa Introduce a type into k8s.io/apimachinery/pkg/apis/meta/v1 for `Condition` that looks like ```go type Condition struct { - // Type of condition in CamelCase. + // Type of condition in CamelCase or in foo.example.com/CamelCase. + // Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be + // useful (see .node.status.conditions), the ability to deconflict is important. // +required Type string `json:"type" protobuf:"bytes,1,opt,name=type"` // Status of the condition, one of True, False, Unknown. // +required Status ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status"` + // If set, this represents the .metadata.generation that the condition was set based upon. + // For instance, if .metadata.generation is currently 12, but the .status.condition[x].observedGeneration is 9, the condition is out of date + // with respect to the current state of the instance. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,3,opt,name=observedGeneration"` // Last time the condition transitioned from one status to another. - // This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + // This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. // +required - LastTransitionTime metav1.Time `json:"lastTransitionTime" protobuf:"bytes,3,opt,name=lastTransitionTime"` + LastTransitionTime metav1.Time `json:"lastTransitionTime" protobuf:"bytes,4,opt,name=lastTransitionTime"` // The reason for the condition's last transition in CamelCase. - // The specific API may choose whether or not this field is considered a guaranteed API. + // The specific API may choose whether or not this field is considered a guaranteed API. + // This field may not be empty. // +required - Reason string `json:"reason" protobuf:"bytes,4,opt,name=reason"` + Reason string `json:"reason" protobuf:"bytes,5,opt,name=reason"` // A human readable message indicating details about the transition. - // This field is never considered a guaranteed API and may be empty/missing. - // +optional - Message string `json:"message,omitempty" protobuf:"bytes,5,opt,name=message"` + // This field may be empty. + // +required + Message string `json:"message" protobuf:"bytes,6,opt,name=message"` } ``` @@ -106,8 +114,8 @@ However, it encapsulates the best of what we've learned and will allow new APIs 1. `lastTransitionTime` is required. Some current implementations allow this to be missing, but this makes it difficult for consumers. By requiring it, the actor setting the field can set it to the best possible value instead of having clients try to guess. - 2. `reason` is required. - The actor setting the value should always describe why the condition is the way it is, even if that value is, unknown unknowns. + 2. `reason` is required and must not be empty. + The actor setting the value should always describe why the condition is the way it is, even if that value is "unknown unknowns". No other actor has the information to make a better choice. 3. `lastHeartbeatTime` is removed. This field caused excessive write loads as we scaled. diff --git a/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml b/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml index 80d2bf3bf8e..0dc4aba3ffb 100644 --- a/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml +++ b/keps/sig-api-machinery/1623-standardize-conditions/kep.yaml @@ -2,9 +2,9 @@ title: KEP Template kep-number: 1623 authors: - "@deads2k" -owning-sig: sig-apimachinery +owning-sig: sig-api-machinery participating-sigs: - - sig-apimachinery + - sig-api-machinery - sig-apps - sig-architecture status: implementable From e4e99850831a45b924f66f67ecd388c36a23ab6c Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 1 May 2020 13:45:23 -0400 Subject: [PATCH 3/3] provide reasoning for why the condition.observedGeneration is optional and not a pointer --- .../1623-standardize-conditions/README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/keps/sig-api-machinery/1623-standardize-conditions/README.md b/keps/sig-api-machinery/1623-standardize-conditions/README.md index 8232ae455a5..9d95cf3637e 100644 --- a/keps/sig-api-machinery/1623-standardize-conditions/README.md +++ b/keps/sig-api-machinery/1623-standardize-conditions/README.md @@ -120,6 +120,16 @@ However, it encapsulates the best of what we've learned and will allow new APIs 3. `lastHeartbeatTime` is removed. This field caused excessive write loads as we scaled. If an API needs this concept, it should codify it separately and possibly using a different resource. + 4. `observedGeneration` does not follow the standard requirement of "all optional fields are pointers". + This rule originated from the need to distinguish intent of zero value versus unset. + The `.metadata.generation` is never set to zero. + See the [CR strategy](https://github.com/kubernetes/apiextensions-apiserver/blob/release-1.18/pkg/registry/customresource/strategy.go#L88) + and [deployment strategy](https://github.com/kubernetes/kubernetes/blob/release-1.18/pkg/registry/apps/deployment/strategy.go) + as examples. + Because `.metadata.generation` is never zero-value, it is not necessary to distinguish between absent and zero-value observedGeneration. + Whether a client omits `observedGeneration` (because it is unaware of the new field) or explicitly sets it to 0, the + meaning is the same: the condition does not correspond to a known generation. + This also provides parity the `.metadata.generation` field [Generation int64 \`json:"generation,omitempty" protobuf:"varint,7,opt,name=generation"\`](https://github.com/kubernetes/apimachinery/blob/release-1.18/pkg/apis/meta/v1/types.go#L182-L185). ### Graduation Criteria