Merge #105854 #106341 #106380

105854: upgrades: fix the write of 'version' to system.tenant_settings r=ajstorm a=knz

Prior to this patch, a bogus value was written to the tenant_id column for the 'version' override. This patch fixes it.

We also suspect this will help with the multitenant-upgrade roachtest but this will be checked in a later step.

Informs #105858.
Release note: None
Epic: CRDB-26691

106341: roachprod: fix leaky goroutine in `SyncedCluster.Monitor` r=srosenberg a=renatolabs

The `Monitor` function would leak a goroutine per node waiting for context cancelation even after the monitor loop had already exited (for example, when a `OneShot` monitor check was requested).

This commit updates that function so that we use a cancelable context derived from the context passed as argument; when the monitor loop exits, we cancel that context, which causes the leaky goroutine to terminate appropriately.

This leaked goroutine shows up in the newly introduced roachtest `leaktest`, where there is one leaked goroutine per node in the cluster created by the test; the monitor is created during the `assertNoDeadNode` post-test assertion.

Epic: none

Release note: None

106380: kvserver: unskip and deflake `TestShowTraceReplica` r=erikgrinaker a=erikgrinaker

This test asserts on replica placement. By default, the replicate queue can take up to 10 minutes to execute config changes, which can fail the test.

This patch speeds up the replicate queue. It also skips the test under stressrace and deadlock, since it is timing sensitive.

Resolves #34213.

Epic: none
Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/tenant_settings

@@ -8,10 +8,13 @@ true
 user host-cluster-root
 
 # Ensure that the version is set the same in the system tenant and in the tenant override.
-query I
-select count(*) from system.settings JOIN system.tenant_settings on tenant_settings.name = settings.name where tenant_settings.name = 'version' and tenant_settings.value = settings.value;
+query TI rowsort
+SELECT settings.name, tenant_id
+FROM system.settings
+JOIN system.tenant_settings ON tenant_settings.name = settings.name
+WHERE tenant_settings.name = 'version' AND tenant_settings.value = settings.value;
 ----
-1
+version 0
 
 statement ok
 ALTER TENANT [10] SET CLUSTER SETTING sql.notices.enabled = false

pkg/roachprod/install/cluster_synced.go

@@ -638,6 +638,7 @@ func (c *SyncedCluster) Monitor(
 	ch := make(chan NodeMonitorInfo)
 	nodes := c.TargetNodes()
 	var wg sync.WaitGroup
+	monitorCtx, cancel := context.WithCancel(ctx)
 
 	for i := range nodes {
 		wg.Add(1)
@@ -758,9 +759,10 @@ done
 				return
 			}
 
-			// Watch for context cancellation.
+			// Watch for context cancellation, which can happen either if
+			// the test fails, or if the monitor loop exits.
 			go func() {
-				<-ctx.Done()
+				<-monitorCtx.Done()
 				sess.Close()
 			}()
 
@@ -776,6 +778,7 @@ done
 	}
 	go func() {
 		wg.Wait()
+		cancel()
 		close(ch)
 	}()
 

pkg/server/tenantsettingswatcher/BUILD.bazel

@@ -47,6 +47,7 @@ go_test(
     embed = [":tenantsettingswatcher"],
     deps = [
         "//pkg/base",
+        "//pkg/clusterversion",
         "//pkg/keys",
         "//pkg/kv/kvpb",
         "//pkg/roachpb",

pkg/server/tenantsettingswatcher/watcher_test.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/tenantsettingswatcher"
@@ -60,6 +61,9 @@ func TestWatcher(t *testing.T) {
 		t.Helper()
 		var vals []string
 		for _, s := range overrides {
+			if s.Name == clusterversion.KeyVersionSetting {
+				continue
+			}
 			vals = append(vals, fmt.Sprintf("%s=%s", s.Name, s.Value.Value))
 		}
 		if actual := strings.Join(vals, " "); actual != expected {

pkg/sql/logictest/testdata/logic_test/tenant

@@ -415,11 +415,12 @@ EXCEPT SELECT capability_name, capability_value FROM [SHOW TENANT othertenant WI
 ----
 
 # Check that the setting overrides were copied.
-query TTTT
-SELECT variable, value, type, origin FROM [SHOW CLUSTER SETTINGS FOR TENANT othertenant]
+query TTTT rowsort
+SELECT variable, IF(variable='version','--',value), type, origin FROM [SHOW CLUSTER SETTINGS FOR TENANT othertenant]
 WHERE origin != 'no-override'
 ----
 trace.debug.enable  true  b  per-tenant-override
+version             --    m  all-tenants-override
 
 # Check that the resource usage parameters were copied.
 query IIRRRRI
@@ -463,11 +464,12 @@ EXCEPT SELECT capability_name, capability_value FROM [SHOW TENANT othertenant WI
 ----
 
 # Check the setting overrides were taken over.
-query TTTT
-SELECT variable, value, type, origin FROM [SHOW CLUSTER SETTINGS FOR TENANT othertenant]
+query TTTT rowsort
+SELECT variable, IF(variable='version','--',value), type, origin FROM [SHOW CLUSTER SETTINGS FOR TENANT othertenant]
 WHERE origin != 'no-override'
 ----
 trace.debug.enable  true  b  per-tenant-override
+version             --    m  all-tenants-override
 
 # Check that the resource usage parameters were copied.
 query IIRRRRI

pkg/sql/show_trace_replica_test.go

@@ -15,6 +15,7 @@ import (
 	"fmt"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
@@ -34,7 +35,8 @@ func TestShowTraceReplica(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.WithIssue(t, 98598)
+	skip.UnderStressRace(t) // too slow
+	skip.UnderDeadlock(t)   // too slow
 
 	const numNodes = 4
 
@@ -44,6 +46,7 @@ func TestShowTraceReplica(t *testing.T) {
 	ctx := context.Background()
 	tsArgs := func(node string) base.TestServerArgs {
 		return base.TestServerArgs{
+			ScanMaxIdleTime: 10 * time.Millisecond, // speed up replicate queue
 			Knobs: base.TestingKnobs{
 				Server: &server.TestingKnobs{
 					DefaultZoneConfigOverride:       &zoneConfig,

pkg/upgrade/upgrades/permanent_upgrades.go

@@ -135,11 +135,12 @@ func populateVersionSetting(
 
 	// Tenant ID 0 indicates that we're overriding the value for all
 	// tenants.
-	tenantID := tree.NewDInt(0)
 	_, err = ie.Exec(
 		ctx,
 		"insert-setting", nil, /* txn */
-		fmt.Sprintf(`INSERT INTO system.tenant_settings (tenant_id, name, value, "last_updated", "value_type") VALUES (%d, 'version', x'%x', now(), 'm') ON CONFLICT(tenant_id, name) DO NOTHING`, tenantID, b),
+		fmt.Sprintf(`
+INSERT INTO system.tenant_settings (tenant_id, name, value, last_updated, value_type)
+VALUES (0, 'version', x'%x', now(), 'm') ON CONFLICT(tenant_id, name) DO NOTHING`, b),
 	)
 	if err != nil {
 		return err