Merge #50083

50083: logictest: triage remaining tests that block 3node-tenant r=tbg,jordanlewis a=asubiotto This PR adds a way to specify an associated issue number when adding a blocklist directive and uses that to associate issues with tests that block 3node-tenant. A small downside is that all configs from all files are parsed up front, which means that all issues are printed out upfront in the parsing stage. I'm too lazy to plumb this information down to execution time since maybe the correct fix is to not parse all file configs up front, but I think that's out of scope for this PR, so let's either decide to keep the behavior to print issue URLs or remove it if it's annoying. Release note: None (testing change) Closes #48798 Co-authored-by: Alfonso Subiotto Marques <[email protected]>
cockroachdb · Jun 15, 2020 · 9a38931 · 9a38931
2 parents 01a780b + f107077
commit 9a38931
Show file tree

Hide file tree

Showing 22 changed files with 58 additions and 32 deletions.
diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go
@@ -57,6 +57,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -1400,7 +1401,7 @@ CREATE DATABASE test;
 // applyBlocklistToConfigIdxs applies the given blocklist to config idxs,
 // returning the result.
 func applyBlocklistToConfigIdxs(
-	configIdxs []logicTestConfigIdx, blocklist map[string]struct{},
+	configIdxs []logicTestConfigIdx, blocklist map[string]int,
 ) []logicTestConfigIdx {
 	if len(blocklist) == 0 {
 		return configIdxs
@@ -1415,18 +1416,42 @@ func applyBlocklistToConfigIdxs(
 	return newConfigIdxs
 }
 
+// getBlocklistIssueNo takes a blocklist directive with an optional issue number
+// and returns the stripped blocklist name with the corresponding issue number
+// as an integer.
+// e.g. an input of "3node-tenant(123456)" would return "3node-tenant", 123456
+func getBlocklistIssueNo(blocklistDirective string) (string, int) {
+	parts := strings.Split(blocklistDirective, "(")
+	if len(parts) != 2 {
+		return blocklistDirective, 0
+	}
+
+	issueNo, err := strconv.Atoi(strings.TrimRight(parts[1], ")"))
+	if err != nil {
+		panic(fmt.Sprintf("possibly malformed blocklist directive: %s: %v", blocklistDirective, err))
+	}
+	return parts[0], issueNo
+}
+
 // processConfigs, given a list of configNames, returns the list of
 // corresponding logicTestConfigIdxs.
 func processConfigs(t *testing.T, path string, configNames []string) []logicTestConfigIdx {
 	const blocklistChar = '!'
-	blocklist := make(map[string]struct{})
+	// blocklist is a map from a blocked config to a corresponding issue number.
+	// If 0, there is no associated issue.
+	blocklist := make(map[string]int)
 	allConfigNamesAreBlocklistDirectives := true
 	for _, configName := range configNames {
 		if configName[0] != blocklistChar {
 			allConfigNamesAreBlocklistDirectives = false
 			continue
 		}
-		blocklist[configName[1:]] = struct{}{}
+
+		blockedConfig, issueNo := getBlocklistIssueNo(configName[1:])
+		if issueNo != 0 {
+			t.Logf("will skip %s config in test %s due to issue: %s", blockedConfig, path, unimplemented.MakeURL(issueNo))
+		}
+		blocklist[blockedConfig] = issueNo
 	}
 
 	var configs []logicTestConfigIdx

diff --git a/pkg/sql/logictest/testdata/logic_test/alter_primary_key b/pkg/sql/logictest/testdata/logic_test/alter_primary_key
@@ -1,6 +1,4 @@
-# 3node-tenant fails due to
-# https://github.com/cockroachdb/cockroach/issues/47900.
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50048)
 
 statement ok
 SET experimental_enable_hash_sharded_indexes = true

diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function
@@ -1,8 +1,6 @@
-# 3node-tenant fails due to:
-# https://github.com/cockroachdb/cockroach/issues/48375
-# Specifically, a status server is unavailable when attempting to set a zone
-# config.
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49854)
+# TODO(asubiotto): Possibly split out the test that attempts to set a zone
+# config in order to run the other ones.
 
 statement ok
 CREATE TABLE foo (a int)

diff --git a/pkg/sql/logictest/testdata/logic_test/cascade b/pkg/sql/logictest/testdata/logic_test/cascade
@@ -1,5 +1,4 @@
-# This test times out when run with 3node-tenant.
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50048)
 
 # The tests in this file target the legacy FK paths.
 statement ok

diff --git a/pkg/sql/logictest/testdata/logic_test/cascade_opt b/pkg/sql/logictest/testdata/logic_test/cascade_opt
@@ -1,5 +1,4 @@
-# This test times out when run with 3node-tenant.
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50048)
 
 # The tests in this file target the new optimizer-driven FK paths (with
 # fall back on the legacy paths for unsupported cases).

diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(47895)
 query error database "crdb_internal" does not exist
 ALTER DATABASE crdb_internal RENAME TO not_crdb_internal
 

diff --git a/pkg/sql/logictest/testdata/logic_test/delete b/pkg/sql/logictest/testdata/logic_test/delete
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50048)
 statement ok
 CREATE TABLE kv (
   k INT PRIMARY KEY,

diff --git a/pkg/sql/logictest/testdata/logic_test/enums b/pkg/sql/logictest/testdata/logic_test/enums
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50049)
 
 statement ok
 SET experimental_enable_enums=true;

diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50047)
 ##################
 # TABLE DDL
 ##################

diff --git a/pkg/sql/logictest/testdata/logic_test/family b/pkg/sql/logictest/testdata/logic_test/family
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50048)
 # a is the primary key so b gets optimized into a one column value. The c, d
 # family has two columns, so it's encoded as a tuple
 statement ok

diff --git a/pkg/sql/logictest/testdata/logic_test/fk b/pkg/sql/logictest/testdata/logic_test/fk
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49582)
 # The tests in this file target the legacy FK paths.
 statement ok
 SET optimizer_foreign_keys = false

diff --git a/pkg/sql/logictest/testdata/logic_test/interleaved b/pkg/sql/logictest/testdata/logic_test/interleaved
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49854)
 # Grandparent table
 statement ok
 CREATE TABLE p2 (i INT PRIMARY KEY, s STRING)

diff --git a/pkg/sql/logictest/testdata/logic_test/interleaved_join b/pkg/sql/logictest/testdata/logic_test/interleaved_join
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(50050)
 # The following tables form the interleaved hierarchy:
 #   name:             primary key:                # rows:   'a' = id mod X :
 #   parent1           (pid1)                      40        8

diff --git a/pkg/sql/logictest/testdata/logic_test/lookup_join b/pkg/sql/logictest/testdata/logic_test/lookup_join
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49582)
 statement ok
 CREATE TABLE abc (a INT, b INT, c INT, PRIMARY KEY (a, c));
 INSERT INTO abc VALUES (1, 1, 2), (2, 1, 1), (2, NULL, 2)

diff --git a/pkg/sql/logictest/testdata/logic_test/run_control b/pkg/sql/logictest/testdata/logic_test/run_control
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(47895)
 query error job with ID 1 does not exist
 PAUSE JOB 1
 

diff --git a/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn b/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49854)
 # Disable automatic stats to avoid flakiness (sometimes causes retry errors).
 statement ok
 SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false

diff --git a/pkg/sql/logictest/testdata/logic_test/select_index b/pkg/sql/logictest/testdata/logic_test/select_index
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49582)
 statement ok
 CREATE TABLE t (
   a INT PRIMARY KEY,

diff --git a/pkg/sql/logictest/testdata/logic_test/sqlsmith b/pkg/sql/logictest/testdata/logic_test/sqlsmith
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(47899)
 # This file contains regression tests discovered by sqlsmith.
 
 

diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49854)
 query T
 SHOW DATABASES
 ----

diff --git a/pkg/sql/logictest/testdata/logic_test/system_namespace b/pkg/sql/logictest/testdata/logic_test/system_namespace
@@ -1,4 +1,6 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(48778)
+# When run with a tenant, system.namespace has an extra entry for
+# descriptor_id_seq.
 query IITI rowsort
 SELECT * FROM system.namespace
 ----

diff --git a/pkg/sql/logictest/testdata/logic_test/zone_config b/pkg/sql/logictest/testdata/logic_test/zone_config
@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant(49854)
 # Check that we can alter the default zone config.
 
 statement ok

diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go
@@ -103,7 +103,7 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla
 		return nil, err
 	}
 	if !p.ExecCfg().Codec.ForSystemTenant() {
-		return nil, errorutil.UnsupportedWithMultiTenancy()
+		return nil, errorutil.UnsupportedWithMultiTenancy(multitenancyZoneCfgIssueNo)
 	}
 
 	var yamlConfig tree.TypedExpr
@@ -837,6 +837,8 @@ func validateZoneAttrsAndLocalities(
 	return nil
 }
 
+const multitenancyZoneCfgIssueNo = 49854
+
 func writeZoneConfig(
 	ctx context.Context,
 	txn *kv.Txn,
@@ -846,6 +848,9 @@ func writeZoneConfig(
 	execCfg *ExecutorConfig,
 	hasNewSubzones bool,
 ) (numAffected int, err error) {
+	if !execCfg.Codec.ForSystemTenant() {
+		return 0, errorutil.UnsupportedWithMultiTenancy(multitenancyZoneCfgIssueNo)
+	}
 	if len(zone.Subzones) > 0 {
 		st := execCfg.Settings
 		zone.SubzoneSpans, err = GenerateSubzoneSpans(