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(eviction): numa pressure support victim minimum usage thr… #296

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

zzzzhhb
Copy link
Collaborator

@zzzzhhb zzzzhhb commented Sep 26, 2023

…eshold

What type of PR is this?

Enhancements

What this PR does / why we need it:

This PR support NumaVictimMinimumUsageThreshold configuration. DefaultNumaVictimMinimumUsageThreshold is the victim's minimum memory usage on a NUMA node. If a pod uses less memory on a NUMA node than this threshold, it won't be evicted by this NUMA's memory pressure.

Which issue(s) this PR fixes:

Special notes for your reviewer:

@zzzzhhb zzzzhhb self-assigned this Sep 26, 2023
@zzzzhhb zzzzhhb added enhancement New feature or request workflow/need-review review: test succeeded, need to review labels Sep 26, 2023
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

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

Comparison is base (fc80e13) 53.26% compared to head (de0fa2c) 53.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
+ Coverage   53.26%   53.33%   +0.06%     
==========================================
  Files         397      397              
  Lines       43432    43448      +16     
==========================================
+ Hits        23135    23172      +37     
+ Misses      17758    17726      -32     
- Partials     2539     2550      +11     
Flag Coverage Δ
unittest 53.33% <80.00%> (+0.06%) ⬆️

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

Files Coverage Δ
...t/evictionmanager/plugin/memory/system_pressure.go 72.95% <100.00%> (ø)
...amic/adminqos/eviction/memory_pressure_eviction.go 52.83% <50.00%> (-0.24%) ⬇️
...ent/evictionmanager/plugin/memory/numa_pressure.go 80.39% <85.00%> (+8.00%) ⬆️
pkg/metaserver/agent/metric/helper/system.go 68.96% <80.00%> (-3.35%) ⬇️

... and 14 files with indirect coverage changes

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

@zzzzhhb zzzzhhb requested a review from csfldf September 26, 2023 08:05
go.mod Outdated
@@ -141,6 +141,7 @@ require (
)

replace (
github.com/kubewharf/katalyst-api => github.com/zzzzhhb/katalyst-api v0.0.0-20230926073642-3971260393a0
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls fix this import

return GetNodeMetricWithTime(metricsFetcher, emitter, consts.MetricMemKswapdstealSystem)
}

func GetNodeMetricWithTime(metricsFetcher metric.MetricsFetcher, emitter metrics.MetricEmitter, metricName string) (metricutil.MetricData, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

GetNodeMetricWithTime is kind of weird, why node needs a function like this while numa not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It depends if there is a caller who needs timestamp of metric. For now, only node metrics faces this kind of need, so I just provide this kind of function for node metrics. I'll provide one for numa metrics to make them looks more consistent.

return metricWithTime.Value, err
}

func GetNumaMetric(metricsFetcher metric.MetricsFetcher, emitter metrics.MetricEmitter, metricName string, numaID int) (float64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

some functions should be exported outside while others not, we should apparently separate them, either by a different file or lowercased functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

which one should not be exported outside?

@zzzzhhb zzzzhhb force-pushed the optimize_memory_eviction branch from 733e6a7 to 25fc574 Compare September 27, 2023 02:51
@zzzzhhb zzzzhhb force-pushed the optimize_memory_eviction branch from 25fc574 to de0fa2c Compare September 27, 2023 03:11
@waynepeking348 waynepeking348 merged commit 435e927 into kubewharf:main Oct 7, 2023
9 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/need-review review: test succeeded, need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants