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

Issue #410: fix reconcile if foreground cascading deletion is used #530

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lipov3cz3k
Copy link

Change log description

Fix unexpected resource creation when the ZookeeperCluster is already deleted

Purpose of the change

Fixes #410

What the code does

Exit reconcile when resource has finalizer and is marked for delete. That's prevent to apply other reconcile functions to create already deleted resources.

How to verify it

Delete zookeepercluster resource in foreground mode -> without this patch it goes to loop (you can fix it by deleting zookeeper in default background mode)

kubectl delete zookeeperclusters.zookeeper.pravega.io zookeeper  --cascade=foreground

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (2c8bfec) 85.91% compared to head (d383afb) 85.81%.

Files Patch % Lines
controllers/zookeepercluster_controller.go 69.23% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
- Coverage   85.91%   85.81%   -0.10%     
==========================================
  Files          12       12              
  Lines        1633     1636       +3     
==========================================
+ Hits         1403     1404       +1     
  Misses        145      145              
- Partials       85       87       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janhoy
Copy link

janhoy commented Feb 28, 2023

I also experienced this multiple times. Have to explicitly use non-cascading delete for it to work.
Can someone please review and approve this?

@janhoy
Copy link

janhoy commented Mar 7, 2023

@lipov3cz3k can you perhaps fix the two failed tests "DCO" and "codecov/patch", so it is more likely that the maintainers can approve and merge this?

@lipov3cz3k lipov3cz3k force-pushed the issue-410-fix-reconcile branch from edc3bef to 2c0927d Compare March 7, 2023 12:18
@anishakj
Copy link
Contributor

anishakj commented Mar 9, 2023

@lipov3cz3k Could you please increase the code coverage?

@anishakj anishakj requested a review from jkhalack March 9, 2023 16:13
@@ -100,8 +100,11 @@ func (r *ZookeeperClusterReconciler) Reconcile(_ context.Context, request ctrl.R
}
return reconcile.Result{Requeue: true}, nil
}
if finalized, err := r.reconcileFinalizers(instance); err != nil || finalized {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be much cleaner if we returned an error when there is a need to do another attempt to run a reconcile loop.

}
}
}
return nil
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be cleaner to return some kind of error at this point instead of introducing a boolean.

@lipov3cz3k lipov3cz3k force-pushed the issue-410-fix-reconcile branch from a2bbf41 to 84bb982 Compare April 3, 2023 12:14
Signed-off-by: Lipovsky, Tomas <[email protected]>
@lipov3cz3k lipov3cz3k force-pushed the issue-410-fix-reconcile branch from 84bb982 to ea2812b Compare April 3, 2023 12:15
@anishakj anishakj requested a review from jkhalack December 14, 2023 12:14
@anishakj
Copy link
Contributor

@lipov3cz3k Is it possible to increase the coverage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unexpected resource creation when the ZookeeperCluster is already deleted
4 participants