Merge #154164 #154266 #154363 #154369

154164: roachtest: add github issue posting dry run mode r=williamchoe3,herkolategan a=DarrylWong

We already have the functionality in datadriven tests to render github issue post requests as clickable links. This change extends that functionality to roachtests in the form of a --dry-run-issue-posting flag. This allows for viewing what the output of a failed roachtest would look like even if github posting is disabled.

Fixes: none
Release note: none
Epic: none

154266: sqlproxyccl: pass in ctx when transferring connections r=jeffswenson a=davidwding

Previously, the span used when transferring connections was reused from
the forwarder. However, transferring connections has async components,
so it's possible that the forwarder calls `Finish` on the span before
the connection transfer is done using the span. This leads to a panic:
`panic: use of Span after Finish`. To address this, this patch passes a
context in when transferring a connection, instead of using the
forwarder's context.

Fixes: #153569
Release note: None

154363: sql/types: reintroduce downgradeType for NAME compatibility in mixed clusters r=spilchen a=spilchen

This patch restores the `downgradeType` hook to maintain serialization compatibility for the NAME type in mixed-version clusters (v25.3 and 25.4).

In previous releases, a NAME type could be misinterpreted by older nodes. Specifically, a v25.3 node deserializing a NAME encoded by a v25.4 node would mistakenly treat it as text (`oid.T_text`). This was due to the old `upgradeType` logic overwriting the OID when no `VisibleType` was set, causing `T_name` to degrade to `T_text`. PR #148694 addressed this issue for v25.4 by changing `upgradeType` (to avoid clobbering a provided OID) and removing the `downgradeType` function.

However, the follow-up fix for mixed-version serialization (PR #152633) relied on the `InternalType.VisibleType` field to tag string types for older nodes. Because the NAME type doesn’t use a `VisibleType` (it’s represented as a STRING family with a special OID), that change didn’t cover NAME. As a result, NAME remained broken in a 25.3/25.4 mixed cluster.

This commit explicitly handles NAME during serialization. When sending a NAME type to the wire, we downgrade it to the legacy encoding: we set its type family to `Family=name` (legacy enum value 11) so that v25.3 nodes recognize it as a NAME. Additionally, if the locale is an empty string, we set it to `nil` to ensure older versions don’t misclassify the NAME as a collated string. Together, these steps preserve the correct type interpretation on older nodes.

Once all nodes are on v25.4, this compatibility shim can be removed. At that point, the improved upgrade logic in v25.4 (which no longer overrides a valid OID) means we won’t need to downgrade types for backward compatibility. We may also perform a one time descriptor migration to update any NAME descriptors to the modern format after the cluster upgrade. In short, `downgradeType` for NAME is a temporary measure to bridge the v25.3 gap.

Fixes #153322

Release note: None

154369: roachest: ensure reader tenant accepts connections before restart r=jeffswenson a=msbutler

Previously, the roachtest would restart the tenant before the tenant could accept connections, which means the tenant could still be bootstrapping. Restarting the tenant service during bootstrap could lead to sadness, as outlined in #154356.

Informs #154356
Informs #154311
Informs #153131

Release note: none

Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: David Ding <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>
Co-authored-by: Michael Butler <[email protected]>

Author: 5 people

pkg/ccl/sqlproxyccl/balancer/balancer.go

@@ -371,7 +371,7 @@ func (b *Balancer) processQueue(ctx context.Context) {
 
 			// Each request is retried up to maxTransferAttempts.
 			for i := 0; i < maxTransferAttempts && ctx.Err() == nil; i++ {
-				err := req.conn.TransferConnection()
+				err := req.conn.TransferConnection(ctx)
 				if err == nil || errors.Is(err, context.Canceled) {
 					break
 				}

pkg/ccl/sqlproxyccl/balancer/conn.go

@@ -19,7 +19,7 @@ type ConnectionHandle interface {
 	// TransferConnection performs a connection migration on the connection
 	// handle. Invoking this blocks until the connection migration process has
 	// been completed.
-	TransferConnection() error
+	TransferConnection(context.Context) error
 
 	// IsIdle returns true if the connection is idle, and false otherwise.
 	IsIdle() bool

pkg/ccl/sqlproxyccl/balancer/conn_test.go

@@ -44,7 +44,7 @@ func (h *testConnHandle) Close() {
 }
 
 // TransferConnection implements the ConnectionHandle interface.
-func (h *testConnHandle) TransferConnection() error {
+func (h *testConnHandle) TransferConnection(ctx context.Context) error {
 	h.mu.Lock()
 	defer h.mu.Unlock()
 	h.mu.onTransferConnectionCount++

pkg/ccl/sqlproxyccl/conn_migration.go

@@ -127,7 +127,7 @@ var errTransferCannotStart = errors.New("transfer cannot be started")
 // error).
 //
 // TransferConnection implements the balancer.ConnectionHandle interface.
-func (f *forwarder) TransferConnection() (retErr error) {
+func (f *forwarder) TransferConnection(ctx context.Context) (retErr error) {
 	// A previous non-recoverable transfer would have closed the forwarder, so
 	// return right away.
 	if f.ctx.Err() != nil {
@@ -146,18 +146,18 @@ func (f *forwarder) TransferConnection() (retErr error) {
 	// whenever the context expires. We have to close the forwarder because
 	// the transfer may be blocked on I/O, and the only way for now is to close
 	// the connections. This then allow TransferConnection to return and cleanup.
-	ctx, cancel := newTransferContext(f.ctx)
+	transferCtx, cancel := newTransferContext(ctx)
 	defer cancel()
 
 	// Use a separate handler for timeouts. This is the only way to handle
 	// blocked I/Os as described above.
 	go func() {
-		<-ctx.Done()
+		<-transferCtx.Done()
 		// This Close call here in addition to the one in the defer callback
 		// below is on purpose. This would help unblock situations where we're
 		// blocked on sending/reading messages from connections that couldn't
 		// be handled with context.Context.
-		if !ctx.isRecoverable() {
+		if !transferCtx.isRecoverable() {
 			f.Close()
 		}
 	}()
@@ -179,7 +179,7 @@ func (f *forwarder) TransferConnection() (retErr error) {
 
 		// When TransferConnection returns, it's either the forwarder has been
 		// closed, or the procesors have been resumed.
-		if !ctx.isRecoverable() {
+		if !transferCtx.isRecoverable() {
 			log.Dev.Infof(logCtx, "transfer failed: connection closed, latency=%v, err=%v", latencyDur, retErr)
 			f.metrics.ConnMigrationErrorFatalCount.Inc(1)
 			f.Close()
@@ -201,16 +201,16 @@ func (f *forwarder) TransferConnection() (retErr error) {
 
 	// Suspend both processors before starting the transfer.
 	request, response := f.getProcessors()
-	if err := request.suspend(ctx); err != nil {
+	if err := request.suspend(transferCtx); err != nil {
 		return errors.Wrap(err, "suspending request processor")
 	}
-	if err := response.suspend(ctx); err != nil {
+	if err := response.suspend(transferCtx); err != nil {
 		return errors.Wrap(err, "suspending response processor")
 	}
 
 	// Transfer the connection.
 	clientConn, serverConn := f.getConns()
-	newServerConn, err := transferConnection(ctx, f, f.connector, f.metrics, clientConn, serverConn)
+	newServerConn, err := transferConnection(transferCtx, f, f.connector, f.metrics, clientConn, serverConn)
 	if err != nil {
 		return errors.Wrap(err, "transferring connection")
 	}

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -2018,12 +2018,12 @@ func TestConnectionMigration(t *testing.T) {
 		require.Equal(t, totalAttempts, count)
 	}
 
-	transferConnWithRetries := func(t *testing.T, f *forwarder) error {
+	transferConnWithRetries := func(t *testing.T, ctx context.Context, f *forwarder) error {
 		t.Helper()
 
 		var nonRetriableErrSeen bool
 		err := testutils.SucceedsSoonError(func() error {
-			err := f.TransferConnection()
+			err := f.TransferConnection(ctx)
 			if err == nil {
 				return nil
 			}
@@ -2091,7 +2091,7 @@ func TestConnectionMigration(t *testing.T) {
 			require.NoError(t, err)
 
 			// Show that we get alternating SQL pods when we transfer.
-			require.NoError(t, transferConnWithRetries(t, f))
+			require.NoError(t, transferConnWithRetries(t, tCtx, f))
 			require.Equal(t, int64(1), f.metrics.ConnMigrationSuccessCount.Count())
 			require.Equal(t, tenant2.SQLAddr(), queryAddr(tCtx, t, db))
 
@@ -2102,7 +2102,7 @@ func TestConnectionMigration(t *testing.T) {
 			_, err = db.Exec("SET application_name = 'bar'")
 			require.NoError(t, err)
 
-			require.NoError(t, transferConnWithRetries(t, f))
+			require.NoError(t, transferConnWithRetries(t, tCtx, f))
 			require.Equal(t, int64(2), f.metrics.ConnMigrationSuccessCount.Count())
 			require.Equal(t, tenant1.SQLAddr(), queryAddr(tCtx, t, db))
 
@@ -2120,14 +2120,14 @@ func TestConnectionMigration(t *testing.T) {
 			go func() {
 				defer wg.Done()
 				for subCtx.Err() == nil {
-					_ = f.TransferConnection()
+					_ = f.TransferConnection(tCtx)
 					time.Sleep(100 * time.Millisecond)
 				}
 			}()
 
 			// This loop will run approximately 5 seconds.
 			var tenant1Addr, tenant2Addr int
-			for i := 0; i < 100; i++ {
+			for range 100 {
 				addr := queryAddr(tCtx, t, db)
 				if addr == tenant1.SQLAddr() {
 					tenant1Addr++
@@ -2167,7 +2167,7 @@ func TestConnectionMigration(t *testing.T) {
 			err = crdb.ExecuteTx(tCtx, db, nil /* txopts */, func(tx *gosql.Tx) error {
 				// Run multiple times to ensure that connection isn't closed.
 				for i := 0; i < 5; {
-					err := f.TransferConnection()
+					err := f.TransferConnection(tCtx)
 					if err == nil {
 						return errors.New("no error")
 					}
@@ -2201,7 +2201,7 @@ func TestConnectionMigration(t *testing.T) {
 			require.Equal(t, int64(0), f.metrics.ConnMigrationErrorFatalCount.Count())
 
 			// Once the transaction is closed, transfers should work.
-			require.NoError(t, transferConnWithRetries(t, f))
+			require.NoError(t, transferConnWithRetries(t, tCtx, f))
 			require.NotEqual(t, initAddr, queryAddr(tCtx, t, db))
 			require.Nil(t, f.ctx.Err())
 			require.Equal(t, initSuccessCount+1, f.metrics.ConnMigrationSuccessCount.Count())
@@ -2223,7 +2223,7 @@ func TestConnectionMigration(t *testing.T) {
 			lookupAddrDelayDuration = 10 * time.Second
 			defer testutils.TestingHook(&defaultTransferTimeout, 3*time.Second)()
 
-			err := f.TransferConnection()
+			err := f.TransferConnection(tCtx)
 			require.Error(t, err)
 			require.Regexp(t, "injected delays", err.Error())
 			require.Equal(t, initAddr, queryAddr(tCtx, t, db))
@@ -2319,7 +2319,7 @@ func TestConnectionMigration(t *testing.T) {
 		time.Sleep(2 * time.Second)
 		// This should be an error because the transfer timed out. Connection
 		// should automatically be closed.
-		require.Error(t, f.TransferConnection())
+		require.Error(t, f.TransferConnection(tCtx))
 
 		select {
 		case <-time.After(10 * time.Second):

pkg/cmd/roachtest/github.go

@@ -8,7 +8,9 @@ package main
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/bazci/githubpost/issues"
@@ -35,6 +37,7 @@ type GithubPoster interface {
 // githubIssues struct implements GithubPoster
 type githubIssues struct {
 	disable     bool
+	dryRun      bool
 	issuePoster func(context.Context, issues.Logger, issues.IssueFormatter, issues.PostRequest,
 		*issues.Options) (*issues.TestFailureIssue, error)
 	teamLoader func() (team.Map, error)
@@ -327,6 +330,10 @@ func (g *githubIssues) MaybePost(
 	message string,
 	params map[string]string,
 ) (*issues.TestFailureIssue, error) {
+	if g.dryRun {
+		return nil, g.dryRunPost(t, issueInfo, l, message, params)
+	}
+
 	skipReason := g.shouldPost(t)
 	if skipReason != "" {
 		l.Printf("skipping GitHub issue posting (%s)", skipReason)
@@ -352,3 +359,75 @@ func (g *githubIssues) MaybePost(
 		opts,
 	)
 }
+
+// dryRunPost simulates posting an issue to GitHub by rendering
+// the issue and logging a clickable link.
+func (g *githubIssues) dryRunPost(
+	t *testImpl,
+	issueInfo *githubIssueInfo,
+	l *logger.Logger,
+	message string,
+	params map[string]string,
+) error {
+	postRequest, err := g.createPostRequest(
+		t.Name(), t.start, t.end, t.spec, t.failures(),
+		message,
+		roachtestutil.UsingRuntimeAssertions(t), t.goCoverEnabled, params, issueInfo,
+	)
+	if err != nil {
+		return err
+	}
+	_, url, err := formatPostRequest(postRequest)
+	if err != nil {
+		return err
+	}
+	l.Printf("GitHub issue posting in dry-run mode:\n%s", url)
+	return nil
+}
+
+// formatPostRequest returns a string representation of the rendered PostRequest
+// as well as a link that can be followed to open the issue in Github. The rendered
+// PostRequest also includes the labels that would be applied to the issue as part
+// of the body.
+func formatPostRequest(req issues.PostRequest) (string, string, error) {
+	data := issues.TemplateData{
+		PostRequest:      req,
+		Parameters:       req.ExtraParams,
+		CondensedMessage: issues.CondensedMessage(req.Message),
+		Branch:           "test_branch",
+		Commit:           "test_SHA",
+		PackageNameShort: strings.TrimPrefix(req.PackageName, issues.CockroachPkgPrefix),
+	}
+
+	formatter := issues.UnitTestFormatter
+	r := &issues.Renderer{}
+	if err := formatter.Body(r, data); err != nil {
+		return "", "", err
+	}
+
+	var post strings.Builder
+	post.WriteString(r.String())
+
+	// Github labels are normally not part of the rendered issue body, but we want to
+	// still test that they are correctly set so append them here.
+	post.WriteString("\n------\nLabels:\n")
+	for _, label := range req.Labels {
+		post.WriteString(fmt.Sprintf("- <code>%s</code>\n", label))
+	}
+
+	u, err := url.Parse("https://github.com/cockroachdb/cockroach/issues/new")
+	if err != nil {
+		return "", "", err
+	}
+	q := u.Query()
+	q.Add("title", formatter.Title(data))
+	q.Add("body", post.String())
+	// Adding a template parameter is required to be able to view the rendered
+	// template on GitHub, otherwise it just takes you to the template selection
+	// page.
+	q.Add("template", "none")
+	u.RawQuery = q.Encode()
+	post.WriteString(fmt.Sprintf("Rendered:\n%s", u.String()))
+
+	return post.String(), u.String(), nil
+}

pkg/cmd/roachtest/github_test.go

@@ -7,7 +7,6 @@ package main
 import (
 	"context"
 	"fmt"
-	"net/url"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -90,7 +89,7 @@ func TestShouldPost(t *testing.T) {
 
 		ti := &testImpl{spec: testSpec}
 		ti.mu.failures = c.failures
-		github := &githubIssues{disable: c.disableIssues}
+		github := &githubIssues{disable: c.disableIssues, dryRun: false}
 
 		skipReason := github.shouldPost(ti)
 		require.Equal(t, c.expectedReason, skipReason)
@@ -184,7 +183,7 @@ func TestCreatePostRequest(t *testing.T) {
 				}
 				require.NoError(t, err)
 
-				post, err := formatPostRequest(req)
+				post, _, err := formatPostRequest(req)
 				require.NoError(t, err)
 
 				return post
@@ -258,49 +257,3 @@ F250826 19:49:07.194443 3106 sql/sem/builtins/builtins.go:6063 ⋮ [T1,Vsystem,n
 		})
 	})
 }
-
-// formatPostRequest returns a string representation of the rendered PostRequest.
-// Additionally, it also includes labels, as well as a link that can be followed
-// to open the issue in Github.
-func formatPostRequest(req issues.PostRequest) (string, error) {
-	data := issues.TemplateData{
-		PostRequest:      req,
-		Parameters:       req.ExtraParams,
-		CondensedMessage: issues.CondensedMessage(req.Message),
-		Branch:           "test_branch",
-		Commit:           "test_SHA",
-		PackageNameShort: strings.TrimPrefix(req.PackageName, issues.CockroachPkgPrefix),
-	}
-
-	formatter := issues.UnitTestFormatter
-	r := &issues.Renderer{}
-	if err := formatter.Body(r, data); err != nil {
-		return "", err
-	}
-
-	var post strings.Builder
-	post.WriteString(r.String())
-
-	// Github labels are normally not part of the rendered issue body, but we want to
-	// still test that they are correctly set so append them here.
-	post.WriteString("\n------\nLabels:\n")
-	for _, label := range req.Labels {
-		post.WriteString(fmt.Sprintf("- <code>%s</code>\n", label))
-	}
-
-	u, err := url.Parse("https://github.com/cockroachdb/cockroach/issues/new")
-	if err != nil {
-		return "", err
-	}
-	q := u.Query()
-	q.Add("title", formatter.Title(data))
-	q.Add("body", post.String())
-	// Adding a template parameter is required to be able to view the rendered
-	// template on GitHub, otherwise it just takes you to the template selection
-	// page.
-	q.Add("template", "none")
-	u.RawQuery = q.Encode()
-	post.WriteString(fmt.Sprintf("Rendered:\n%s", u.String()))
-
-	return post.String(), nil
-}

pkg/cmd/roachtest/roachtestflags/flags.go

@@ -445,6 +445,12 @@ var (
 		Usage: `Disable posting GitHub issue for failures`,
 	})
 
+	DryRunIssuePosting bool
+	_                  = registerRunFlag(&DryRunIssuePosting, FlagInfo{
+		Name:  "dry-run-issue-posting",
+		Usage: `Enable dry-run mode for GitHub issue posting (formats issues but doesn't post them)`,
+	})
+
 	PromPort int = 2113
 	_            = registerRunFlag(&PromPort, FlagInfo{
 		Name: "prom-port",

pkg/cmd/roachtest/run.go

@@ -153,6 +153,7 @@ func runTests(register func(registry.Registry), filter *registry.TestFilter) err
 
 	github := &githubIssues{
 		disable:     runner.config.disableIssue,
+		dryRun:      runner.config.dryRunIssuePosting,
 		issuePoster: issues.Post,
 		teamLoader:  team.DefaultLoadTeams,
 	}

pkg/cmd/roachtest/test_runner.go

@@ -148,6 +148,8 @@ type testRunner struct {
 		skipClusterWipeOnAttach bool
 		// disableIssue disables posting GitHub issues for test failures.
 		disableIssue bool
+		// dryRunIssuePosting enables dry-run mode for GitHub issue posting.
+		dryRunIssuePosting bool
 		// overrideShutdownPromScrapeInterval overrides the default time a test runner waits to
 		// shut down, normally used to ensure a remote prometheus server has scraped the roachtest
 		// endpoint.
@@ -215,6 +217,7 @@ func newTestRunner(cr *clusterRegistry, stopper *stop.Stopper) *testRunner {
 	}
 	r.config.skipClusterWipeOnAttach = !roachtestflags.ClusterWipe
 	r.config.disableIssue = roachtestflags.DisableIssue
+	r.config.dryRunIssuePosting = roachtestflags.DryRunIssuePosting
 	r.workersMu.workers = make(map[string]*workerStatus)
 	return r
 }