Deprecate Status.FailureMessage, FailureReason and use conditions for it

[fabriziopandini]

Conditions in Cluster API are becoming the main tool for reporting status to the users.

In light of that, we should consider if in v1alpha4 we should deprecate Status.FailureMessage, FailureReason and use conditions for it as well.

Related to #3153

/kind feature

[fabriziopandini]

/milestone v0.4.0

[vincepri]

/priority important-soon

[bboreham]

Should this have the kind/api-change label?

[vincepri]

Sure

/kind api-change
/area api

[ncdc]

I'm in favor of switching to conditions as soon as is reasonable.

Some questions:

1. What condition(s) would we create to replace these 2 strings?
2. How would we convert the data between v1alpha3 and v1alpha4?

[srm09]

@ncdc @vincepri and I had a Slack thread around it. Continuing the discussion here.

What condition(s) would we create to replace these 2 strings?

Creating a new condition, say FailedCondition, which would be added to the object in case of a failure. Any logic previously depending upon checking for failure message/reason in the status object will be moved to checking the presence of this condition on the object. Similarly, instead of populating the failure msg/reason, we add a condition to the object during the reconciliation loops.

How would we convert the data between v1alpha3 and v1alpha4?

The best way would be to look for the presence of failure msg/reason on the object and populate the condition on the object as part of the conversion webhook. Not quite sure if this is how it would work, correct me if I am wrong here.

To @vincepri's concern around conditions being mutable, is there no case right now wherein the CAPI objects can recover from the existing failure. My question being around, is there any specific reason why the existing Failure Msg/Reason fields should be immutable?

[ncdc]

Failure message & reason were explicitly designated as indications of a permanent failure from which there is no recovery (assuming you are using a MachineDeployment/Set and you have MachineHealthCheck enabled, this combination would yield the failed Machine being deleted and the MachineSet creating a replacement replica).

[srm09]

I do not see a way around making the, so called, Failed condition immutable except for adding a check in the validation webhook and adding documentation around this condition. @fabriziopandini might have some more thoughts around this one.

[fabriziopandini]

AFAIK failure msg/reason is used only in a few cases, and if we start by mapping those use cases probably this will make the discussion more concrete.

WRT to create new condition for terminal failures I'm a little bit hesitant for two reasons.

• Most probably those failures happen in the context of operation already monitored by other conditions. e.g. Failure messages can flow up from a clusterinfrastructure objects, but we already have the InfrastructureReadyCondition reporting progress for this process. Why not to surface the problem using the existing condition instead of creating a new one/forcing the use to correlate two conditions?
• CAPI conditions already have a severity field that can qualify a condition failure, why not use it?

looping into @yastij that might add more context given #3938

[yastij]

I think we are using those in a few places in CAPV and CAPA

#3938 - is to actually use severity to convey such information, and let the machine healthcheck decide, based on the severity, if it should remediate the machine or not. I'm planning on opening the PR tomorrow to kickoff discussions around this