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

memory advisor plugins #143

Merged
merged 3 commits into from Jul 18, 2023
Merged

memory advisor plugins #143

merged 3 commits into from Jul 18, 2023

Conversation

cheney-lin
Copy link
Member

What type of PR is this?

Features

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/memadvisor branch 3 times, most recently from 5d7328d to 043e2b5 Compare July 12, 2023 03:25
@cheney-lin cheney-lin changed the title Dev/memadvisor memory advisor plugins Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023 •
edited

Codecov Report

Patch coverage: 81.49% and project coverage change: +0.16 🎉

Comparison is base (3ecec50) 50.48% compared to head (73a603b) 50.65%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
+ Coverage   50.48%   50.65%   +0.16%     
==========================================
  Files         393      399       +6     
  Lines       37728    37993     +265     
==========================================
+ Hits        19048    19245     +197     
- Misses      16538    16600      +62     
- Partials     2142     2148       +6
Flag Coverage Δ
unittest 50.65% <81.49%> (+0.16%) ⬆️

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

Impacted Files Coverage Δ
...kg/metaserver/agent/metric/malachite/cgroup/cri.go 0.00% <ø> (ø)
pkg/scheduler/plugins/qosawarenoderesources/fit.go 61.42% <ø> (ø)
pkg/agent/qrm-plugins/cpu/dynamicpolicy/policy.go 43.31% <25.00%> (-0.06%) ⬇️
pkg/metaserver/agent/metric/helper/pod.go 68.42% <68.42%> (ø)
...in/qosaware/resource/memory/plugin/memory_guard.go 69.38% <69.38%> (ø)
...advisor/plugin/qosaware/resource/memory/advisor.go 78.91% <70.90%> (+5.70%) ⬆️
pkg/metaserver/agent/metric/helper/system.go 72.30% <72.30%> (ø)
pkg/metaserver/agent/metric/helper/container.go 73.33% <73.33%> (ø)
.../agent/qrm-plugins/cpu/dynamicpolicy/state/util.go 74.72% <80.00%> (+2.43%) ⬆️
...in/qosaware/resource/memory/plugin/cache_reaper.go 91.30% <91.30%> (ø)
... and 17 more

... and 15 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.

@cheney-lin cheney-lin force-pushed the dev/memadvisor branch 4 times, most recently from 21a2394 to 82e34ae Compare July 13, 2023 09:51
@cheney-lin cheney-lin added enhancement New feature or request workflow/need-review review: test succeeded, need to review labels Jul 13, 2023
waynepeking348
waynepeking348 reviewed Jul 16, 2023
View reviewed changes
pkg/agent/sysadvisor/metacache/metacache.go Outdated Show resolved Hide resolved
pkg/agent/evictionmanager/plugin/memory/numa_pressure.go Outdated
@@ -30,6 +30,7 @@ import (
"github.com/kubewharf/katalyst-core/pkg/client"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we must modify so much for eviction logic? we have currently made it online running for this logic, so if you want to modify this, please contact with @zzzzhhb and make sure you have fully tested (compared with current version) and access permission from @zzzzhhb

Copy link
Member Author

Choose a reason for hiding this comment

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

These modifications do not involve functional changes, but only extract common public functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you should confirm this with @zzzzhhb

pkg/util/qos/qos_helper.go Outdated Show resolved Hide resolved
pkg/consts/metric.go Outdated
@@ -231,3 +231,29 @@ const (
MetricsMemFilePerNumaCgroup = "mem.file.numa.cgroup"
MetricsMemAnonPerNumaCgroup = "mem.anon.numa.cgroup"
)

const (
Copy link
Collaborator

Choose a reason for hiding this comment

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

this defines metric rather than metrics (to emit); actually, we should the metrics in wherever requires them, and no need to define them in a general place

Copy link
Member Author

Choose a reason for hiding this comment

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

They are defined here because they are referenced by eviction and memory advisor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

they should not be defined here, the reason is as above

Copy link
Collaborator

Choose a reason for hiding this comment

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

generally, we don't encourage different components to share the same metric names

Copy link
Collaborator

@zzzzhhb zzzzhhb Jul 17, 2023 •
edited

Choose a reason for hiding this comment

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

maybe we should use different name for these two "metrics", it‘s really easy to get confused. For example:
runtime metric: the information about running containers, pods and system;
metric: the data we used to monitor katalyst itself.

for this certain case, I agree with @waynepeking348 . Maybe someday we will deploy advisor and eviction manager seperately and they might be maintained by different people. If they use same metrics name, it will be another confusing thing because these metrics don't only reflect the status about one component.

pkg/agent/sysadvisor/plugin/qosaware/resource/memory/advisor.go Show resolved Hide resolved
@cheney-lin cheney-lin force-pushed the dev/memadvisor branch 4 times, most recently from ab9810a to 7d74336 Compare July 17, 2023 05:29
@cheney-lin cheney-lin requested a review from zzzzhhb July 17, 2023 06:12
@cheney-lin cheney-lin force-pushed the dev/memadvisor branch 4 times, most recently from 88754df to 7efa5c3 Compare July 18, 2023 03:34
waynepeking348
waynepeking348 previously approved these changes Jul 18, 2023
View reviewed changes
zzzzhhb
zzzzhhb reviewed Jul 18, 2023
View reviewed changes
pkg/agent/evictionmanager/plugin/memory/rss_overuse.go
@@ -164,7 +165,7 @@ func (r *RssOveruseEvictionPlugin) GetEvictPods(_ context.Context, request *plug
continue
}

podRss, found := r.evictionHelper.getPodMetric(pod, consts.MetricMemRssContainer, nonExistNumaID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like evictionHelper is useless in this plugin, maybe we can delete it.

@cheney-lin
feat(sysadvisor): introduce memadvisor plugin cache_reaper
a56a817 
Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
@cheney-lin
feat(sysadvisor): introduce memory guard plugin
79b16e6 
Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
@cheney-lin
feat(sysadvisor): introduce memset binder
73a603b 
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 18, 2023
@waynepeking348 waynepeking348 merged commit d408f67 into kubewharf:main Jul 18, 2023
12 checks passed
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

@zzzzhhb zzzzhhb zzzzhhb approved these changes

Assignees

@cheney-lin cheney-lin

Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cheney-lin @waynepeking348 @zzzzhhb