Operator only sets status when the reconciler succeeds.

[joelanford]

Right now, when a failure occurs in the Helm reconciler, no status updates are made to the resource, so consumers of these resources don't have great visibility when things go wrong. Currently the only way to see failures is by inspecting the operator logs.

We should update the operator to correctly set status at various points of reconciliation, not just on success

[hasbro17]

We could define a condition for whether a release is reconciled/installed(true, false, unknown) with the reason.
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties
Or turn that into a known field in status.

[joelanford]

Yeah I was reading up on conventions for status and I'm thinking the Helm status is due for an overhaul.

I think we need to be able to convey the following states:

Success (steady-state):

• "Deployed", with the deployed release
  • This would cover successes for install, update, and reconcile

Failure:

• "InstallFailed", with the failed release and a reason
• "UpdateFailed", with the deployed release, failed release, and a reason
• "ReconcileFailed", with the deployed release and a reason

Maybe we put "Deployed" and "DeployedRelease" as fields at the top-level status and then have a condition for the failures with a FailedRelease field in that condition?

[joelanford]

Thinking about it more, if we just made them all conditions, we could have multiple conditions.

For example, if we successfully install a release, we add a Deployed condition:

status.AddCondition(HelmAppCondition{
    Type:    ConditionDeployed,
    Status:  StatusTrue,
    Reason:  ReasonInstallSuccessful,
    Message: release.GetInfo().GetStatus().GetNotes(),
    Release: release,
})

Then, if sometime later we fail to update, we would just add another condition, ReleaseFailed:

status.AddCondition(HelmAppCondition{
    Type:    ConditionReleaseFailed,
    Status:  StatusTrue,
    Reason:  ReasonUpdateError,
    Message: err.Error(),
    Release: failedRelease,
})

In that case, we'd have two conditions until the update error is resolved.

So something like the following:

type HelmAppStatus struct {
    Conditions []HelmAppCondition `json:"conditions"`
}

type HelmAppCondition struct {
    Type    HelmAppConditionType   `json:"type"`
    Status  ConditionStatus        `json:"status"`
    Reason  HelmAppConditionReason `json:"reason"`
    Message string                 `json:"message,omitempty"`
    Release *release.Release       `json:"release,omitempty"`
}

const (
    ConditionDeployed       HelmAppConditionType = "Deployed"
    ConditionReleaseFailed  HelmAppConditionType = "ReleaseFailed"
    ConditionIrreconcilable HelmAppConditionType = "Irreconcilable"

    StatusTrue    ConditionStatus = "True"
    StatusFalse   ConditionStatus = "False"
    StatusUnknown ConditionStatus = "Unknown"

    ReasonInstallSuccessful HelmAppConditionReason = "InstallSuccessful"
    ReasonUpdateSuccessful  HelmAppConditionReason = "UpdateSuccessful"
    ReasonInstallError      HelmAppConditionReason = "InstallError"
    ReasonUpdateError       HelmAppConditionReason = "UpdateError"
    ReasonReconcileError    HelmAppConditionReason = "ReconcileError"
)

[joelanford]

@hasbro17 I was about to submit a PR for this when I found the following discussions:

• Should we deprecate status.conditions? kubernetes/kubernetes#50798
• Eliminate Phase and simplify Conditions kubernetes/kubernetes#7856 (comment)

The doc you linked (https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties) was last updated before the above discussions happened, and nothing concrete seems to have been decided otherwise. In the absence of a clearly defined convention, I think I lean towards continuing with conditions as it seems to fit the Helm operator use case fairly well.

Thoughts?

[hasbro17]

Yep we can proceed with conditions. We can't go back to Phases and there doesn't seem to be an alternative convention to conditions yet(the issue on removing conditions is frozen kubernetes/kubernetes#7856 (comment)).
The latest upstream proposals are still using conditions as well.
https://github.com/kubernetes/community/pull/2524/files#diff-61b3bf8a9689fb1a602ce6906cab8c4aR209

[joelanford]

The Helm operator has been integrated into operator-sdk. This issue was solved there by operator-framework/operator-sdk#814