Add Conditions alongside Phases

[0xmichalis]

In a similar fashion to kubernetes/kubernetes#14181

Our build and deployment config API should[1] transition from Phases to Conditions
Deployment related changes are tracked in https://trello.com/c/OIggCmzo/699-5-deployments-downstream-conditions so I am handing this issue to @bparees

[1] Is it something we want to do? I would say that we should follow upstream conventions and furthermore Conditions seem to be more than a simple convention.

@openshift/api-review

[0xmichalis]

Also another thing I realized while reviewing #9308 even though I had written than part of code before: why do we separate Failed from Error and Cancelled for builds? Upstream pod conditions seem to use different kinds of Reasons (CrashLoopBack, ImagePullLoopBack, Error, DeadlineExceeded, etc.) for a Failed condition. Is it sensible to do the same thing for builds?

[bparees]

We wanted to preserve "failed" for indicating an actual build failure (eg compile error) as compared with error (infrastructure problems) or canceled (user choice).

could it be done with reasons? it could. but that means requiring the user look at an additional field and makes filtering/sorting harder.

[0xmichalis]

I don't think the user would need to do anything different from today if we correctly surfaced the reasons on the output of get and describe. Same story with pods today, when you do oc get pods and have a CrashLooping pod you will not see Running (phase) but CrashLooping in the STATUS column.

[bparees]

Reasons do not tend to be as tightly enumerated as Phase, they are more freeform and likely to have new ones added. So again, sorting/filtering is harder (both for users, and for the UI components).

[smarterclayton]

I think we want to add conditions. But existing phases can't change (we can't add to them either). So we should focus on adding conditions that convey info we need.

[smarterclayton]

All error conditions should have well defined reasons and those
reasons should be on conditions. We should not add fields to status
that belong on conditions (in general).

On Wed, Jun 15, 2016 at 12:48 PM, Clayton Coleman ccoleman@redhat.com wrote:

I think we want to add conditions. But existing phases can't change
(we can't add to them either). So we should focus on adding
conditions that convey info we need.

[bparees]

Can someone summarize, or point me to a summary definition and intended usage for:

• Phase
• Condition
• Reason

?

[smarterclayton]

https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md

On Wed, Jun 15, 2016 at 1:36 PM, Ben Parees notifications@github.com
wrote:

Can someone summarize, or point me to a summary definition and intended
usage for:

• Phase
• Condition
• Reason

?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pxXt63VR38g7deQ0n69zVfduSRsLks5qMDgfgaJpZM4I2JSa
.

[deads2k]

Can someone summarize, or point me to a summary definition and intended usage for:

1. Phase: State of the object. Only one per resource allowed. Interpreting the phase requires understanding of the state machine.
2. Condition: State of an individual aspect of the resource. Multiple allowed per resource. Every condition can be true, false, or unknown.
3. Reason: One word explanation of why the condition is true, false, or unknown. "APIServerUnreachable"
4. Message: Long, human readable description of why the condition is true, false, or unknown. "API server unreachable due to: no route to host"

[bparees]

If Phase is being replaced by Condition, but we now have multiple conditions at a time, that does not seem to lend itself to the current "oc get" output format with a single row per resource and single column per field being reported. Has someone address how conditions should be reported in that format?

[deads2k]

Has someone address how conditions should be reported in that format?

So far? Just a column that has the [Not]Condition.Type in a comma delimited list.

[smarterclayton]

Phase is not being replaced by condition. No new phases should be added to
any new objects, conditions do that job better and safer. Pods summarize
conditions in their describer. See the pod output for more.

On Wed, Jun 15, 2016 at 2:58 PM, Ben Parees notifications@github.com
wrote:

If Phase is being replaced by Condition, but we now have multiple
conditions at a time, that does not seem to lend itself to the current "oc
get" output format with a single row per resource and single column per
field being reported. Has someone address how conditions should be reported
in that format?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_px3aGaQYk5eiX8iaH0itKh1ZBpCZks5qMEtXgaJpZM4I2JSa
.

[bparees]

"Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead."

certainly sounds like phase is being replaced by condition (that doesn't mean we're changing existing usage, but going forward it is taking the place of where phase was used previously).

[bparees]

So far? Just a column that has the [Not]Condition.Type in a comma delimited list.

that seems problematic for any constrained width console. we already struggle to keep the get output to 80 chars.

[smarterclayton]

They are actually consolidated. In practice they don't matter as much.

On Wed, Jun 15, 2016 at 4:20 PM, Ben Parees notifications@github.com
wrote:

So far? Just a column that has the [Not]Condition.Type in a comma
delimited list.

that seems problematic for any constrained width console. we already
struggle to keep the get output to 80 chars.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p9LySm2TZfzhpS6QYxupYHjHfYBTks5qMF59gaJpZM4I2JSa
.

[0xmichalis]

If Phase is being replaced by Condition, but we now have multiple conditions at a time, that does not seem to lend itself to the current "oc get" output format with a single row per resource and single column per field being reported. Has someone address how conditions should be reported in that format?

The latest condition. Conditions also have timestamps.

[0xmichalis]

Moved to Trello: https://trello.com/c/NVTCdbKe/1045-conditions-for-builds

cc: @mfojtik