Merge #91770 #92911

91770: log: provide clearer error message when decoder is given empty file r=dhartunian a=abarganier Some users of debug zip were confused about the error messages they were receiving when the LogFile endpoint attempted to read a log file containing 0 bytes. Instead of the error indicating that the file might be empty, it simply said: ``` rpc error: code = Unknown desc = EOF ``` This patch simply updates the log decoder constructor function to detect when it's given an empty file, and return a more meaningful error message. The new error will instead look like: ``` rpc error: code = Unknown desc = cannot read format from empty log file``` ``` Release note: none Fixes: #90770 92911: backupccl: deflake multinode datadriven tests that set cluster settings r=adityamaru a=msbutler The `SET CLUSTER SETTING` stmt propogrates the cluster setting to remote nodes asynchronously, but our data driven multinode tests incorrectly assumed that settings were propogated during stmt execution. This patch adds a new 'set-cluster-setting' dd cmd which will propogate the cluster setting to all nodes before the cmd returns. Fixes #92808 Release note: none Co-authored-by: Alex Barganier <[email protected]> Co-authored-by: Michael Butler <[email protected]>
cockroachdb · Dec 5, 2022 · 14abdbf · 14abdbf
3 parents d1c95d5 + 1ddd641 + 5102486
commit 14abdbf
Show file tree

Hide file tree

Showing 60 changed files with 261 additions and 228 deletions.
diff --git a/pkg/ccl/backupccl/datadriven_test.go b/pkg/ccl/backupccl/datadriven_test.go
diff --git a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/backup-options b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/backup-options
@@ -1,4 +1,4 @@
-new-server name=s1 allow-implicit-access
+new-cluster name=s1 allow-implicit-access
 ----
 
 # Create test schedules.

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/missing-schedule b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/missing-schedule
@@ -1,4 +1,4 @@
-new-server name=s1 allow-implicit-access
+new-cluster name=s1 allow-implicit-access
 ----
 
 # Test that dropping the full backup causes a graceful failure on ALTER.

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/recurrence b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/recurrence
@@ -1,4 +1,4 @@
-new-server name=s1 allow-implicit-access
+new-cluster name=s1 allow-implicit-access
 ----
 
 # Create test schedules.

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options
@@ -1,4 +1,4 @@
-new-server name=s1 allow-implicit-access
+new-cluster name=s1 allow-implicit-access
 ----
 
 # Create test schedules.

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors b/pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors
@@ -2,7 +2,7 @@
 # and type descriptors in the DROP state.
 subtest dropped-database-descriptors
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql
@@ -105,7 +105,7 @@ subtest end
 # Test backup/restore interaction with dropped schema and type in a database.
 subtest dropped-schema-descriptors
 
-new-server name=s2
+new-cluster name=s2
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors-declarative b/pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors-declarative
@@ -2,7 +2,7 @@
 # and type descriptors in the DROP state.
 subtest dropped-database-descriptors
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql
@@ -58,7 +58,7 @@ subtest end
 # Test backup/restore interaction with dropped schema and type in a database.
 subtest dropped-schema-descriptors
 
-new-server name=s2
+new-cluster name=s2
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated
@@ -1,5 +1,5 @@
 # Test permissions checks for non-admin users running BACKUP.
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql
@@ -42,7 +42,7 @@ BACKUP TABLE d.t INTO 'nodelocal://0/test-nonroot-table';
 # Start a new cluster with the same IO dir.
 # nodelocal IO is a form of implicit access (since it is accessed as the node,
 # so we require the allow-implicit-access flag.
-new-server name=s2 allow-implicit-access
+new-cluster name=s2 allow-implicit-access
 ----
 
 exec-sql
@@ -121,18 +121,18 @@ GRANT USAGE ON TYPE d2.greeting TO testuser;
 ----
 
 # testuser should now have all the required privileges.
-exec-sql server=s2 user=testuser
+exec-sql cluster=s2 user=testuser
 BACKUP DATABASE d2 INTO 'nodelocal://0/d2';
 ----
 NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2.
 
-exec-sql server=s2 user=testuser
+exec-sql cluster=s2 user=testuser
 BACKUP TABLE d2.t INTO 'nodelocal://0/d2-table';
 ----
 NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.
 
 # Test that implicit access is disallowed when the testing knob is not set.
-new-server name=s3 share-io-dir=s1
+new-cluster name=s3 share-io-dir=s1
 ----
 
 exec-sql
@@ -142,7 +142,7 @@ CREATE USER testuser;
 GRANT CONNECT ON DATABASE d TO testuser;
 ----
 
-exec-sql server=s3 user=testuser
+exec-sql cluster=s3 user=testuser
 SHOW BACKUP 'http://COCKROACH_TEST_HTTP_SERVER/'
 ----
 pq: only users with the admin role or the EXTERNALIOIMPLICITACCESS system privilege are allowed to access the specified http URI
@@ -153,14 +153,14 @@ BACKUP DATABASE d INTO 'nodelocal://0/test3'
 pq: only users with the admin role or the EXTERNALIOIMPLICITACCESS system privilege are allowed to access the specified nodelocal URI
 
 # Test that http access is disallowed by disable http even if allow-non-admin is on.
-new-server name=s4 allow-implicit-access disable-http
+new-cluster name=s4 allow-implicit-access disable-http
 ----
 
-exec-sql server=s4
+exec-sql cluster=s4
 CREATE USER testuser
 ----
 
-exec-sql server=s4 user=testuser
+exec-sql cluster=s4 user=testuser
 SHOW BACKUP 'http://COCKROACH_TEST_HTTP_SERVER/'
 ----
 pq: make storage: external http access disabled
diff --git a/pkg/ccl/backupccl/testdata/backup-restore/column-families b/pkg/ccl/backupccl/testdata/backup-restore/column-families
@@ -1,6 +1,6 @@
 # disabled to run within tenant because ALTER SPLIT cmd is not supported within tenant
 
-new-server name=s1 disable-tenant localities=us-east-1,us-west-1,us-west-2,eu-central-1
+new-cluster name=s1 disable-tenant localities=us-east-1,us-west-1,us-west-2,eu-central-1
 ----
 
 exec-sql
@@ -23,17 +23,15 @@ exec-sql
 ALTER TABLE cfs SPLIT AT SELECT a FROM cfs;
 ----
 
-exec-sql
--- Split the output files very small to catch output SSTs mid-row.
-SET CLUSTER SETTING bulkio.backup.file_size = '1';
+
+# Split the output files very small to catch output SSTs mid-row.
+set-cluster-setting setting=bulkio.backup.file_size value=1
 ----
 
-exec-sql
-SET CLUSTER SETTING kv.bulk_sst.target_size = '1';
+set-cluster-setting setting=kv.bulk_sst.target_size value=1
 ----
 
-exec-sql
-SET CLUSTER SETTING bulkio.backup.merge_file_buffer_size = '1MiB';
+set-cluster-setting setting=bulkio.backup.merge_file_buffer_size value=1MiB
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/descriptor-broadening b/pkg/ccl/backupccl/testdata/backup-restore/descriptor-broadening
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/descriptor-conflicts b/pkg/ccl/backupccl/testdata/backup-restore/descriptor-conflicts
@@ -7,7 +7,7 @@
 # disabled to run within a tenant because they cannot set zone configs
 # https://github.com/cockroachdb/cockroach/issues/49854?version=v22.2
 
-new-server name=s1 disable-tenant
+new-cluster name=s1 disable-tenant
 ----
 
 exec-sql
@@ -26,7 +26,7 @@ exec-sql
 BACKUP INTO 'nodelocal://0/conflicting-descriptors';
 ----
 
-new-server name=s2 share-io-dir=s1 disable-tenant
+new-cluster name=s2 share-io-dir=s1 disable-tenant
 ----
 
 # Create 4 dummy system tables that will have conflicting IDs with the database,

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/drop-schedule-backup b/pkg/ccl/backupccl/testdata/backup-restore/drop-schedule-backup
@@ -1,7 +1,7 @@
 # These tests validate the DROP SCHEDULE command for dropping the incremental scheduled
 # backup when the full scheduled backup is dropped.
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups b/pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/external-connections-nodelocal b/pkg/ccl/backupccl/testdata/backup-restore/external-connections-nodelocal
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 subtest basic-backup-nodelocal
@@ -99,7 +99,7 @@ subtest end
 
 subtest basic-restore-nodelocal
 
-new-server name=s2 share-io-dir=s1
+new-cluster name=s2 share-io-dir=s1
 ----
 
 # Cluster restore.
@@ -157,7 +157,7 @@ subtest end
 
 subtest incremental-location-backup-restore-nodelocal
 
-switch-server name=s1
+switch-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges b/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 subtest backup-restore-privileges

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/external-connections-userfile b/pkg/ccl/backupccl/testdata/backup-restore/external-connections-userfile
@@ -1,4 +1,4 @@
-new-server name=s1
+new-cluster name=s1
 ----
 
 subtest basic-backup-userfile
@@ -136,7 +136,7 @@ subtest end
 
 subtest incremental-location-backup-restore-userfile
 
-switch-server name=s1
+switch-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/feature-flags b/pkg/ccl/backupccl/testdata/backup-restore/feature-flags
@@ -2,7 +2,7 @@ subtest backup-feature-flags
 
 # disabled for tenants as they can't enable/disable backup and restore features
 
-new-server name=s1 disable-tenant
+new-cluster name=s1 disable-tenant
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/file_table_read_write b/pkg/ccl/backupccl/testdata/backup-restore/file_table_read_write
@@ -1,6 +1,6 @@
 subtest backup_file_table
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/import-start-time b/pkg/ccl/backupccl/testdata/backup-restore/import-start-time
@@ -5,7 +5,7 @@
 ## -check that the ImportStartTime is set on the descriptor
 ## -check that it's removed after cancellation / success
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-missing b/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-missing
@@ -22,25 +22,25 @@
 #  - revision history cluster backups
 #  - revision history table/database backups which includes an incremental backup between t1 and t2
 
-new-server name=s1 allow-implicit-access disable-tenant
+new-cluster name=s1 allow-implicit-access disable-tenant
 ----
 
 # Link 4 backup chains:
 # 1. A corrupt database backup chain with revision history
-link-backup server=s1 src-path=restore_importing_tables,database dest-path=database
+link-backup cluster=s1 src-path=restore_importing_tables,database dest-path=database
 ----
 
 # 2. A clean database backup chain with revision history which includes two
 #    backups that observe the offline descriptors.
-link-backup server=s1 src-path=restore_importing_tables,database_double_inc dest-path=database_double_inc
+link-backup cluster=s1 src-path=restore_importing_tables,database_double_inc dest-path=database_double_inc
 ----
 
 # 3. A clean data database backup chain without revision history
-link-backup server=s1 src-path=restore_importing_tables,database_no_hist dest-path=database_no_hist
+link-backup cluster=s1 src-path=restore_importing_tables,database_no_hist dest-path=database_no_hist
 ----
 
 # 4. A clean cluster backup chain with revision history
-link-backup server=s1 src-path=restore_importing_tables,cluster dest-path=cluster
+link-backup cluster=s1 src-path=restore_importing_tables,cluster dest-path=cluster
 ----
 
 # Note that we're purposely skipping the reintroduction of foo, foofoo, goodfoo in the
@@ -212,7 +212,7 @@ SELECT count(*) FROM d.goodfoo;
 
 
 # Check the cluster level restore
-new-server name=s2 share-io-dir=s1 allow-implicit-access disable-tenant
+new-cluster name=s2 share-io-dir=s1 allow-implicit-access disable-tenant
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback b/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback
@@ -9,7 +9,7 @@
 
 # disabled to run within tenant as they don't have access to the
 # storage.mvcc.range_tombstones.enabled cluster setting
-new-server name=s1 disable-tenant
+new-cluster name=s1 disable-tenant
 ----
 
 ###########

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/in-progress-imports b/pkg/ccl/backupccl/testdata/backup-restore/in-progress-imports
@@ -5,7 +5,7 @@
 # On an unfinalized cluster: a backed up import should not get restored
 
 
-new-server name=s1
+new-cluster name=s1
 ----
 
 
@@ -182,7 +182,7 @@ d foofoo table 3 incremental
 
 
 # Ensure all the RESTOREs contain foo (no data) and foofoo (1 row) as of system time t0
-new-server name=s2 share-io-dir=s1 allow-implicit-access
+new-cluster name=s2 share-io-dir=s1 allow-implicit-access
 ----
 
 
@@ -246,7 +246,7 @@ SELECT * FROM d.foofoo;
 
 
 # Ensure the imported data exists as of latest time
-new-server name=s3 share-io-dir=s1 allow-implicit-access
+new-cluster name=s3 share-io-dir=s1 allow-implicit-access
 ----
 
 
@@ -334,7 +334,7 @@ SELECT * FROM d.foofoo;
 # More investigation required.
 
 
-new-server name=s4 share-io-dir=s1 allow-implicit-access beforeVersion=Start22_2 disable-tenant
+new-cluster name=s4 share-io-dir=s1 allow-implicit-access beforeVersion=Start22_2 disable-tenant
 ----
 
 exec-sql
@@ -508,7 +508,7 @@ d foo table 2 incremental
 d foofoo table 3 incremental
 
 
-upgrade-server version=Start22_2
+upgrade-cluster version=Start22_2
 ----
 
 exec-sql
@@ -532,7 +532,7 @@ d foofoo table 3 incremental
 
 
 # Restore the backups taken from a mixed version chain
-new-server name=s5 share-io-dir=s1 allow-implicit-access disable-tenant
+new-cluster name=s5 share-io-dir=s1 allow-implicit-access disable-tenant
 ----