Operator only sets status when the reconciler succeeds. about helm-app-operator-kit HOT 6 CLOSED

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.

from helm-app-operator-kit.

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?

from helm-app-operator-kit.

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"
)

from helm-app-operator-kit.

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

• kubernetes/kubernetes#50798
• 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?

from helm-app-operator-kit.

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

from helm-app-operator-kit.

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

from helm-app-operator-kit.