gc: consider whether the sts pod is alive during lsp gc #5122
Conversation
Signed-off-by: zhangzujian <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @zhangzujian, 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!
Summary of Changes
This pull request addresses a bug in the garbage collection (GC) logic for logical switch ports (LSPs) associated with StatefulSet pods. The changes ensure that the GC process correctly identifies and handles LSPs of StatefulSet pods, especially when the StatefulSet is being deleted, recreated, or scaled down. The key change is the introduction of a new function isStatefulSetPodToGC to determine if a StatefulSet pod's LSP should be garbage collected, taking into account the pod's liveness and the StatefulSet's state.
Highlights
- GC Logic Enhancement: The pull request introduces
isStatefulSetPodToGCfunction to accurately determine if a StatefulSet pod's LSP should be garbage collected. - StatefulSet Handling: The changes consider scenarios where the StatefulSet is being deleted, recreated, or scaled down, ensuring proper LSP garbage collection.
- Pod Liveness Check: The pull request incorporates a check to determine if a pod is alive before garbage collecting its LSP, preventing premature deletion in scaled-up scenarios.
Changelog
Click here to see the changelog
- pkg/controller/gc.go
- Modified the
markAndCleanLSPfunction to useisStatefulSetPodToGCinstead ofisStatefulSetPodToDelto determine if a StatefulSet pod's LSP should be garbage collected. - Line 334: Changed the function call from
isStatefulSetPodToDeltoisStatefulSetPodToGC.
- Modified the
- pkg/controller/pod.go
- Modified
handleDeletePodfunction to improve logging and ensure correct handling of StatefulSet pod deletions. - Line 928: Changed the condition to check if
pod.DeletionTimestampis non-zero using!pod.DeletionTimestamp.IsZero(). - Line 929: Updated log message to use
podKeyinstead ofpodName. - Line 952: Updated log message to use
podKeyinstead ofpod.Name. - Line 963: Updated log message to use
podKeyinstead ofpodName. - Line 985: Updated log message to include
podKeyfor better identification. - Line 990: Updated log message to use
podKeyinstead ofpod.Name. - Modified
isStatefulSetPodToDelfunction to improve logging and handle cases where the StatefulSet is being deleted or recreated. - Line 1276: Updated log message to include namespace.
- Line 1279: Updated log message to include namespace.
- Line 1285: Updated log message to include namespace.
- Line 1289: Updated log message to include namespace.
- Modified
isStatefulSetPodToDelfunction to return true when the statefulset is downscaled. - Line 1303: Updated log message to include namespace.
- Added a new function
isStatefulSetPodToGCto determine if a StatefulSet pod's LSP should be garbage collected based on the StatefulSet's state and the pod's liveness. - Lines 1313-1368: Added the
isStatefulSetPodToGCfunction with detailed logic for garbage collection decisions.
- Modified
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-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-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.
A StatefulSet's pod,
Lives, dies, scales, a complex node,
GC must be wise.
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. ↩
There was a problem hiding this comment.
Code Review
The pull request focuses on improving the garbage collection (GC) process for logical switch ports (LSPs) associated with StatefulSets in Kube-OVN. The changes aim to ensure that LSPs are only garbage collected when the corresponding StatefulSet pod is no longer considered 'alive' or required. The changes include renaming a function, updating log messages for clarity, and introducing a new function isStatefulSetPodToGC to determine if an LSP should be garbage collected. Overall, the changes seem reasonable and contribute to a more robust and accurate GC process.
Summary of Findings
- Function Naming: The function
isStatefulSetPodToDelis renamed toisStatefulSetPodToGCto better reflect its purpose. This change improves code clarity and maintainability. - Logging Improvements: Log messages are updated to include the namespace of the StatefulSet, providing more context for debugging and monitoring.
- GC Logic Enhancement: The introduction of
isStatefulSetPodToGCfunction enhances the GC logic by considering various scenarios such as StatefulSet deletion, recreation, and downscaling, ensuring that LSPs are only GCed when appropriate.
Merge Readiness
The pull request appears to be well-structured and addresses an important aspect of Kube-OVN's GC process. However, there are a few issues that should be addressed before merging. Specifically, the logging statements should be reviewed for consistency and clarity. Additionally, the logic within isStatefulSetPodToGC should be carefully examined to ensure it covers all relevant edge cases. I am unable to approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issues should be addressed before merging.
| klog.Infof("statefulset %s/%s is down scaled", pod.Namespace, statefulSetName) | ||
| if !isPodAlive(pod) { | ||
| // we must check whether the pod is alive because we have to consider the following case: | ||
| // 1. the statefulset is down scaled to zero | ||
| // 2. the lsp gc is triggered | ||
| // 3. gc interval, e.g. 90s, is passed and the second gc is triggered | ||
| // 4. the sts is up scaled to the original replicas | ||
| // 5. the pod is still running and it will not be recreated | ||
| return true |
There was a problem hiding this comment.
This section checks if the StatefulSet is downscaled and the pod is not alive. The comment explains a specific case where the pod might be running even after downscaling. Ensure that this logic is thoroughly tested and covers all possible scenarios. Consider adding a test case that specifically covers the scenario described in the comment.
if index >= startOrdinal+int64(*sts.Spec.Replicas) {
klog.Infof("statefulset %s/%s is down scaled", pod.Namespace, statefulSetName)
if !isPodAlive(pod) {
// we must check whether the pod is alive because we have to consider the following case:
// 1. the statefulset is down scaled to zero
// 2. the lsp gc is triggered
// 3. gc interval, e.g. 90s, is passed and the second gc is triggered
// 4. the sts is up scaled to the original replicas
// 5. the pod is still running and it will not be recreated
return true| if isStsPod, stsName, stsUID := isStatefulSetPod(pod); isStsPod { | ||
| if isStatefulSetPodToDel(c.config.KubeClient, pod, stsName, stsUID) { | ||
| if isStatefulSetPodToGC(c.config.KubeClient, pod, stsName, stsUID) { |
There was a problem hiding this comment.
Consider whether renaming the function here is the best approach. While the new name isStatefulSetPodToGC is more descriptive, ensure that all call sites are updated and that the change doesn't introduce any unintended side effects. It might be useful to add a comment explaining why the function was renamed.
| if isStsPod, stsName, stsUID := isStatefulSetPod(pod); isStsPod { | |
| if isStatefulSetPodToDel(c.config.KubeClient, pod, stsName, stsUID) { | |
| if isStatefulSetPodToGC(c.config.KubeClient, pod, stsName, stsUID) { | |
| if isStsPod, stsName, stsUID := isStatefulSetPod(pod); isStsPod { | |
| // Renamed from isStatefulSetPodToDel to better reflect its purpose: determining if the LSP should be garbage collected. | |
| if isStatefulSetPodToGC(c.config.KubeClient, pod, stsName, stsUID) { |
Pull Request Test Coverage Report for Build 14247883375Details
💛 - Coveralls |
Signed-off-by: zhangzujian <[email protected]>
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)