Bump observedGeneration first

[dgerd]

Move observedGeneration bump to the top of reconcile()

Fixes #4937

[knative-prow-robot]

@dgerd: 0 warnings.

In response to this:

Move observedGeneration bump to the top of reconcile()

Fixes #4937

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/reconciler/OWNERS [dgerd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/route/route.go	81.2%	81.1%	-0.1

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/activator/net.TestMultipleActivators

[dgerd]

/test pull-knative-serving-unit-tests

[dprotaso]

/lgtm

Probably worth noting in some 'best-practice' location that we should always set the observed generation prior to any operation that can cause the reconciliation to 'fail'

[dgerd]

/hold

I am going to hold this for the next release since the release is so close.

I have looked through the code that depends on observedGeneration and don't see any potential problems moving this. Unit and integration tests pass. I also played around creating and updating services with Kn without problem.

@dprotaso My plan is to start with these controllers (Service, Configuration, Route, and Revision) and then will do the same with SKS, PA, etc. Once we have serving and eventing all doing it at the beginning it probably makes sense to make this part of the framework in pkg so people don't have to worry about observedGeneration at all.

/cc @mattmoor

[mattmoor]

Thanks for holding. I'm not sure this is completely correct either... What I'm imagining is a flow like:

InitConditions()
SetObservedGeneration()
if err := SomethingThatCouldFail(); err != nil {
   return err
}
// More

When the error-path is taken, suppose our previous state was Ready: True and that hasn't been downgraded to reflect what should maybe be Ready: Unknown due the the error (maybe that happens in // More?).

To offer an alternative: would it be bad if we didn't update ObservedGeneration when Reconcile returns an error? What if we changed out boilerplate to:

	config := original.DeepCopy()
	reconcileErr := c.reconcile(ctx, config)

	// If the reconcile succeeded, then we always update observed generation.
	if reconcileErr == nil {
		config.Status.ObservedGeneration = original.Generation
	}

	// Same from here on.
	if equality.Semantic.DeepEqual(original.Status, config.Status) {

[taragu]

Should we apply the same changes to ingress and sks?

[dgerd]

When the error-path is taken, suppose our previous state was Ready: True and that hasn't been downgraded to reflect what should maybe be Ready: Unknown due the the error

I explored this in #5076 (comment) and hope to discuss the findings more in the next API working group. We currently have multiple places where we exit the loop early in a terminal state, but do not update status. I was hoping to fix that in a subsequent change, but thinking about this further you are probably right that these concepts are to coupled to commit separately.

Rather than holding back observedGeneration and keeping FooReady: True we should be setting FooReady to False and bumping observedGeneration.

would it be bad if we didn't update ObservedGeneration when Reconcile returns an error?

Yes I think this would be bad. My understanding of observedGeneration is to determine if the controller has seen the updated -- it is not meant to notate that that updated spec has successfully reconciled. If we update it only when reconcile succeeds we (1) will have problems with our Service's getting stuck in "Unknown" due to mismatched generations and (2) we will be missing data that helps separate failures in a reconciler seeing an update from failures to reach the desired state.

@taragu Yes, my goal is for all reconcilers in Knative to follow the same pattern. I started this change as a bit of an experiment, and I do want to break up changing this into smaller bits to reduce risk. See previous comment on the PR.