Support THP in tiup cluster check #964

anywhy · 2020-12-03T01:37:10Z

What problem does this PR solve?

try implement #798

What is changed and how it works?

Check List

Tests

Integration test

Release notes:

NONE

codecov-io · 2020-12-03T01:52:06Z

Codecov Report

Merging #964 (331114f) into master (a9370cd) will decrease coverage by 3.82%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #964      +/-   ##
==========================================
- Coverage   55.44%   51.61%   -3.83%     
==========================================
  Files         263      263              
  Lines       19833    19854      +21     
==========================================
- Hits        10996    10248     -748     
- Misses       7118     7972     +854     
+ Partials     1719     1634      -85

Flag	Coverage Δ
cluster	`38.35% <0.00%> (-5.26%)`	⬇️
dm	`24.18% <0.00%> (-0.09%)`	⬇️
integrate	`46.38% <0.00%> (-3.88%)`	⬇️
playground	`20.56% <ø> (ø)`
tiup	`16.79% <ø> (ø)`
unittest	`22.94% <0.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
components/cluster/command/check.go	`6.27% <0.00%> (-72.93%)`	⬇️
pkg/cluster/operation/check.go	`0.00% <0.00%> (-51.95%)`	⬇️
pkg/cluster/task/check.go	`0.00% <0.00%> (-24.44%)`	⬇️
pkg/cluster/task/limits.go	`0.00% <0.00%> (-68.75%)`	⬇️
pkg/cluster/task/sysctl.go	`0.00% <0.00%> (-66.67%)`	⬇️
components/cluster/command/audit.go	`27.27% <0.00%> (-54.55%)`	⬇️
pkg/cluster/task/rmdir.go	`0.00% <0.00%> (-50.00%)`	⬇️
pkg/cluster/operation/operation.go	`34.78% <0.00%> (-43.48%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9370cd...331114f. Read the comment docs.

9547 · 2020-12-03T16:10:00Z

pkg/cluster/operation/check.go

@@ -58,6 +58,7 @@ var (
 	CheckNameSELinux     = "selinux"
 	CheckNameCommand     = "command"
 	CheckNameFio         = "fio"
+	CheckNameTHP         = "THP"


To keep up with the previous code style, use lowercase

thx, got it

9547 · 2020-12-03T16:11:49Z

pkg/cluster/task/check.go

@@ -33,6 +33,7 @@ var (
 	CheckTypePackage      = "package"
 	CheckTypePartitions   = "partitions"
 	CheckTypeFIO          = "fio"
+	CheckTypeTHP          = "THP"


IMO, THP should be part of CheckTypeSystemConfig

9547 · 2020-12-03T16:23:21Z

pkg/cluster/operation/check.go

@@ -651,3 +652,20 @@ func CheckFIOResult(rr, rw, lat []byte) []*CheckResult {

 	return results
 }
+
+// CheckTHPEnabled checks THP in /sys/kernel/mm/transparent_hugepage/enabled,/sys/kernel/mm/transparent_hugepage/defrag
+func CheckTHPEnabled(opt *CheckOptions, l []byte) []*CheckResult {


s/CheckTHPEnabled/CheckTHP/

9547 · 2020-12-03T16:24:35Z

pkg/cluster/operation/check.go

+	var results []*CheckResult
+
+	for _, line := range strings.Split(string(l), "\n") {
+		if !strings.Contains(line, "[never]") {


IMO, if THP is disabled, we should also add a Msg result

9547 · 2020-12-03T16:24:50Z

tests/tiup-cluster/script/cmd_subtest.sh

@@ -31,6 +31,7 @@ function cmd_subtest() {
    echo $check_result | grep "os-version"
    echo $check_result | grep "selinux"
    echo $check_result | grep "service"
+    echo $check_result | grep "THP"


9547 · 2020-12-03T16:28:34Z

components/cluster/command/check.go

+				).
+				Shell(
+					inst.GetHost(),
+					"cat /sys/kernel/mm/transparent_hugepage/defrag",


Maybe you can merge the two command into one:

cat /sys/kernel/mm/transparent_hugepage/{enabled,defrag}

IMO, maybe you can implement the THP Check like CheckSELinux()

9547 · 2020-12-05T09:08:01Z

components/cluster/command/check.go

@@ -442,6 +442,11 @@ func fixFailedChecks(ctx *task.Context, host string, res *operator.CheckResult,
 			),
 			true)
 		msg = fmt.Sprintf("will try to %s, reboot might be needed", color.HiBlueString("disable SELinux"))
+	case operator.CheckNameTHP:
+		t.Shell(host,
+			"echo never > /sys/kernel/mm/transparent_hugepage/enabled && echo never > /sys/kernel/mm/transparent_hugepage/defrag",


This will disable THP in runtime, if the system rebooted, the THP's status maybe enabled again? Should we disable it permanently?

Need to permanently disabled: seems need to change the grub.cfg, but this file maybe differ in different Linux System;

Don't need to: we should tell the user this fix is available until reboot

I perfer to the latter one.

Since the origin issue is reference to tidb-ansible, maybe just keep the same behavior as tidb-ansible is enough

It seems this config does not have an entry in sysctl, so the only way to make it consist after reboot would be adding a kernel parameter at boot. ref https://www.kernel.org/doc/html/v5.4/admin-guide/mm/transhuge.html#boot-parameter

It's unpractical to add a kernel parameter, just remind the user to run tiup cluster check again after reboot.

9547

/lgtm

lucklove · 2020-12-09T03:01:31Z

/merge

ti-chi-bot · 2020-12-09T03:01:45Z

@lucklove: adding 'status/can-merge' to this PR must have 1 LGTMs

In response to this:

/merge

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 tidb-community-bots/ti-community-prow repository.

lucklove · 2020-12-09T03:17:23Z

/lgtm

The bot will ignore the lgtm in this case： https://book.prow.tidb.io/plugins/lgtm.html#%E4%B8%BA%E4%BB%80%E4%B9%88%E6%88%91%E4%BD%BF%E7%94%A8-github-review-%E5%8A%9F%E8%83%BD%E7%9A%84-approve-%E5%B9%B6%E4%B8%94%E5%9C%A8-comment-%E4%B8%AD%E5%A1%AB%E5%86%99-lgtm-%E5%AF%BC%E8%87%B4-approve-%E6%97%A0%E6%95%88

lucklove · 2020-12-09T03:18:11Z

/lgtm

lucklove · 2020-12-09T03:18:42Z

/merge

ti-chi-bot · 2020-12-09T03:18:46Z

Can merge label has been added.

Git tree hash: 9725709

ti-chi-bot · 2020-12-09T03:28:32Z

@anywhy: Your PR has out-of-dated and I have automatically updated it for you.

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 tidb-community-bots/ti-community-prow repository.

Introduced by pingcap#964 There is a newline at the end of output of cat command, so the '[never]' check will fail on the empty newline.

Introduced by #964 There is a newline at the end of output of cat command, so the '[never]' check will fail on the empty newline.

ti-chi-bot requested review from lonng and lucklove December 3, 2020 01:37

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2020

9547 reviewed Dec 3, 2020

View reviewed changes

Support THP in tiup cluster check

ff6a572

anywhy force-pushed the disable_THP branch from e6818e3 to ff6a572 Compare December 4, 2020 05:09

anywhy added 2 commits December 4, 2020 13:11

fix typo

fc51772

update err msg

81bd712

9547 reviewed Dec 5, 2020

View reviewed changes

lucklove and others added 3 commits December 8, 2020 12:10

Merge branch 'master' into disable_THP

ab097e0

Update auto fix prompt message

153a943

Merge branch 'master' into disable_THP

a2eec36

9547 approved these changes Dec 8, 2020

View reviewed changes

Merge branch 'master' into disable_THP

9725709

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

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

Merge branch 'master' into disable_THP

331114f

ti-chi-bot merged commit 250c47b into pingcap:master Dec 9, 2020

anywhy deleted the disable_THP branch December 19, 2020 11:50

lucklove added a commit to lucklove/tiup that referenced this pull request Dec 22, 2020

Fix tail newline on thp check

26748bb

Introduced by pingcap#964 There is a newline at the end of output of cat command, so the '[never]' check will fail on the empty newline.

lucklove mentioned this pull request Dec 22, 2020

Fix tail newline on thp check #1005

Merged

ti-chi-bot pushed a commit that referenced this pull request Dec 24, 2020

Fix tail newline on thp check (#1005)

081877a

Introduced by #964 There is a newline at the end of output of cat command, so the '[never]' check will fail on the empty newline.

lucklove added a commit that referenced this pull request Dec 31, 2020

Fix tail newline on thp check (#1005)

0de52d5

Introduced by #964 There is a newline at the end of output of cat command, so the '[never]' check will fail on the empty newline.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support THP in tiup cluster check #964

Support THP in tiup cluster check #964

anywhy commented Dec 3, 2020 •

edited

Loading

codecov-io commented Dec 3, 2020 •

edited

Loading

9547 Dec 3, 2020

anywhy Dec 4, 2020

9547 Dec 3, 2020

9547 Dec 3, 2020

9547 Dec 3, 2020

9547 Dec 3, 2020

9547 Dec 3, 2020

9547 Dec 5, 2020 •

edited

Loading

lucklove Dec 8, 2020

AstroProfundis Dec 8, 2020

lucklove Dec 8, 2020

9547 left a comment

lucklove commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

lucklove commented Dec 9, 2020 •

edited

Loading

lucklove commented Dec 9, 2020

lucklove commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

Support THP in tiup cluster check #964

Support THP in tiup cluster check #964

Conversation

anywhy commented Dec 3, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Dec 3, 2020 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9547 Dec 5, 2020 • 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

9547 left a comment

Choose a reason for hiding this comment

lucklove commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

lucklove commented Dec 9, 2020 • edited Loading

lucklove commented Dec 9, 2020

lucklove commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

ti-chi-bot commented Dec 9, 2020

anywhy commented Dec 3, 2020 •

edited

Loading

codecov-io commented Dec 3, 2020 •

edited

Loading

9547 Dec 5, 2020 •

edited

Loading

lucklove commented Dec 9, 2020 •

edited

Loading