Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle finalizers (creation and deletion) better #409

Closed
anik120 opened this issue Sep 19, 2024 · 1 comment · Fixed by #411
Closed

Handle finalizers (creation and deletion) better #409

anik120 opened this issue Sep 19, 2024 · 1 comment · Fixed by #411
Assignees
Labels
v1.0 Issues related to the initial stable release of OLMv1
Milestone

Comments

@anik120
Copy link
Collaborator

anik120 commented Sep 19, 2024

Deleting the finalizer and updating the status (introduced in #379) in the same reconciliation loop is never actually executed in the same reconciliation loop because of the way Reconcile is setup.

// Update the status subresource before updating the main object. This is
// necessary because, in many cases, the main object update will remove the
// finalizer, which will cause the core Kubernetes deletion logic to
// complete. Therefore, we need to make the status update prior to the main
// object update to ensure that the status update can be processed before
// a potential deletion.
if !equality.Semantic.DeepEqual(existingCatsrc.Status, reconciledCatsrc.Status) {
        if updateErr := r.Client.Status().Update(ctx, reconciledCatsrc); updateErr != nil {
	        return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr})
        }
}
existingCatsrc.Status, reconciledCatsrc.Status = v1alpha1.ClusterCatalogStatus{}, v1alpha1.ClusterCatalogStatus{}
if !equality.Semantic.DeepEqual(existingCatsrc, reconciledCatsrc) {
        if updateErr := r.Client.Update(ctx, reconciledCatsrc); updateErr != nil {
	        return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr})
        }
}

When the status and the main body (eg metadata.finalizer) is updated in the same reconcile loop, the change request to the metadata.finalizer is lost when the status is updated first. This requires another loop to then actually delete the finalizer.

See this for a better way to handle updating the status and the body in the same reconciliation loop.

anik120 added a commit to anik120/catalogd that referenced this issue Sep 19, 2024
anik120 added a commit to anik120/catalogd that referenced this issue Sep 19, 2024
@LalatenduMohanty LalatenduMohanty added the v1.0 Issues related to the initial stable release of OLMv1 label Sep 19, 2024
@LalatenduMohanty LalatenduMohanty added this to the v1.0.0 milestone Sep 19, 2024
@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Sep 19, 2024

#411 is fixing this.

anik120 added a commit to anik120/catalogd that referenced this issue Sep 19, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 19, 2024
* 🐛 Handle finalizers (creation and deletion) better

Closes [409](#409)

* Move the finalizer setup

Move the finalizer setup out from main.go, and into the controller
code. This allows the finalizers to be initialized and used in the
test code.

Signed-off-by: Todd Short <[email protected]>

* Undo rename of fbcDeletionFinalizer

Signed-off-by: Todd Short <[email protected]>

---------

Signed-off-by: Todd Short <[email protected]>
Co-authored-by: Todd Short <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.0 Issues related to the initial stable release of OLMv1
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants