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

enhancement(sysadvisor): export region reclaimable info as non-reclaim when dedicated_cores can not be co-located with reclaimed_cores #345

Merged

Conversation

cheney-lin
Copy link
Member

What type of PR is this?

Enhancements

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

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

Comparison is base (1882504) 53.43% compared to head (bdd0f20) 53.55%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   53.43%   53.55%   +0.11%     
==========================================
  Files         434      436       +2     
  Lines       47563    47963     +400     
==========================================
+ Hits        25415    25685     +270     
- Misses      19295    19396     +101     
- Partials     2853     2882      +29     
Flag Coverage Δ
unittest 53.55% <53.84%> (+0.11%) ⬆️

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

Files Coverage Δ
...plugin/qosaware/resource/cpu/region/region_base.go 79.17% <100.00%> (+0.24%) ⬆️
...urce/cpu/region/region_dedicated_numa_exclusive.go 75.49% <42.85%> (-8.47%) ⬇️

... 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 force-pushed the dev/export_region_reclaim branch from 94713d5 to abcc858 Compare November 6, 2023 10:09
@cheney-lin cheney-lin force-pushed the dev/export_region_reclaim branch from abcc858 to e2d8795 Compare November 6, 2023 10:25
@cheney-lin cheney-lin changed the title enhancement(sysadvisor): export region reclaimable info enhancement(sysadvisor): export region reclaimable info as non-reclaim when dedicated_cores are non-reclaim Nov 6, 2023
@cheney-lin cheney-lin force-pushed the dev/export_region_reclaim branch from e2d8795 to ef3b565 Compare November 6, 2023 10:28
@cheney-lin cheney-lin changed the title enhancement(sysadvisor): export region reclaimable info as non-reclaim when dedicated_cores are non-reclaim enhancement(sysadvisor): export region reclaimable info as non-reclaim when dedicated_cores can not be co-located with reclaimed_cores Nov 6, 2023
sun-yuliang
sun-yuliang previously approved these changes Nov 6, 2023
@@ -38,6 +42,7 @@ import (

type QoSRegionDedicatedNumaExclusive struct {
*QoSRegionBase
podUID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

personally, I would prefer not to define podUID here, since it indicates that only contain one pod can be supported, but the podSet indicates that we should support multiple pods, and it may cause a lot of confusion as the logic goes more and more complex

Copy link
Collaborator

Choose a reason for hiding this comment

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

or we can redefine the high-level design, i.e. "whether it is reasonable to make every region to contain a podSet "

Copy link
Member Author

Choose a reason for hiding this comment

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

It is necessary to keep the podset because we need to calculate resource usage by it. NewQoSRegionDedicatedNumaExclusive accept containerInfo as one of the parameters indicate that only this region only belong to single Pod.

Copy link
Collaborator

@waynepeking348 waynepeking348 Nov 6, 2023

Choose a reason for hiding this comment

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

still confused for keeping both podSet and podUID; besides, NewQoSRegionDedicatedNumaExclusive accept containerInfo as one of the parameters is not a solid reason for keeping podUID or indicating there should be only one pod in this region, since all regions accept containerInfo as one of the parameters (cause we need for its owner pool name) ...

Copy link
Member Author

Choose a reason for hiding this comment

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

well, I removed podUID of QoSRegionDedicatedNumaExclusive and use getPodUID to get it.

@waynepeking348 waynepeking348 added the workflow/need-review review: test succeeded, need to review label Nov 6, 2023
…m when dedicated_cores can not be co-located with reclaimed_cores

Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
@cheney-lin cheney-lin added workflow/merge-ready merge-ready: code is ready and can be merged enhancement New feature or request and removed workflow/need-review review: test succeeded, need to review labels Nov 7, 2023
@waynepeking348 waynepeking348 merged commit ad438bc into kubewharf:main Nov 7, 2023
12 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants