Enhance cluster deletion logic and index registration #6897
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @Chen-speculation! It looks like this is your first PR to karmada-io/karmada 🎉 |
Summary of ChangesHello @Chen-speculation, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the stability of cluster deletion operations and the availability of crucial data indexes. By relocating index registration to an earlier stage, it ensures that necessary indexes are always ready for use. Furthermore, it introduces a resilient fallback for cluster deletion, allowing the process to complete successfully even if index queries encounter issues, thereby preventing potential deadlocks or failures during cluster lifecycle management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
karmada-bot
added
the
size/M
Chen-speculation
force-pushed
the
bugfix/fix-cluster-deletion-robustness
branch
from
1ada922 to
fb99d2a
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the cluster deletion logic by adding a fallback mechanism for when index queries fail. It also moves the index registration to an earlier stage in the controller manager setup, ensuring indexes are available before dependent controllers start. The changes are logical and enhance system reliability. My main feedback is regarding code duplication in the new fallback logic within pkg/controllers/cluster/cluster_controller.go. The logic for checking ResourceBinding and ClusterResourceBinding is nearly identical. I've suggested simplifying the implementation and pointed out the duplication, which could be refactored to improve maintainability.
- Move index registration to setupControllers for early availability - Add fallback mechanism in cluster deletion when index queries fail - Improve system robustness and reliability Signed-off-by: 止鹜 <[email protected]>
Chen-speculation
force-pushed
the
bugfix/fix-cluster-deletion-robustness
branch
from
fb99d2a to
eb22eac
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6897 +/- ##
==========================================
- Coverage 46.34% 46.34% -0.01%
==========================================
Files 697 697
Lines 47629 47643 +14
==========================================
+ Hits 22074 22078 +4
- Misses 23889 23900 +11
+ Partials 1666 1665 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@zach593 Can you help take a look? |
| // Register indexes that are used by multiple controllers to ensure they are available | ||
| // before any controller that depends on them starts. | ||
| // These indexes help cluster-controller and TaintManager quickly locate ResourceBinding | ||
| // and ClusterResourceBinding resources associated with a given cluster. | ||
| if err := indexregistry.RegisterResourceBindingIndexByFieldCluster(ctx, mgr); err != nil { | ||
| klog.Fatalf("Failed to register ResourceBinding index: %v", err) | ||
| } | ||
| if err := indexregistry.RegisterClusterResourceBindingIndexByFieldCluster(ctx, mgr); err != nil { | ||
| klog.Fatalf("Failed to register ClusterResourceBinding index: %v", err) | ||
| } |
There was a problem hiding this comment.
@Chen-speculation, I don't get why moving the index registration earlier. Can you remind me?
|
Retrying after the error would certainly resolve the symptom, but we still need to understand why the error happens in the first place before adding this fix. I’ll try to reproduce the issue described in #6886 when I have time. |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #6886
Special notes for your reviewer:
Does this PR introduce a user-facing change?: