pkg icon indicating copy to clipboard operation
pkg copied to clipboard

`genreconciler`: successful finalization breaks status updates

Open mattmoor opened this issue 3 years ago • 4 comments

/area API /kind bug

This is a largely superficial issue, which I only noticed because of some unit testing I was writing and trying to explain this odd behavior.

Expected Behavior

If in FinalizeKind a resource is marked Ready, and nil is returned, then we see both updates (path for finalizer, update status for Ready).

Actual Behavior

Instead the finalizer is removed and the status is NOT updated.

The bug

Using Deployment as an example... If in FinalizeKind we update status and return nil then we will call through here: https://github.com/knative/pkg/blob/bfab3c8fc5a0a3f725aa0e3478631384ec97d43c/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go#L242-L244

This function uses the input resource (with the updated status) to access the finalizers, find ours and remove it: https://github.com/knative/pkg/blob/bfab3c8fc5a0a3f725aa0e3478631384ec97d43c/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go#L438-L441

The resource we pass in is used to compute a PATCH on the finalizers specifically: https://github.com/knative/pkg/blob/bfab3c8fc5a0a3f725aa0e3478631384ec97d43c/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go#L373-L378

However, any other mutations (e.g. our status update) are ignored here. This is a problem because the resource that is returned by this function is the updated copy that is returned by the PATCH which does not have our status update reflected: https://github.com/knative/pkg/blob/bfab3c8fc5a0a3f725aa0e3478631384ec97d43c/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go#L388-L396

mattmoor avatar May 17 '22 20:05 mattmoor

I think the best "fix" for this would be to not return updated directly, but to reflect its resourceVersion change back to resource and return that so the status is preserved.

mattmoor avatar May 17 '22 20:05 mattmoor

cc @dprotaso @n3wscott

mattmoor avatar May 17 '22 20:05 mattmoor

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Aug 16 '22 01:08 github-actions[bot]

/lifecycle frozen

psschwei avatar Aug 16 '22 12:08 psschwei