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

🐛 Apply finalizer on rancher cluster to avoid re-create race condition #597

Merged

Conversation

Danil-Grigorev
Copy link
Contributor

What this PR does / why we need it:

As we are unable to set ownership references from CAPI cluster to a cluster scoped management v3 cluster, we have to manage deletion manually. This comes with a problem of tracking deletion order for rancher/capi cluster, and setting imported annotation to prevent re-import scenario.

There is a race condition if removal of management v3 cluster happens too fast - turtles misses the opportunity to set imported annotation first and causes immediate re-import of the cluster. To prevent such scenario, we need to set capicluster.turtles.cattle.io to the management v3 resource we created at all times, and clean it up within the imported annotation handling scope on deletion.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #589

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@Danil-Grigorev Danil-Grigorev added the kind/bug Something isn't working label Jul 10, 2024
@Danil-Grigorev Danil-Grigorev requested a review from a team as a code owner July 10, 2024 15:47
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
@Danil-Grigorev Danil-Grigorev force-pushed the cluster-recreate-after-delete-race branch from 7784884 to a815fa5 Compare July 10, 2024 16:47
Copy link
Member

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

the change makes sense, thank for catching it

Copy link
Contributor

@salasberryfin salasberryfin left a comment

Choose a reason for hiding this comment

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

Thanks @Danil-Grigorev. Just nit-picking a bit on a log message but LGTM.

internal/controllers/import_controller_v3.go Show resolved Hide resolved
@Danil-Grigorev Danil-Grigorev merged commit 8b39e86 into rancher:main Jul 11, 2024
9 checks passed
@Danil-Grigorev Danil-Grigorev mentioned this pull request Jul 16, 2024
4 tasks
salasberryfin pushed a commit to salasberryfin/rancher-turtles that referenced this pull request Jul 18, 2024
salasberryfin pushed a commit to salasberryfin/rancher-turtles that referenced this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cluster getting re-imported after removal from Rancher
3 participants