Auto set observed generation #1081
Conversation
knative-prow-robot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vaikas 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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
cc @dgerd related: knative/serving#4937 |
|
The reason I did not do this is there is nothing that changes the resource for you in the generated reconcilers so that means you can use this as a read only observer type like my graph program or as a first class reconciler like w use in Knative I don't think we should do with without a way to turn it off |
| @@ -236,6 +236,10 @@ func (r *reconcilerImpl) Reconcile(ctx context.Context, key string) error { | |||
| } | |||
| } | |||
|
|
|||
| // Since the Reconciler observed this object, update the Status.ObservedGeneration | |||
| // to match the objects ObservedGeneration. | |||
| resource.Status.ObservedGeneration = original.Generation | |||
There was a problem hiding this comment.
I don't think that this alone is enough. If ReconcileKind returned an error before updating Status then we might inadvertently convey readiness before we mean to by auto-bumping ObsGen before the conditions are (successfully) updated. I would be supportive of doing this whenever the return code is not an error 🤷♂
There was a problem hiding this comment.
@n3wscott has a good point too, we [will] use this for a couple resources in Serving that don't touch status, and shouldn't update this. I'd be ok with having a way of turning all the status stuff off.
There was a problem hiding this comment.
we might inadvertently convey readiness before we mean to by auto-bumping ObsGen before the conditions are (successfully) updated.
I am not sure I follow this. Both this and the status of conditions are updated in updateStatus. No? This does however mean that reconcilers have to follow the contract of always setting Conditions before returning.
I'd be ok with having a way of turning all the status stuff off.
Yes i agree with this.
|
@n3wscott I think for that case, having a: @mattmoor I don't quite understand in which case you should not still bump the observed generation even in the case of error? Each of the uses does the same two lines at the beginning of their reconcilekind: I'm totes fine for making it optional and not super wedded to it, but given the long history of bugs that have been done here. It's my understanding that ObservedGeneration is meant to convey to the user that a reconciler has taken a look and I don't see how that would convey or be any different from how it's being used in all the reconcilers that are being migrated to the new generic reconciler. Again, not super wedded just something that thought might be useful. |
There is some talk in various K8s issues around multiple observedGenerations for situations when you have multiple controllers working on the same object. Knowing the observed generation is valuable for read-only reconcilers or autoscaling-like reconcilers, but I agree that clobbering the 'primary' controllers observedGeneration is not the right approach. I do not think K8s offers guidance here yet. |
I can't think of a single case where we wouldn't want to do this for our resources and can think of many cases where it's been a source of bugs.
The one reason why we wouldn't want to do this is that if this is reconciling objects that do not expose the Status.ObservedGeneration field however. Since have that in stub however as well, seemed like the intent is that the resources will have these fields, but I can see the stub breaking is less of an issue than the generated one.
If I had enough template fu, maybe we could look and if the type implements this will we do it.
But in any case, just wanted to throw it out there as a thought.