genreconciler: successful finalization breaks status updates

/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:

pkg/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go

Lines 242 to 244 in bfab3c8

    
           if resource, err = r.clearFinalizer(ctx, resource, reconcileEvent); err != nil { 
        
           	return fmt.Errorf("failed to clear finalizers: %w", err) 
        
           }

This function uses the input resource (with the updated status) to access the finalizers, find ours and remove it:

pkg/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go

Lines 438 to 441 in bfab3c8

    
           resource.Finalizers = finalizers.List() 
        
           // Synchronize the finalizers filtered by r.finalizerName. 
        
           return r.updateFinalizersFiltered(ctx, resource)

The resource we pass in is used to compute a PATCH on the finalizers specifically:

pkg/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go

Lines 373 to 378 in bfab3c8

    
           mergePatch := map[string]interface{}{ 
        
           	"metadata": map[string]interface{}{ 
        
           		"finalizers":      finalizers, 
        
           		"resourceVersion": existing.ResourceVersion, 
        
           	}, 
        
           }

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:

pkg/client/injection/kube/reconciler/apps/v1beta1/deployment/reconciler.go

Lines 388 to 396 in bfab3c8

    
           updated, err := patcher.Patch(ctx, resourceName, types.MergePatchType, patch, metav1.PatchOptions{}) 
        
           if err != nil { 
        
           	r.Recorder.Eventf(existing, v1.EventTypeWarning, "FinalizerUpdateFailed", 
        
           		"Failed to update finalizers for %q: %v", resourceName, err) 
        
           } else { 
        
           	r.Recorder.Eventf(updated, v1.EventTypeNormal, "FinalizerUpdate", 
        
           		"Updated %q finalizers", resource.GetName()) 
        
           } 
        
           return updated, err

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.

@dprotaso

cc @dprotaso @n3wscott

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.

/lifecycle frozen

Navigation Menu

Repository navigation

`genreconciler`: successful finalization breaks status updates #2519

Expected Behavior

Actual Behavior

The bug

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if resource, err = r.clearFinalizer(ctx, resource, reconcileEvent); err != nil {
	return fmt.Errorf("failed to clear finalizers: %w", err)
	}

	resource.Finalizers = finalizers.List()

	// Synchronize the finalizers filtered by r.finalizerName.
	return r.updateFinalizersFiltered(ctx, resource)

	mergePatch := map[string]interface{}{
	"metadata": map[string]interface{}{
	"finalizers": finalizers,
	"resourceVersion": existing.ResourceVersion,
	},
	}

	updated, err := patcher.Patch(ctx, resourceName, types.MergePatchType, patch, metav1.PatchOptions{})
	if err != nil {
	r.Recorder.Eventf(existing, v1.EventTypeWarning, "FinalizerUpdateFailed",
	"Failed to update finalizers for %q: %v", resourceName, err)
	} else {
	r.Recorder.Eventf(updated, v1.EventTypeNormal, "FinalizerUpdate",
	"Updated %q finalizers", resource.GetName())
	}
	return updated, err

genreconciler: successful finalization breaks status updates #2519

Description

Expected Behavior

Actual Behavior

The bug

Activity

mattmoor commented on May 17, 2022

mattmoor commented on May 17, 2022

github-actions commented on Aug 16, 2022

psschwei commented on Aug 16, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`genreconciler`: successful finalization breaks status updates #2519