Update Condition guidance

[evankanderson]

Which issue(s) this PR fixes:
Fixes #4471

I took a first stab at updating these conventions based on kubernetes-sigs/cluster-api#1658 (comment) and kubernetes-sigs/gateway-api#47 (comment).

/assign @smarterclayton @liggitt

I'm happy to recommend different top-level conditions type names. Note that I recommended Succeeded over Complete as a top-level condition, because Complete doesn't indicate whether the Job succeeded or failed.

[k8s-ci-robot]

Hi @evankanderson. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[youngnick]

When looking at this across a bunch of types, I wondered how we could possibly reconcile the history of Conditions in a way that would be machine-parseable easily. The best I could come up with was recommending that new Conditions add a polarity field (or whatever the name ends up being) that indicates if Status True or False is the healthy condition. If that is assumed to be False when not present, then a library could make a fair guess at what the Status means regardless of when it was created.

In no way intended to block this PR, which I fully support.

[mrbobbytables]

/ok-to-test

[evankanderson]

(I thought I'd sent an update from mobile while I was on vacation, but I either hallucinated it or technology ate it)

I'd like to defer the discussion of how to migrate existing controllers into the new pattern to a subsequent PR, possibly as a KEP or introduction of common infrastructure for managing Conditions. However, since you brought it up, my preference would be to introduce a severity field which could be used for both ErrorIfTrue and ErrorIfFalse signalling, but also indicate that some Conditions provide information without signalling a fatal error.

[danehans]

@evankanderson thank you for putting this PR together. kubernetes-sigs/gateway-api#104 is on hold until this merges. @smarterclayton @liggitt is anything needed to move this PR forward?

[therealmitchconnors]

These arguments are compelling, but run opposite of previous guidance used for projects like kstatus. Can we get sig-architecture clarification on whether healthy conditions should be true or false?

[erictune]

Big fan of this change.

[liggitt]

if this changes the guidance around positive/negative conditions, we can drop the (despite it running afoul of current guidance) parenthetical

[evankanderson]

Thanks for catching this, updated.

[evankanderson]

Can you approve this, or is there someone else I should chase?

[evankanderson]

Thanks for catching this, updated.

[youngnick]

@therealmitchconnors This PR is codifying the answers you'll get from people if you ask around into the official docs, so we can all be on the same page from now on. Any stuff that's already done will stay how it is, but when I asked sig-arch and sig-api-machinery, I was told this was the new way. I agree that having someone confirm that here would be ideal though.

[liggitt]

cc @deads2k re kubernetes/enhancements#1624

[liggitt]

Clarify this is a guide for choosing condition names and polarity, not a guide for consuming arbitrary conditions. As written, it sounds like I can write a client that assumes all conditions with a False status represent failure.

[evankanderson]

I would like to reach that point sometime in the future, but you're correct that we can't do this for a long time, if ever.

Adjusted this to suggest that consistency within a resource was most important consideration, and positive polarity should be chosen if there isn't already a consistent polarity.

[liggitt]

The term default is overloaded... this makes it sound as though a condition that omits the status field has its status defaulted to Unknown, which is not the case. The phrase later is clearer:

The absence of a condition should be interpreted the same as Unknown.

[liggitt]

this also needs to be clearly oriented at the condition producer; as phrased it could be misunderstood by a simple consumer to mean that they can consistently expect to be able to use Ready / Succeeded conditions.

[evankanderson]

Again, this is the long-term situation that I'd like to steer the world towards, but you're correct that this could be read by many consumers to demand more from API implementers than is likely in the short term.

Amended, PTAL.

[lavalamp]

I guess your goal is that some watcher can observe progress and know that it's done? You should document how long after the operation the condition should be left there.

But I'm not convinced this is the way to do it, what if there are two resizes back to back? I don't think you can really have conditions 1:1 with operations.

I think "Resizing=true" that is present during the resize and removed after the resize is also a legitimate--and possibly less confusing--way to represent this. Seeing "ResizingSucceeded=Unknown" is disconcerting, even if it's technically accurate.

Maybe the way to strengthen your point is to talk about how the failure of the operation will be represented. I think you can't really do that with a "Reszing" condition, but if you have another way of representing failure, it's OK. If you need a way to represent failure, then "ResizingSucceeded=false" works better.

[evankanderson]

Actually, my concern is that the watcher can determine whether progress is being made in a generic way. (Particularly in the presence of CRDs and a variety of types, detecting "controller is stuck" vs "backend system is slow" can be quite difficult.)

I'd suggest not removing the condition when the operation is done (hence, Provisioned or SizeAllocated). The goal is twofold:

1. Presence of the condition along with LastTransitionTime helps to debug whether the controller has seen the most recent spec.

2. Ability to signal success and failure of these operations in a generic way that will stick with the resource seems useful for the "hey, could you help debug this with me" case -- a resize failed condition that disappears after 2 hours might be gone by the time an expert takes a look at the correct resource in the cluster.

As for the human ergonomics, I was hoping that the Reason and Message fields would suffice:

status:
  conditions:
  - name: Resizing
    status: True
    Reason: FileSystemExtensionInProgress
    Message: The system is currently extending the filesystem on the disk after resize.

status:
  conditions:
  - name: SizeAvailable
    status: Unknown
    Reason: FileSystemExtensionInProgress
    Message: The system is currently extending the filesystem on the disk after resize.

(Note that I changed this to SizeAvailable because there's probably a distinction between allocation failures and filesystem extension failures.)

[lavalamp]

Maybe a way to say this is something like "conditions should not be transient, they should more or less always be on the object with the appropriate value".

But this doesn't rule out something like "Resizing", you can just set it False when the object is the correct size. At first it seems like people would need to look at the Reason to know if it's a "good" false (no resize needed) or a "bad" false (resize failed), but in the latter case we can just leave it in "resizing" (Reason is e.g. "stuck", message is "resize operation can't complete due to error foo").

[evankanderson]

That works for me -- my concern was more specifically that the set of signalled Conditions not be transient, which this addresses. PTAL!

[evankanderson]

As a followup to this PR, I notice that Eric's analysis highlights a number of cases where developers are prefixing their Condition types, ala AppDBInstanceReady for a resource named AppDBOperator. Are we tired of swinging at this, or would a subsequent PR along the lines of smarterclayton's comment be a good follow-on?

Conditions summarize characteristics of object lifecycle that must be generic across multiple types, have a need to convey both state and human message, and can be extended by third parties to add more nuance to existing lifecycle.

An object in Kube is a distributed state machine. The state specified to that object should always be fields. Some objects have consistent fields that describe status of the state machine. One of those is observedGeneration. Another one is conditions. Some conditions define generic, multi-object state machine characteristics like Available, Progressing, Ready.

(I don't want to derail this PR with extra load at this point, but it's been bugging me since I came back to this.)

[lavalamp]

Resizing is another natural example, but no need to add it here. :)

[lavalamp]

/lgtm
/approve

Let's get this in before anyone thinks of another thing to change. There can always be a follow-up!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, lavalamp

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:

• contributors/devel/sig-architecture/OWNERS [lavalamp]

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

[lavalamp]

Are we tired of swinging at this, or would a subsequent PR along the lines of smarterclayton's comment be a good follow-on?

I bet we can do a followup in less time than this PR took :)

[thockin]

I'm still super torn on this. This PR has been reduced almost beyond arguability, but still...

I don't think we have a clear rule for when some datum wants to be a field or a condition. Half of the examples I see here would, IMO, be better as fields. So why are we suggesting conditions for them?

[lavalamp]

I agree with that, but this PR has been pending for more than half a year and is an improvement over the existing text.

[dims]

@thockin if you want to throw some outline of what you want to see, i can try to convince @evankanderson to do a follow up :)

[evankanderson]

I'd be happy to schedule a chat with whomever is interested to try to work out some better / additional guidance.

I can say that we've found the structure of Conditions (short code plus human-readable text and the tri-state Status) very useful for communicating to humans, so I'd like to preserve that.

[dims]

@evankanderson let's line it up for next sig-arch?

[thockin]

I mean, I get that field-level metadata is nice, but if that's the case, we should have it on all fields. We shouldn't force things to fit into a string-bool just to get metadata. Or else the metadata is "nice" and not "critical"

…

On Fri, Sep 25, 2020 at 2:59 PM Evan Anderson ***@***.***> wrote: I'd be happy to schedule a chat with whomever is interested to try to work out some better / additional guidance. I can say that we've found the structure of Conditions (short code plus human-readable text and the tri-state Status) very useful for communicating to humans, so I'd like to preserve that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4521 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVHATNPK5GUGA3GZAOLSHUHF3ANCNFSM4KVRDSEQ> .

[lavalamp]

Practical reasons not to apply this to all fields: that go doesn't do templates (yet), and objects would get very large.

One practical reason to keep it a bool is that it's simple to express what you want to watch for with kubectl tools.

My current guidance would be, conditions are good for:

• Top-level facts about the object,
• Which can change over the lifetime of the object (i.e., could be a valid target to wait for),
• And are not overly detailed (i.e., don't make a separate condition for the version of every image cached on the node),
• And have a unique owner (i.e., each condition has a single setter, but it's ok to have different setters for different conditions).

I think the "too detailed" and "valid target to wait for" tests are subjective and require some taste.

I think there's a bit of an open question when multiple instances of the same binary want to vote/set a condition; apiserver does this in multiple places and I'm not sure we're super happy about it. @deads2k