Design for customer facing resource states

[matthchr]

Closes #1448

What this PR does / why we need it:
A design for how to expose the state of a resource to customers in ASO v2

[matthchr]

I filed #1615 for actually implementing this. We can decide if we want to do it in alpha-0 or some other alpha release.

[matthchr]

Based on comments during live review, will update to be more like CAPI's.
Also change the "Deploying" to "Reconciling"

[codecov-commenter]

Codecov Report

Merging #1614 (eaf4c27) into master (744f90b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1614   +/-   ##
=======================================
  Coverage   66.63%   66.63%           
=======================================
  Files         204      204           
  Lines       14760    14760           
=======================================
  Hits         9835     9835           
  Misses       4169     4169           
  Partials      756      756           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 744f90b...eaf4c27. Read the comment docs.

[matthchr]

Saw some formatting issues -- will fix those

[devigned]

I left a little grammar nit, but otherwise lgtm.

The uniformity of providing a Ready condition feels like a good choice. It simplifies the Azure reconciliation cycle and is easily consumed by tooling. I also like the use of reason and severity to provide guidance on progress as well as failure states (still progressing, user intervention needed, and terminal).

I don't have a strong opinion about kstatus. It seems like the tooling would pick up on Ready as effectively as kstatus.

[CecileRobertMichon]

I believe the reason Reason (😄 ) is optional is that it should only be set when the condition is False. There is no Reason on True Conditions

[CecileRobertMichon]

I guess the official recommendations do say it should be required though

reason is required and must not be empty. The actor setting the value should always describe why the condition is the way it is, even if that value is "unknown unknowns". No other actor has the information to make a better choice.

need to go back and check why that's not the case in CAPI

[matthchr]

Given that there are infinitely many failure cases and really only one success case, it feels "ok" to give a (somewhat useless) Reason like Success for the one success case, as it then saves everybody from then having think about an empty reason.

With that said the guidance isn't really crystal clear as Reason is clearly optional here.

[CecileRobertMichon]

Got a better answer on the CAPI slack: https://kubernetes.slack.com/archives/C8TSNPY4T/p1625615995097600

In case you can't open the link:

cecile Yesterday at 4:59 PM
Does anyone know/remember why CAPI conditions have Reason as optional? The KEP from apimachinery says it should be required:
reason is required and must not be empty. The actor setting the value should always describe why the condition is the way it is, even if that value is “unknown unknowns”. No other actor has the information to make a better choice.

fabrizio.pandini 6 hours ago
Umhh, this comment does not match what the API types are defining few lines above in the doc
// The reason for the condition’s last transition in CamelCase.
// The specific API may choose whether or not this field is considered a guaranteed API.
// This field may not be empty.

fabrizio.pandini 6 hours ago
I personally don’t think that always adding a reason to a condition makes sense,
thing e.g,. when a condition signals everything is ok, like e.g. Machine Ready true (adding a reason in this case will be just redundant)
Instead, I think that adding a reason in case the condition are signalling a problem makes much more sense

[matthchr]

I'm not entirely sure I follow this answer. It seems to say:

1. Actually each specific API can choose if Reason is required or not.
2. Fabrizio (and a few others who thumbs up-ed his answer) don't see much reason to include Reason when the condition signals OK.

I agree with point 2... but it's a really minor quibble. I can't imagine people getting confused by a reason that was included in the success case, and always including reason has the advantage that you can make a nice simple statement about reason: "It's always set". More importantly than that though, it matches the KEP. The other places where the CAPI diverged from the KEP were explicitly called out.

@Porges, @theunrepentantgeek, @babbageclunk - you have thoughts on this topic?

[theunrepentantgeek]

A reason like ReconciliationComplete would make it clear that the reason (heh) Ready is now true is that we've done all our stuff. Removes ambiguity about how we got here.

[babbageclunk]

I can see that Reason is redundant when Ready is true (it's like that Tolstoy quote about happy families), but I think the simplicity of having it always present is worth that wrinkle.

[matthchr]

Note to self to add another open question: LastTransitionTime in the spec is documented as: "the last time the condition transitioned from one status to another." Status is a particular field, which can oftentimes not change even while the underlying Reason, Severity, or Message does change.

Do we want to define LastTransitionTime as the last time anything changed, or just the last time the Status field changed?

[theunrepentantgeek]

Looks really good. A bunch of comments, nothing particularly important.

[theunrepentantgeek]

A reason like ReconciliationComplete would make it clear that the reason (heh) Ready is now true is that we've done all our stuff. Removes ambiguity about how we got here.

[theunrepentantgeek]

Looks good to me. Thanks for the updates.

[CecileRobertMichon]

Do we want to define LastTransitionTime as the last time anything changed, or just the last time the Status field changed?

FWIW, this is how CAPI answers that question:

// NOTE: If a condition already exists, the LastTransitionTime is updated only if a change is detected
// in any of the following fields: Status, Reason, Severity and Message.

[matthchr]

Thanks @CecileRobertMichon - I've also adopted that approach.