[cozystack-controller] Fix deleting workloads - #1229
Conversation
WalkthroughThe workload reconciliation logic was refactored to delete workloads lacking a valid Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant K8sAPI
participant WorkloadMonitor
User->>K8sAPI: Create/Update Workload
K8sAPI->>Controller: Trigger Reconcile(Workload)
Controller->>K8sAPI: Get Workload
alt Workload missing monitor label
Controller->>K8sAPI: Delete Workload
else Workload has monitor label
Controller->>K8sAPI: Get WorkloadMonitor by label
alt WorkloadMonitor not found
Controller->>K8sAPI: Delete Workload
else WorkloadMonitor found
Controller-->>K8sAPI: No action
end
end
Estimated code review effort2 (~20 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello Andrei Kvapil (@kvaps), 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 refactors the lifecycle management of Workload custom resources within the system. The primary goal is to establish a more robust and explicit mechanism for deleting Workload objects by adjusting their ownership references for proper Kubernetes garbage collection and introducing a new label-based dependency check. This ensures Workload resources are reliably cleaned up when their associated monitored resources or WorkloadMonitor objects are removed, improving overall resource consistency and stability.
Highlights
- Revised Workload Deletion Logic: The
WorkloadReconcilerhas been updated to simplify and improve the deletion ofWorkloadobjects. It now primarily relies on a new label (workloadmonitor.cozystack.io/name) to identify its associatedWorkloadMonitor. If this label is missing or the referencedWorkloadMonitordoes not exist, theWorkloadobject is automatically deleted. - Updated Workload Ownership and Labeling: The
WorkloadMonitorReconcilernow correctly sets the owner reference ofWorkloadobjects to their actual monitored Kubernetes resources (e.g., Pod, Service, PVC) instead of theWorkloadMonitor. This enables Kubernetes' native garbage collection. Additionally, it ensures thatWorkloadobjects are labeled withworkloadmonitor.cozystack.io/nameto facilitate the new deletion logic in theWorkloadReconciler. - Enhanced Unit Tests: New unit tests have been added to
workload_controller_test.goto thoroughly validate the revisedWorkloaddeletion logic, covering scenarios where the associated monitor is missing and where all dependencies are present.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | Gemini (@gemini-code-assist) | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
c283c66 to
ebe0f2e
Compare
There was a problem hiding this comment.
Code Review
This PR refactors the Workload deletion logic to be dependent on the existence of its corresponding WorkloadMonitor, identified by a new label. This is a good improvement for ensuring workloads are properly garbage collected. The changes include updates to the WorkloadReconciler and the WorkloadMonitorReconciler to manage this new label, along with new tests to verify the behavior.
My review has identified a critical bug in how labels are applied, which would break the intended functionality, and a couple of medium/high-severity issues related to potential side-effects and label handling that could be improved for robustness and correctness.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
ebe0f2e to
46662fe
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
216fef9 to
ee5a724
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
4c80f40 to
da3f233
Compare
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
fixes #1222
Release note
Summary by CodeRabbit
Refactor
Bug Fixes
Tests