knative / knative/serving

Consistent and Correct Conditions

Open
#5,076 22 comments 4 reactions 1 assignee Claimed by @dprotaso View on GitHub
area/API kind/cleanup lifecycle/frozen triage/accepted
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:
![image](https://user-images.githubusercontent.com/17863526/62574616-c21a3200-b84d-11e9-9174-b56539082d81.png)

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)):
![image](https://user-images.githubusercontent.com/17863526/62574711-f4c42a80-b84d-11e9-9c45-4ba903bf3075.png)

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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.