diff --git a/pkg/cli/interactive_tests/test_local_cmds.tcl b/pkg/cli/interactive_tests/test_local_cmds.tcl index 3707454ccc36..8982afebfea9 100755 --- a/pkg/cli/interactive_tests/test_local_cmds.tcl +++ b/pkg/cli/interactive_tests/test_local_cmds.tcl @@ -105,7 +105,30 @@ eexpect "display_format\ttsv" eexpect root@ end_test +start_test "Check that \\x toggles display format" +send "\\x\r\\set\r" +eexpect "Option*|*display_format" +eexpect "Value*|*records" +eexpect root@ + +send "\\x\r\\set\r" +eexpect "display_format*|*table" +eexpect root@ +end_test + +start_test "Check that \\x with on or off enables/disables records display format" +send "\\x on\r\\set\r" +eexpect "Option*|*display_format" +eexpect "Value*|*records" +eexpect root@ + +send "\\x off\r\\set\r" +eexpect "display_format*|*table" +eexpect root@ +end_test + start_test "Check various ways to set a boolean flag." +send "\\set display_format=tsv\r" send "\\set show_times=false\r\\set\r" eexpect "show_times\tfalse" eexpect root@ diff --git a/pkg/cli/sql.go b/pkg/cli/sql.go index 5dc2efad0b46..bebc3b0abfe7 100644 --- a/pkg/cli/sql.go +++ b/pkg/cli/sql.go @@ -79,6 +79,9 @@ Informational \du list the users for all databases. \d [TABLE] show details about columns in the specified table, or alias for '\dt' if no table is specified. +Formatting + \x [on|off] toggle records display format. + Operating System \! CMD run an external command and print its results on standard output. @@ -310,15 +313,13 @@ var options = map[string]struct { isBoolean: false, validDuringMultilineEntry: false, set: func(val string) error { - val = strings.ToLower(strings.TrimSpace(val)) - switch val { - case "false", "0", "off": - sqlCtx.autoTrace = "" - case "true", "1": - val = "on" - fallthrough - default: + b, err := parseBool(val) + if err != nil { sqlCtx.autoTrace = "on, " + val + } else if b { + sqlCtx.autoTrace = "on, on" + } else { + sqlCtx.autoTrace = "" } return nil }, @@ -482,15 +483,12 @@ func (c *cliState) handleSet(args []string, nextState, errState cliStateEnum) cl var err error if !opt.isBoolean { err = opt.set(val) + } else if b, e := parseBool(val); e != nil { + return c.invalidOptSet(errState, args) + } else if b { + err = opt.set("true") } else { - switch val { - case "true", "1", "on": - err = opt.set("true") - case "false", "0", "off": - err = opt.reset() - default: - return c.invalidOptSet(errState, args) - } + err = opt.reset() } if err != nil { @@ -1166,6 +1164,28 @@ func (c *cliState) doHandleCliCmd(loopState, nextState cliStateEnum) cliStateEnu } return c.invalidSyntax(errState, `%s. Try \? for help`, c.lastInputLine) + case `\x`: + format := tableDisplayRecords + switch len(cmd) { + case 1: + if cliCtx.tableDisplayFormat == tableDisplayRecords { + format = tableDisplayTable + } + case 2: + b, err := parseBool(cmd[1]) + if err != nil { + return c.invalidSyntax(errState, `%s. Try \? for help.`, c.lastInputLine) + } else if b { + format = tableDisplayRecords + } else { + format = tableDisplayTable + } + default: + return c.invalidSyntax(errState, `%s. Try \? for help.`, c.lastInputLine) + } + cliCtx.tableDisplayFormat = format + return loopState + case `\demo`: return c.handleDemo(cmd[1:], loopState, errState) diff --git a/pkg/cli/sql_util.go b/pkg/cli/sql_util.go index b881b9b81db8..9e07bfb2afae 100644 --- a/pkg/cli/sql_util.go +++ b/pkg/cli/sql_util.go @@ -1165,3 +1165,15 @@ func formatVal(val driver.Value, showPrintableUnicode bool, showNewLinesAndTabs return fmt.Sprint(val) } + +// parseBool parses a boolean string for use in slash commands. +func parseBool(s string) (bool, error) { + switch strings.TrimSpace(strings.ToLower(s)) { + case "true", "on", "yes", "1": + return true, nil + case "false", "off", "no", "0": + return false, nil + default: + return false, errors.Newf("invalid boolean value %q", s) + } +} diff --git a/pkg/cli/sql_util_test.go b/pkg/cli/sql_util_test.go index 9bf51f691ba6..f0a5a08edef2 100644 --- a/pkg/cli/sql_util_test.go +++ b/pkg/cli/sql_util_test.go @@ -23,6 +23,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestConnRecover(t *testing.T) { @@ -264,3 +266,39 @@ func TestTransactionRetry(t *testing.T) { t.Fatalf("expected transaction to require at least two tries, but it only required %d", tries) } } + +func TestParseBool(t *testing.T) { + defer leaktest.AfterTest(t)() + testcases := []struct { + input string + expect bool + expectErr bool + }{ + {"true", true, false}, + {"on", true, false}, + {"yes", true, false}, + {"1", true, false}, + {" TrUe ", true, false}, + + {"false", false, false}, + {"off", false, false}, + {"no", false, false}, + {"0", false, false}, + {" FaLsE ", false, false}, + + {"", false, true}, + {"foo", false, true}, + } + + for _, tc := range testcases { + t.Run(tc.input, func(t *testing.T) { + b, err := parseBool(tc.input) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expect, b) + } + }) + } +} diff --git a/pkg/util/cgroups/cgroups.go b/pkg/util/cgroups/cgroups.go index b549c858d7ab..5ccb8cd858f5 100644 --- a/pkg/util/cgroups/cgroups.go +++ b/pkg/util/cgroups/cgroups.go @@ -305,8 +305,29 @@ func getCgroupDetails(mountinfoPath string, cRoot string, controller string) (st } ver, ok := detectCgroupVersion(fields, controller) - if ok && (ver == 1 && string(fields[3]) == cRoot) || ver == 2 { - return string(fields[4]), ver, nil + if ok { + mountPoint := string(fields[4]) + if ver == 2 { + return mountPoint, ver, nil + } + // It is possible that the controller mount and the cgroup path are not the same (both are relative to the NS root). + // So start with the mount and construct the relative path of the cgroup. + // To test: + // cgcreate -t $USER:$USER -a $USER:$USER -g memory:crdb_test + // echo 3999997952 > /sys/fs/cgroup/memory/crdb_test/memory.limit_in_bytes + // cgexec -g memory:crdb_test ./cockroach start-single-node + // cockroach.log -> server/config.go:433 ⋮ system total memory: ‹3.7 GiB› + // without constructing the relative path + // cockroach.log -> server/config.go:433 ⋮ system total memory: ‹63 GiB› + nsRelativePath := string(fields[3]) + if !strings.Contains(nsRelativePath, "..") { + // We don't expect to see err here ever but in case that it happens + // the best action is to ignore the line and hope that the rest of the lines + // will allow us to extract a valid path. + if relPath, err := filepath.Rel(nsRelativePath, cRoot); err == nil { + return filepath.Join(mountPoint, relPath), ver, nil + } + } } } diff --git a/pkg/util/cgroups/cgroups_test.go b/pkg/util/cgroups/cgroups_test.go index 87e116c5a1ac..1c2d257f36d9 100644 --- a/pkg/util/cgroups/cgroups_test.go +++ b/pkg/util/cgroups/cgroups_test.go @@ -66,6 +66,15 @@ func TestCgroupsGetMemory(t *testing.T) { }, limit: 2936016896, }, + { + name: "fetches the limit for cgroup v1 when the NS relative paths of mount and cgroup don't match", + paths: map[string]string{ + "/proc/self/cgroup": v1CgroupWithMemoryControllerNS, + "/proc/self/mountinfo": v1MountsWithMemControllerNS, + "/sys/fs/cgroup/memory/cgroup_test/memory.stat": v1MemoryStat, + }, + limit: 2936016896, + }, { name: "fails when the stat file is missing for cgroup v2", paths: map[string]string{ @@ -167,6 +176,59 @@ func TestCgroupsGetCPU(t *testing.T) { system: uint64(123), user: uint64(456), }, + { + name: "fetches the cpu quota and usage for cgroup v1 where the mount and cgroup path don't match", + paths: map[string]string{ + "/proc/self/cgroup": v1CgroupWithCPUControllerNS, + "/proc/self/mountinfo": v1MountsWithCPUControllerNS, + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_quota_us": "12345", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_period_us": "67890", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_sys": "123", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_user": "456", + }, + quota: int64(12345), + period: int64(67890), + system: uint64(123), + user: uint64(456), + }, + { + name: "fetches the cpu quota and usage for cgroup v1 where the mount is relative", + paths: map[string]string{ + "/proc/self/cgroup": v1CgroupWithCPUControllerNSMountRel, + "/proc/self/mountinfo": v1MountsWithCPUControllerNSMountRel, + }, + errMsg: "failed to detect cgroup root mount and version", + }, + { + name: "fetches the cpu quota and usage for cgroup v1 where the mount and cgroup match but there is extra mount", + paths: map[string]string{ + "/proc/self/cgroup": v1CgroupWithCPUControllerNSMountRelRemount, + "/proc/self/mountinfo": v1MountsWithCPUControllerNSMountRelRemount, + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_quota_us": "12345", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_period_us": "67890", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_sys": "123", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_user": "456", + }, + quota: int64(12345), + period: int64(67890), + system: uint64(123), + user: uint64(456), + }, + { + name: "fetches the cpu quota and usage for cgroup v1 where the mount and cgroup match", + paths: map[string]string{ + "/proc/self/cgroup": v1CgroupWithCPUControllerNS2, + "/proc/self/mountinfo": v1MountsWithCPUControllerNS2, + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_quota_us": "12345", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpu.cfs_period_us": "67890", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_sys": "123", + "/sys/fs/cgroup/cpu,cpuacct/crdb_test/cpuacct.usage_user": "456", + }, + quota: int64(12345), + period: int64(67890), + system: uint64(123), + user: uint64(456), + }, { name: "fetches the cpu quota for cgroup v1 even if usage nonexistent", paths: map[string]string{ @@ -534,4 +596,34 @@ total_inactive_file 1363746816 total_active_file 308867072 total_unevictable 0 ` + + // Both /proc//mountinfo and /proc//cgroup will show the mount and the cgroup relative to the cgroup NS root + // This tests the case where the memory controller mount and the cgroup are not exactly the same (as is with k8s pods). + v1CgroupWithMemoryControllerNS = "12:memory:/cgroup_test" + v1MountsWithMemControllerNS = "50 35 0:44 / /sys/fs/cgroup/memory rw,nosuid,nodev,noexec,relatime shared:25 - cgroup cgroup rw,memory" + + // Example where the paths in /proc/self/mountinfo and /proc/self/cgroup are not the same for the cpu controller + // + // sudo cgcreate -t $USER:$USER -a $USER:$USER -g cpu:crdb_test + // echo 100000 > /sys/fs/cgroup/cpu/crdb_test/cpu.cfs_period_us + // echo 33300 > /sys/fs/cgroup/cpu/crdb_test/cpu.cfs_quota_us + // cgexec -g cpu:crdb_test ./cockroach ... + v1CgroupWithCPUControllerNS = "5:cpu,cpuacct:/crdb_test" + v1MountsWithCPUControllerNS = "43 35 0:37 / /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,cpu,cpuacct" + + // Same as above but with unshare -C + // Can't determine the location of the mount + v1CgroupWithCPUControllerNSMountRel = "5:cpu,cpuacct:/" + v1MountsWithCPUControllerNSMountRel = "43 35 0:37 /.. /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,cpu,cpuacct" + + // Same as above but with mounting the cgroup fs one more time in the NS + // sudo mount -t cgroup -o cpu,cpuacct none /sys/fs/cgroup/cpu,cpuacct/crdb_test + v1CgroupWithCPUControllerNSMountRelRemount = "5:cpu,cpuacct:/" + v1MountsWithCPUControllerNSMountRelRemount = ` +43 35 0:37 /.. /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,cpu,cpuacct +161 43 0:37 / /sys/fs/cgroup/cpu,cpuacct/crdb_test rw,relatime shared:95 - cgroup none rw,cpu,cpuacct +` + // Same as above but exiting the NS w/o unmounting + v1CgroupWithCPUControllerNS2 = "5:cpu,cpuacct:/crdb_test" + v1MountsWithCPUControllerNS2 = "161 43 0:37 /crdb_test /sys/fs/cgroup/cpu,cpuacct/crdb_test rw,relatime shared:95 - cgroup none rw,cpu,cpuacct" )