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

chore: unify finalizers #995

Merged
merged 6 commits into from
Feb 3, 2025
Merged

Conversation

tmilos77
Copy link
Contributor

@tmilos77 tmilos77 commented Feb 1, 2025

Description

Changes proposed in this pull request:

  • add new common finalizer for all CloudManager resources cloud-manager.kyma-project.io/deletion-hook
    • refactor all places old two finalizers were called and use a new one instead
  • modify common actions AddFinalizer() and RemoveFinalizer() to use new finalizer
    • keep removing old finalizers as well, since migration can not migrate resources being deleted
    • modify RemoveFinalizer() not to return StopAndForget
    • modify each reconciler flow that calls RemoveFinalizer() and add StopAndForget after it
  • add migration func, not an action, since beside CloudManager resources being reconciled there are resources that need finalizer migration as ConfigMap, Secret, PV, PVC, Kyma...
  • call the migration func
    • for KCP when CloudManager is started
    • for SKR when CloudManager connects to SKR
  • record successful migration outcome so it doesn't have to be executed repeatedly due to performance
    • for KCP with a ConfigMap cloud-manager-finalizer-migration
    • for SKR with a KCP Kyma annotation cloud-manager.kyma-project.io/finalizer-migration: "true"

Related issue(s)

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2025
@CLAassistant
Copy link

CLAassistant commented Feb 1, 2025

CLA assistant check
All committers have signed the CLA.

@kyma-bot kyma-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 1, 2025
@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 3, 2025
@tmilos77 tmilos77 marked this pull request as ready for review February 3, 2025 09:46
@tmilos77 tmilos77 requested a review from a team as a code owner February 3, 2025 09:46
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2025
ijovovic
ijovovic previously approved these changes Feb 3, 2025
Copy link
Contributor

@ijovovic ijovovic left a comment

Choose a reason for hiding this comment

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

lgtm

@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 3, 2025
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Feb 3, 2025
@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 3, 2025
@tmilos77 tmilos77 enabled auto-merge (squash) February 3, 2025 11:46
@tmilos77 tmilos77 merged commit 0c65d0d into kyma-project:main Feb 3, 2025
11 of 13 checks passed
@tmilos77 tmilos77 deleted the unify-finalizers branch February 3, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants