Print the scheduler configurations into logs for easier debugging#5128
Print the scheduler configurations into logs for easier debugging#5128Tau721 wants to merge 1 commit intovolcano-sh:masterfrom
Conversation
|
@Tau721: The label(s) 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
volcano-sh-bot
added
the
size/M
Summary of ChangesHello, 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 enhances the scheduler's debugging capabilities by printing detailed configurations to the logs. It addresses issue #5127 by providing more information about the scheduler's state, including plugin arguments and configurations. The changes involve modifying the scheduler's configuration loading process and updating the logging output. 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. Footnotes
|
![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
The pull request introduces a schedulerConfContent field to the Scheduler struct to store the raw configuration content. The loadSchedulerConf function is updated to populate this field with mutex protection, both for default and custom configurations, and to log the raw content, replacing the getSchedulerConf helper function. A minor YAML tag casing fix for EnabledClusterOrder is also included. A review comment suggests refactoring the logging of schedulerConfContent in loadSchedulerConf to minimize lock contention by copying the content to a local variable before releasing the mutex and performing I/O.
pkg/scheduler/scheduler.go
Outdated
| pc.mutex.Lock() | ||
| for _, line := range strings.Split(pc.schedulerConfContent, "\n") { | ||
| klog.V(2).Infoln(strings.TrimSpace(line)) | ||
| } | ||
| pc.mutex.Unlock() |
There was a problem hiding this comment.
To minimize lock contention, it's better to not hold the lock during I/O operations like logging. You can copy pc.schedulerConfContent to a local variable under the lock, release the lock, and then perform the logging.
pc.mutex.Lock()
confContent := pc.schedulerConfContent
pc.mutex.Unlock()
for _, line := range strings.Split(confContent, "\n") {
klog.V(2).Infoln(strings.TrimSpace(line))
}There was a problem hiding this comment.
Pull request overview
This PR enhances the scheduler's debugging capabilities by printing detailed configuration content to logs whenever the scheduler configuration is loaded or changed. Previously, the scheduler only logged the names of its actions and plugins, which was insufficient for debugging. Now it logs the full configuration content, making it easier for users to understand and debug scheduler behavior.
Changes:
- Added a new
schedulerConfContentfield to the Scheduler struct to store the full configuration as a string - Modified the
loadSchedulerConf()function's defer block to print the complete configuration content line-by-line instead of just action and plugin names - Updated all places where scheduler configuration is set to also update
schedulerConfContentwith proper mutex protection - Removed the
getSchedulerConf()helper function that extracted action and plugin names (no longer needed) - Fixed YAML tag for
EnabledClusterOrderfield from uppercase"EnabledClusterOrder"to lowercase (though with potential naming inconsistency)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/scheduler/scheduler.go | Added configuration content storage and printing; enhanced mutex protection for thread-safe concurrent access |
| pkg/scheduler/conf/scheduler_conf.go | Fixed YAML tag naming for EnabledClusterOrder field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tau721
force-pushed
the
fix_log_scheduler_conf
branch
2 times, most recently
from
55f84e6 to
b3c0784
Compare
Tau721
force-pushed
the
fix_log_scheduler_conf
branch
2 times, most recently
from
68ed992 to
7ca9938
Compare
…bugging Signed-off-by: caotuo721 <caotuo721@yeah.net>
Tau721
force-pushed
the
fix_log_scheduler_conf
branch
from
583d279 to
645bbc8
Compare
|
/retest |
|
@Tau721: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
What type of PR is this?
/kind Enhancement
What this PR does / why we need it:
Currently, the scheduler only prints the names of its actions and plugins into logs, and such information is usually insufficient for users to debug.
To facilitate debugging, the scheduler would better print the detailed configurations as the users edit, including the arguments and configurations of each plugin.
Which issue(s) this PR fixes:
Fixes #5127
Special notes for your reviewer:
The scheduler saves its detailed configurations and prints them into logs whenever they are changed.
Does this PR introduce a user-facing change?