fix: add ResourceStrategyFit plugin - #4391
Conversation
volcano-sh-bot
added
the
size/XL
|
@JesseStutler please check |
JesseStutler
left a comment
JesseStutler
left a comment
There was a problem hiding this comment.
Should also add a design doc, state the design concept of the plugin, how to use it, and how to choose between it and binpack, there needs to be a best practice.
get |
volcano-sh-bot
added
size/XXL
@JesseStutler The design has been supplemented, which includes a scarce resource avoidance strategy, and Jinshan students can adjust it later. |
|
@JesseStutler hi，Any further adjustments needed? |
|
/cc @lowang-bh @Monokaix |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new scheduler plugin (resource-strategy-fit) that allows per-resource aggregation or dispersion strategies with weights, including tests and documentation, and registers it in the plugin factory.
- Introduce
resource-strategy-fitplugin implementation and its configuration parsing - Add extensive unit tests covering scoring logic
- Register plugin in
factory.goand update design docs
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/scheduler/plugins/resource-strategy-fit/resource_strategy_fit.go | Plugin implementation, argument parsing, and scoring |
| pkg/scheduler/plugins/resource-strategy-fit/resource_strategy_fit_test.go | Unit tests for weight calculation and scoring logic |
| pkg/scheduler/plugins/factory.go | Plugin registration in factory |
| docs/design/resource-strategy-fit-scheduling.md | Design documentation for the new plugin |
Comments suppressed due to low confidence (3)
pkg/scheduler/plugins/resource-strategy-fit/resource_strategy_fit.go:61
- The parameter name
arugumentsis misspelled. Rename it toargumentsfor clarity and consistency.
func New(aruguments framework.Arguments) framework.Plugin {
pkg/scheduler/plugins/resource-strategy-fit/resource_strategy_fit.go:85
- The argument key
resourceStrategyFitWeightis not consistent with tests that useResourceStrategyFitPlusWeight. Align the key name between code, tests, and documentation to avoid confusion.
resourceStrategyFitPluginWeight, b := framework.Get[int](args, "resourceStrategyFitWeight")
pkg/scheduler/plugins/resource-strategy-fit/resource_strategy_fit_test.go:478
- The error message references
mostRequestedScore()in the test forleastRequestedScore. Update the message to the correct function name.
t.Errorf("mostRequestedScore() error = %v, wantErr %v", err, tt.wantErr)
|
Please also change the title to |
LY-today
changed the title
done |
volcano-sh-bot
added
size/XXL
|
@JesseStutler @Monokaix Just rebased the latest branch, please re-lgtm |
cc：@Monokaix |
|
New changes are detected. LGTM label has been removed. |
5794b54 to
be6b3ba
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Monokaix 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 |
volcano-sh-bot
added
the
approved
Signed-off-by: LY-today <724102053@qq.com>
be6b3ba to
d43a885
Compare
|
@JesseStutler @Monokaix |
|
/lgtm |
|
/gemini review |
![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 the ResourceStrategyFit plugin, a valuable addition that allows for configuring different scheduling strategies per resource type. The implementation is well-structured and the overall approach is sound.
My review feedback primarily focuses on improving the accuracy of the design documentation, correcting several issues in the test suite (including incorrect argument keys and flawed test logic), and making minor refinements to the plugin code for better clarity and robustness. Addressing these points will ensure the new feature is well-documented and thoroughly validated.
|
@LY-today Could you fix these issues as gemini's suggestion? |
Of course, good standards make good community projects. |
|
@JesseStutler Resubmit PR? |
Just open a new PR :) This PR is closed already |
ok |
#4382