Add pd global memory limiter and gctuner #5848

hnes · 2023-01-10T08:43:55Z

What problem does this PR solve?

Issue Number: Close #5827

What is changed and how does it work?

This PR is mainly based on the former support GC tuner PR from @rleungx and the gctuner implementation in TiDB

Add pd global memory limiter and gctuner

Check List

Tests

Unit test

Code changes

Has configuration change

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:

Release note

None

ti-chi-bot · 2023-01-10T08:43:58Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-01-10T08:44:05Z

Hi @hnes. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Ryan Leung <[email protected]>

Signed-off-by: Sen Han <[email protected]>

go.mod

pkg/cgroup/cgroup.go

pkg/gctuner/memory_limit_tuner.go

Signed-off-by: Sen Han <[email protected]>

pkg/cgroup/cgroup_cpu.go

server/config/config.go

Signed-off-by: Sen Han <[email protected]>

rleungx · 2023-02-08T01:57:15Z

/ok-to-test

Signed-off-by: Sen Han <[email protected]>

codecov · 2023-02-09T02:46:11Z

Codecov Report

Base: 75.35% // Head: 75.12% // Decreases project coverage by -0.23% ⚠️

Coverage data is based on head (e8f6755) compared to base (28a95cd).
Patch coverage: 66.05% of modified lines in pull request are covered.

❗ Current head e8f6755 differs from pull request most recent head 9312795. Consider uploading reports for the commit 9312795 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5848      +/-   ##
==========================================
- Coverage   75.35%   75.12%   -0.23%     
==========================================
  Files         348      358      +10     
  Lines       35486    36085     +599     
==========================================
+ Hits        26741    27110     +369     
- Misses       6414     6593     +179     
- Partials     2331     2382      +51

Flag	Coverage Δ
unittests	`75.12% <66.05%> (-0.23%)`	⬇️

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

Impacted Files	Coverage Δ
server/config/persist_options.go	`91.98% <0.00%> (-1.20%)`	⬇️
server/config/config.go	`75.92% <29.62%> (-1.69%)`	⬇️
pkg/memory/meminfo.go	`35.36% <35.36%> (ø)`
pkg/memory/memstats.go	`35.71% <35.71%> (ø)`
pkg/cgroup/cgroup_cpu_linux.go	`38.88% <38.88%> (ø)`
pkg/gogc/gogc.go	`63.63% <63.63%> (ø)`
pkg/cgroup/cgroup.go	`67.77% <67.77%> (ø)`
pkg/gctuner/tuner.go	`72.00% <72.00%> (ø)`
pkg/cgroup/cgroup_cpu.go	`78.12% <78.12%> (ø)`
server/cluster/cluster.go	`81.49% <78.84%> (-0.14%)`	⬇️
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Sen Han <[email protected]>

server/cluster/cluster.go

rleungx

The rest LGTM

Signed-off-by: Sen Han <[email protected]>

rleungx · 2023-02-09T05:09:53Z

server/cluster/cluster.go

+	defer tick.Stop()
+	totalMem, err := memory.MemTotal()
+	if err != nil {
+		log.Fatal("fail to get total-mem:%s", zap.Error(err))


Suggested change

log.Fatal("fail to get total-mem:%s", zap.Error(err))

log.Fatal("fail to get total memory", zap.Error(err))

Thanks a lot.

nolouch

rest lgtm

nolouch · 2023-02-09T05:18:06Z

server/cluster/cluster.go

+	defer logutil.LogPanic()
+	defer c.wg.Done()
+
+	tick := time.NewTicker(10 * time.Second)


better to be a const.

Signed-off-by: Sen Han <[email protected]>

nolouch · 2023-02-09T05:38:39Z

/merge

ti-chi-bot · 2023-02-09T05:38:40Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-02-09T05:38:42Z

This pull request has been accepted and is ready to merge.

Commit hash: 9312795

ti-chi-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jan 10, 2023

ti-chi-bot requested review from rleungx and Yisaer January 10, 2023 08:44

rleungx and others added 5 commits January 10, 2023 17:07

add previous implementation from Ryan

e83bb9f

Signed-off-by: Ryan Leung <[email protected]>

add gctuner implementations from tidb and some fixes

38f808d

Signed-off-by: Sen Han <[email protected]>

add configurations for gctuner

0b8fb41

Signed-off-by: Sen Han <[email protected]>

change the log level of memoryLimitTuner start into debug

0840914

Signed-off-by: Sen Han <[email protected]>

change some variable name in startGCTuner

f18ab7b

Signed-off-by: Sen Han <[email protected]>

hnes force-pushed the add-gctuner3 branch from b9a5a44 to f18ab7b Compare January 10, 2023 09:09

rleungx reviewed Jan 10, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/cgroup/cgroup.go Outdated Show resolved Hide resolved

pkg/gctuner/memory_limit_tuner.go Outdated Show resolved Hide resolved

hnes added 3 commits January 10, 2023 17:16

delete unnecessary items

5a9769f

Signed-off-by: Sen Han <[email protected]>

update the date in the copyright lines

98e13c7

Signed-off-by: Sen Han <[email protected]>

lint

538e604

Signed-off-by: Sen Han <[email protected]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

change minGCTunerThreshold into 0

cbfc8b9

Signed-off-by: Sen Han <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 12, 2023

rleungx reviewed Jan 18, 2023

View reviewed changes

pkg/cgroup/cgroup_cpu.go Outdated Show resolved Hide resolved

server/config/config.go Outdated Show resolved Hide resolved

hnes added 5 commits February 7, 2023 12:19

fix copyright titles

683ff0c

Signed-off-by: Sen Han <[email protected]>

add string opt into the tag of EnableGOGCTuner in PDServerConfig

7af9df8

Signed-off-by: Sen Han <[email protected]>

merge master into add-gctuner3

e4f3a34

Signed-off-by: Sen Han <[email protected]>

go mod tidy

6fb849f

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

3c9b2be

ti-chi-bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 8, 2023

hnes added 7 commits February 8, 2023 18:45

fix

e3d008d

Signed-off-by: Sen Han <[email protected]>

suppress some complain from gosec

b63970d

Signed-off-by: Sen Han <[email protected]>

rename getCgroupCPU into getCgroupCPUHelper

d62b44b

Signed-off-by: Sen Han <[email protected]>

fix

4585c55

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

d41ad71

default off

c293427

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

3f47f52

hnes added 2 commits February 9, 2023 12:26

fix

3a8d0f7

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

8844f6f

rleungx reviewed Feb 9, 2023

View reviewed changes

server/cluster/cluster.go Outdated Show resolved Hide resolved

server/cluster/cluster.go Outdated Show resolved Hide resolved

server/cluster/cluster.go Outdated Show resolved Hide resolved

rleungx approved these changes Feb 9, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 9, 2023

fix

ef0c569

Signed-off-by: Sen Han <[email protected]>

rleungx reviewed Feb 9, 2023

View reviewed changes

nolouch reviewed Feb 9, 2023

View reviewed changes

hnes added 2 commits February 9, 2023 13:18

fix

1a1eb35

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

e8f6755

nolouch approved these changes Feb 9, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 9, 2023

hnes added 2 commits February 9, 2023 13:33

use const instead of number directly

f15dbdd

Signed-off-by: Sen Han <[email protected]>

Merge branch 'master' into add-gctuner3

9312795

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 9, 2023

ti-chi-bot merged commit 0e08a5e into tikv:master Feb 9, 2023

lhy1024 mentioned this pull request Feb 9, 2023

*: make test is failed about cgroup #5953

Closed

bufferflies mentioned this pull request Jan 23, 2024

memory: support cgroup with systemd (#7627) #7664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pd global memory limiter and gctuner #5848

Add pd global memory limiter and gctuner #5848

hnes commented Jan 10, 2023 •

edited by nolouch

Loading

ti-chi-bot commented Jan 10, 2023 •

edited

Loading

ti-chi-bot commented Jan 10, 2023

rleungx commented Feb 8, 2023

codecov bot commented Feb 9, 2023 •

edited

Loading

rleungx left a comment

rleungx Feb 9, 2023

hnes Feb 9, 2023

nolouch left a comment

nolouch Feb 9, 2023

nolouch commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

	log.Fatal("fail to get total-mem:%s", zap.Error(err))
	log.Fatal("fail to get total memory", zap.Error(err))

Add pd global memory limiter and gctuner #5848

Add pd global memory limiter and gctuner #5848

Conversation

hnes commented Jan 10, 2023 • edited by nolouch Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Jan 10, 2023 • edited Loading

ti-chi-bot commented Jan 10, 2023

rleungx commented Feb 8, 2023

codecov bot commented Feb 9, 2023 • edited Loading

Codecov Report

rleungx left a comment

Choose a reason for hiding this comment

rleungx Feb 9, 2023

Choose a reason for hiding this comment

hnes Feb 9, 2023

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch Feb 9, 2023

Choose a reason for hiding this comment

nolouch commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

hnes commented Jan 10, 2023 •

edited by nolouch

Loading

ti-chi-bot commented Jan 10, 2023 •

edited

Loading

codecov bot commented Feb 9, 2023 •

edited

Loading