ObservedGeneration should only be set if Status is fully updated

[grantr]

Expected Behavior

Followup from this thread in Slack #serving-api

If ObservedGeneration == Generation, I expect that the object's Status reflects the actual status of the resource.

Actual Behavior

If the reconcile is written to return errors early, then the Status may not be fully accurate.

Example from @mattmoor:

foo.Status.ObsGen = foo.Gen
if bar, err := reconcileFoo(); err != nil {
  // What happens here?
  return err
} if bar.Something() {
  bar.Status.MarkSomething()
}
foo.Status.MarkReady()
return nil

If the reconciler returns on line 4, then the statuses of the Something and Ready conditions actually reflect the previous generation's status, not the current generation.

Additional Info

One solution mentioned in the thread is to re-initialize status to fully unknown at the beginning of every reconcile so that no information is carried over from the previous generation. This works for non-timestamped status fields like address, but is suboptimal for conditions as it would make lastTransitionTime inaccurate.

Instead of replacing each condition, we could mark existing conditions before reconcile and unmark them when updated during reconcile. After the reconcile, any conditions that are still marked are set to unknown. This preserves the functionality of lastTransitionTime.

@dprotaso @markusthoemmes @mattmoor @vaikas

[mattmoor]

One solution mentioned in the thread is to re-initialize status to fully unknown at the beginning of every reconcile so that no information is carried over from the previous generation. This works for non-timestamped status fields like address, but is suboptimal for conditions as it would make lastTransitionTime inaccurate.

This isn't entirely accurate. The proposal in the thread was scoped to conditions, and lastTransitionTime's use of apis.VolatileTime combined with how we guard status updates is specifically designed to avoid timestamp churn.

A while back I actually tried to do this and uncovered a few places in Serving that have a pretty hard dependency on the imperative use of Status as state, so I gave up, but I like the suggestion @vaikas made that we not have a bad default behavior because of a few bad apples.

To avoid back-compat issues, I'd propose that we introduce ResetConditions() to force all of a resources conditions to Unknown, and shift all of our controllers (that can) over to this model.

[whaught]

I tried to take a stab at this issue with PR #1261

The idea here is that rather than reset all conditions, we include logic in gen-reconciler to bump observed generation. If we've bumped generation with an error and detect that the readiness status is unchanged, we move it to unknown with a warning.

This avoids the need to reset all conditions and might be a little more easily backwards compatible. It's using a new tag for generation so we can migrate reconcilers to it one-by-one to ensure it functions properly.

[whaught]

/assign