Merge #153962 #156489

153962:  roachtest: extend github issue poster to include ip node mapping r=srosenberg,DarrylWong a=williamchoe3

Resolves #138356

#### Motivation
Quality of life improvement, previously to find this information, you would need to find the name of the cluster in`test.log`, then use that cluster name to find it's cluster creation log in `{artifacts_dir}/_runner-logs/cluster-create`.

#### Approach
Get's node ip info from roachprod's `cloud.Cluster`
~~Similar to #151850
~~In the test cleanup phase in `inspectArtifacts` added a new function `gatherNodeIpMapping` which is best effort to gather the cluster information in the log.~~
* ~~First we need to find the right log file given that there could be retries for cluster creation which creates additional log files. Given the naming convention, I opted to the just sort the log files that contained the cluster name.  I realized a bit later though that because lexicographical sorting on numerical strings, this approach only works if retry attempts is <10. I was trying to avoid unnecessary string parsing if not needed. Also I would assume cluster retry attempts >=10 would be rare / too high of a retry limit to be ever set.~~
After finding the correct file, use regex to find the string in the log. Then store it in `test_impl`, then pass to `issues.Post` and I added a new case for parsing out the IP table
* The regex is flexible to number of columns so we can change the table fields without having to modify the regex 

Adds Cluster Node IP information to github issue e.g. 
```
| Node | Private IP | Public IP | Machine Type |
| --- | --- | --- | --- |
| willchoe-1758834520-01-n1cpu4-0001 | 10.142.0.2 | 34.139.44.53 | n2-standard-4 |
```
* Opted for keep the VM name as is vs. replacing with something like `n1`. My thinking was that if people wanted to reference the vm in logs, it would be nice to keep the names consistent, but if folks want this as n1 / the vm name isn't helpful then happy to change to `n1`, etc.

Also added a new `node-ips.log` that will also contain this table

#### Notes
Saw "cluster-create" being used as a magic string so created a const for it `clusterCreateDir = "cluster-create"`

#### Verification
Added datadriven test 

Manual Verification
Verified the renderer in `issues` was formatting the new code block correctly in debug mode, holding off on generating more debug github issues, but can if folks want to see

156489: sql, sqlstats: Refactor Statement Recording r=kyle-a-wong a=kyle-a-wong

A new RecordedStatementStatsBuilder was added to the sql package to make building a RecordedStmtStats struct easier.

New interfaces were also added to sqlstats to make make building RecordedStmtStats easier.

These new interfaces and builders should help to decouple Recording statement stats from the conn executor, allowing them to be recorded in other places as well 

Epic: None
Release note: None

Co-authored-by: William Choe <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>

Author: 3 people

pkg/cmd/bazci/githubpost/issues/condense.go

@@ -55,6 +55,13 @@ type FatalNodeRoachtest struct {
 	FatalLogs string
 }
 
+// NodeToIpMappingRoachtest contains the node to ip mapping from a roachtest
+// cluster
+type NodeToIpMappingRoachtest struct {
+	Message,
+	NodeToIpMapping string
+}
+
 // A CondensedMessage is a test log output garnished with useful helper methods
 // that extract concise information for seamless debugging.
 type CondensedMessage string
@@ -67,7 +74,16 @@ var fatalRE = regexp.MustCompile(`(?ms)(^F\d{6}.*?\n)(goroutine \d+.*?\n)\n`)
 var crasherRE = regexp.MustCompile(`(?s)( *rsg_test.go:\d{3}: Crash detected:.*?\n)(.*?;\n)`)
 var reproRE = regexp.MustCompile(`(?s)( *rsg_test.go:\d{3}: To reproduce, use schema:)`)
 
-var roachtestNodeFatalRE = regexp.MustCompile(`(?ms)\A(.*?\n)((?:^F\d{6}\b[^\n]*(?:\n|$))+)`)
+var nodeFatalRoachtestRE = regexp.MustCompile(`(?ms)\A(.*?\n)((?:^F\d{6}\b[^\n]*(?:\n|$))+)`)
+
+// nodeToIpRoachtestRE matches an entire Markdown table block
+// - including header, separator, and data rows:
+// - handles an arbitrary number of columns
+//
+// | Node | Public IP | Private IP |
+// | --- | --- | --- |
+// | node-0001 | 1.1.1.0 | 1.1.1.1 |
+var nodeToIpRoachtestRE = regexp.MustCompile(`(?m)^[[:space:]]*\|(?:[^|\n]*\|)+[[:space:]]*\n^[[:space:]]*\|(?:[[:space:]]*:?-{3,}:?[[:space:]]*\|)+[[:space:]]*\n(?:^[[:space:]]*\|(?:[^|\n]*\|)+[[:space:]]*(?:\n|$))+`)
 
 // FatalOrPanic constructs a FatalOrPanic. If no fatal or panic occurred in the
 // test, ok=false is returned.
@@ -112,17 +128,35 @@ func (s CondensedMessage) RSGCrash(lineLimit int) (c RSGCrash, ok bool) {
 // ok=false is returned
 func (s CondensedMessage) FatalNodeRoachtest() (fnr FatalNodeRoachtest, ok bool) {
 	ss := string(s)
-	if matches := roachtestNodeFatalRE.FindStringSubmatchIndex(ss); matches != nil {
+	if matches := nodeFatalRoachtestRE.FindStringSubmatchIndex(ss); matches != nil {
 		if len(matches) != 6 {
 			return FatalNodeRoachtest{}, false
 		}
-		fnr.Message = ss[matches[2] : matches[3]-1]
+		msg := ss[matches[2]:matches[3]]
+		fnr.Message = strings.TrimRight(msg, "\n")
 		fnr.FatalLogs = ss[matches[4]:matches[5]]
 		return fnr, true
 	}
 	return FatalNodeRoachtest{}, false
 }
 
+// NodeToIpMappingRoachtest constructs a NodeToIpMappingRoachtest which is
+// used to construct an issue that contains cluster information
+func (s CondensedMessage) NodeToIpMappingRoachtest() (nodeIpMap NodeToIpMappingRoachtest, ok bool) {
+	ss := string(s)
+	if matches := nodeToIpRoachtestRE.FindStringSubmatchIndex(ss); matches != nil {
+		if len(matches) != 2 {
+			// only expecting a single match
+			return NodeToIpMappingRoachtest{}, false
+		}
+		msg := ss[0:matches[0]]
+		nodeIpMap.Message = strings.TrimRight(msg, "\n")
+		nodeIpMap.NodeToIpMapping = ss[matches[0]:matches[1]]
+		return nodeIpMap, true
+	}
+	return NodeToIpMappingRoachtest{}, false
+}
+
 // String calls .Digest(30).
 func (s CondensedMessage) String() string {
 	return s.Digest(30)

pkg/cmd/bazci/githubpost/issues/formatter_unit.go

@@ -74,6 +74,17 @@ func (unitTestFormatterTyp) Body(r *Renderer, data TemplateData) error {
 		r.CodeBlock("", fnr.Message)
 		r.Escaped("Fatal entries found in Cockroach logs:")
 		r.CodeBlock("", fnr.FatalLogs)
+		if nodeIpMap, ok := data.CondensedMessage.NodeToIpMappingRoachtest(); ok {
+			r.Escaped("Cluster Node to Ip Mapping:")
+			r.nl()
+			r.Escaped(nodeIpMap.NodeToIpMapping)
+		}
+	} else if nodeIpMap, ok := data.CondensedMessage.NodeToIpMappingRoachtest(); ok {
+		r.Escaped("Failed with:")
+		r.CodeBlock("", nodeIpMap.Message)
+		r.Escaped("Cluster Node to Ip Mapping:")
+		r.nl()
+		r.Escaped(nodeIpMap.NodeToIpMapping)
 	} else {
 		r.CodeBlock("", data.CondensedMessage.Digest(50))
 	}

pkg/cmd/roachtest/cluster.go

@@ -797,7 +797,7 @@ type clusterFactory struct {
 	// counter is incremented with every new cluster. It's used as part of the cluster's name.
 	// Accessed atomically.
 	counter atomic.Uint64
-	// The registry with whom all clustered will be registered.
+	// The registry with whom all clusters will be registered.
 	r *clusterRegistry
 	// artifactsDir is the directory in which the cluster creation log file will be placed.
 	artifactsDir string
@@ -977,7 +977,7 @@ func (f *clusterFactory) newCluster(
 		if i > 1 {
 			retryStr = "-retry" + strconv.Itoa(i-1)
 		}
-		logPath := filepath.Join(f.artifactsDir, runnerLogsDir, "cluster-create", genName+retryStr+".log")
+		logPath := filepath.Join(f.artifactsDir, runnerLogsDir, clusterCreateDir, genName+retryStr+".log")
 		l, err := logger.RootLogger(logPath, teeOpt)
 		if err != nil {
 			log.Fatalf("%v", err)

pkg/cmd/roachtest/github_test.go

@@ -121,6 +121,7 @@ func TestCreatePostRequest(t *testing.T) {
 	type githubIssueOpts struct {
 		failures        []failure
 		loadTeamsFailed bool
+		message         string
 	}
 
 	datadriven.Walk(t, datapathutils.TestDataPath(t, "github"), func(t *testing.T, path string) {
@@ -168,7 +169,7 @@ func TestCreatePostRequest(t *testing.T) {
 					// on where this test is run and prone to flaking.
 					fmt.Fprintf(&b, "%v", f.squashedErr)
 				}
-				message := b.String()
+				message := b.String() + testCase.message
 
 				params := getTestParameters(ti, issueInfo.cluster, issueInfo.vmCreateOpts)
 				req, err := github.createPostRequest(
@@ -225,10 +226,7 @@ func TestCreatePostRequest(t *testing.T) {
 							// Lose the error object which should make our flake detection fail.
 							refError = errors.Newf("%s", redact.SafeString(refError.Error()))
 						case "node-fatal":
-							refError = errors.Newf(`(monitor.go:267).Wait: monitor failure: dial tcp 127.0.0.1:29000: connect: connection refused
-test artifacts and logs in: artifacts/roachtest/manual/monitor/test-failure/node-fatal-explicit-monitor/cpu_arch=arm64/run_1
-F250826 19:49:07.194443 3106 sql/sem/builtins/builtins.go:6063 ⋮ [T1,Vsystem,n1,client=127.0.0.1:54552,hostssl,user=‹roachprod›] 250  force_log_fatal(): ‹oops›
-`)
+							refError = errors.Newf(`(monitor.go:267).Wait: monitor failure: dial tcp 127.0.0.1:29000: connect: connection refused`)
 						}
 					}
 				}
@@ -251,6 +249,18 @@ F250826 19:49:07.194443 3106 sql/sem/builtins/builtins.go:6063 ⋮ [T1,Vsystem,n
 				ti.spec.CockroachBinary = registry.RuntimeAssertionsCockroach
 			case "set-coverage-enabled-build":
 				ti.goCoverEnabled = true
+			case "add-additional-info":
+				msg_type := d.CmdArgs[0].Vals[0]
+				switch msg_type {
+				case "ip-node-info":
+					testCase.message = fmt.Sprintf("%s\n%s", testCase.message, `| Node | Public IP | Private IP |
+| --- | --- | --- |
+| teamcity-1758834520-01-n1cpu4-0001 | 34.139.44.53 | 10.142.0.2 |`)
+				case "fatal-logs":
+					testCase.message = fmt.Sprintf("%s\n%s", testCase.message, `F250826 19:49:07.194443 3106 sql/sem/builtins/builtins.go:6063 ⋮ [T1,Vsystem,n1,client=127.0.0.1:54552,hostssl,user=‹roachprod›] 250  force_log_fatal(): ‹oops›`)
+				default:
+					return fmt.Sprintf("unknown additional info argument: %s", msg_type)
+				}
 			}
 
 			return "ok"

pkg/cmd/roachtest/main.go

@@ -56,6 +56,10 @@ const (
 	// runnerLogsDir is the dir under the artifacts root where the test runner log
 	// and other runner-related logs (i.e. cluster creation logs) will be written.
 	runnerLogsDir = "_runner-logs"
+
+	// clusterCreateDir is the dir under runnerLogsDir where cluster creation
+	// related logs will be written
+	clusterCreateDir = "cluster-create"
 )
 
 func main() {

pkg/cmd/roachtest/roachtestutil/utils.go

@@ -395,3 +395,42 @@ var ClusterSettingRetryOpts = retry.Options{
 	MaxBackoff:     5 * time.Second,
 	MaxRetries:     5,
 }
+
+var (
+	errEmptyTableData = errors.New("empty table data")
+	errMismatchedCols = errors.New("row has mismatched number of columns")
+)
+
+// ToMarkdownTable renders a 2D list of strings as a Markdown table. Assumes
+// header is the first entry.
+//
+// Example:
+//
+//	| Name    | Age | City          |
+//	| ---     | --- | ---           |
+//	| Alice   | 30  | New York      |
+//	| Bob     | 25  | San Francisco |
+//	| Charlie | 35  | Chicago       |
+func ToMarkdownTable(data [][]string) (string, error) {
+	if len(data) == 0 || len(data[0]) == 0 {
+		return "", errEmptyTableData
+	}
+	var sb strings.Builder
+	// Write header row
+	sb.WriteString("| " + strings.Join(data[0], " | ") + " |\n")
+	// Write separator row (--- under each header)
+	separators := make([]string, len(data[0]))
+	for i := range separators {
+		separators[i] = "---"
+	}
+	sb.WriteString("| " + strings.Join(separators, " | ") + " |\n")
+	// Write remaining rows
+	for i, row := range data[1:] {
+		if len(data[0]) != len(row) {
+			return "", fmt.Errorf("%w: row %d has %d columns, expected %d",
+				errMismatchedCols, i, len(row), len(data[0]))
+		}
+		sb.WriteString("| " + strings.Join(row, " | ") + " |\n")
+	}
+	return sb.String(), nil
+}

pkg/cmd/roachtest/roachtestutil/utils_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCmdLogFileName(t *testing.T) {
@@ -27,3 +28,54 @@ func TestCmdLogFileName(t *testing.T) {
 		cmdLogFileName(ts, nodes, "./cockroach bla --foo bar"),
 	)
 }
+
+func TestToMarkdownTable(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       [][]string
+		expectedOut string
+		expectedErr error
+	}{
+		{
+			name: "valid table",
+			input: [][]string{
+				{"Name", "Age", "City"},
+				{"Alice", "30", "New York"},
+				{"Bob", "25", "San Francisco"},
+			},
+			expectedOut: `| Name | Age | City |
+| --- | --- | --- |
+| Alice | 30 | New York |
+| Bob | 25 | San Francisco |
+`,
+			expectedErr: nil,
+		},
+		{
+			name:        "empty data",
+			input:       [][]string{},
+			expectedOut: "",
+			expectedErr: errEmptyTableData,
+		},
+		{
+			name: "row with wrong number of columns",
+			input: [][]string{
+				{"Name", "Age", "City"},
+				{"Alice", "30"},
+			},
+			expectedOut: "",
+			expectedErr: errMismatchedCols,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out, err := ToMarkdownTable(tt.input)
+			if tt.expectedErr != nil {
+				require.ErrorIs(t, err, tt.expectedErr, "expected %v, got %v", tt.expectedErr, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expectedOut, out)
+			}
+		})
+	}
+}

pkg/cmd/roachtest/test_impl.go

@@ -141,9 +141,13 @@ type testImpl struct {
 		// in case github issue posting is disabled.
 		extraParams map[string]string
 
-		// githubMessage contains additional message information that will be
-		// passed to github.MaybePost
-		githubMessage string
+		// githubFatalLogs contains node fatal logs that will be passed to
+		// github.MaybePost
+		githubFatalLogs string
+
+		// githubIpToNodeMapping contains the ip to node map that will be passed to
+		// github.MaybePost
+		githubIpToNodeMapping string
 	}
 	// Map from version to path to the cockroach binary to be used when
 	// mixed-version test wants a binary for that binary. If a particular version
@@ -562,16 +566,45 @@ func (t *testImpl) failureMsg() string {
 	return b.String()
 }
 
-func (t *testImpl) getGithubMessage() string {
+func (t *testImpl) getGithubFatalLogs() string {
 	t.mu.RLock()
 	defer t.mu.RUnlock()
-	return t.mu.githubMessage
+	return t.mu.githubFatalLogs
 }
 
-func (t *testImpl) appendGithubMessage(msg string) {
+func (t *testImpl) appendGithubFatalLogs(msg string) {
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	t.mu.githubMessage += msg
+	t.mu.githubFatalLogs += msg
+}
+
+func (t *testImpl) getGithubIpToNodeMapping() string {
+	t.mu.RLock()
+	defer t.mu.RUnlock()
+	return t.mu.githubIpToNodeMapping
+}
+
+func (t *testImpl) appendGithubIpToNodeMapping(msg string) {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	t.mu.githubIpToNodeMapping += msg
+}
+
+// getGithubMessage abstracts details on how the Github message is constructed
+// TODO(wchoe): Currently, all information we want to include in the Github
+// message must be passed in as a single string. This is not ideal, especially
+// as we add more information or want to format based on the failure or what
+// information is available. #157021 aims to address this.
+func (t *testImpl) getGithubMessage(failureMsg string) string {
+	githubMsg := failureMsg
+	if githubFatalLogs := t.getGithubFatalLogs(); githubFatalLogs != "" {
+		githubMsg = fmt.Sprintf("%s\n%s", githubMsg, githubFatalLogs)
+	}
+	if githubIpToNodeMapping := t.getGithubIpToNodeMapping(); githubIpToNodeMapping != "" {
+		githubMsg = fmt.Sprintf("%s\n%s", githubMsg, githubIpToNodeMapping)
+	}
+	t.L().Printf("github message: %s", githubMsg)
+	return githubMsg
 }
 
 // failuresMatchingError checks whether the first error in trees of