provide recommended .status.conditions schema #1624
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| # KEP-1623: Standardize Conditions. | ||
|
|
||
| <!-- toc --> | ||
| - [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) | ||
| <!-- /toc --> | ||
|
|
||
| ## Release Signoff Checklist | ||
|
|
||
| <!-- | ||
| **ACTION REQUIRED:** In order to merge code into a release, there must be an | ||
| issue in [kubernetes/enhancements] referencing this KEP and targeting a release | ||
| milestone **before the [Enhancement Freeze](https://git.k8s.io/sig-release/releases) | ||
| of the targeted release**. | ||
|
|
||
| For enhancements that make changes to code or processes/procedures in core | ||
| Kubernetes i.e., [kubernetes/kubernetes], we require the following Release | ||
| Signoff checklist to be completed. | ||
|
|
||
| Check these off as they are completed for the Release Team to track. These | ||
| checklist items _must_ be updated for the enhancement to be released. | ||
| --> | ||
|
|
||
| - [ ] 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 | ||
|
|
||
| <!-- | ||
| **Note:** This checklist is iterative and should be reviewed and updated every time this enhancement is being considered for a milestone. | ||
| --> | ||
|
|
||
| [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. | ||
|
There was a problem hiding this comment. We've documented CamelCase, but we've also told people to use conditions as an extension mechanism. To do that safely, we should probably support or endorse company.com/label style names... There was a problem hiding this comment. Do we expect multiple controllers to be updating the same object's If we assume that a single controller owns the If we expect multiple controllers to be updating There was a problem hiding this comment.
Yes. NPD, kubelet, and the node controller all set node conditions.
The entire k8s API system is about making it safe for humans and controllers (and multiple controllers) work together on particular objects, both on their status and spec. There was a problem hiding this comment. A while back we documented Conditions a bit more. The use of conditions has evolved a lot from "a single controller" to "how 3rd parties add extension status". Case in point - pod I raised the prefixing at that time, but we didn't really resolve it. There was a problem hiding this comment.
That seems reasonable. There was a problem hiding this comment. Define "first-class" ? labels are GENERALLY not using CamelCase, but in this case I won't balk. |
||
| // +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"` | ||
|
There was a problem hiding this comment. If we're normalizing, should we consider bigger changes - like making this an optional bool? There was a problem hiding this comment.
I think we've been happy with this as an enumerated string to cover Unknown as well. There was a problem hiding this comment. Who is happy? All I see are calls to TL;DR I am not super happy with Conditions. There was a problem hiding this comment. You called this out as unresolved. I guess I'm arguing for a larger, deeper, not really compatible change.
If we were rebooting There was a problem hiding this comment. Given that everyone is already using "Unknown" as a value, I think we have to keep it. :/ Documentation should make clear that "Unknown" is a fact about the writer of the condition, and not a claim about the object. Good: Good: Very bad: There was a problem hiding this comment. +1 But also There was a problem hiding this comment.
Would it be reasonable to fill in There was a problem hiding this comment. Yes, I think There was a problem hiding this comment. "Default to", or just "assumed to mean"? There was a problem hiding this comment.
I would require condition writers to say what they mean and specify the status value. I'd much rather have a condition-adding patch that typos |
||
| // 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"` | ||
liggitt marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. A side note (from a project that standardized this ourselves for our CRDs): We found that using
If you're curious, see knative/serving#1663 and https://github.com/knative/pkg/blob/master/apis/volatile_time.go |
||
| // 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"` | ||
|
There was a problem hiding this comment. Maybe it's moot discussion, but for optional field don't we want to make the type There was a problem hiding this comment.
I really dislike pointers in golang code. I'll spell this however is required to allow omiting to be acceptable and empty string to be valid. There was a problem hiding this comment. Types like string "work" both as primitives and pointers for omitempty. The distinction is that without a pointer, you can't tell if the user said "" or didn't say anything at all. It probably doesn't matter here. The last argument, then, is consistency. we have documented that optional fields be pointers. If you are saying this is a required field, but "" is allowed, that's fine, but then omitempty is probably wrong. There was a problem hiding this comment.
Ok. I'll remove omit empty and make it required with "" as allowed. There was a problem hiding this comment. The end result here is really confusing to me. The combination of |
||
| } | ||
| ``` | ||
|
There was a problem hiding this comment. You may also want to define a type for Cribbing from this interface, we've found the following useful: type ConditionManager interface {
// SetCondition sets or updates the Condition on Conditions for Condition.Type.
// If there is an update, Conditions are stored back sorted.
SetCondition(new Condition)
// ClearCondition removes the non terminal condition that matches the ConditionType
ClearCondition(t ConditionType) error
// MarkTrue sets the status of t to true, and then marks the happy condition to
// true if all dependents are true.
MarkTrue(t ConditionType)
// MarkTrueWithReason sets the status of t to true with the reason, and then marks the happy
// condition to true if all dependents are true.
MarkTrueWithReason(t ConditionType, reason, messageFormat string, messageA ...interface{})
// MarkUnknown sets the status of t to Unknown and also sets the happy condition
// to Unknown if no other dependent condition is in an error state.
MarkUnknown(t ConditionType, reason, messageFormat string, messageA ...interface{})
// MarkFalse sets the status of t and the happy condition to False.
MarkFalse(t ConditionType, reason, messageFormat string, messageA ...interface{})
}The ConditionManager interface also codifies patterns around having a single top-level "happy" Condition (aka "Ready" or "Succeeded") and supports feeding dependent conditions into the top-level condition. Note that the There was a problem hiding this comment. This looks like a great candidate for client-go and/or controller helper libraries. There was a problem hiding this comment.
Yeah, probably something very similar to that interface above like https://github.com/openshift/library-go/blob/master/pkg/operator/v1helpers/helpers.go#L49-L108 There was a problem hiding this comment. Evan: does your suggestion require the helper methoda to be defined in the |
||
|
|
||
| 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. | ||
|
There was a problem hiding this comment. You might want to mention whether There was a problem hiding this comment.
It should always be non-empty. I'll update to be more clear. |
||
| 3. `lastHeartbeatTime` is removed. | ||
| This field caused excessive write loads as we scaled. | ||
|
There was a problem hiding this comment. Also, other resource types named this field differently, such as lastProbeTime. I agree that the cost has outweighed the benefit, though. |
||
| 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. | ||
liggitt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very excited to see this proposal!
As the list of the common utility methods is still in discussion, wanted to list a couple of condition util methods we found to be very frequently used at Rancher - callbacks DoUntilTrue/Once/Do and ReasonAndMessageFromError (method setting condition fields from error obj): https://github.com/rancher/norman/blob/146f45dafa623a34864cbeeb3a39d5903daa2995/condition/condition.go#L131