br: set memory limit #53793

Leavrth · 2024-06-04T07:03:44Z

What problem does this PR solve?

Issue Number: close #53777

Problem Summary:
need to automatically adjust GOMEMLIMIT for br clp

What changed and how does it work?

automatically adjust GOMEMLIMIT for br clp

[INFO] [meminfo.go:196] ["use physical memory hook"] [cgroupMemorySize=9223372036854771712] [physicalMemorySize=33731502080]
[INFO] [cmd.go:168] ["memory limit"] [limit=9223372036854775807]
[INFO] [cmd.go:189] [memory] [memtotal=33731502080] [memused=5004333056]
[INFO] [cmd.go:191] ["set memory limit"] [limit=28190298112]

[INFO] [meminfo.go:179] ["use cgroup memory hook because TiDB is in the container"]
[INFO] [cmd.go:168] ["memory limit"] [limit=9223372036854775807]
[INFO] [cmd.go:189] [memory] [memtotal=8589934592] [memused=971722752]
[INFO] [cmd.go:191] ["set memory limit"] [limit=7081340928]

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

tiprow · 2024-06-04T07:04:01Z

Hi @Leavrth. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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-sigs/prow repository.

codecov · 2024-06-04T07:31:55Z

Codecov Report

Attention: Patch coverage is 69.23077% with 12 lines in your changes missing coverage. Please review.

Project coverage is 60.5272%. Comparing base (d760a06) to head (1e15fea).
Report is 474 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #53793         +/-   ##
=================================================
- Coverage   72.5737%   60.5272%   -12.0466%     
=================================================
  Files          1506       1712        +206     
  Lines        431247     673023     +241776     
=================================================
+ Hits         312972     407362      +94390     
- Misses        98987     241598     +142611     
- Partials      19288      24063       +4775

Flag	Coverage Δ
integration	`42.0422% <69.2307%> (?)`
unit	`74.5862% <33.3333%> (+3.0291%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.3749% <ø> (+0.4092%)`	⬆️
parser	`∅ <ø> (∅)`
br	`62.9318% <69.2307%> (+21.1187%)`	⬆️

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

br/cmd/br/cmd.go

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

br/cmd/br/cmd.go

Tristan1900

Overall LGTM, thanks for the change. just a few questions and suggestions,

Tristan1900 · 2024-07-15T16:06:48Z

br/cmd/br/cmd.go

+			memlimit := calculateMemoryLimit(memleft)
+			log.Info("calculate the rest memory",
+				zap.Uint64("memtotal", memtotal), zap.Uint64("memused", memused), zap.Uint64("memlimit", memlimit))
+			debug.SetMemoryLimit(int64(memlimit))


to be extra safe, can we do a safe cast from uint64 to int64, something like

func safeCastUint64ToInt64(u uint64) int64 { if u > uint64(math.MaxInt64) { return math.MaxInt64 } return int64(u) }

and make it a util method?
cuz in the doc
A zero limit or a limit that's lower than the amount of memory used by the Go runtime may cause the garbage collector to run nearly continuously. However, the application may still make progress.

br/cmd/br/cmd.go

Tristan1900 · 2024-07-15T16:33:36Z

br/cmd/br/cmd.go

+	const halfGB = uint64(512 * 1024 * 1024)
+	const fourGB = 8 * halfGB
+	// memreserved = f(memleft) = 512MB * memleft / (memleft + 4GB)
+	//  * f(0) = 0


if the mem unused is 0, do we even bother to start the BR? I guess a correctly configured machine should not use up all its mem and if they do they are facing a bigger problem and we probably don't want to start a BR to make things more complicated. Let me know your thoughts!

I think maybe it is already OOM. Maybe we can directly limit the minimum remaining memory.

yeah, I feel like we can have some minimum memory not starting from 0, if below that we just throw error and exit.

I add the minimum value (256MiB) of memory limit. It's hard to determine the minimum memory br usage. In my test, br uses only about 200 MB~300 MB memory to backup data generated by tiup bench tpcc prepare --warehouses 3.

thanks for digging into that, yeah having an estimation of memory usage is very useful.

Tristan1900 · 2024-07-15T16:34:46Z

br/cmd/br/cmd.go

@@ -107,6 +110,23 @@ func AddFlags(cmd *cobra.Command) {
 	_ = cmd.PersistentFlags().MarkHidden(FlagRedactLog)
 }

+func calculateMemoryLimit(memleft uint64) uint64 {
+	const halfGB = uint64(512 * 1024 * 1024)


I feel like it would be better to put it into some util file for reuse.

done. Reuse the tidb/pkg/util/size.MB.

Tristan1900 · 2024-07-15T16:44:15Z

br/cmd/br/cmd.go

+	//  * f(0) = 0
+	//  * f(4GB) = 256MB
+	//  * f(+inf) -> 512MB
+	memreserved := halfGB / (1 + fourGB/(memleft+1))


extreme case, if memleft is unit64 max it's going to overflow I guess

in what case would memleft be uint64max (= 16 EiB) 👀

anyway changing memleft + 1 to memleft | 1 should be good enough

in the unexpected error case I would say, the memleft is passed down by memleft := memtotal - memused, if memtotal < memused, it can underflow as in my other comment. it's always better to explicitly check and make sure things won't go wrong for corners cases, cuz a lot of assumptions can turn out to be wrong in production based on my experience : ) (in this case the assumption is memory.MemTotal >= memory.MemUsed )

done. And add check memtotal < memused in another comment.

Tristan1900 · 2024-07-15T16:46:14Z

br/cmd/br/cmd.go

+				err = e
+				return
+			}
+			memleft := memtotal - memused


do we need to add a safety check here to make sure memtotal-memused won't underflow? since we own the memory package and it might have a bug that returned memtotal is actually smaller than memused

done. If underflow, output a warning log and skip set memory limit.

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

BornChanger · 2024-07-22T11:49:11Z

/retest

tiprow · 2024-07-22T11:49:33Z

@BornChanger: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

kennytm · 2024-07-22T14:55:53Z

/retest-required

tiprow · 2024-07-22T14:56:16Z

@kennytm: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest-required

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-sigs/prow repository.

Tristan1900 · 2024-07-22T15:42:44Z

br/cmd/br/cmd.go

+				memleft := memtotal - memused
+				memlimit := calculateMemoryLimit(memleft)
+				// BR command needs 256 MiB at least, if the left memory is less than 256 MiB,
+				// the memory limit cannot limit anyway and then finally OOM.


It seems at here the code will keep running and let the system OOM and stop.
In my mind it's better to explicitly panic or throw error by ourselves and exit early instead of actually OOM the system, OOM might cause unexpected issue to the other processes running on the same machine. what do you think?

The memory usage of br depends in part on the cluster tables' information. Only when br starts a domain to loads all the cluster's table's information into memory, it will know how much memory it will actually use at least. Maybe it is 200 MB or 1 GB, so I think it's hard to determine the memory usage here. Besides, setting go memory limit in this PR just makes the process runtime free the unused memory back to system in time, I think estimate the process memory usage is another issue.

If the command is br debug ..., it will use a little memory. Maybe more work is needed.

3pointer

LGTM

3pointer · 2024-08-05T05:45:55Z

br/cmd/br/cmd.go

+				log.Info("calculate the rest memory",
+					zap.Uint64("memtotal", memtotal), zap.Uint64("memused", memused), zap.Uint64("memlimit", memlimit))
+				// No need to set memory limit because the left memory is sufficient.
+				if memlimit < uint64(math.MaxInt64) {


when will memlimit large than math.MaxInt64

ti-chi-bot · 2024-08-05T05:46:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, BornChanger

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [3pointer,BornChanger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-08-05T05:46:13Z

[LGTM Timeline notifier]

Timeline:

2024-07-10 08:13:04.635116729 +0000 UTC m=+431681.870350842: ☑️ agreed by BornChanger.
2024-08-05 05:46:12.215652097 +0000 UTC m=+244502.082751186: ☑️ agreed by 3pointer.

Leavrth · 2024-08-05T06:11:21Z

/ok-to-test

Leavrth · 2024-08-05T07:17:38Z

/retest

tiprow · 2024-08-05T07:28:54Z

@Leavrth: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tidb_parser_test	`1e15fea`	link	true	`/test tidb_parser_test`
fast_test_tiprow	`1e15fea`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

set memory limit

c8f2b0a

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 4, 2024

set memory limit

5ac7699

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

kennytm reviewed Jun 5, 2024

View reviewed changes

br/cmd/br/cmd.go Outdated Show resolved Hide resolved

calculate the memory limit

55bc0b4

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 5, 2024

bazel prepare

e01fe8b

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

BornChanger reviewed Jun 5, 2024

View reviewed changes

br/cmd/br/cmd.go Show resolved Hide resolved

BornChanger mentioned this pull request Jun 5, 2024

br: reduce memory footprint and HWM (high water mark) #51573

Open

BornChanger approved these changes Jul 10, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 10, 2024

Tristan1900 reviewed Jul 15, 2024

View reviewed changes

Leavrth added 4 commits July 19, 2024 15:49

commit some suggestions

438d462

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

add the minimum value of memlimit

e1650d5

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

commit some suggestions

d9c42cb

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

commit some suggestions

1e15fea

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

BornChanger requested review from Tristan1900 and BornChanger July 22, 2024 12:23

Tristan1900 reviewed Jul 22, 2024

View reviewed changes

3pointer approved these changes Aug 5, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 5, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Aug 5, 2024

ti-chi-bot bot merged commit 2c47cd5 into pingcap:master Aug 5, 2024
30 of 32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: set memory limit #53793

br: set memory limit #53793

Leavrth commented Jun 4, 2024 •

edited

Loading

tiprow bot commented Jun 4, 2024

codecov bot commented Jun 4, 2024 •

edited

Loading

Tristan1900 left a comment

Tristan1900 Jul 15, 2024

Leavrth Jul 19, 2024 •

edited

Loading

Tristan1900 Jul 15, 2024

Leavrth Jul 17, 2024

Tristan1900 Jul 18, 2024

Leavrth Jul 19, 2024

Tristan1900 Jul 22, 2024 •

edited

Loading

Tristan1900 Jul 15, 2024

Leavrth Jul 19, 2024

Tristan1900 Jul 15, 2024

kennytm Jul 17, 2024

Tristan1900 Jul 18, 2024 •

edited

Loading

Leavrth Jul 19, 2024

Tristan1900 Jul 15, 2024 •

edited

Loading

Leavrth Jul 19, 2024

BornChanger commented Jul 22, 2024

tiprow bot commented Jul 22, 2024

kennytm commented Jul 22, 2024

tiprow bot commented Jul 22, 2024

Tristan1900 Jul 22, 2024 •

edited

Loading

Tristan1900 Jul 22, 2024

Leavrth Jul 26, 2024

Leavrth Jul 26, 2024

3pointer left a comment

3pointer Aug 5, 2024

ti-chi-bot bot commented Aug 5, 2024

ti-chi-bot bot commented Aug 5, 2024

Leavrth commented Aug 5, 2024

Leavrth commented Aug 5, 2024

tiprow bot commented Aug 5, 2024

br: set memory limit #53793

br: set memory limit #53793

Conversation

Leavrth commented Jun 4, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jun 4, 2024

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

Tristan1900 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leavrth Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tristan1900 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tristan1900 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tristan1900 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BornChanger commented Jul 22, 2024

tiprow bot commented Jul 22, 2024

kennytm commented Jul 22, 2024

tiprow bot commented Jul 22, 2024

Tristan1900 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3pointer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 5, 2024

ti-chi-bot bot commented Aug 5, 2024

[LGTM Timeline notifier]

Leavrth commented Aug 5, 2024

Leavrth commented Aug 5, 2024

tiprow bot commented Aug 5, 2024

Leavrth commented Jun 4, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

Leavrth Jul 19, 2024 •

edited

Loading

Tristan1900 Jul 22, 2024 •

edited

Loading

Tristan1900 Jul 18, 2024 •

edited

Loading

Tristan1900 Jul 15, 2024 •

edited

Loading

Tristan1900 Jul 22, 2024 •

edited

Loading