[misc] refactor component cleanup#145
Conversation
slin1237
requested review from
CatherineSue,
YouNeedCryDear and
zhyncs
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @slin1237, 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 refactors the InferenceService controller's resource management to improve the cleanup of orphaned Kubernetes resources. By centralizing and generalizing the deletion process, I've eliminated a problematic reconciliation anti-pattern, simplified component interfaces, and enhanced the controller's overall reliability and maintainability. The changes ensure that when components are removed from an InferenceService spec, their associated resources are immediately and correctly garbage collected.
Highlights
- Refactored Component Cleanup: I've introduced a new, centralized cleanup mechanism for InferenceService components (engine, decoder, router). This addresses a Kubernetes reconciliation anti-pattern where 'deletion reconcilers' were previously created with nil specs when components were removed from the InferenceService specification. The new approach leverages Kubernetes owner references and label-based selection for more robust and immediate resource deletion.
- Simplified Component Interface: The
Componentinterface has been simplified by removing theDelete()andShouldExist()methods. The responsibility for cleaning up orphaned resources is now handled globally by the InferenceService controller, rather than individually by each component reconciler. This reduces complexity and potential for errors within component-specific logic. - Dynamic Resource Discovery and Deletion: The new cleanup logic in
cleanup.godynamically discovers and deletes resources using unstructured objects, avoiding hard-coding specific resource types. It also includes checks for CRD availability, gracefully handling optional resources like Ray, Knative, KEDA, and Istio, ensuring compatibility across different cluster configurations. - Improved Reconciliation Flow: The main InferenceService reconciliation loop in
controller.gohas been streamlined. The previous conditional logic for creating deletion reconcilers and calling component-specificDeletemethods has been removed. Cleanup is now a distinct, early step in the reconciliation process, ensuring orphaned resources are addressed promptly. - Comprehensive Unit Testing: New, comprehensive unit tests have been added in
cleanup_test.goto validate the correctness and robustness of the new cleanup logic, covering various scenarios including the deletion of orphaned resources and the preservation of active ones.
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.
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. ↩
slin1237
force-pushed
the
slin/resource-cleanup
branch
from
a900b88 to
1c23218
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the component cleanup logic, moving away from an anti-pattern to a cleaner, more robust implementation using owner references and dynamic resource discovery. The changes are well-structured and the addition of comprehensive unit tests is excellent. I have a few suggestions to further improve the new cleanup logic, mainly around error handling and removing a piece of unused code.
| if err := r.Delete(ctx, &obj); err != nil && !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("delete %s/%s: %w", gvk.Kind, obj.GetName(), err) | ||
| } |
There was a problem hiding this comment.
The current implementation returns on the first error encountered during resource deletion. This means if deleting one orphaned resource fails, the cleanup for other orphaned resources of the same type (GVK) is aborted for the current reconciliation cycle. It would be more robust to attempt to delete all orphaned resources of the current type, logging any errors but not returning immediately.
| if err := r.Delete(ctx, &obj); err != nil && !apierrors.IsNotFound(err) { | |
| return fmt.Errorf("delete %s/%s: %w", gvk.Kind, obj.GetName(), err) | |
| } | |
| if err := r.Delete(ctx, &obj); err != nil && !apierrors.IsNotFound(err) { | |
| log.Error(err, "Failed to delete orphaned resource", "gvk", gvk, "name", obj.GetName()) | |
| } |
| // cleanupRemovedComponentsDynamic uses discovery to dynamically clean up unknown resource types. | ||
| func (r *InferenceServiceReconciler) cleanupRemovedComponentsDynamic( | ||
| ctx context.Context, | ||
| isvc *v1beta1.InferenceService, | ||
| activeComponents map[v1beta1.ComponentType]bool, | ||
| ) error { | ||
| log := log.FromContext(ctx) | ||
| selector := labels.Set{constants.InferenceServicePodLabelKey: isvc.Name}.AsSelector() | ||
|
|
||
| apiLists, err := r.Clientset.Discovery().ServerPreferredResources() | ||
| if err != nil { | ||
| log.Info("Partial resource discovery failure", "error", err) | ||
| } | ||
|
|
||
| for _, list := range apiLists { | ||
| gv, err := schema.ParseGroupVersion(list.GroupVersion) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| for _, res := range list.APIResources { | ||
| if !contains(res.Verbs, "list") || !contains(res.Verbs, "delete") || strings.Contains(res.Name, "/") { | ||
| continue | ||
| } | ||
| gvk := schema.GroupVersionKind{Group: gv.Group, Version: gv.Version, Kind: res.Kind} | ||
| if err := r.cleanupResourcesOfType(ctx, gvk, isvc, selector, activeComponents); err != nil { | ||
| log.V(1).Info("Failed to cleanup dynamically discovered resource", "gvk", gvk, "error", err) | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The function cleanupRemovedComponentsDynamic appears to be unused. To improve code clarity and maintainability, it's best to remove dead code. If this is intended for future use, please add a comment explaining its purpose.
| } | ||
| ok, err := utils.IsCrdAvailable(r.ClientConfig, res.gvk.GroupVersion().String(), res.gvk.Kind) | ||
| if err != nil { | ||
| log.Log.V(1).Info("Failed to check CRD", "gvk", res.gvk, "error", err) |
There was a problem hiding this comment.
It's a best practice to use an instance-specific logger (r.Log) instead of the global log.Log. This improves consistency and allows for better contextual logging.
| log.Log.V(1).Info("Failed to check CRD", "gvk", res.gvk, "error", err) | |
| r.Log.V(1).Info("Failed to check CRD", "gvk", res.gvk, "error", err) |
2 participants
What type of PR is this?
/kind cleanup
/kind bug
What this PR does / why we need it:
This PR refactors the InferenceService controller to fix a Kubernetes reconciliation anti-pattern where "deletion reconcilers" were being created
with nil specs when components (router, decoder, engine) were removed from the InferenceService spec.
Previous behavior (problematic):
New behavior (fixed):
Key improvements:
Delete()andShouldExist()methodsWhich issue(s) this PR fixes:
Special notes for your reviewer:
cleanup.gouses unstructured objects to avoid hard-coding resource types, making it maintainable as new resourcesare added
Does this PR introduce a user-facing change?