Suggestion: can we put nodeID, Private IP, Public IP?, and Machine Type as the first four fields? i.e. so I can see what I care about 99% of the time without scrolling?

Arch is already listed in the github issue (and shouldn't differ across machines in a test). The average engineer debugging a roachtest probably doesn't care about CPU Family or Provision Model either.

Oh, just saw your other comment about Stan mentioning something similar. My argument against including all the fields is that this is just a convenience mapping so we don't want to overload it with information that normally isn't pertinent to investigation. Remember that we already gather vm_specs which has already has all of this information already.

We definitely don't need the DNS column, and the Name column can be replaced by the Node column; nodes are always canonically named, i.e., n1, n2, etc. Thus, the most important are the three columns as illustrated in [1].

[1] #153962 (comment)

redid my approach, no more log parsing. reference the new description for the new table. I'm open to changing the node names btw, there's a bullet in the description with my thoughts on it

The VM information should already be available through clusterImpl -> getCachedCluster() -> cloud.Cluster -> VMs. Was there a reason to opt for parsing the logs here which might break if we change the output of the logs?

yea no good reason 😅 was mainly reading Cluster Interface's methods and the test_runner.go's cluster creation code path so didn't realize there was all this built out utility already in clusterImpl, redoing my approach will undraft the PR when done

Nit: this regex is an eye sore. A comment with an example would be helpful.

len(data)-1 is not the ith row.

fixed 😅

Might as well check for the expected err since the above would also output the last row index in case of (column number) mismatch.

added dedicated error vars and checking in the type in the unit test

Nit: the "not needed" modifier is confusing; the two outputs are mutually disjoint. Perhaps renaming output to stdoutMsg would suffice to disambiguate?

// Construct stdoutMsg which will be shouted and githubMsg which will
// be passed to github.MaybePost to be formatted in the github issue
// body

Renamed some vars to make what's going on more clear, so now failureMsg will be the base for both stdoutMsg & githubMsg

err != nil implies that errors.Join(inspectArtifactsErr, err) != nil. The nested if is redundant.

makes sense, removed the redundant check

It seems a bit off to me that we are encoding these as errors. Shouldn't this be just be an additional string appended to the message? That way we can test how ip node info interacts with other failures, e.g. node-fatal, which also seems like it shouldn't be an error?

It seems a bit off to me that we are encoding these as errors.

Yea, I agree it's all a bit convoluted, and not just in the unit test logic. I think it's because the message parameter is conflated as an error message and not as just a generic message.

So before my changes, github.MaybePost and subsequently createPostRequest messsage parameter was just the error message. So when I added the node-fatal, it sort of made sense that it was it's own error so it's additional information i.e. the node logs could just be appended with the error. But with the ip node logic, that thinking sort of breaks down because it's additional information that we want to just append to every type of message or error message.

I think a better approach to all of this would be to change the issues.PostRequest struct to take in an additional []string field in addition to message, something like info that would contain the fatal node logs, and the ip node info, etc. Then in unitTestFormatterTyp.Body, could parse each element in info. Basically, it doesn't seem like issues was originally written to pass in "additional info", only the error.

As for the unit test logic, the error message message is just being formed from the errors i.e. refError that's passed in the first step.

				// See: `formatFailure` which formats failures for roachtests. Try to
				// follow it here.
				var b strings.Builder
				for i, f := range testCase.failures {
					if i > 0 {
						fmt.Fprintln(&b)
					}
					// N.B. Don't use %+v here even though roachtest does. We don't
					// want the stack trace to be outputted which will differ based
					// on where this test is run and prone to flaking.
					fmt.Fprintf(&b, "%v", f.squashedErr)
				}
				message := b.String()

Then we're calling github.createPostRequest with that message. This doesn't really match what test_runner.go is doing anymore because I added the string appending logic for ipnode info to append to message before github.createPostRequest is called.

I was initially thinking that if we're just testing the format, then passing in a refError with what I wanted to format would suffice.

Instead I can add a step to pass along the ip node info so would just be something like message.append(additionalInfo) and remove the case "error-with-ip-node-info"

Long comment but just wanted to also address why I think a lot of this feels a bit "hacky" from the random string appends to the reliance on long confusing regex strings in issues

tl;dr:

Instead I can add a step to pass along the ip node info so would just be something like message.append(additionalInfo) and remove the case "error-with-ip-node-info"

I agree, we should refactor this because it gets even messier; i.e., more cases (and obscure regexes). Note that we already have the ExtraParams map in the PostRequest, but it serves a different purpose. Maybe another map, e.g., Diagnostics map[string]string would do the trick?

Let's write up an issue for it and address it in a subsequent PR.

#154274

Wrote this up, mentioned it in a comment in the code in a TODO, kind of just mind dumped, if I end up doing it I have enough context, but mainly made it verbose for others / if someone else ends up doing it

You mean #157021, right?

nit: why can't t.getGithubMessage just call both getGithubIpToNodeMapping and getGithubFatalLogs then do the message construction? Seems cleaner to have the test runner not be aware of the github message details.

In hindsight, we probably don't need the machine type since its static, i.e. I can just look at the test to figure it out

Agreed, machine type is easily inferred and not really relevant in majority of (triage) cases. Let's remove.

Should be fine to swallow the error here and continue, we will at least get the mapping in the github issue if one exists

nit: change seems redundant since command is never modified, makes it marginally harder to follow

nit: not a big deal but some of these fatal node log fixes could've been their own commit

One tiny nit... Could we swap the positions of public and private ips? In most infra flakes I can think of, a public rather than private ip is implicated. So, I worry about the case where the cluster name is very long and the public ip is obscured by the scrollbar; i.e., this optimizes for (clusterName, publicIp) to be always visible, even on tiny screens :)

sure np

You're inside the case for FatalNodeRoachtest. How can the message be that and also NodeToIpMappingRoachtest?

Yea so this is another consequence of doing the message parsing all in one go instead of more modular (see long comment #153962 (comment))

This is the case where we both have fatal node logs and ip node info. I went ahead with this hacky way because i wanted to keep my changes to the if else if else control chain as minimal as possible. I would have liked if all these conditionals where just if clauses instead of else if clauses, not sure if the other formatters potentially evaluate to true multiple times

I thought I had examples of these somewhere in this PR but can't find the comment but this is the corresponding datadriven test for this case https://github.com/williamchoe3/cockroach/blob/e7155110ece307284e073a7e0d3c871d830411de/pkg/cmd/roachtest/testdata/github/node_fatal_with_ip_node_info

I see it's effectively demultiplexing. But then we have a bug in getGithubMessage which overwrites instead of appends, in the case of fatal logs?

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

I'm doing a weird append, I think i did it this way because I needed the\n prepended to githubMsg, but also a \n char inbetween the two info types if they both existed

this logic looked better to me before, but on 2nd pass it does look confusing, I can just use append if that makes things clearer

		githubMsg = append(githubMsg + '\n' + githubIpToNodeMapping)
		...
		githubMsg = append('\n', githubMsg)

Oh, 🤦. Ignore me, somehow I missed githubMsg in the RHS (of the assignment).