Consistent and Correct Conditions
- Dominant language
- Go
- Stars
- 6.1k
- Forks
- 1.2k
- Avg merge
- 2d 7h
- Merged PRs (30d)
- 2
Description
Not all of our conditions follow [our condition conventions](https://github.com/knative/serving/blob/master/docs/spec/errors.md#conditions).
We also don't propagate conditions in a consistent (or even correct) way: https://github.com/knative/serving/issues/4937
I'm opening this to track some work around wrangling our conditions.
1. Resources that reconcile child resources should have a condition for each child that reflects the child's top-level "happy state" (TLHS) condition.
1. This makes it easy to propagate conditions from our children.
1. A resource's own TLHS condition should depend on those child conditions via ConditionSets.
1. This makes it easy to propagate conditions to our parent.
1. We should *always* update a resource's observed generation to indicate that we've attempted to reconcile its spec.
1. This allows our parent to know if our status is up to date.
1. If the observed generation of a child resource does not match its current generation, we should mark it as NotReconciled since its state does not yet reflect its spec yet.
1. This ensures we don’t reflect stale child statuses.
1. This might not make sense in some cases (e.g. a Revision reconciling a Deployment would flip back and forth between Ready and Unknown, given that the KPA will be constantly bumping the Deployment's Generation).
1. If there is an non-terminal error during reconciliation, Ready should become unknown.
1. This ensures we aren’t prematurely Ready and that we aren’t swallowing errors.
1. If the observed generation does match, and we did not encounter an error, we should propagate the child’s TLHS condition into our own.
1. This is the happy path.
In general, everything should look something like this:

Here's (a bit simplified) [Service](https://github.com/knative/serving/blob/d38697e7c16a95ec11d56e00d403e40ee179f306/pkg/reconciler/service/service.go#L66) as an example. Service is pretty close to correct, but it's not unconditionally bumping ObservedGeneration or updating its status with reconciliation errors (bolded things aren't correct (yet)):

We'll also need to rework the Revision and KPA and SKS conditions to fit better into this world, since they're all over the place.
### TODO
- [ ] Consistently update `ObservedGeneration`
- [ ] Surface reconciliation errors in conditions.
- [ ] Modify conditions to be in terms of child resources.
- [ ] Consistently propagate child conditions.
- [ ] Attempt to refactor/code-gen this stuff so we don't regress.
- [ ] Update [sample-controller](https://github.com/knative/sample-controller) with parent->child best practices.
/area API
Contributor guide
Assessment
This issue has not been assessed yet.