Merge #56829 #56840

56829: cli: add \x command to toggle records display format r=knz a=erikgrinaker

`psql` has a `\x` command to toggle extended output for results. CRDB
already supports this output format via `\set display_format=records`.
This commit adds `\x [on|off]` to toggle between `records` and `table`
display formats.

The `auto` option from `psql` to automatically enable extended output
depending on the result width is not supported, only `on` and `off`.

Resolves #56706.

Release note (cli change): A `\x [on|off]` command has been added to
toggle the `records` display format, following `psql` behavior.

Also adds support for `yes` and `no` as boolean options for slash commands as a separate commit, following `psql`.


56840: util, server: fix memory detection with non-root cgroups r=darinpp a=darinpp

The cgroup information is constructed by looking at `/proc/self/cgroup`,
matching with the mount point in `/proc/self/mountinfo`
and then inpsecting the `memory.stat` file. The matching between the
cgroup and the mount point was working only in case that the mount and
the cgroup are exact match relative to cgroup NS. This is however
inadequate as while true for the docker case, it isn't true in the general
case. In this example determining the cgroup memory limit isn't possible:
```
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-nod
```
To address this, this patch instead constructs the expected
relative path of the cgroup's memory info by starting with the
mount path and then adding the cgroup.

Release note: None

Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Darin Peshev <[email protected]>

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@

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)
 

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)
+	}
+}

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)
+			}
+		})
+	}
+}

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
+				}
+			}
 		}
 	}
 

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/<pid>/mountinfo and /proc/<pid>/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"
 )