kstatus/kpt integration: Ready vs Reconciled/Stalled conditions

[seaneagan]

Currently the toolkit controllers provide a Ready condition. While there is support for this in kstatus/kpt, there are some caveats mentioned:

If a resource doesn't set the Ready condition until it is True, the library have no way of telling whether the resource is using the Ready condition, so it will fall back to the strategy for unknown resources, which is to assume they are always reconciled.
If the library sees the resource before the controller has had a chance to update the conditions, it also will not realize the resource use the Ready condition.

These seem already handled (only checked helm-controller), since the ready condition is set to unknown before setting the observed generation, and then set to true/false simultaneously with setting the observed generation

There is no way to determine if a resource with the Ready condition set to False is making progress or is doomed.

This is a fundamental limitation of the Ready condition, so the controllers will have no way of signalling fatal / non-recoverable errors, so a client e.g. kpt cannot determine this, and must continue to wait until whatever timeout it has. Using the recommended Stalled/Reconciled conditions would resolve this. If a controller supports retries (at least helm-controller does) it should be marked Stalled once retries are exhausted.

Whatever the kstatus/kpt solution is, it would be nice to have it documented, and potentially gated against.

[seaneagan]

Whichever conditions are used, it would be useful to have as much information as possible inside of these condition(s) so it can be surfaced to the end user by kstatus/kpt. For example with the helm-controller, when an install / upgrade / non-ignored test fails the Reason/Message i.e. InstallFailed / UpgradeFailed / TestFailed would ideally be propogated to the Reconciled/Stalled/Ready condition, whereas currently a generic ReconciliationFailed / "reconciliation of %s failed" is used.

[stefanprodan]

We could do that, but we need to trim the errors after a max length. In Kustomize controller I've seen reconciliation errors that could potentially hit the etcd limit, one may have thousandths of manifests and a single typo will yell an error that contains all the manifesta in a single blog of text.

[stefanprodan]

The convention we follow now is what the Kubernetes builtin controllers do. If Stalled/Reconciled makes it into a Kubernetes KEP and becomes a standard we will adopt it.

Currently we set the ready condition like so:

• Read=Unknown signals that a reconciliation is underway
• Ready=True signals that a reconciliation has finished successfully
• Ready=False signals that a reconciliation has finished with errors

This allows users of kubectl and tk to wait on the Ready condition in a reliable way with a single command.

Should we consider adding Stalled/Reconciled and keep Ready just to play nice with kpt?

[seaneagan]

If Stalled/Reconciled makes it into a Kubernetes KEP and becomes a standard we will adopt it.

I think the approach they are taking is more democratized by providing common tooling (sig-cliutils kstatus library), and then perhaps if these conditions are well accepted by the community, that would induce a KEP, so it will take some early adopters first. Btw, I mistyped, it's actually Reconciling/Stalled.

Currently we set the ready condition like so:

* `Read=Unknown` signals that a  reconciliation is underway

* `Ready=True` signals that a reconciliation has finished successfully

* `Ready=False` signals that a reconciliation has finished with errors

This is very economical, one condition instead of two for all 3 states. However, kstatus/kpt interpret Ready=False as simply "not (yet) ready" (which makes sense grammatically) rather than "finished with errors" as I assume they determined this is how many existing controllers (such as kubernetes builtin ones) have implemented it.

This allows users of kubectl and tk to wait on the Ready condition in a reliable way with a single command.

Should point out, one thing that kubectl wait doesn't handle (that kstatus does) is status.observedGeneration checking, which is necessary to know that you are checking the status of the latest applied state vs an old one.

Should we consider adding Stalled/Reconciled and keep Ready just to play nice with kpt?

That may get confusing, probably should pick one or the other. They both work with kstatus/kpt, the former just provides them (and their end user) a bit more information they can consume. Particularly the early exit when say a chart rendering fails vs waiting the entire configured timeout for the release is attractive.

[hiddeco]

That may get confusing, probably should pick one or the other. They both work with kstatus/kpt, the former just provides them (and their end user) a bit more information they can consume. Particularly the early exit when say a chart rendering fails vs waiting the entire configured timeout for the release is attractive.

Before kstatus has been integrated into kubectl, I feel very little for supporting a library / 3rd party tool over the official kubectl wait

[seaneagan]

While on the topic of conditions, the draft conventions mention:

• Condition type names should describe the current observed state of the
  resource, rather than describing the current state transitions. This
  typically means that the name should be an adjective ("Ready", "OutOfDisk")
  or a past-tense verb ("Succeeded", "Failed") rather than a present-tense verb
  ("Deploying"). Intermediate states may be indicated by setting the status of
  the condition to Unknown.

The helm-controller currently uses: Install, Upgrade, Uninstall, Rollback, Test conditions. To match the above it would need to be something like: Installed, Upgraded, Uninstalled, Rolledback, Tested.