Move LastTransitionTime grooming to reconcile post-process

[whaught]

Expected Behavior

When updating conditions, we want to ensure that LastTransitionTime does not change if a condition is set but otherwise keeps the same value. Currently we use the ConditionManager to do so on each modification of a condition.

Actual Behavior

Some implementers might reset the conditions or switch between conditions within the same reconciliation. On write-back this can cause churn of the LastTransitionTime field which may result in unneeded watch events / resource versions.

Example: https://github.com/knative/serving/blob/baace2803b2ce4ff17078db8410ad884c2a2a6f5/pkg/reconciler/autoscaling/hpa/hpa.go#L64

In the case where this reconciler is Inactive we get here: https://github.com/knative/serving/blob/baace2803b2ce4ff17078db8410ad884c2a2a6f5/pkg/reconciler/autoscaling/hpa/hpa.go#L106

We'll keep flipping from Active to Inactive. The user will just see Inactive with a marching timestamp.

Proposed Solution

In the post-process step for reconciliation, compare the original resource to the written one and preserve the old condition if nothing has changed (exempting timestamps for equality). This will make it so implementers don't need to worry about changing the resource more than once with unintended side-effects.

At the moment its expected that all writes to conditions go through the ConditionManager, but this will also make it so that direct changes to the condition struct won't have unintended side effects either (in practice I don't think people are doing this, but could potentially make future implementations more readable)

[whaught]

/assign

[mattmoor]

Some implementers might reset the conditions or switch between conditions within the same reconciliation. On write-back this can cause churn of the LastTransitionTime field which may result in unneeded watch events / resource versions.

So yes and no. This is why VolatileTime was introduced, so that during Reconcile a condition can flip/flop and so long as the reconciliation returns Status to the same state (except LTTs) then the status update is elided. One of the symptoms without this was the potential for infinite reconciliation where each Reconcile simply "touches" the timestamp.

It is still possible for LTT to move forwards when status is updated, but there MUST be another change to status for it to get written back. Changes to LTT alone should never result in status being written.

We might be talking past each other, so let's plan to either cover this during the API call Weds, or grab some time to sync up out of band to get on the same page.

[whaught]

a condition can flip/flop and so long as the reconciliation returns Status to the same state (except LTTs) then the status update is elided.

I believe that would only be the case if the reconciliation did not modify anything else in status except for the single condition in question.

Scheduled for the API call.