feat(colocation): make node monitor thresholds configurable - #4935
feat(colocation): make node monitor thresholds configurable#4935AmanKumar1411 wants to merge 3 commits into
Conversation
|
[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 |
|
Welcome @AmanKumar1411! It looks like this is your first PR to volcano-sh/volcano 🎉 |
Summary of ChangesHello @AmanKumar1411, 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 flexibility of Volcano's colocation eviction mechanism by making key node monitor thresholds configurable. Previously, parameters like resource utilization monitoring intervals, eviction detection intervals, and the consecutive high-usage count limit were hard-coded, limiting tuning options. The changes introduce a new 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
|
volcano-sh-bot
added
the
size/L
![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
This pull request introduces configurability for the node monitor's thresholds, which is a great enhancement for tuning node pressure sensitivity. The implementation correctly defines the new configuration options and adds validation for them.
However, I've identified a few issues that need to be addressed:
- In
pkg/agent/config/api/validate.go, the new validation error messages are hardcoded, which is inconsistent with the existing practice of using constants. - The
RefreshCfgfunction inpkg/agent/events/probes/nodemonitor/node_monitor.gois incomplete. It doesn't update the monitor with the new configuration values forhighUsageCountLimit,utilizationInterval, anddetectInterval. - Most critically, the
Runmethod innode_monitor.gouseswait.Until, which does not support dynamic updates of the execution interval. This means that changes toutilizationIntervalSecondsanddetectIntervalSecondsin the configuration will not take effect at runtime.
I've provided detailed comments and suggestions to address these points. Once these are fixed, the feature will be robust and work as expected.
| func (m *monitor) Run(stop <-chan struct{}) { | ||
| klog.InfoS("Started nodePressure probe") | ||
| go wait.Until(m.utilizationMonitoring, 10*time.Second, stop) | ||
| go wait.Until(m.detect, 10*time.Second, stop) | ||
| go wait.Until(m.utilizationMonitoring, m.utilizationInterval, stop) | ||
| go wait.Until(m.detect, m.detectInterval, stop) | ||
| } |
There was a problem hiding this comment.
The Run method starts two goroutines using wait.Until. The period argument for wait.Until is passed by value, which means the goroutines will continue to use the initial interval values for their entire lifetime. Any subsequent updates to m.utilizationInterval or m.detectInterval via RefreshCfg will not be picked up by the running goroutines. This defeats the purpose of making the intervals configurable at runtime.
To fix this, you should change the implementation to periodically re-evaluate the interval. A simple way is to replace wait.Until with a custom loop that reads the interval on each iteration.
func (m *monitor) Run(stop <-chan struct{}) {
klog.InfoS("Started nodePressure probe")
go func() {
for {
m.Lock()
interval := m.utilizationInterval
m.Unlock()
select {
case <-time.After(interval):
m.utilizationMonitoring()
case <-stop:
return
}
}
}()
go func() {
for {
m.Lock()
interval := m.detectInterval
m.Unlock()
select {
case <-time.After(interval):
m.detect()
case <-stop:
return
}
}
}()
}There was a problem hiding this comment.
Thanks for the detailed review.
I’ve updated the Run loop to dynamically re-evaluate intervals, wired NodeMonitor values in RefreshCfg, and moved validation messages to constants for consistency.
| if n.UtilizationIntervalSeconds != nil && *n.UtilizationIntervalSeconds <= 0 { | ||
| errs = append(errs, errors.New("utilizationIntervalSeconds must be positive")) | ||
| } | ||
| if n.DetectIntervalSeconds != nil && *n.DetectIntervalSeconds <= 0 { | ||
| errs = append(errs, errors.New("detectIntervalSeconds must be positive")) | ||
| } | ||
| if n.HighUsageCountLimit != nil && *n.HighUsageCountLimit <= 0 { | ||
| errs = append(errs, errors.New("highUsageCountLimit must be positive")) | ||
| } |
There was a problem hiding this comment.
For consistency with other validation functions in this file, it's better to define the error messages as constants at the package level instead of using hardcoded strings. This improves maintainability and makes it easier to manage error messages.
You can add these constants to the var block at the top of the file:
var (
// ... existing error messages
IllegalUtilizationIntervalSecondsMsg = "utilizationIntervalSeconds must be positive"
IllegalDetectIntervalSecondsMsg = "detectIntervalSeconds must be positive"
IllegalHighUsageCountLimitMsg = "highUsageCountLimit must be positive"
)| if n.UtilizationIntervalSeconds != nil && *n.UtilizationIntervalSeconds <= 0 { | |
| errs = append(errs, errors.New("utilizationIntervalSeconds must be positive")) | |
| } | |
| if n.DetectIntervalSeconds != nil && *n.DetectIntervalSeconds <= 0 { | |
| errs = append(errs, errors.New("detectIntervalSeconds must be positive")) | |
| } | |
| if n.HighUsageCountLimit != nil && *n.HighUsageCountLimit <= 0 { | |
| errs = append(errs, errors.New("highUsageCountLimit must be positive")) | |
| } | |
| if n.UtilizationIntervalSeconds != nil && *n.UtilizationIntervalSeconds <= 0 { | |
| errs = append(errs, errors.New(IllegalUtilizationIntervalSecondsMsg)) | |
| } | |
| if n.DetectIntervalSeconds != nil && *n.DetectIntervalSeconds <= 0 { | |
| errs = append(errs, errors.New(IllegalDetectIntervalSecondsMsg)) | |
| } | |
| if n.HighUsageCountLimit != nil && *n.HighUsageCountLimit <= 0 { | |
| errs = append(errs, errors.New(IllegalHighUsageCountLimitMsg)) | |
| } |
There was a problem hiding this comment.
Thanks for the detailed review.
I’ve updated the Run loop to dynamically re-evaluate intervals, wired NodeMonitor values in RefreshCfg, and moved validation messages to constants for consistency.
Signed-off-by: AmanKumar <itsamankumar786@gmail.com>
Signed-off-by: AmanKumar <itsamankumar786@gmail.com>
Signed-off-by: AmanKumar <itsamankumar786@gmail.com>
|
Hi @AmanKumar1411 I think #4924 already worked on this |
|
@JesseStutler i think the #4924 is till open |
3 participants
What is the problem you're trying to solve
Currently, in the Volcano colocation scenario, when the usage of a resource such as CPU exceeds a configured threshold, eviction of BE pods is triggered only after six consecutive high-usage detections. These detections are performed every 10 seconds. Both the detection intervals and the consecutive high-usage count limit are hard-coded in the node monitor implementation, which prevents users from tuning node pressure sensitivity based on their cluster size or workload characteristics.
Specifically, the highUsageCountLimit is hard-coded, and the utilisation monitoring and eviction detection intervals are also fixed at 10 seconds.
Describe the solution you'd like
Introduce configurable node monitor parameters under ColocationConfig to allow users to customise the utilisation monitoring interval, eviction detection interval, and the consecutive high-usage count limit. The default values will remain unchanged, with 10 seconds for both intervals and 6 as the highUsageCountLimit, ensuring backward compatibility.
Additional context
No additional context.
Documentation Updates
This change introduces new user-facing configuration options. Documentation updates are required to describe the new nodeMonitor configuration fields. The relevant documentation will be updated and published to the Volcano official website through the volcano-sh/website repository