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

feat(qrm): implement a native policy for cpu qrm plugin #144

Merged
merged 9 commits into from Jul 27, 2023

Conversation

caohe
Copy link
Member

@caohe caohe commented Jul 13, 2023

What type of PR is this?

Features

What this PR does / why we need it:

  • implement a native policy for cpu qrm plugin
  • refine the dynamic policy for cpu qrm plugin

Which issue(s) this PR fixes:

Special notes for your reviewer:

This PR needs #150 to be merged first.

@caohe caohe added the enhancement New feature or request label Jul 13, 2023
@caohe caohe added this to the v0.3 milestone Jul 13, 2023
@caohe caohe marked this pull request as draft July 13, 2023 07:49
@caohe caohe added workflow/draft draft: no need to review workflow/merge-hold merge-hold: code is ready but still has dependency and removed workflow/merge-hold merge-hold: code is ready but still has dependency labels Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Patch coverage: 51.13% and project coverage change: -0.26% ⚠️

Comparison is base (a6c6b38) 51.21% compared to head (ab1dc70) 50.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   51.21%   50.95%   -0.26%     
==========================================
  Files         412      418       +6     
  Lines       39094    40409    +1315     
==========================================
+ Hits        20023    20592     +569     
- Misses      16834    17562     +728     
- Partials     2237     2255      +18     
Flag Coverage Δ
unittest 50.95% <51.13%> (-0.26%) ⬇️

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

Files Changed Coverage Δ
...-plugins/cpu/dynamicpolicy/policy_async_handler.go 15.82% <0.00%> (ø)
...-plugins/cpu/dynamicpolicy/policy_hint_handlers.go 48.22% <0.00%> (+8.69%) ⬆️
pkg/util/native/container.go 55.55% <0.00%> (-44.45%) ⬇️
...kg/agent/qrm-plugins/cpu/nativepolicy/util/util.go 11.76% <11.76%> (ø)
...m-plugins/cpu/nativepolicy/policy_async_handler.go 15.68% <15.68%> (ø)
pkg/agent/qrm-plugins/cpu/dynamicpolicy/policy.go 43.43% <16.66%> (ø)
...ins/cpu/nativepolicy/policy_allocation_handlers.go 26.63% <26.63%> (ø)
pkg/agent/qrm-plugins/cpu/nativepolicy/policy.go 27.84% <27.84%> (ø)
pkg/agent/qrm-plugins/util/util.go 54.16% <47.05%> (-2.47%) ⬇️
...m-plugins/cpu/nativepolicy/policy_hint_handlers.go 61.90% <61.90%> (ø)
... and 9 more

... and 15 files with indirect coverage changes

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

@caohe caohe force-pushed the add-native-policy branch 5 times, most recently from 38e98ba to 6aad57f Compare July 17, 2023 03:35
@caohe caohe marked this pull request as ready for review July 17, 2023 03:46
@caohe caohe requested review from waynepeking348, luomingmeng and csfldf and removed request for waynepeking348 July 17, 2023 03:47
@caohe caohe added workflow/need-review review: test succeeded, need to review workflow/merge-hold merge-hold: code is ready but still has dependency and removed workflow/draft draft: no need to review labels Jul 17, 2023
@caohe caohe added workflow/merge-ready merge-ready: code is ready and can be merged and removed workflow/merge-hold merge-hold: code is ready but still has dependency labels Jul 26, 2023
@caohe caohe force-pushed the add-native-policy branch 2 times, most recently from fcdbe8f to f90628a Compare July 26, 2023 07:16
Signed-off-by: caohe <caohe9603@gmail.com>
Signed-off-by: caohe <caohe9603@gmail.com>
Signed-off-by: caohe <caohe9603@gmail.com>
Signed-off-by: caohe <caohe9603@gmail.com>
Signed-off-by: caohe <caohe9603@gmail.com>
Signed-off-by: caohe <caohe9603@gmail.com>
@caohe caohe force-pushed the add-native-policy branch 4 times, most recently from a2de941 to 269b5be Compare July 26, 2023 08:46
Signed-off-by: caohe <caohe9603@gmail.com>
csfldf
csfldf previously approved these changes Jul 26, 2023
@@ -402,6 +407,7 @@ func (ns *NUMANodeState) SetAllocationInfo(podUID string, containerName string,
}

// GetDefaultCPUSet returns default cpuset in this node
// TODO: add lock to NUMANodeMap
Copy link
Collaborator

Choose a reason for hiding this comment

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

since stateCheckpoint functions are all under locks, it seems that we don't need to add lock in the operations in
specifc state struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

@@ -19,4 +19,16 @@ package consts
const (
// KubeletQoSResourceManagerCheckpoint is the name of the checkpoint file for kubelet QoS resource manager
KubeletQoSResourceManagerCheckpoint = "kubelet_qrm_checkpoint"

// CPUResourcePluginPolicyNameDynamic is the name of the dynamic policy.
CPUResourcePluginPolicyNameDynamic = "dynamic"
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it be better to put cpu related consts under cpu package?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

SkipCPUStateCorruption: false,
EnableCPUPressureEviction: false,
EnableSyncingCPUIdle: false,
EnableCPUIdle: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it's better to separate options/configs for different policies, ie.

  1. define general options/configs in CPUOptions/CPUConfigurations
  2. define native-policy-specified options/configs in CPUNativePolicyOptions/CPUNativePolicyConfigurations
  3. define dynamic-policy-specified options/configs in CPUDynamicPolicyOptions/CPUDynamicPolicyConfiguration

Copy link
Member Author

Choose a reason for hiding this comment

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

done

waynepeking348
waynepeking348 previously approved these changes Jul 27, 2023
@caohe caohe requested a review from csfldf July 27, 2023 02:34
Signed-off-by: caohe <caohe9603@gmail.com>
@waynepeking348 waynepeking348 merged commit 2e80ceb into kubewharf:main Jul 27, 2023
10 checks passed
@caohe caohe deleted the add-native-policy branch July 27, 2023 03:13
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: 👀 In review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants