Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refine cpu provision framework and share region support restrict poli… #419

Conversation

luomingmeng
Copy link
Collaborator

…cies controlKnob

What type of PR is this?

Enhancements

What this PR does / why we need it:

refine cpu provision framework and share region support restrict controlKnob of policies.

Which issue(s) this PR fixes:

Special notes for your reviewer:

@luomingmeng luomingmeng marked this pull request as ready for review December 22, 2023 02:35
@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch from 48bfa9d to 0e4bccd Compare December 22, 2023 02:39
@luomingmeng luomingmeng self-assigned this Dec 22, 2023
@luomingmeng luomingmeng added enhancement New feature or request workflow/need-review review: test succeeded, need to review labels Dec 22, 2023
@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch from 0e4bccd to 27c569d Compare December 22, 2023 08:27
reg.SetEssentials(r.essentials)
}

reg.SetLatestCPURequirement(int(knob.Value))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if control knob's value is meaningless and only action is used? Regulate action doesn't make sense. Regulate ControlKnobNonReclaimedCPUSize only.

// restrictProvisionControlKnob is to restrict provision control knob by reference policy
func (r *QoSRegionShare) restrictProvisionControlKnob(originControlKnob map[types.CPUProvisionPolicyName]types.ControlKnob) map[types.CPUProvisionPolicyName]types.ControlKnob {
restrictedControlKnob := make(map[types.CPUProvisionPolicyName]types.ControlKnob)
for baseName, controlKnob := range originControlKnob {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

baseName and refName is confusing since they have similar meaning. baseName -> policyName(or some better name), refName -> refPolicyName might be better.

@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch from 27c569d to 5477031 Compare December 23, 2023 16:53
Copy link

codecov bot commented Dec 23, 2023

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (9b5c40a) 53.68% compared to head (cf58cce) 53.84%.
Report is 1 commits behind head on main.

Files Patch % Lines
...lugin/qosaware/resource/cpu/region/region_share.go 34.48% 38 Missing ⚠️
...sadvisor/qosaware/resource/cpu/region/cpu_share.go 66.66% 13 Missing and 3 partials ⚠️
...plugin/qosaware/resource/cpu/region/region_base.go 86.48% 9 Missing and 6 partials ⚠️
...ns/sysadvisor/qosaware/resource/cpu/cpu_advisor.go 66.66% 2 Missing ⚠️
...advisor/qosaware/resource/cpu/region/cpu_region.go 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
+ Coverage   53.68%   53.84%   +0.15%     
==========================================
  Files         448      450       +2     
  Lines       50138    50385     +247     
==========================================
+ Hits        26918    27129     +211     
- Misses      20149    20187      +38     
+ Partials     3071     3069       -2     
Flag Coverage Δ
unittest 53.84% <73.55%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch 4 times, most recently from 211aa49 to 6487ed5 Compare December 25, 2023 02:26
@@ -158,6 +227,10 @@ func (r *QoSRegionBase) Name() string {
return r.name
}

func (r *QoSRegionBase) TryUpdateProvision() {
panic("QoSRegionBase TryUpdateProvision is not implemented")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't panic during running states

@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch from 3eb6deb to 6cf8c61 Compare December 25, 2023 05:34
sun-yuliang
sun-yuliang previously approved these changes Dec 25, 2023
@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch 2 times, most recently from 4cec379 to e887eb3 Compare December 25, 2023 09:57
@luomingmeng luomingmeng force-pushed the dev/refractor-sysadvisor-cpu-provision-policy branch from e887eb3 to cf58cce Compare December 26, 2023 03:30
@waynepeking348 waynepeking348 added the workflow/merge-ready merge-ready: code is ready and can be merged label Dec 26, 2023
@waynepeking348 waynepeking348 merged commit 5fdaf30 into kubewharf:main Dec 26, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged workflow/need-review review: test succeeded, need to review
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants