Merge #159006 #159152 #159179 #159200 #159253 #159280 #159281

159006: backup: remove TestBackupSharedProcessTenantNodeDown r=msbutler a=kev-cao

This test is flaky and superfluous as our mixed version roachtests already test backup with offline nodes.

Fixes: #158654

Release note: None

159152: roachprod/vm: fix JSON serialization of VM.Errors field r=petermattis a=petermattis

The VM.Errors field was typed as []error, but Go's encoding/json cannot
unmarshal into an error interface type because it doesn't know what
concrete type to create. This caused roachprod list to fail when loading
cached cluster info containing VMs with errors:

  json: cannot unmarshal object into Go struct field
  VM.vms.errors of type error

This change introduces a VMError wrapper type that:
- Implements the error interface
- Implements json.Marshaler to serialize as a JSON string (error message)
- Implements json.Unmarshaler to handle both the new format (strings) and
  legacy format (empty objects from broken serialization)
- Implements Unwrap() so errors.Is/As still work

All cloud providers (AWS, Azure, GCE, IBM) are updated to use the new
VMError type via NewVMError(). The BadInstanceErrors() function now
returns map[string]vm.List (grouped by error message string) instead of
map[error]vm.List since serialization loses pointer identity.

Release note: None
Epic: None

Co-authored-by: Claude <[email protected]>


159179: ui: bump cluster-ui version to 26.2.0-prerelease.0 r=kyle-a-wong a=kyle-a-wong

Epic: REL-3909
Resolves: CRDB-57502
Release note: None

159200: sql: add query id to recorded statement stats in UDFs / SPs r=kyle-a-wong a=kyle-a-wong

Generates a query id for sub statements executed in UDFs and SPs. This is used by the insights system to provide execution specific insights.

Epic: None
Release note: None

159253: cli/debug: narrow scope of --include-goroutine-stacks to just debug=2 r=dt a=dt

Release note: none.
Epic: none.

159280: kvserver: rm unused ID in snapWriteBuilder r=arulajmani a=pav-kv

After #155953, this ID is not used.

Epic: CRDB-55220

159281: backup: disable AC in datadriven tests r=dt a=dt

These tests, via 'new-cluster', run a large number of nodes in a single process, overloading that process and causing bulk work to starve if it is restricted to using strictly spare capacity (of which there is none).

Release note: none.
Epic: none.

Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: Peter Mattis <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>

Author: 6 people

pkg/backup/BUILD.bazel

@@ -293,7 +293,6 @@ go_test(
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondata",
         "//pkg/sql/sqlclustersettings",
-        "//pkg/sql/sqlliveness/slbase",
         "//pkg/sql/sqltestutils",
         "//pkg/sql/stats",
         "//pkg/storage",

pkg/backup/backup_tenant_test.go

@@ -9,19 +9,16 @@ import (
 	"context"
 	"fmt"
 	"testing"
-	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils"
 	_ "github.com/cockroachdb/cockroach/pkg/cloud/impl" // register cloud storage providers
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilitiespb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	_ "github.com/cockroachdb/cockroach/pkg/sql/importer"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlclustersettings"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness/slbase"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -30,78 +27,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
 
-func TestBackupSharedProcessTenantNodeDown(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	ctx := context.Background()
-
-	skip.UnderRace(t, "multi-node, multi-tenant test too slow under race")
-	skip.UnderDeadlock(t, "too slow under deadlock detector")
-	params := base.TestClusterArgs{
-		ServerArgs: base.TestServerArgs{
-			DefaultTestTenant: base.TestControlsTenantsExplicitly,
-		},
-	}
-	params.ServerArgs.Knobs.JobsTestingKnobs = jobs.NewTestingKnobsWithShortIntervals()
-	tc, hostDB, _, cleanup := backupRestoreTestSetupWithParams(t, multiNode, 0, /* numAccounts */
-		InitManualReplication, params)
-	defer cleanup()
-
-	hostDB.Exec(t, "ALTER TENANT ALL SET CLUSTER SETTING server.sqlliveness.ttl='2s'")
-	hostDB.Exec(t, "ALTER TENANT ALL SET CLUSTER SETTING server.sqlliveness.heartbeat='250ms'")
-
-	testTenantID := roachpb.MustMakeTenantID(11)
-	tenantApp, tenantDB, err := tc.Server(0).TenantController().StartSharedProcessTenant(ctx,
-		base.TestSharedProcessTenantArgs{
-			TenantID:   testTenantID,
-			TenantName: "test",
-		})
-	require.NoError(t, err)
-
-	tc.GrantTenantCapabilities(
-		ctx, t, testTenantID,
-		map[tenantcapabilitiespb.ID]string{tenantcapabilitiespb.CanUseNodelocalStorage: "true"})
-
-	tenantSQL := sqlutils.MakeSQLRunner(tenantDB)
-	tenantSQL.Exec(t, "CREATE TABLE foo AS SELECT generate_series(1, 4000)")
-	tenantSQL.Exec(t, "ALTER TABLE foo SPLIT AT VALUES (500), (1000), (1500), (2000), (2500), (3000)")
-	tenantSQL.Exec(t, "ALTER TABLE foo SCATTER")
-
-	t.Log("waiting for SQL instances")
-	waitStart := timeutil.Now()
-	for i := 1; i < multiNode; i++ {
-		testutils.SucceedsSoon(t, func() error {
-			t.Logf("waiting for server %d", i)
-			db, err := tc.Server(i).SystemLayer().SQLConnE(serverutils.DBName("cluster:test/defaultdb"))
-			if err != nil {
-				return err
-			}
-			return db.Ping()
-		})
-	}
-	t.Logf("all SQL instances (took %s)", timeutil.Since(waitStart))
-
-	// Shut down a node.
-	t.Log("shutting down server 2 (n3)")
-	tc.StopServer(2)
-
-	// We use succeeds soon here since it still takes some time
-	// for instance-based planning to recognize the downed node.
-	sv := &tenantApp.ClusterSettings().SV
-	padding := 10 * time.Second
-	timeout := slbase.DefaultTTL.Get(sv) + slbase.DefaultHeartBeat.Get(sv) + padding
-	testutils.SucceedsWithin(t, func() error {
-		_, err := tenantDB.Exec("BACKUP INTO 'nodelocal://1/worker-failure'")
-		return err
-	}, timeout)
-}
-
 func TestBackupTenantImportingTable(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/backup/datadriven_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/kv/bulk"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
@@ -45,6 +46,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/admission"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
 	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/errors"
@@ -194,6 +196,12 @@ func (d *datadrivenTestState) addCluster(t *testing.T, cfg clusterCfg) error {
 	closedts.SideTransportCloseInterval.Override(context.Background(), &settings.SV, 10*time.Millisecond)
 	kvserver.RangeFeedRefreshInterval.Override(context.Background(), &settings.SV, 10*time.Millisecond)
 	sql.TempObjectWaitInterval.Override(context.Background(), &settings.SV, time.Millisecond)
+	// Disable AC yielding as these tests can run many in-process clusters at once
+	// and overload the host. Generally overload would mean bulk work, which only
+	// uses strictly spare capacitym gets starved, but these tests expect it to
+	// still run (just slowly, along with everything else).
+	bulk.YieldIfNoPacer.Override(context.Background(), &settings.SV, false)
+	admission.YieldInPacer.Override(context.Background(), &settings.SV, false)
 	params.ServerArgs.Settings = settings
 
 	clusterSize := cfg.nodes

pkg/cli/cliflags/flags.go

@@ -1825,12 +1825,13 @@ Labs support.
 	ZipIncludeGoroutineStacks = FlagInfo{
 		Name: "include-goroutine-stacks",
 		Description: `
-Fetch stack traces for all goroutines running on each targeted node in nodes/*/stacks.txt
-and nodes/*/stacks_with_labels.txt files. Note that fetching stack traces for all goroutines is
-a "stop-the-world" operation, which can momentarily have negative impacts on SQL service
-latency. Note that any periodic goroutine dumps previously taken on the node will still be
-included in nodes/*/goroutines/*.txt.gz, as these would have already been generated and don't
-require any additional stop-the-world operations to be collected.
+Fetch full stack traces for all goroutines running on each targeted node in nodes/*/stacks.txt files.
+Note that fetching text stack traces for all goroutines incurs a brief "stop-the-world" pause of each
+node which can momentarily have negative impacts on SQL service latency. This flag only controls
+collection of new full dump of all current goroutine stacks -- any previously recorded, periodic
+goroutine dumps retained in the logs directories are still included (in nodes/*/goroutines/*.txt.gz)
+and collection of aggregate counts of current goroutine stacks -- which does not incur a stop-the-world
+pause -- remains enabled regardless of this flag's value.
 `,
 	}
 

pkg/cli/context.go

@@ -359,10 +359,9 @@ type zipContext struct {
 	includeRangeInfo bool
 
 	// includeStacks fetches all goroutines running on each targeted node in
-	// nodes/*/stacks.txt and nodes/*/stacks_with_labels.txt files. Note that
-	// fetching stack traces for all goroutines is a temporary "stop the world"
-	// operation, which can momentarily have negative impacts on SQL service
-	// latency.
+	// nodes/*/stacks.txt. Note that fetching debug=2 stack traces for all
+	// goroutines incurs a temporary "stop the world" operation, which can
+	// momentarily have negative impacts on SQL service latency.
 	includeStacks bool
 
 	// includeRunningJobTraces includes the active traces of each running

pkg/cli/testdata/zip/testzip_exclude_goroutine_stacks

@@ -25,6 +25,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s --validate-zip-file=false --
 [node 1] requesting data for debug/nodes/1/details... received response... writing JSON output: debug/nodes/1/details.json... done
 [node 1] requesting data for debug/nodes/1/gossip... received response... writing JSON output: debug/nodes/1/gossip.json... done
 [node 1] Skipping fetching goroutine stacks. Enable via the --include-goroutine-stacks flag.
+[node 1] requesting stacks with labels... received response... writing binary output: debug/nodes/1/stacks_with_labels.txt... done
 [node 1] requesting heap profile... received response... writing binary output: debug/nodes/1/heap.pprof... done
 [node 1] requesting engine stats... received response... writing binary output: debug/nodes/1/lsm.txt... done
 [node 1] requesting heap profile list... received response... done

pkg/cli/zip_per_node.go

@@ -579,6 +579,12 @@ func (zc *debugZipContext) getCurrentHeapProfile(
 func (zc *debugZipContext) getStackInformation(
 	ctx context.Context, nodePrinter *zipReporter, id string, prefix string,
 ) error {
+	// zipCtx.includeStacks controls specifically the inclusion of stacks.txt, as
+	// the debug=2 collection scheme has a "stop the world" implementation that
+	// makes it potentially disruptive and thus has an option to skip it. This
+	// option does _not_ need to skip collection of stack groups (debug=1) or the
+	// binary goroutine profile (debug=3 or debug=0), as these do not have that
+	// same stop-the-world disruption.
 	if zipCtx.includeStacks {
 		if zipCtx.files.shouldIncludeFile(stacksFileName) {
 			var stacksData []byte
@@ -600,32 +606,32 @@ func (zc *debugZipContext) getStackInformation(
 		} else {
 			nodePrinter.info("skipping %s due to file filters", stacksFileName)
 		}
+	} else {
+		nodePrinter.info("Skipping fetching goroutine stacks. Enable via the --%s flag.", cliflags.ZipIncludeGoroutineStacks.Name)
+	}
 
-		var stacksDataWithLabels []byte
-		if zipCtx.files.shouldIncludeFile(stacksWithLabelFileName) {
-			s := nodePrinter.start("requesting stacks with labels")
-			requestErr := zc.runZipFn(ctx, s,
-				func(ctx context.Context) error {
-					stacks, err := zc.status.Stacks(ctx, &serverpb.StacksRequest{
-						NodeId: id,
-						Type:   serverpb.StacksType_GOROUTINE_STACKS_DEBUG_1,
-					})
-					if err == nil {
-						stacksDataWithLabels = stacks.Data
-					}
-					return err
+	var stacksDataWithLabels []byte
+	if zipCtx.files.shouldIncludeFile(stacksWithLabelFileName) {
+		s := nodePrinter.start("requesting stacks with labels")
+		requestErr := zc.runZipFn(ctx, s,
+			func(ctx context.Context) error {
+				stacks, err := zc.status.Stacks(ctx, &serverpb.StacksRequest{
+					NodeId: id,
+					Type:   serverpb.StacksType_GOROUTINE_STACKS_DEBUG_1,
 				})
-			if zipCtx.redact {
-				stacksDataWithLabels = redactStackTrace(stacksDataWithLabels)
-			}
-			if err := zc.z.createRawOrError(s, prefix+"/"+stacksWithLabelFileName, stacksDataWithLabels, requestErr); err != nil {
+				if err == nil {
+					stacksDataWithLabels = stacks.Data
+				}
 				return err
-			}
-		} else {
-			nodePrinter.info("skipping %s due to file filters", stacksWithLabelFileName)
+			})
+		if zipCtx.redact {
+			stacksDataWithLabels = redactStackTrace(stacksDataWithLabels)
+		}
+		if err := zc.z.createRawOrError(s, prefix+"/"+stacksWithLabelFileName, stacksDataWithLabels, requestErr); err != nil {
+			return err
 		}
 	} else {
-		nodePrinter.info("Skipping fetching goroutine stacks. Enable via the --%s flag.", cliflags.ZipIncludeGoroutineStacks.Name)
+		nodePrinter.info("skipping %s due to file filters", stacksWithLabelFileName)
 	}
 	return nil
 }

pkg/cmd/roachprod/cli/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//pkg/roachprod/logger",
         "//pkg/roachprod/roachprodutil",
         "//pkg/roachprod/ssh",
-        "//pkg/roachprod/ui",
         "//pkg/roachprod/vm",
         "//pkg/roachprod/vm/gce",
         "//pkg/util/envutil",

pkg/cmd/roachprod/cli/commands.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/config"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/roachprodutil"
-	"github.com/cockroachdb/cockroach/pkg/roachprod/ui"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm/gce"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -547,15 +546,15 @@ hosts file.
 				if listDetails {
 					collated := filteredCloud.BadInstanceErrors()
 
-					// Sort by Error() value for stable output
-					var errors ui.ErrorsByError
-					for err := range collated {
-						errors = append(errors, err)
+					// Sort by error message for stable output
+					var errMsgs []string
+					for msg := range collated {
+						errMsgs = append(errMsgs, msg)
 					}
-					sort.Sort(errors)
+					sort.Strings(errMsgs)
 
-					for _, e := range errors {
-						fmt.Printf("%s: %s\n", e, collated[e].Names())
+					for _, msg := range errMsgs {
+						fmt.Printf("%s: %s\n", msg, collated[msg].Names())
 					}
 				}
 			}

pkg/kv/bulk/cpu_pacer.go

@@ -25,7 +25,8 @@ var cpuPacerRequestDuration = settings.RegisterDurationSetting(
 	50*time.Millisecond,
 )
 
-var yieldIfNoPacer = settings.RegisterBoolSetting(
+// YieldIfNoPacer is exported so it can be overridden in tests.
+var YieldIfNoPacer = settings.RegisterBoolSetting(
 	settings.ApplicationLevel,
 	"bulkio.elastic_cpu_control.always_yield.enabled",
 	"if true, yield the CPU as needed even when time-based elastic pacing is not enabled",
@@ -38,7 +39,7 @@ func NewCPUPacer(ctx context.Context, db *kv.DB, setting *settings.BoolSetting)
 	if db == nil || db.AdmissionPacerFactory == nil || !setting.Get(db.SettingsValues()) {
 		log.Dev.Infof(ctx, "admission control is not configured to pace bulk ingestion")
 
-		if db != nil && yieldIfNoPacer.Get(db.SettingsValues()) {
+		if db != nil && YieldIfNoPacer.Get(db.SettingsValues()) {
 			// Return a Pacer that just yields.
 			return &admission.Pacer{Yield: true}
 		}