Workload manager Cleanup - #139
#139Workload manager Cleanup#139
Conversation
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Summary of ChangesHello @hzxuzhonghu, 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 represents the initial phase of a broader cleanup and modularization effort within the workload manager. It primarily focuses on improving code organization and maintainability by refactoring data structures, consolidating helper functions into a new dedicated file, and removing a previously standalone 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
|
| } | ||
|
|
||
| // NewSandboxStore creates a new sandbox store | ||
| func NewSandboxStore() *SandboxStore { |
There was a problem hiding this comment.
this store is deleted, just keep the helper functions in sandbox_helper.go
There was a problem hiding this comment.
Code Review
This pull request introduces some excellent cleanup and refactoring, particularly by removing the local SandboxStore in favor of a generic store.Store interface. The renaming of sandboxExternalInfo to sandboxEntry and other variable name changes also improve clarity. However, there are a few significant concerns that need to be addressed. A critical issue is the removal of kubebuilder:rbac annotations, which will likely cause the CodeInterpreterReconciler to fail due to lack of permissions. Additionally, the new hardcoded high QPS and Burst settings for the Kubernetes client pose a high risk of overloading the API server. I've also noted a couple of medium-severity issues related to a confusing log message and the removal of a default port configuration that could be a breaking change.
I am having trouble creating individual review comments. Click here to see my feedback.
pkg/workloadmanager/codeinterpreter_controller.go (47-53)
The removal of these +kubebuilder:rbac markers is concerning. The CodeInterpreterReconciler performs operations (get, create, update, delete) on codeinterpreters, sandboxes, sandboxtemplates, and sandboxwarmpools. These RBAC annotations grant the necessary permissions for the controller to function correctly. Without them, the controller will likely encounter permission errors at runtime, breaking its functionality. If these permissions are defined elsewhere, that's fine, but if not, this change should be reverted.
pkg/workloadmanager/k8s_client.go (98-100)
The hardcoded QPS and Burst values (1000 and 2000 respectively) are extremely high and could potentially overwhelm the Kubernetes API server, leading to cluster-wide instability. It's highly recommended to make these values configurable rather than hardcoding them. If such high values are indeed necessary, please add a comment explaining the reasoning.
pkg/workloadmanager/handlers.go (180)
This TODO is a good catch. It's important to handle rollback for SandboxClaim as well to prevent orphaned resources, especially in timeout or failure scenarios. This will improve the robustness of the sandbox creation process.
pkg/workloadmanager/sandbox_controller.go (73)
This log message is a bit confusing. The watcher entry in the watchers map is deleted before this select block. The old message resultChan is full... was more descriptive of the immediate situation. Consider using a message that clarifies that a notification was dropped because the channel was full, which might indicate a race or a stalled consumer.
klog.Warningf("Notification for sandbox %s/%s dropped, channel was full", sandbox.Namespace, sandbox.Name)
pkg/workloadmanager/sandbox_controller.go (101)
Instead of deleting and re-adding the watcher, you can just delete(r.watchers, key). delete is a no-op if the key doesn't exist, so the if check is redundant. This simplifies the code.
delete(r.watchers, key)
pkg/workloadmanager/workload_builder.go (325-330)
The removal of this block means that CodeInterpreter resources without explicitly defined ports will no longer have a default port assigned. This could be a breaking change for users who relied on the default port 8080. If this is an intentional design change to enforce explicit port configuration, it should be documented. Otherwise, this might be an unintended regression.
There was a problem hiding this comment.
Pull request overview
This PR performs cleanup of the workload manager by removing the SandboxStore implementation, renaming types and variables for clarity, and extracting helper functions into a dedicated file.
Changes:
- Removed
store.gocontaining the in-memorySandboxStoreimplementation and migrated helper functions tosandbox_helper.go - Renamed
sandboxExternalInfotosandboxEntryandpendingRequeststowatchersfor improved clarity - Increased Kubernetes client QPS/Burst limits and adjusted logging verbosity levels
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workloadmanager/workload_builder.go | Renamed type from sandboxExternalInfo to sandboxEntry, renamed parameter ifm to informer, removed default port configuration for CodeInterpreter |
| pkg/workloadmanager/store.go | Deleted entire file containing SandboxStore and related conversion functions |
| pkg/workloadmanager/server.go | Removed sandboxStore field and related initialization/cleanup code |
| pkg/workloadmanager/sandbox_helper.go | New file containing migrated helper functions with renamed parameters and updated status return value |
| pkg/workloadmanager/sandbox_controller.go | Renamed pendingRequests to watchers, updated logging verbosity, improved error message wording |
| pkg/workloadmanager/k8s_client.go | Renamed sandboxExternalInfo to sandboxEntry, removed unused annotation constant, increased QPS/Burst limits |
| pkg/workloadmanager/handlers.go | Updated variable names to use sandboxEntry instead of externalInfo, added TODO comment |
| pkg/workloadmanager/codeinterpreter_controller.go | Removed unused kubebuilder RBAC comments |
| pkg/workloadmanager/auth.go | Removed outdated comment about filtering by CreatorServiceAccount |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| klog.V(2).Infof("Notified waiter about sandbox %s/%s reaching Running state", sandbox.Namespace, sandbox.Name) | ||
| default: | ||
| klog.Warningf("resultChan is full for sandbox %s/%s", sandbox.Namespace, sandbox.Name) | ||
| klog.Warningf("watch chan for sandbox %s/%s should be deleted", sandbox.Namespace, sandbox.Name) |
There was a problem hiding this comment.
The error message "watch chan for sandbox %s/%s should be deleted" is unclear and doesn't provide actionable information. Consider a more descriptive message like "Failed to notify watcher for sandbox %s/%s: channel buffer full or closed" to better explain the issue.
| klog.Warningf("watch chan for sandbox %s/%s should be deleted", sandbox.Namespace, sandbox.Name) | |
| klog.Warningf("Failed to notify watcher for sandbox %s/%s: channel buffer full or not receiving", sandbox.Namespace, sandbox.Name) |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
=======================================
Coverage ? 29.20%
=======================================
Files ? 28
Lines ? 2551
Branches ? 0
=======================================
Hits ? 745
Misses ? 1690
Partials ? 116
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:
|
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
|
@hzxuzhonghu: GitHub didn't allow me to assign the following users: acsoto, tjucoder. Note that only volcano-sh members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| // Set conservative QPS and Burst to avoid overloading the API server | ||
| config.QPS = 50 | ||
| config.Burst = 100 | ||
|
|
There was a problem hiding this comment.
This is to improve concurrency
| klog.Infof("Cleaning up pending request for sandbox %s/%s", key.Namespace, key.Name) | ||
| delete(r.pendingRequests, key) | ||
| } | ||
| delete(r.watchers, key) |
| Endpoint: net.JoinHostPort(podIP, strconv.Itoa(int(port.Port))), | ||
| }) | ||
| } | ||
| sandboxStore := &types.SandboxInfo{ |
There was a problem hiding this comment.
sandboxStore naming is still suitable here?
There was a problem hiding this comment.
I feel andboxInfo or sandbox fit more
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "running" | ||
| } | ||
| } | ||
| return "unknown" |
There was a problem hiding this comment.
The return value for non-running sandboxes has been changed from "paused" to "unknown". This represents a behavioral change that may affect consumers of the SandboxInfo status field. The status "unknown" is semantically different from "paused" and could break existing logic that depends on the "paused" status. Consider whether this change is intentional and if it maintains backward compatibility with existing clients.
| return "unknown" | |
| return "paused" |
| } | ||
|
|
||
| func buildSandboxByCodeInterpreter(namespace string, codeInterpreterName string, ifm *Informers) (*sandboxv1alpha1.Sandbox, *extensionsv1alpha1.SandboxClaim, *sandboxExternalInfo, error) { | ||
| func buildSandboxByCodeInterpreter(namespace string, codeInterpreterName string, informer *Informers) (*sandboxv1alpha1.Sandbox, *extensionsv1alpha1.SandboxClaim, *sandboxEntry, error) { |
There was a problem hiding this comment.
The parameter name change from "ifm" to "informer" is inconsistent with the other function buildSandboxByAgentRuntime which still uses "ifm" for the same parameter. Consider using consistent naming across related functions for better code maintainability.
| config.QPS = 50 | ||
| config.Burst = 100 |
There was a problem hiding this comment.
The QPS and Burst configuration values (50 and 100) are hardcoded. Consider making these values configurable through the Config struct or constants to allow tuning based on cluster capacity and workload requirements without code changes.
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This is the first step cleaning workload manager, later i plan to separate the implement by its three modules.