knative / knative/pkg

`genreconciler`: successful finalization breaks status updates

Open
#2,519 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

area/API kind/bug lifecycle/frozen
Dominant language
Go
Stars
276
Forks
344
Avg merge
1d 1h
Merged PRs (30d)
1

Description

/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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go at the finalization and patch paths cited in the issue. Trace how the updated resource is used when removing the finalizer, then verify with unit coverage that a successful FinalizeKind persists both the finalizer removal and the Ready status update.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.