Document conditions vs fields in API conventions #2350
Conversation
k8s-ci-robot
added
size/M
| an observation are not a priori known or would not apply to all instances of a | ||
| 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 regular field is Pod's "Ready" condition - it is managed by core |
|
|
||
| // +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"` |
There was a problem hiding this comment.
do we want to note issues with write load in steady state with heartbeat timestamps?
| 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 regular field is Pod's "Ready" condition - it is managed by core | ||
| controllers, it is well understood and applies to all Pods. |
There was a problem hiding this comment.
probably worth noting that it is difficult/impossible to change the factors that combine to determine a single field value. For example, pod readiness could not decide in the future to also include other factors like service endpoint propagation, etc, without potentially breaking API clients that depended on a particular meaning of pod readiness.
There was a problem hiding this comment.
Interesting choice, since we are, in fact, changing the factors that combine to define pod readiness :)
I'll take a stab at words.
thockin
force-pushed
the
conditions-api-convention
branch
from
89fc788 to
a2baa4c
Compare
|
re-pushed |
|
lgtm |
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I list "ready" as an example a couple lines later. Not sufficient?
There was a problem hiding this comment.
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.
|
Examples could be anything that we don't know when designing a type. An
example of the opposite of "Ready" might be a security scanner - it could
add a "Vulnerable" condition, but we're not defining a field `vulnerable`
for it because we obviously don't have cause for that.
…On Mon, Jul 9, 2018 at 9:31 AM krmayankk ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In contributors/devel/api-conventions.md
<#2350 (comment)>:
> @@ -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
thanks @thockin <https://github.com/thockin> we were basically writing a
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVI--J74QDLoGpz6BsD8211_UV4ylks5uE4VTgaJpZM4VGFsX>
.
|
|
thanks @thockin would that mean, that we should add fields in status(rather than a Condition) when we know that fields represents a condition that applies to all instances of that object AND has a well defined/know cause ? I am just clarifying my own thinking. Feel free to include that in the doc if you think that will help |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Yes, exactly. A condition like "ready" should have just been a field.
…On Tue, Jul 10, 2018 at 11:38 AM krmayankk ***@***.***> wrote:
thanks @thockin <https://github.com/thockin> would that mean, that we
should add fields in status when we know that fields represents a condition
that applies to all instances of that object AND has a well defined/know
cause ? I am just clarifying my own thinking. Feel free to include that in
the doc if you think that will help
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVOJ2_KZwNcP-pPOaioVqnUJ25EjIks5uFPSngaJpZM4VGFsX>
.
|
|
This is still not merged, no reason why not. /approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thockin If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
|
|
||
| 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". |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| Use of the `Reason` field is encouraged. | ||
|
|
||
| Use the `LastHeartbeatTime` with great caution - frequent changes to this field |
There was a problem hiding this comment.
LastProbeTime is also in use.
The general version of this would be LastUpdateTime.
Given that it's discouraged, isn't urgent to clarify.
7 participants
No description provided.