importinto, disttask: allow multi import job & adjust total memory for cgroup v2 #51575

D3Hunter · 2024-03-07T04:56:01Z

What problem does this PR solve?

Issue Number: ref #49008

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

on a cgroup v2 32c64g node, physical mem is 62.9GiB and from cgroup we got 64GiB, but the real usable memory is 57.35GiB

before

[2024/03/11 04:45:31.337 +00:00] [INFO] [manager.go:94] ["build task executor manager"] [total-cpu=32] [total-mem=62.9GiB]

after

[2024/03/12 16:56:20.409 +00:00] [INFO] [manager.go:108] ["adjust memory limit for cgroup v2"] [before=62.9GiB] [after=56.32GiB]
[2024/03/12 16:56:20.409 +00:00] [INFO] [manager.go:114] ["build task executor manager"] [total-cpu=32] [total-mem=56.32GiB]

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

tiprow · 2024-03-07T04:56:19Z

Hi @D3Hunter. 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/test-infra repository.

codecov · 2024-03-07T05:15:40Z

Codecov Report

Merging #51575 (4f48b92) into master (478ab1f) will increase coverage by 1.9545%.
Report is 25 commits behind head on master.
The diff coverage is 57.1428%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #51575        +/-   ##
================================================
+ Coverage   70.7436%   72.6982%   +1.9545%     
================================================
  Files          1474       1476         +2     
  Lines        437068     441701      +4633     
================================================
+ Hits         309198     321109     +11911     
+ Misses       108517     100493      -8024     
- Partials      19353      20099       +746

Flag	Coverage Δ
integration	`49.0054% <42.8571%> (?)`
unit	`70.4923% <51.4285%> (-0.0687%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`50.9772% <ø> (+5.0761%)`	⬆️

This reverts commit 7c04d5a.

This reverts commit 3850549.

This reverts commit 95ca981.

Benjamin2037

LGTM

code changed a little

D3Hunter · 2024-03-13T16:41:43Z

/approve cancel

D3Hunter · 2024-03-13T16:42:11Z

/retest

tiprow · 2024-03-13T16:42:32Z

@D3Hunter: 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/test-infra repository.

D3Hunter · 2024-03-13T16:46:38Z

manually test again after code change

[2024/03/13 16:44:56.899 +00:00] [INFO] [manager.go:106] ["adjust memory limit for cgroup v2"] [before=62.9GiB] [after=56.32GiB]
[2024/03/13 16:44:56.899 +00:00] [INFO] [manager.go:111] ["build task executor manager"] [total-cpu=32] [total-mem=56.32GiB]

Benjamin2037

LGTM

ti-chi-bot · 2024-03-14T01:27:29Z

@Benjamin2037: Your lgtm message is repeated, so it is ignored.

In response to this:

LGTM

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.

Benjamin2037 · 2024-03-14T01:41:59Z

pkg/disttask/framework/taskexecutor/manager.go

@@ -91,6 +92,22 @@ func NewManager(ctx context.Context, id string, taskTable TaskTable) (*Manager,
 	if totalCPU <= 0 || totalMem <= 0 {
 		return nil, errors.Errorf("invalid cpu or memory, cpu: %d, memory: %d", totalCPU, totalMem)
 	}
+	cgroupLimit, version, err := cgroup.GetCgroupMemLimit()
+	// ignore the error of cgroup.GetCgroupMemLimit, as it's not a must-success step.
+	if err == nil && version == cgroup.V2 {


From the code, it seems only the Cgroup V2 return mem limit should be adjusted.

yes, v1 returns the hierarchy mem limit which is what we expect

ti-chi-bot · 2024-03-14T03:52:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, lance6716

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:

~~OWNERS~~ [Benjamin2037,lance6716]
~~pkg/disttask/OWNERS~~ [Benjamin2037,lance6716]
~~pkg/executor/importer/OWNERS~~ [lance6716]

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-03-14T03:52:30Z

[LGTM Timeline notifier]

Timeline:

2024-03-13 07:35:39.245708237 +0000 UTC m=+843766.267954625: ☑️ agreed by Benjamin2037.
2024-03-14 03:52:28.819535533 +0000 UTC m=+916775.841781921: ☑️ agreed by lance6716.

D3Hunter added 4 commits March 11, 2024 18:26

change

3e433db

change

2915129

change

6bf8152

changeg

2bc444e

D3Hunter force-pushed the multi-import-job branch from 2ce9162 to 2bc444e Compare March 12, 2024 02:49

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2024

D3Hunter added 2 commits March 12, 2024 10:59

revert some change

38d64d7

Merge branch 'incontainer' into multi-import-job

5debf21

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 Mar 12, 2024

D3Hunter added 2 commits March 12, 2024 11:41

change

e358b24

change block size

95ca981

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 12, 2024

D3Hunter added 5 commits March 12, 2024 18:30

change

3850549

change

7c04d5a

Revert "change"

ec00a97

This reverts commit 7c04d5a.

Revert "change"

469c212

This reverts commit 3850549.

Revert "change block size"

f06ecef

This reverts commit 95ca981.

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2024

Merge remote-tracking branch 'upstream/master' into multi-import-job

9166c89

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Mar 13, 2024

D3Hunter changed the title ~~[WIP]: Multi import job~~ [WIP]: allow multi import job & adjust total memory for cgroup v2 Mar 13, 2024

D3Hunter changed the title ~~[WIP]: allow multi import job & adjust total memory for cgroup v2~~ [WIP]importinto, disttask: allow multi import job & adjust total memory for cgroup v2 Mar 13, 2024

D3Hunter mentioned this pull request Mar 13, 2024

resource control inside dist framework #49008

Open

43 tasks

D3Hunter changed the title ~~[WIP]importinto, disttask: allow multi import job & adjust total memory for cgroup v2~~ importinto, disttask: allow multi import job & adjust total memory for cgroup v2 Mar 13, 2024

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 13, 2024

Benjamin2037 previously approved these changes Mar 13, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Mar 13, 2024

change

5315507

D3Hunter requested a review from Benjamin2037 March 13, 2024 13:20

D3Hunter removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Mar 13, 2024

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Mar 13, 2024

change

4f48b92

Benjamin2037 approved these changes Mar 14, 2024

View reviewed changes

Benjamin2037 reviewed Mar 14, 2024

View reviewed changes

lance6716 approved these changes Mar 14, 2024

View reviewed changes

ti-chi-bot bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 14, 2024

ti-chi-bot bot merged commit dbdae85 into pingcap:master Mar 14, 2024
23 checks passed

D3Hunter deleted the multi-import-job branch March 14, 2024 05:06

D3Hunter mentioned this pull request Mar 21, 2024

importinto: precheck to avoid run 2 jobs to import the same table #51997

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importinto, disttask: allow multi import job & adjust total memory for cgroup v2 #51575

importinto, disttask: allow multi import job & adjust total memory for cgroup v2 #51575

D3Hunter commented Mar 7, 2024 •

edited

Loading

tiprow bot commented Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

Benjamin2037 left a comment

D3Hunter commented Mar 13, 2024

D3Hunter commented Mar 13, 2024

tiprow bot commented Mar 13, 2024

D3Hunter commented Mar 13, 2024

Benjamin2037 left a comment

ti-chi-bot bot commented Mar 14, 2024

Benjamin2037 Mar 14, 2024

D3Hunter Mar 14, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 14, 2024

ti-chi-bot bot commented Mar 14, 2024

importinto, disttask: allow multi import job & adjust total memory for cgroup v2 #51575

importinto, disttask: allow multi import job & adjust total memory for cgroup v2 #51575

Conversation

D3Hunter commented Mar 7, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Mar 7, 2024

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

Benjamin2037 left a comment

Choose a reason for hiding this comment

D3Hunter commented Mar 13, 2024

D3Hunter commented Mar 13, 2024

tiprow bot commented Mar 13, 2024

D3Hunter commented Mar 13, 2024

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Mar 14, 2024

Benjamin2037 Mar 14, 2024

Choose a reason for hiding this comment

D3Hunter Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Mar 14, 2024

ti-chi-bot bot commented Mar 14, 2024

[LGTM Timeline notifier]

D3Hunter commented Mar 7, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

D3Hunter Mar 14, 2024 •

edited

Loading