Skip to content

Document conditions vs fields in API conventions #2350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 18, 2018
Merged

Document conditions vs fields in API conventions #2350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions contributors/devel/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,34 +306,57 @@ response reduces the complexity of these clients.
##### Typical status properties

**Conditions** represent the latest available observations of an object's
current state. Objects may report multiple conditions, and new types of
conditions may be added in the future. Therefore, conditions are represented
using a list/slice, where all have similar structure.
state. They are an extension mechanism intended to be used when the details of
an observation are not a priori known or would not apply to all instances of a
Copy link

@krmayankk krmayankk Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be good to have an example of this line when the details of an observation are not a priori known . I am struggling to think of what that means. Would be helpful to others as well

Copy link
Member Author

@thockin thockin Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I list "ready" as an example a couple lines later. Not sufficient?

Copy link

@krmayankk krmayankk Jul 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @thockin we were basically writing an operator and struggling to find some guidelines on when to use conditions in it when i stumbled upon this. The ready example below is mainly an example of what should not be a condition since it applies to all instances of the Pod. I am mainly trying to decipher this line when the details of an observation are not a priori known for an example of a condition/observation to which that applies. Thanks for updating the guidelines.

given Kind. For observations that are well known and apply to all instances, a
regular field is preferred. An example of a Condition that probably should
have been a regular field is Pod's "Ready" condition - it is managed by core
controllers, it is well understood, and it applies to all Pods.

Objects may report multiple conditions, and new types of conditions may be
added in the future or by 3rd party controllers. Therefore, conditions are
represented using a list/slice, where all have similar structure.

The `FooCondition` type for some resource type `Foo` may include a subset of the
following fields, but must contain at least `type` and `status` fields:

```go
Type FooConditionType `json:"type" description:"type of Foo condition"`
Status ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`
Type FooConditionType `json:"type" description:"type of Foo condition"`
Status ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`

// +optional
LastHeartbeatTime unversioned.Time `json:"lastHeartbeatTime,omitempty" description:"last time we got an update on a given condition"`
Reason *string `json:"reason,omitempty" description:"one-word CamelCase reason for the condition's last transition"`
// +optional
LastTransitionTime unversioned.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
Message *string `json:"message,omitempty" description:"human-readable message indicating details about last transition"`

// +optional
Reason string `json:"reason,omitempty" description:"one-word CamelCase reason for the condition's last transition"`
LastHeartbeatTime *unversioned.Time `json:"lastHeartbeatTime,omitempty" description:"last time we got an update on a given condition"`
Copy link
Member

@liggitt liggitt Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to note issues with write load in steady state with heartbeat timestamps?

Copy link
Member Author

@thockin thockin Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add a note.

// +optional
Message string `json:"message,omitempty" description:"human-readable message indicating details about last transition"`
LastTransitionTime *unversioned.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
```

Additional fields may be added in the future.

Do not use fields that you don't need - simpler is better.

Use of the `Reason` field is encouraged.

Use the `LastHeartbeatTime` with great caution - frequent changes to this field
Copy link
Member

@bgrant0607 bgrant0607 Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LastProbeTime is also in use.

The general version of this would be LastUpdateTime.

Given that it's discouraged, isn't urgent to clarify.

can cause a large fan-out effect for some resources.

Conditions should be added to explicitly convey properties that users and
components care about rather than requiring those properties to be inferred from
other observations.
other observations. Once defined, the meaning of a Condition can not be
changed arbitrarily - it becomes part of the API, and has the same backwards-
and forwards-compatibility concerns of any other part of the API.

Condition status values may be `True`, `False`, or `Unknown`. The absence of a
condition should be interpreted the same as `Unknown`.
condition should be interpreted the same as `Unknown`. How controllers handle
`Unknown` depends on the Condition in question.

Condition types should indicate state in the "abnormal-true" polarity. For
example, if the condition indicates when a policy is invalid, the "is valid"
case is probably the norm, so the condition should be called "Invalid".
Copy link

@dmage dmage Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next paragraphs say about the "Ready" and "Succeeded" conditions. Should these examples be updated? It's a bit confusing to have counterexamples in the same document.

Copy link
Member Author

@thockin thockin Dec 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3049

Copy link
Contributor

@smarterclayton smarterclayton Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I read this and agreed with it. Is there an earlier discussion? For abnormal conditions, sure. Available = true is not abnormal, and it is the behavior we want (wait for this condition to be true) when new conditions are added (a new client waiting for an old controller to report a condition it shouldn't know about has to wait for both presence and value).

Forcing humans to negate positives is bad UX.


In general, condition values may change back and forth, but some condition
transitions may be monotonic, depending on the resource and condition type.
Expand Down