provide recommended .status.conditions schema

[deads2k]

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

• keps/sig-api-machinery/OWNERS [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wojtek-t]

Nice - I like the suggestion. Just couple minor comments.

[wojtek-t]

Maybe it's moot discussion, but for optional field don't we want to make the type *string?
@liggitt

[deads2k]

Maybe it's moot discussion, but for optional field don't we want to make the type *string?
@liggitt

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.

[thockin]

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.

[deads2k]

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.

Ok. I'll remove omit empty and make it required with "" as allowed.

[robscott]

The end result here is really confusing to me. The combination of This field may be empty and +required does not make a lot of sense to me. It seems like our standard has been to use pointers for optional fields. Although I agree that pointers can be a pain to work with, I think an optional field is easier to understand than a "required" field that can be empty.

[deads2k]

Suggestions from @lavalamp in the issue

+1

@etune @liggitt fyi

I'd like to suggest the current pattern should add:

Observed generation field (what version of the spec is this condition about, if applicable?
Identity field (what controller / actor observed this condition)? (possibly, we could use the fieldManager instead of adding a field.)
Both would make multiple controller interactions simpler / more transparent. (Think: kubelet / NPD / node controller)

I think I'm a -1 on the identity field since that can be gotten via the fieldManager.

Observed generation as a optional field is interesting to me. There is some fanout concern, but it doesn't appear excessive to me. I could go either way, but I'm biased towards a small merge of existing function first before extending it. /cc @liggitt

[bgrant0607]

To link the issues, previous analyses of conditions usage:
kubernetes/kubernetes#7856 (comment)
kubernetes/kubernetes#50798
kubernetes/kubernetes#51594

Example code that interprets conditions across many resource types:
https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/status/core.go

cc @mortent

[evankanderson]

Sorry for the drive-by, but I'm interested in kubernetes/community#4521, which seems related.

[evankanderson]

A side note (from a project that standardized this ourselves for our CRDs):

We found that using metav1.Time added a non-trivial amount of bookkeeping using a condition update semantic of "compute the new status, then update if any conditions changed value". To avoid this, we did the following:

1. Sort the Conditions by Type
2. Compare the slices with equality.Semantic, ignoring LastTransitionTime
3. Only update Conditions which were not equal.

If you're curious, see knative/serving#1663 and https://github.com/knative/pkg/blob/master/apis/volatile_time.go

[evankanderson]

You may also want to define a type for metav1.Conditions []metav1.Condition, to support adding helper methods for operating on the set of conditions.

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 MarkUnknown and MarkFalse methods both require reason and message fields, but MarkTrue does not.

[lavalamp]

This looks like a great candidate for client-go and/or controller helper libraries.

[deads2k]

This looks like a great candidate for client-go and/or controller helper libraries.

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

[MikeSpreitzer]

Evan: does your suggestion require the helper methoda to be defined in the metav1 package? If so, then it does not suffice to define an interface; we would need to also write the implementations for metav1.Conditions in metav1.

[evankanderson]

You might want to mention whether reason needs to be non-empty when the condition is True. (For example, if Ready=True, the reason may be superfluous.)

[deads2k]

You might want to mention whether reason needs to be non-empty when the condition is True. (For example, if Ready=True, the reason may be superfluous.)

It should always be non-empty. I'll update to be more clear.

[bgrant0607]

Also, other resource types named this field differently, such as lastProbeTime. I agree that the cost has outweighed the benefit, though.

[lavalamp]

I think I'm a -1 on the identity field since that can be gotten via the fieldManager.

I guess the argument for including it in the condition is that it'd let two parties express differing opinions about the same condition. Obviously we hope not to be in that situation, but that's a desired state and the world might not always match it :)

Observed generation as a optional field is interesting to me. There is some fanout concern, but it doesn't appear excessive to me. I could go either way, but I'm biased towards a small merge of existing function first before extending it.

Usually I'd agree with that, but in this case, if we think it's a good idea, I think we should probably just do it all at once. I'm not sure why I feel that way. Maybe to give people only a single change to adjust to rather than multiple.

[thockin]

Is the proposal that specific types literally use this schema or that they embed-and-extend it?

[thockin]

If we're normalizing, should we consider bigger changes - like making this an optional bool?

[deads2k]

If we're normalizing, should we consider bigger changes - like making this an optional bool?

I think we've been happy with this as an enumerated string to cover Unknown as well.

[thockin]

Who is happy? All I see are calls to ParseBool. "Unknown" as a value is just weird because we also have to handle a non-specified condition, which is truly unknown.

TL;DR I am not super happy with Conditions.

[thockin]

You called this out as unresolved.

I guess I'm arguing for a larger, deeper, not really compatible change.

I think the acknowledgment of unknown has worked well. It gives an option for acknowledging awareness of a condition without the ability to inspect the current value. For instance, an inability to reach an external system results in an Unknown status for the "is-it-installed" condition. You cannot do that with the absence of a condition. I think the concern about ParseBool is well handled by an IsConditionTrue and IsConditionFalse which comments here indicate are commonly used.

If we were rebooting Condition, I would argue harder for *bool as the totally unambiguous representation. Given that you're not rebooting, just normalizing, I will waive my objection. I just wanted it on the record, I guess. :)

[lavalamp]

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: {Ready, Unknown} to mean: "it might or might not be ready, we don't know."

Good: {IPAssigned, Unknown} to mean: "it might or might not have gotten an IP address, we don't know."

Very bad: {IPAssigned, Unknown} to mean: "it has an IP address, but we don't know what it is."

[thockin]

+1

But also <no key "IPAssigned"> is equivalent to {IPAssigned, Unknown} for most intents.

[evankanderson]

{IPAssigned, Unknown} gives a hint to other participants that the "IPAssigned" condition may still be reconciled, while <no key "IPAssigned"> makes it more difficult for other participants to know whether IP assignment is applicable in this case. But this is mostly about the case for *bool vs bool, rather than the string enum that we have vs what we wish.

Would it be reasonable to fill in status: "" as status: "Unknown" somewhere in the apiserver side for Conditions (read side or write side)? My gut says "no", but my heart says "please".

[lavalamp]

Yes, I think status: "" should be canonicalized as status: "Unknown".

[thockin]

"Default to", or just "assumed to mean"?

[liggitt]

Would it be reasonable to fill in status: "" as status: "Unknown" somewhere in the apiserver side for Conditions (read side or write side)? My gut says "no", but my heart says "please".

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 "statuss":"True" get rejected with a "status is required" error than get accepted with status defaulted to "Unknown"

[thockin]

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.

[thockin]

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...

[evankanderson]

Do we expect multiple controllers to be updating the same object's status with different Conditions?

If we assume that a single controller owns the status of the object, then I don't think we need namespaced labels for the types (since the controller should be able to ensure uniqueness).

If we expect multiple controllers to be updating status, then this is more of a problem -- I'd be concerned about other implications (controller fighting) in that design, though.

[lavalamp]

Do we expect multiple controllers to be updating the same object's status with different Conditions?

Yes. NPD, kubelet, and the node controller all set node conditions.

If we expect multiple controllers to be updating status, then this is more of a problem -- I'd be concerned about other implications (controller fighting) in that design, though.

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.

[thockin]

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 readinessGates expect conditions to be populated to indicate "readiness" from an external POV (eg. LB programming).

I raised the prefixing at that time, but we didn't really resolve it.

[deads2k]

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...

That seems reasonable. company.com/CamelCase and CamelCase for first-class conditions? I do think that some are realistically more first class than others.

[thockin]

Define "first-class" ?

labels are GENERALLY not using CamelCase, but in this case I won't balk.

[alena1108]

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

[thockin]

Assigning myself, just so I see updates :)

[deads2k]

Is the proposal that specific types literally use this schema or that they embed-and-extend it?

I recommend literally embedding it.

[zhouhaibing089]

Thanks for sharing those insights! The observedGeneration in conditions is something I think pretty useful.

[thockin]

LGTM

[thockin]

"Default to", or just "assumed to mean"?

[thockin]

If we never start committing to this, we'll never reach consistency.

I waive my concern - it's not worth arguing about piece by piece. It doesn't matter much for JSON. I would expect it changes the on-wire proto representation, but on inspection it seems not. That is surprising to me, but the proto marshal/unmarshal code is human-hostile, so ... <shrug>

[robscott]

The combination of "This field may be empty." and "+required" is confusing to me. How should this be interpreted?

[deads2k]

The combination of "This field may be empty." and "+required" is confusing to me. How should this be interpreted?

required is about the serialization format, empty is about the content of that serialization.

[robscott]

Thanks for the explanation. I get what it means now, but still think it's confusing. Is this how we should be implementing all new optional string fields?

[bowei]

If the field is a non-pointer type -- can it even be optional? (how would the consumer on the controller side even know)

[lavalamp]

The default value can be omitted in the serialization if it's "omitempty", so yes, it's possible.

[lavalamp]

But it's not clear to me why @deads2k wants this field to be this way? Why not call it optional?

[deads2k]

But it's not clear to me why @deads2k wants this field to be this way? Why not call it optional?

Because optional required a pointer and I will do nearly anything to avoid a pointer in a commonly used struct like this.

[robscott]

It looks like observedGeneration is an optional field that is also not a pointer. It sounds like the guidance for that is when "the zero value is sufficiently invalid that it doesn't create any ambiguity", would that also apply here? It seems like there's no point in distinguishing between empty and nil messages here.

[deads2k]

It looks like observedGeneration is an optional field that is also not a pointer. It sounds like the guidance for that is when "the zero value is sufficiently invalid that it doesn't create any ambiguity", would that also apply here? It seems like there's no point in distinguishing between empty and nil messages here.

I'd rather fuss over this in the actual API PR. observedGeneration was not an easy sell.

[liggitt]

You can mark it +optional without making it omitempty so a client can submit a condition without the field without client-side validation complaining bitterly, but still always serialize it in server responses.

Agree we can review this bit during API review in the PR, when we have the benefit of fixture tests and generated openapi schemas showing us exactly the effect on clients and server output.

[hectorj2f]

Looking forward to see this proposal approved! In our case, this becomes quite important when troubleshooting the status of federated resources (each with a different schema for the status and status.conditions) across multiple clusters. So whatever it means to normalize the status schema would reduce that pain.

[liggitt]

/lgtm

[Jamstah]

I know this PR has already been merged so this question comes a little late, do we want to present an opinion on the ordering of this array in this proposal?

For consumers, it would be good to know if this array should be presented in the order it is given (which I expect is the right approach), so that controllers managing the status array can provide whatever significant ordering they feel is appropriate.

[dims]

There's a follow up PR in progress kubernetes/community#4521