init e2e: add e2e tests of command addons and command deinit #7054
Conversation
karmada-bot
added
the
kind/cleanup
karmada-bot
added
the
size/L
Summary of ChangesHello @zhzhuang-zju, 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 expands the end-to-end testing capabilities for Karmada by integrating tests for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
Thanks~ Hi @mohamedawnallah, would you like to have a review? |
![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 introduces e2e tests for karmadactl addons and karmadactl deinit commands, enhancing the test coverage for Karmada's lifecycle management. The changes are well-structured and logically extend the existing init e2e test suite. I've provided a couple of suggestions to improve the robustness of the test checks and fix some minor comment inconsistencies. Overall, this is a valuable addition to the project.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7054 +/- ##
=======================================
Coverage 46.55% 46.56%
=======================================
Files 700 700
Lines 48084 48084
=======================================
+ Hits 22387 22389 +2
Misses 24013 24013
+ Partials 1684 1682 -2
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:
|
zhzhuang-zju
force-pushed
the
clie2e-new
branch
from
9e8ea0d to
2b96dbc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the init e2e tests to cover karmadactl addons and karmadactl deinit commands, which is a great addition for improving test coverage. The overall implementation is solid. I've made a few suggestions to enhance code quality, including improving a function comment to align with the style guide and refactoring some repetitive test code into loops for better maintainability.
zhzhuang-zju
force-pushed
the
clie2e-new
branch
from
2b96dbc to
1b76288
Compare
Thanks @XiShanYongYe-Chang and @zhzhuang-zju. I put it on my work queue ❤️ Relatedly, I missed some feedback from @zhzhuang-zju on that relevant PR #5446 (sorry for that). I will try to address it this week |
mohamedawnallah
left a comment
mohamedawnallah
left a comment
There was a problem hiding this comment.
Love the separation of concerns done in this PR. That would be handy in addons lifecycle management
Before
graph TD
A1[karmadactl init] --> B1[karmadactl join/unregister]
B1 --> C1[Simple propagation]
C1 --> D1[karmadactl unjoin/unregister]
style A1 fill:#e1f5ff
After
graph TD
A2[karmadactl init] --> B2[karmadactl join/unregister]
B2 --> C2[karmadactl addons enable]
C2 --> D2[Simple propagation]
D2 --> E2[karmadactl addons disable]
E2 --> F2[karmadactl unjoin/unregister]
F2 --> G2[karmadactl deinit]
style A2 fill:#e1f5ff
style C2 fill:#fff4e1
style E2 fill:#fff4e1
style G2 fill:#ffe1e1
| }) | ||
| }) | ||
|
|
||
| ginkgo.By("Disable descheduler, metrics-adapter, scheduler-estimator and search by command addons", func() { |
There was a problem hiding this comment.
For my education: is now the new Karmada workflow (apart from tests) requiring the user to disable addons before doing unjoining/unregistering? So the user supposed to get something along those lines if they do unregister operation without disabling addons (please disable addons first before unregistering) or this only limited to tests?
There was a problem hiding this comment.
@mohamedawnallah Good question! In practice, there's no strict requirement on the order between running addons disable and performing unjoin/unregister. However, the reason for this test ordering is that the scheduler-estimator addons—are tightly coupled with member clusters on a one-to-one basis. Disabling the addon first helps avoid potential issues with the scheduler-estimator’s functionality if the cluster is unjoined or unregistered prematurely.
|
Hi @mohamedawnallah, do you think the current PR can be merged now? If you agree, you can give an LGTM. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mohamedawnallah The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thx for your review @mohamedawnallah. Btw, nice diagram~ |
| ginkgo.DeferCleanup(func() { | ||
| // Unjoin the push mode cluster | ||
| cmd := framework.NewKarmadactlCommand(karmadaConfigFilePath, "", karmadactlPath, "", karmadactlTimeout, | ||
| "unjoin", "--cluster-kubeconfig", pushModeKubeConfigPath, "--cluster-context", pushModeClusterName, "--cluster-namespace", "karmada-cluster", | ||
| "--v", "4", pushModeClusterName) | ||
| _, err := cmd.ExecOrDie() | ||
| gomega.Expect(err).ShouldNot(gomega.HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
I have a question I'd like to understand: If an error occurs in a subsequent step before executing the final unjoin command, will there be any residual resources?
There was a problem hiding this comment.
Nice catch! Now that Defer is no longer in use, there is no assurance that the cluster can complete the unjoin process. The rationale behind this change is that the E2E test concludes with a Karmada instance cleanup. Using Defer would lead to unjoin failure, given that the Karmada instance would already be gone by that stage. Any suggestions on this?
There was a problem hiding this comment.
If that is the case, we can make the DeferCleanup as clear as possible. If an error occurs, we should not attempt to judge the error but simply print a warning log. Similarly, the deletion of Karmada instances can be handled in the same way.
There was a problem hiding this comment.
Thx, I can give it a try
There was a problem hiding this comment.
@XiShanYongYe-Chang I placed karmadactl unjoin/unregister/deinit all within defercleanup to best ensure resource cleanup.
Signed-off-by: zhzhuang-zju <[email protected]>
zhzhuang-zju
force-pushed
the
clie2e-new
branch
from
1b76288 to
d029879
Compare
5 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
karmadactl addonsis used to install components such askarmada-deschedulerandkarmada-scheduler-estimatoron a Karmada instance.karmadactl deinitis used to delete an installed Karmada instance.To better maintain related functionalities, tests for
karmadactl addonsandkarmadactl deinitare added to the currentinite2e test module.The previous
base_testworkflow was:karmadactl init→karmadactl join/unregister→ a simple propagation →karmadactl unjoin/unregister.The updated
base_testworkflow is now:karmadactl init→karmadactl join/unregister→karmadactl addons enable→ a simple propagation →karmadactl addons disable→karmadactl unjoin/unregister→karmadactl deinit.Which issue(s) this PR fixes:
Fixes #4544
Special notes for your reviewer:
Does this PR introduce a user-facing change?: