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

fix(sysadvisor): set numa capicity as requirement of numabinding pods when reclaimed disabled #158

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

cheney-lin
Copy link
Member

… when reclaimed disabled

What type of PR is this?

Bug fixes

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

@cheney-lin cheney-lin force-pushed the dev/requirement-setting branch from c1b3ea4 to 6a11f3a Compare July 19, 2023 08:28
@cheney-lin cheney-lin changed the title fix(sysadvisor): set requirement of numabinding pods as numa capicity… fix(sysadvisor): set numa capicity as requirement of numabinding pods when reclaimed disabled Jul 19, 2023
@cheney-lin cheney-lin force-pushed the dev/requirement-setting branch from 6a11f3a to e910da6 Compare July 19, 2023 08:37
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: +0.46% 🎉

Comparison is base (c8b2187) 50.20% compared to head (a099272) 50.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   50.20%   50.67%   +0.46%     
==========================================
  Files         408      408              
  Lines       38774    38833      +59     
==========================================
+ Hits        19467    19677     +210     
+ Misses      17126    16963     -163     
- Partials     2181     2193      +12     
Flag Coverage Δ
unittest 50.67% <75.00%> (+0.46%) ⬆️

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

Files Changed Coverage Δ
...rce/cpu/region/provisionpolicy/policy_canonical.go 67.46% <64.70%> (+1.75%) ⬆️
...urce/cpu/region/headroompolicy/policy_canonical.go 77.35% <66.66%> (+77.35%) ⬆️
...resource/memory/headroompolicy/policy_canonical.go 73.03% <66.66%> (-3.36%) ⬇️
pkg/util/machine/topology.go 78.18% <85.71%> (+0.93%) ⬆️
...ns/sysadvisor/qosaware/resource/cpu/cpu_advisor.go 73.07% <100.00%> (+14.74%) ⬆️
pkg/metaserver/spd/manager.go 50.84% <100.00%> (+1.72%) ⬆️
pkg/util/machine/machine_linux.go 50.00% <100.00%> (+1.72%) ⬆️

... and 22 files with indirect coverage changes

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

@cheney-lin cheney-lin added the workflow/draft draft: no need to review label Jul 19, 2023
@cheney-lin cheney-lin force-pushed the dev/requirement-setting branch 2 times, most recently from 2563ab7 to a4cdf16 Compare July 21, 2023 07:38
@cheney-lin cheney-lin added bug Something isn't working workflow/need-review review: test succeeded, need to review and removed workflow/draft draft: no need to review labels Jul 21, 2023
@cheney-lin cheney-lin force-pushed the dev/requirement-setting branch 2 times, most recently from 433c81b to f8813fa Compare July 21, 2023 09:37
sun-yuliang
sun-yuliang previously approved these changes Jul 24, 2023
@@ -55,6 +55,14 @@ func (p *PolicyCanonical) Update() error {
return err
}

if p.regionType == types.QoSRegionTypeDedicatedNumaExclusive && !enableReclaim {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be moved out of range loop for for podUID, containerSet := range p.podSet {? since you will add up multiple times for cpuEstimation and containerCnt this way

Copy link
Collaborator

Choose a reason for hiding this comment

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

and the same for provision_canonical.go

… when reclaimed disabled

Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
@cheney-lin cheney-lin added workflow/merge-ready merge-ready: code is ready and can be merged and removed workflow/need-review review: test succeeded, need to review labels Jul 25, 2023
@waynepeking348 waynepeking348 merged commit 1253f13 into kubewharf:main Jul 25, 2023
luomingmeng pushed a commit to luomingmeng/katalyst-core that referenced this pull request Oct 11, 2024
… when reclaimed disabled (kubewharf#158)

Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working workflow/merge-ready merge-ready: code is ready and can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants