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.

Sign up for GitHub

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

Jump to bottom

feat(qrm): support memory enhancement numa_exclusive #19

Merged
merged 1 commit into from
May 24, 2023
Merged

feat(qrm): support memory enhancement numa_exclusive #19

merged 1 commit into from
May 24, 2023

Conversation

csfldf
Copy link
Collaborator

@csfldf csfldf commented Apr 17, 2023

What type of PR is this?

Enhancements

What this PR does / why we need it:

  • cpu/memory plugin support memory enhancement numa_exclusive key/value

Which issue(s) this PR fixes:

resolve #18

@csfldf csfldf added this to the v0.3 milestone Apr 17, 2023
@csfldf csfldf self-assigned this Apr 17, 2023
@caohe caohe added the enhancement New feature or request label Apr 17, 2023
@csfldf csfldf force-pushed the main branch 2 times, most recently from 14015a4 to 767ba8c Compare April 17, 2023 13:02
@codecov
Copy link

codecov bot commented Apr 17, 2023 •
edited
Loading

Codecov Report

Patch coverage: 66.37% and project coverage change: +0.18 🎉

Comparison is base (21d86d9) 51.30% compared to head (a67ff83) 51.48%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
+ Coverage   51.30%   51.48%   +0.18%     
==========================================
  Files         318      331      +13     
  Lines       32418    33501    +1083     
==========================================
+ Hits        16632    17249     +617     
- Misses      13840    14218     +378     
- Partials     1946     2034      +88
Flag Coverage Δ
unittest 51.48% <66.37%> (+0.18%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/qrm-plugins/cpu/dynamicpolicy/policy.go 38.26% <0.00%> (-0.09%) ⬇️
...ourcemanager/fetcher/kubelet/topology/interface.go 0.00% <0.00%> (ø)
...nt/sysadvisor/plugin/qosaware/reporter/reporter.go 39.00% <0.00%> (-37.55%) ⬇️
pkg/client/control/cnr.go 37.80% <0.00%> (-1.94%) ⬇️
pkg/config/agent/global/base.go 100.00% <ø> (ø)
pkg/config/agent/global/metaserver.go 100.00% <ø> (ø)
pkg/config/generic/generic_base.go 100.00% <ø> (ø)
pkg/util/general/error.go 0.00% <0.00%> (ø)
pkg/util/native/informer_dynamic.go 64.10% <ø> (ø)
...ysadvisor/plugin/qosaware/server/cpu/cpu_server.go 54.26% <3.44%> (-1.40%) ⬇️
... and 59 more

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@csfldf csfldf force-pushed the main branch 2 times, most recently from 09c2e22 to 77d1be9 Compare April 20, 2023 12:19
@csfldf csfldf added the workflow/need-review review: test succeeded, need to review label Apr 20, 2023
waynepeking348
waynepeking348 reviewed Apr 23, 2023
View reviewed changes
pkg/config/generic/qos.go Outdated Show resolved Hide resolved
pkg/util/qos/mem_enhancement.go Show resolved Hide resolved
pkg/config/generic/qos.go
// we use options to control those different values
// the key here is specific enhancement key such as "numa_binding", "numa_exclusive"
// the value is the default value of the key
EnhancementDefaultValues map[string]string
Copy link
Collaborator

Choose a reason for hiding this comment

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

the qos adaption logic has already becomes a little complicated, maybe we should add a new issue to add more documentation with examples to explain the behaviors; I will add it here #27, will submit a new pr to resolve this in the future

pkg/agent/qrm-plugins/cpu/dynamicpolicy/hint_handlers.go
@@ -150,13 +152,28 @@ func (p *DynamicPolicy) calculateHints(reqInt int, machineState state.NUMANodeMa
return nil, fmt.Errorf("GetNUMANodesCountToFitCPUReq failed with error: %v", err)
}

// because it's hard to control memory allocation accurately,
Copy link
Collaborator

@waynepeking348 waynepeking348 Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

need to refine, but will not block this pr from merge, same as #30

@waynepeking348
Copy link
Collaborator

please have a look at the comments about qos and function placement, for the other two comments, will take time to refine qrm implementation and qos documentations since they become a little difficult to maintain

@csfldf csfldf force-pushed the main branch 2 times, most recently from 732e23e to 1d8b92c Compare April 26, 2023 09:24
@csfldf csfldf 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 May 24, 2023
@waynepeking348 waynepeking348 merged commit 48338d0 into kubewharf:main May 24, 2023
luomingmeng pushed a commit to luomingmeng/katalyst-core that referenced this pull request Oct 11, 2024
@csfldf @luomingmeng
feat(qrm): support memory enhancement numa_exclusive (kubewharf#19)
ff06b11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynepeking348 waynepeking348 waynepeking348 approved these changes

@luomingmeng luomingmeng luomingmeng approved these changes

Assignees

@csfldf csfldf

Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged
Projects
Katalyst Development Dashboard
Status: 👀 In review
Milestone
v0.3
Development

Successfully merging this pull request may close these issues.

[Proposal] QRM cpu/memory plugins support memory enhancement numa_exclusive
4 participants
@csfldf @waynepeking348 @luomingmeng @caohe